Skip to content

Commit c1f4d84

Browse files
chore(api): modernize FromRow with #[derive] + RowAccessor (#61, #62) (#74)
* chore(api): introduce RowAccessor + change FromRow trait signature (#61, #62 part 1) Foundation commit for the FromRow modernization PR. Introduces RowAccessor and the Row::get_by_name accessor; changes the FromRow trait signature from `&Row` to `RowAccessor<'_>`; deletes the 1/2/3/4- tuple FromRow impls; migrates the 3 hand-written FromRow impls; and wires the cached column-name -> index lookup through fetch_one_as / fetch_all_as (sync + async). This is a breaking change to the FromRow trait. Lands as `chore:` to defer release-please version bump until the v0.3.0 rollup commit. What changed: - New `hyperdb-api/src/row_accessor.rs` with `RowAccessor<'a>`. Borrows &Row + a pre-built HashMap<&str, usize>. Methods: `get<T>(name)`, `get_opt<T>(name)`, `position<T>(idx)`, `row()`. Errors map to Error::Column { kind: Missing | Null | TypeMismatch } and Error::ColumnIndexOutOfBounds. Includes 7 unit tests covering each error path and the happy paths. - New `Row::get_by_name<T>(name)` for hand-coded paths that don't go through FromRow. Uses ResultSchema::column_index (linear scan). Doc recommends #[derive(FromRow)] or fetch_*_as for hot paths (both build the cache once per query). - FromRow trait signature changed: `fn from_row(row: &Row)` -> `fn from_row(row: RowAccessor<'_>)`. Trait rustdoc updated with the recommended derive form (preview for the upcoming proc-macro crate) and a hand-written impl example using `row.get(...)` / `row.get_opt(...)`. - Deleted the 4 tuple FromRow impls (1/2/3/4-tuple). Migration: callers define a struct with #[derive(FromRow)] (added in next commit) or use Row::get(idx) directly. - Migrated the 3 hand-written FromRow impls to the new shape: TestUser (tests/remaining_features_tests.rs), User (tests/async_connection_tests.rs), Order (examples/async_parity_smoke.rs). - fetch_one_as / fetch_all_as (4 sites: sync + async) now build the HashMap lookup once per query from the result schema, then construct a RowAccessor per row. All rows in a result set share the same Arc<ResultSchema>, so this is safe. - Doctest examples in connection.rs and result.rs updated to the new trait shape; the example_raw_transaction direct call to Order::from_row in async_parity_smoke.rs replaced with a fetch_all_as call (since the trait now takes a RowAccessor that needs pre-built indices). Verification: - cargo build --workspace --all-targets -- clean - cargo clippy --workspace --all-targets -- -D warnings -- clean - cargo test --workspace -- all targets pass (including doctests) - cargo fmt --check -- clean * chore(api): add hyperdb-api-derive crate with #[derive(FromRow)] (#61, #62 part 2) Adds the proc-macro crate `hyperdb-api-derive` that provides `#[derive(FromRow)]`. Re-exported from `hyperdb-api` so callers don't need a direct dependency — same pattern as serde / thiserror. What changed: - New workspace member `hyperdb-api-derive/` with `proc-macro = true`, deps on syn v2 (with the `full` feature), quote, proc-macro2. - `#[derive(FromRow)]` proc-macro generates an `impl FromRow` that uses the `RowAccessor` API from Commit 1: `row.get("col")?` for required fields, `row.get_opt("col")?` for `Option<T>` fields. Field name → column name match is exact by default; the `#[hyperdb(rename = "...")]` attribute overrides on a per-field basis. - Helpful compile errors for unsupported shapes: - Tuple structs → "tuple-struct fields are not supported" - Enums → "FromRow cannot be derived on enums" - Unions → "FromRow cannot be derived on unions" - Unknown attribute → "unrecognized hyperdb attribute `{x}`; expected `rename = \"...\"`" - `hyperdb-api`'s lib.rs adds `pub use hyperdb_api_derive::FromRow;` alongside the trait re-export. The derive macro and the trait share the name "FromRow" (Rust treats them as different namespaces). - 3 integration tests in `hyperdb-api/tests/remaining_features_tests.rs`: - `test_derive_from_row_parity_with_handwritten`: derived `TestUserDerived` produces identical values to the hand-written `TestUser` impl for the same query. - `test_derive_from_row_with_rename`: `#[hyperdb(rename = "score")]` redirects field-name lookup. - `test_derive_from_row_missing_column_errors`: a derived struct with a column not in the SELECT list surfaces as `Error::Column { kind: Missing }`. Verification: - cargo build --workspace --all-targets — clean - cargo clippy --workspace --all-targets -- -D warnings — clean - cargo test --workspace --lib + --doc — all pass - cargo fmt --check — clean * docs(api): add #61+#62 migration recipes to MIGRATING-0.3.md Adds a "FromRow modernization" section to the consolidated migration guide covering: - Trait signature change (&Row -> RowAccessor<'_>) - Tuple impl deletion + recipes for migrating (define a struct with #[derive(FromRow)] or use Row::get(idx) directly) - New #[derive(FromRow)] proc-macro with #[hyperdb(rename = "...")] - New Row::get_by_name accessor - Error::Column / ColumnErrorKind error shape (already in #70) - Performance note about cached-index lookup vs. linear scan - Note that hyperdb-api-derive doesn't need to be a direct dep Also fixes a broken intra-doc link in row_accessor.rs (FromRow -> crate::FromRow); doc-warning count back to baseline 6. Verification: - cargo build --workspace --all-targets — clean - cargo doc --workspace --no-deps — 6 warnings (= post-#70 baseline) - cargo fmt --check — clean * chore(api): address final-review findings on FromRow modernization (#61, #62 part 4) Three findings from the architectural pre-PR review: - M1 (consistency): RowAccessor::position now produces Error::Column with ColumnErrorKind::Null / TypeMismatch (synthesized name "col[{idx}]") instead of Error::Conversion. This aligns positional errors with the named-access error shape, so callers can match on Error::Column { kind, .. } uniformly across get/get_opt/position rather than special-casing positional access. Out-of-bounds still uses Error::ColumnIndexOutOfBounds for index integrity. Added a dedicated unit test (position_null_errors_with_kind_null). - m1 (API surface): Removed RowAccessor::row(). It was a leak vector that let FromRow impls drop down to bare Row methods, bypassing the cached-index lookup the accessor exists to provide — exactly the anti-pattern this PR set out to eliminate. With no production caller needing it (verified by build), removing rather than gating to pub(crate) is the cleanest move. Easy to add back if a real consumer surfaces. - m2 (forward-compat): Derive macro's "unrecognized hyperdb attribute" error now reads "supported attributes: rename" instead of pinning a specific syntax. When new attributes (skip, default, with) ship in v0.3.x, the message stays accurate without a macro patch. Verification: - cargo build --workspace --all-targets — clean - cargo clippy --workspace --all-targets -- -D warnings — clean - cargo test --workspace --lib — 8 row_accessor tests pass (incl. new position_null_errors_with_kind_null) - cargo fmt --check — clean * chore(api): add #[hyperdb(index = N)] derive attribute + RowAccessor::position_opt Extends #[derive(FromRow)] with a positional access mode for queries where columns have no stable name (e.g. SELECT id, COUNT(*) FROM ... GROUP BY id). Mutually exclusive with #[hyperdb(rename = "...")]. Adds RowAccessor::position_opt as the symmetric Option<T> counterpart to position, mirroring the get/get_opt naming pair. NULL becomes None; out-of-bounds and type-mismatch still error. The macro emits position(N)? for non-Option fields and position_opt(N)? for Option<T> fields, matching the existing get/get_opt dispatch on name-based access. Updates MIGRATING-0.3.md and the derive crate README. Doc-warning count back at baseline 6. Verification: - cargo build/clippy/test/fmt/doc all clean on workspace - new RowAccessor::position_opt unit tests (NULL, happy path, OOB) - new integration test test_derive_from_row_with_index runs against a real query with COUNT(*) (unnamed column) * docs(derive): clarify RowAccessor accessor pairs and zero-based indexing in README
1 parent 5c7e78b commit c1f4d84

16 files changed

Lines changed: 1231 additions & 109 deletions

‎Cargo.lock‎

Lines changed: 10 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ resolver = "2"
33
members = [
44
"hyperdb-api-core",
55
"hyperdb-api",
6+
"hyperdb-api-derive",
67
"hyperdb-api-node",
78
"hyperdb-api-salesforce",
89
"hyperdb-mcp",

‎MIGRATING-0.3.md‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,136 @@ The MCP server's `Engine::execute_in_transaction` helper takes `&self` and so ca
243243

244244
---
245245

246+
## #61 + #62 — FromRow modernization
247+
248+
The `FromRow` trait was redesigned around a new [`RowAccessor`] type and a new [`#[derive(FromRow)]`][derive] proc-macro. The blanket tuple impls (1/2/3/4-tuple) were deleted; hand-written impls have a new signature.
249+
250+
[`RowAccessor`]: https://docs.rs/hyperdb-api/latest/hyperdb_api/struct.RowAccessor.html
251+
[derive]: https://docs.rs/hyperdb-api/latest/hyperdb_api/derive.FromRow.html
252+
253+
### What's changed
254+
255+
| Surface | Before (v0.2.x) | After (v0.3.0) |
256+
| ------- | --------------- | -------------- |
257+
| `FromRow::from_row` signature | `fn from_row(row: &Row) -> Result<Self>` | `fn from_row(row: RowAccessor<'_>) -> Result<Self>` |
258+
| Blanket tuple impls | `(Option<A>,)` … `(Option<A>, Option<B>, Option<C>, Option<D>)` | **Deleted.** Define a struct with `#[derive(FromRow)]` instead. |
259+
| Derive macro | did not exist | `#[derive(FromRow)]` from the new `hyperdb-api-derive` crate (re-exported by `hyperdb-api`) |
260+
| Name-based access on a single row | did not exist | `Row::get_by_name<T>(name)` |
261+
| Cached column-name → index lookup | did not exist | `RowAccessor` carries one; built once per query in `fetch_*_as` |
262+
263+
### What's new
264+
265+
- **`#[derive(FromRow)]`** generates the `impl FromRow` for you. Field names match column names by default; `#[hyperdb(rename = "...")]` overrides the column name; `#[hyperdb(index = N)]` switches to positional access at column `N`. `Option<T>` fields use `get_opt` / `position_opt` (NULL → `None`); other fields use `get` / `position` (NULL → error). `rename` and `index` are mutually exclusive.
266+
267+
```rust
268+
use hyperdb_api::FromRow;
269+
270+
#[derive(FromRow)]
271+
struct User {
272+
id: i32,
273+
name: String,
274+
#[hyperdb(rename = "email_address")]
275+
email: Option<String>,
276+
}
277+
278+
// Useful for queries with computed/unnamed columns, e.g.
279+
// `SELECT id, COUNT(*) FROM ... GROUP BY id`.
280+
#[derive(FromRow)]
281+
struct Aggregate {
282+
#[hyperdb(index = 0)]
283+
id: i32,
284+
#[hyperdb(index = 1)]
285+
total: Option<i64>,
286+
}
287+
```
288+
289+
- **`RowAccessor<'a>`** is the parameter type of the new `FromRow::from_row`. It exposes:
290+
- `get<T>(name: &str) -> Result<T>` — required field; missing/NULL/type-mismatch return `Error::Column`.
291+
- `get_opt<T>(name: &str) -> Result<Option<T>>` — optional field; NULL becomes `None`.
292+
- `position<T>(idx: usize) -> Result<T>` — positional access; out-of-range returns `Error::ColumnIndexOutOfBounds`.
293+
- `position_opt<T>(idx: usize) -> Result<Option<T>>` — positional access; NULL becomes `None`.
294+
295+
- **`Row::get_by_name<T>(name)`** does the same name-based lookup but on a single `Row` (no cached lookup map). Convenient for hand-coded paths that don't go through `FromRow`. Doc warns that it's a linear scan; recommends `#[derive(FromRow)]` or `fetch_*_as` for hot paths.
296+
297+
### Migration recipes
298+
299+
#### Hand-written `FromRow` impl
300+
301+
```rust
302+
// Before
303+
impl FromRow for User {
304+
fn from_row(row: &Row) -> Result<Self> {
305+
Ok(User {
306+
id: row.get::<i32>(0).ok_or_else(|| Error::conversion("NULL id"))?,
307+
name: row.get::<String>(1).unwrap_or_default(),
308+
})
309+
}
310+
}
311+
312+
// After
313+
impl FromRow for User {
314+
fn from_row(row: RowAccessor<'_>) -> Result<Self> {
315+
Ok(User {
316+
id: row.get("id")?,
317+
name: row.get_opt("name")?.unwrap_or_default(),
318+
})
319+
}
320+
}
321+
```
322+
323+
The new shape is shorter, more readable, and decouples your code from column position — reordering `SELECT` columns no longer breaks your impl.
324+
325+
#### Tuple destructuring (deleted)
326+
327+
```rust
328+
// Before — blanket tuple impl
329+
let row = conn.fetch_one("SELECT id, name FROM users")?;
330+
let (id, name): (Option<i32>, Option<String>) = FromRow::from_row(&row)?;
331+
332+
// After — define a struct
333+
#[derive(FromRow)]
334+
struct User { id: Option<i32>, name: Option<String> }
335+
let user: User = conn.fetch_one_as("SELECT id, name FROM users")?;
336+
```
337+
338+
Or, if you really want positional access without a struct, use `Row::get(idx)` directly:
339+
340+
```rust
341+
let row = conn.fetch_one("SELECT id, name FROM users")?;
342+
let id: Option<i32> = row.get(0);
343+
let name: Option<String> = row.get(1);
344+
```
345+
346+
#### Direct `T::from_row(&row)` calls
347+
348+
If you previously called `T::from_row(&row)` directly (outside `fetch_*_as`), the new signature requires a `RowAccessor`. Easiest migration: use `fetch_one_as` / `fetch_all_as` instead, which build the cached lookup for you.
349+
350+
If you must construct a `RowAccessor` yourself (e.g. processing rows from a custom source), the constructor is `pub(crate)`. File an issue if you need this surfaced — current direction is to keep `RowAccessor` construction internal so the cache lifetime stays tied to `fetch_*_as`.
351+
352+
### Errors
353+
354+
The derive and `RowAccessor` accessors return `Error::Column { name, kind }` for column-access failures, where `ColumnErrorKind` is one of:
355+
356+
- `Missing` — column with that name not in the result schema
357+
- `Null` — required field, but the cell is SQL `NULL`
358+
- `TypeMismatch { expected, actual }` — the cell value couldn't be decoded as `T`
359+
360+
`Error::ColumnIndexOutOfBounds { idx, column_count }` is returned by `RowAccessor::position` when `idx` is out of range.
361+
362+
These variants were shipped in `#70` so this PR doesn't re-break the error type.
363+
364+
### Performance note
365+
366+
`fetch_*_as` builds a `HashMap<&str, usize>` once per query (O(N) in the column count). Each row's `RowAccessor::get(name)` then runs a single hash lookup followed by typed access — O(1) per field per row. This is strictly better than the previous behavior, where a hand-written impl using `try_get(idx, name)` had to know column positions hard-coded.
367+
368+
For one-off named access on a `Row` outside `fetch_*_as`, `Row::get_by_name` is a linear scan over `ResultSchema::column_index`. For hot paths (many rows × many fields), prefer `#[derive(FromRow)]`.
369+
370+
### `hyperdb-api-derive` crate
371+
372+
The proc-macro lives in a new `hyperdb-api-derive` workspace crate (Rust requires proc-macro code to live in its own `proc-macro = true` crate). It's re-exported from `hyperdb-api`, so callers don't need a direct dependency — same pattern as serde / thiserror. **Don't add `hyperdb-api-derive` to your `Cargo.toml`**; just `use hyperdb_api::FromRow;`.
373+
374+
---
375+
246376
## #70 (continued) — Ergonomic constructors across all workspace error types
247377

248378
The same ergonomic-constructor pattern was applied to every error type in the workspace that user code might construct, so call sites no longer need `.to_string()` ceremony for string-literal arguments.

‎hyperdb-api-derive/Cargo.toml‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
[package]
2+
name = "hyperdb-api-derive"
3+
version.workspace = true
4+
edition.workspace = true
5+
rust-version.workspace = true
6+
description = "Procedural macros for hyperdb-api (FromRow derive)"
7+
license.workspace = true
8+
repository.workspace = true
9+
homepage.workspace = true
10+
readme = "README.md"
11+
keywords = ["database", "hyper", "derive", "proc-macro"]
12+
categories = ["database"]
13+
14+
[lib]
15+
proc-macro = true
16+
17+
[dependencies]
18+
syn = { version = "2", features = ["full"] }
19+
quote = "1"
20+
proc-macro2 = "1"
21+
22+
[lints]
23+
workspace = true

‎hyperdb-api-derive/README.md‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
# hyperdb-api-derive
2+
3+
⚠️ **This crate is an implementation detail of
4+
[`hyperdb-api`](https://crates.io/crates/hyperdb-api).**
5+
Use `hyperdb-api` directly; don't add `hyperdb-api-derive` to your dependencies.
6+
7+
This crate provides the procedural macros that `hyperdb-api` re-exports
8+
(currently just `#[derive(FromRow)]`). Use them through `hyperdb-api`.
9+
10+
## Quick example
11+
12+
```rust
13+
use hyperdb_api::{Connection, ConnectionBuilder, FromRow, Result};
14+
15+
#[derive(Debug, FromRow)]
16+
struct User {
17+
id: i32,
18+
name: String,
19+
#[hyperdb(rename = "email_address")]
20+
email: Option<String>,
21+
}
22+
23+
fn main() -> Result<()> {
24+
let conn: Connection = ConnectionBuilder::new("localhost:7483").connect()?;
25+
26+
let alice: User = conn.fetch_one_as("SELECT * FROM users WHERE id = 1")?;
27+
println!("{alice:?}");
28+
29+
let everyone: Vec<User> = conn.fetch_all_as("SELECT * FROM users ORDER BY id")?;
30+
for u in &everyone {
31+
println!("{u:?}");
32+
}
33+
Ok(())
34+
}
35+
```
36+
37+
## Field-to-column mapping rules
38+
39+
- **Field name = column name** by default. A field `name: String` reads
40+
the column called `name`.
41+
- **`#[hyperdb(rename = "...")]`** overrides the column name. Use this
42+
when the SQL column doesn't match the Rust field — snake_case
43+
mismatches, reserved words, columns named after Rust keywords, etc.
44+
- **`#[hyperdb(index = N)]`** switches that field to positional access
45+
at column `N` (zero-based). Useful for queries with computed/unnamed
46+
columns where there's no stable name to match — e.g. `SELECT id,
47+
COUNT(*) FROM ... GROUP BY id`. Mutually exclusive with `rename`.
48+
- **`Option<T>` fields tolerate SQL NULL** (become `None`). Non-`Option`
49+
fields error with `Error::Column { kind: Null, .. }` if the cell is
50+
NULL.
51+
- **Missing columns** (the column isn't in the result schema) error
52+
with `Error::Column { kind: Missing, .. }` at fetch time.
53+
54+
```rust
55+
#[derive(FromRow)]
56+
struct Aggregate {
57+
#[hyperdb(index = 0)]
58+
id: i32,
59+
#[hyperdb(index = 1)]
60+
total: Option<i64>,
61+
}
62+
// Works against `SELECT id, COUNT(*) FROM ... GROUP BY id`
63+
```
64+
65+
## When to hand-write `FromRow` instead
66+
67+
The derive emits a straightforward mapping. If you need transformation
68+
in the mapping — parsing a string column into a Rust enum, splitting a
69+
single column into multiple fields, defaulting NULLs to a non-`Option`
70+
value, etc. — write the impl directly:
71+
72+
```rust
73+
impl FromRow for User {
74+
fn from_row(row: hyperdb_api::RowAccessor<'_>) -> Result<Self> {
75+
Ok(User {
76+
id: row.get("id")?,
77+
name: row.get("full_name")?, // SQL column "full_name"
78+
email: row.get_opt("email_address")?,
79+
})
80+
}
81+
}
82+
```
83+
84+
In a hand-written impl, the string passed to `row.get(...)` /
85+
`row.get_opt(...)` *is* the column name — no `#[hyperdb(rename)]` is
86+
needed, since you're spelling the column out yourself. Your `SELECT`
87+
just needs to actually return that column (use `AS full_name` if the
88+
underlying table column has a different name).
89+
90+
### `RowAccessor` accessor cheat sheet
91+
92+
`RowAccessor` exposes four accessors. Pick by access mode (name vs.
93+
index) and required vs. optional. Indices are **zero-based**.
94+
95+
| | Required (`T`) | Optional (`Option<T>`) |
96+
|---|---|---|
97+
| **By name** | `row.get(name)?` | `row.get_opt(name)?` |
98+
| **By index** | `row.position(idx)?` | `row.position_opt(idx)?` |
99+
100+
NULL handling differs between the two columns of the table:
101+
102+
- **`get` / `position`** — NULL errors with `Error::Column { kind: Null, .. }`.
103+
Use these for required fields where NULL is a problem.
104+
- **`get_opt` / `position_opt`** — NULL becomes `Ok(None)`. Use these for
105+
fields whose Rust type is `Option<T>`.
106+
107+
The Rust field type and the accessor must agree: `position` returns
108+
`Result<T>`, `position_opt` returns `Result<Option<T>>`. Mixing them
109+
across types is a compile error, not a runtime mismatch:
110+
111+
```rust
112+
// ✅ field type matches accessor return
113+
let email: Option<String> = row.position_opt(2)?;
114+
let id: i32 = row.position(0)?;
115+
116+
// ❌ compile errors
117+
let email: Option<String> = row.position(2)?; // returns T, not Option<T>
118+
let id: i32 = row.position_opt(0)?; // returns Option<T>
119+
```
120+
121+
If you want to silently default a NULL on a non-`Option` field, opt in
122+
explicitly:
123+
124+
```rust
125+
name: row.position_opt(1)?.unwrap_or_default(), // NULL → ""
126+
```
127+
128+
See the [`hyperdb-api` docs](https://docs.rs/hyperdb-api) for full usage.
129+
130+
This crate has no stable API. Breaking changes land here without a major
131+
version bump of `hyperdb-api-derive`; your build may break on any
132+
`hyperdb-api` patch release if you depend on `hyperdb-api-derive` directly.

0 commit comments

Comments
 (0)