[AdamMagued]

Fixes #187

Summary

Add support for expanding child slice and slice pointer fields in Struct to map one-to-many relationship rows into parent structs with nested slices.

Key Changes

• Field parsing in structfields.go:
  • Detect slice and slice pointer struct types in canExpandType.
  • Track SliceIndex and SliceDepth for child fields during parser recursion.
  • Forward field tags and inheritance down to child slice fields.
• Mapping and address assignment in struct.go:
  • In Addr, allocate and append child slice elements dynamically per row, supporting []*T, []T, *[]*T, and *[]T.
  • In Values, extract values from the most recent child element, returning nil when the slice is empty.
  • In Update, omit fields with SliceDepth > 0 to prevent mutating child tables in parent update statements.
  • Provide SliceFields and SliceColumns to inspect projected slice fields.
  • Provide PruneEmptySliceElements to strip empty child elements produced by LEFT JOIN queries where child fields scanned NULL.
  • Implement ScanJoined and ScanJoinedWithCols for scanning scannable rows directly into aggregate structs with automatic empty record pruning.
• Tests:
  • Add unit tests covering column generation, multi-row address extraction, values serialization, pointer-to-slice fields, joined row scanning, and empty element pruning.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[huandu]

Thanks @AdamMagued for the contribution. Mapping joined rows into nested slices is non-trivial reflection work, and the API surface you added (SliceFields, SliceColumns, ScanJoined) shows real thought about how users would consume it.

I want to be candid about why #187 has stayed open for so long. It is not that the issue was forgotten. It is that I have never found a way to handle JOIN deserialization that is both simple enough for this library — the ORM side here is deliberately thin and only builds on database/sql — and free of incorrect behavior. My conclusion so far is that such a design may not exist at this level: as soon as a struct field accumulates child rows, someone has to decide who owns result grouping, how NULL child rows (LEFT JOIN) are detected, what happens with two child slices (the cartesian product), and how to do all of that without changing the meaning of methods that already exist. I would rather keep the issue open as a documented limitation than ship a mapping layer that can quietly return wrong data.

One more principle matters here. Struct is deliberately designed as a stateless, thread-safe value: there is not a single lock in it, and the documented contract is "All methods in Struct are thread-safe. We can define a global variable to hold a Struct and use it in any goroutine." That is only possible because a Struct never accumulates per-query state. Join result shaping inherently needs state — which child element am I appending to, where does the next parent begin — and this PR pushes that state into Addr, a method on that shared, stateless value. I want to keep Struct state-free: if a mapping helper is ever added, the state belongs in a separate, explicitly created value owned by the caller (one per query), not in Struct and not implicitly in Addr.

This PR lands right in that gap. It solves a narrow part of the problem — one parent and one child slice — while introducing new problems and changing existing behavior:

• Struct.Addr becomes stateful: it appends a new child element on every call, so a previously pure and repeatable method on a stateless type now depends on call history; reusing a struct across rows silently accumulates children, and there is no way to reset.
• ScanJoined has no parent-key grouping, so rows belonging to different parents are merged into one struct, and with two child slices the rows get paired up and inflated.
• PruneEmptySliceElements infers "this child row was NULL" from "all fields are zero"; with a non-pointer child field a real NULL fails at scan time, and legitimate all-zero rows are dropped.
• Values() returns the last child element's values while Update() skips those same fields, and the INSERT path still treats the slice as an ordinary column.
• Backward compatibility: named slice types implementing sql.Scanner/driver.Valuer are now expanded into child columns, and fieldtag propagates from a nested struct field to its children, which changes the columns produced by WithTag/WithoutTag (the latter can silently drop columns).

Because this changes the behavior of existing public API while still not being correct for the general one-to-many case, I do not think we can merge it as it is. I would like to ask you to reconsider the approach: keep slice expansion explicitly opt-in so existing structs are unaffected, and move the aggregation into a separate, explicit API that owns the grouping key instead of extending Addr. If it turns out this cannot be both simple and correct, I am perfectly happy to leave #187 open and point users at a small grouping loop built on the existing primitives — that is a better outcome than a mapping helper that can return silently wrong results.

I have left inline comments on the specific spots. Thanks again for taking the time to work on this.

[huandu]

Thanks for the work here. I left inline notes on the individual spots; my overall reasoning is in my summary comment on this PR.

[huandu]

Nested slices (a slice of structs that itself contains a slice of structs) are accepted by the parser — Columns() returns [id orders.id items.id] and SliceFields() lists both — but Addr returns nil at scan time because the intermediate slice has no element yet, so ScanJoined fails with the generic cannot get scan addresses for .... Either support this shape or reject it in NewStruct with a clear error; silently accepting it and failing later is the worst combination.

[huandu]

Propagating the parent's fieldtag to child fields changes the column set of existing structs, and not only for slice expansions. On master, for a nested struct field tagged g1, WithoutTag("g1").Columns() returns [id in.a b]; with this change it returns [id b], i.e. columns are silently dropped from the generated SELECT/UPDATE. WithTag results change as well. If the inheritance is needed for the slice case, please restrict it to slice children and add tests for both WithTag and WithoutTag (plus a README note).

[huandu]

A slice field whose type itself implements sql.Scanner/driver.Valuer is now expanded, because this only inspects the element type and never the field type:

type Addresses []Address   // implements Scan/Value, e.g. for a JSON/array column
type Row struct {
    Addr Addresses          // db:"addr"
}

For that struct, Columns() goes from [id addr] to [id addr.city addr.zip], and Addr()/Values() change with it (verified against current master). That is a silent break for existing users — they would have to add fieldopt:"noexpand" to get the old behavior back. canExpandStructType already refuses types implementing Scanner/Valuer; this path should apply the same check (including for *[]T), or slice expansion should be opt-in.

[huandu]

Skipping child slice fields here is a sensible improvement, but the INSERT path is left inconsistent: buildColsAndValuesForTag still emits the slice field as an ordinary column, so InsertInto produces INSERT INTO t (id, kids) VALUES (?, ?) with the whole slice as the second argument. (This happens on master too, so it is not a regression — but the same guard should apply to both write paths.)

[huandu]

Addr is now stateful: every call appends a new child element to each slice field, so a method that used to be a pure "give me the scan destinations for this struct" conversion now depends on how many times it has been called. Calling it twice for the same row (a retry, or reusing the struct for the next parent row) silently duplicates children, and there is no way to reset. This matters beyond the individual bug: Struct is documented as stateless and thread-safe ("All methods in Struct are thread-safe. We can define a global variable to hold a Struct and use it in any goroutine."), and it holds no lock precisely because it never accumulates per-query state. Aggregation state like this needs an explicit owner — a separate value created per query — instead of living implicitly in Addr.

[huandu]

SliceFields/SliceColumns recover the slice field name by string-splitting sf.Alias on the first .. That is fragile: the alias is not a reliable identifier (two slice fields sharing a db tag in different nested structs produce the same prefix, so the seen dedup here collapses them into one name while SliceColumns matches both), whereas the parser already knows the slice field and its SliceIndex. Deriving this from the parser would be robust and would let SliceFields/SliceColumns share one implementation.

[huandu]

Using "all fields are zero" as the test for "this LEFT JOIN child row was NULL" has two problems: (1) with a real driver, scanning NULL into a non-pointer child field (say uint64) fails with converting NULL to uint64 is unsupported before any pruning can happen, so this only works when the child struct uses pointer/Null* fields — which is not documented; (2) it also removes legitimate child rows whose selected columns happen to be zero. Judging NULL from a designated join-key column (or letting the caller pass one) would be reliable. Note the new test for this path feeds zero values through a mock that skips nil, so the LEFT JOIN case is effectively untested.

[huandu]

ScanJoined folds every row into a single destination struct with no grouping by a parent key. Feeding it rows for two parents (user 1: 1 row, user 2: 2 rows) yields ID=2, Name=Bob, len(Expenses)=3 — user 1's child row ends up under user 2. With two child slices the rows are paired up too (3 expenses × 2 roles => both slices end up with 6 elements), because each row appends to every slice. So the API is only incidentally correct for "one parent row set + one child slice", which the doc comment does not state. Making this correct needs a collector that groups by an explicit key and returns a slice of parents; otherwise grouping belongs to the caller.

[huandu]

Small but visible: errors here are prefixed sqlbuilder:, while the rest of the package uses go-sqlbuilder: — e.g. ErrInterpolateNotImplemented = errors.New("go-sqlbuilder: interpolation for this flavor is not implemented") in flavor.go. Please make the prefix consistent.

[huandu]

This example builds SQL no server will accept: the slice field's db tag is expenses, so the columns are emitted as expenses.id, ..., but the table is aliased to e, and expenses is then not a valid table reference (PostgreSQL: missing FROM-clause entry for table "expenses"). The convention documented in the README is that the db tag is the table alias (db:"post" with Join("posts post", ...)), so this should be db:"e" or a join without an alias. As a copy-paste template it should be fixed — and the new API needs a README section, since docs are currently untouched in this PR.

[coveralls]

coverage: 95.043% (-1.1%) from 96.17% — AdamMagued:fix-issue-187 into huandu:master