feat(mapper): support ,omitzero tag option#50
Conversation
omitempty conflates nil and len-0 slices/maps, both skipped. That makes
it impossible to clear a NOT NULL DEFAULT '{}' array column to empty via
Save/Map-driven UPDATE: the empty slice is treated as zero, the column
is omitted, and the row keeps its stale value.
omitzero, mirroring encoding/json's Go 1.24+ semantics, skips only the
type's true zero: nil pointers, nil slices, nil maps, primitive zeros,
or any type whose IsZero() reports true. A non-nil empty slice/map is
not zero and stays in the UPDATE SET clause, so a clear actually
clears.
- mapper.go: parse omitzero alongside omitempty; for slices/maps the
legacy "empty == zero" rule only applies to omitempty
- mapper_test.go: regression matrix for both options across nil, empty,
populated, time.IsZero, primitives, IncludeZeroed and IncludeNil
- doc comment on Map summarizes both options
The first cut of ,omitzero accidentally rewrote omitempty behavior for two kinds it had no business touching: - map fields: pre-omitzero, omitempty only skipped a nil map (DeepEqual nil-typed-nil vs non-nil-empty is false). A non-nil empty map stayed, so "clear my jsonb" on UPDATE actually cleared. The new generic slice+map branch made empty maps skip, silently breaking that. - array fields: pre-omitzero, omitempty skipped an all-zero array via the DeepEqual fallback (think [16]byte UUIDs, [32]byte hashes). The new array branch checked Len()==0, which is only ever true for the degenerate [0]T, so all-zero arrays of any normal size started emitting where they used to skip. Split slice, map and array into separate branches with the right legacy rules; omitzero keeps its nil-only / IsZero()-true semantics on top. The compose 'skip = (isEmpty if omitempty) or (isStrictZero if omitzero)' falls out cleanly. Tests: - regression guards for the two breakages above - omitzero on a struct without IsZero() (DeepEqual fallback) - both tags on one field (omitempty wins, the broader rule) - map-as-record path unchanged - godoc tightened to 7 lines; drop em-dash Addresses self-review and Codex review on pgkit#50.
|
Pushed Root cause for both: the first cut added kind-specific branches for
Fix: split slice/map/array into separate branches with the right legacy rules. } else if fld.Kind() == reflect.Slice {
if fld.IsNil() {
isEmpty, isStrictZero = true, true
} else if fld.Len() == 0 {
isEmpty = true
}
} else if fld.Kind() == reflect.Map {
if fld.IsNil() {
isEmpty, isStrictZero = true, true
}
} else if fld.Kind() == reflect.Array {
if fld.IsZero() {
isEmpty, isStrictZero = true, true
}
}Extra coverage from this round:
15 subtests now. Also tightened the godoc to 7 lines (was 17) and dropped the em-dash. Thanks for the catch. |
Project style prefers switch when every branch falls through to the same downstream code. The hasIsZero check stays as a pre-guard since it overrides kind-specific handling (a slice type with its own IsZero should use that, not the slice arm).
The previous fix-up rewrote the array branch to skip on fld.IsZero() for both flags. That over-corrected: pre-omitzero, the original code's else-if chain entered the Array|Slice arm and only set isZero on Len()==0. Normal-length all-zero arrays ([16]byte UUIDs, [32]byte hashes, ed25519 keys) were always emitted under omitempty. Split the array branch so omitempty stays Len()==0 (matches legacy) and omitzero gets the strict IsZero() rule (Go 1.24+ json semantic). Tests: - TestMap_OmitEmpty_AllZeroArray flipped to assert pre-PR behavior (all-zero [16]byte kept). Was previously asserting the regression. - TestMap_OmitZero_AllZeroArray added for the strict-zero side. Caught by an adversarial Codex pass on pgkit#50; the earlier self-review traced the pre-PR else-if chain incorrectly and asked for a "fix" that wasn't actually a regression.
Trim WHAT-padding and em-dashes per project style: exported godoc keeps its 1-line WHAT, multi-line content carries WHY only. Test-helper and test-comment edits follow the same rule. No behavior changes; 16 subtests still green.
MapWithOptions was doing struct-walking, zero-classification, and skip composition in one stretch. Pull the classification into a free function so each case becomes a direct return instead of poking shared mutable flags from nested branches. Call site shrinks to one line and the asymmetric slice case (non-nil empty is empty but not strict-zero) is explicit instead of inferred. No behavior changes; 16 subtests still green, full pgkit test suite green.
david-littlefarmer
left a comment
david-littlefarmer
left a comment
There was a problem hiding this comment.
LGTM, just to note for description:
map{} under omitempty should be ✓ (writes empty), not ✗ skipped.
But TestMap_OmitEmpty_EmptyNonNilMap tests covers this.
OR-composing the skip rule made the winner type-dependent (slices got omitempty, all-zero fixed-size arrays got omitzero), silently reversing the array preservation in 1550bde. No coherent precedence to defend, so fail fast at Map time instead of shipping a footgun. Caught by the third adversarial Codex pass.
Previously the nil-pointer branch lumped ,omitzero and ,omitempty into the same forced-include behavior: both emitted sqlDefault. That made "clear a nullable column to NULL via PATCH" inexpressible through any tagged field. ,omitempty keeps its legacy DEFAULT-on-IncludeNil behavior (would have been a breaking change otherwise, even though the option currently has no callers). ,omitzero is the strict tag and surfaces a forced-include as literal NULL — matching the read "user asked to show the nil, show the nil." Documented in the Map godoc. Caught by the third adversarial Codex pass.
|
Pushed Finding #2 —
Fix: reject the combination at Map time with Finding #1 — This one had a real design question behind it. My initial push-back was "preserve symmetry with With the symmetry-preservation argument weak, Codex's read wins: under Fix in the Branch state (8 commits, additive)
16 subtests, full pgkit suite green, |
InsertRecords froze the column list from row 0 and appended values for later rows without re-checking shape. Fine when every row produces the same column set, but ,omitzero (and pre-existing ,omitempty on map fields) make per-row shape vary: nil slice/map gets skipped, non-nil empty gets emitted. Mixed batches produced malformed multi-row SQL that only surfaced as a Postgres parse error at exec time. Track the row-0 column set, slices.Equal-check every subsequent row, return a build-time error on drift. Tests cover both the new ,omitzero path and the latent ,omitempty + map path. Caught by the fourth adversarial Codex pass.
A record whose mapped fields all get skipped (every field tagged ,omitempty / ,omitzero with zero values) makes Map return no columns. squirrel then emits INSERT INTO t VALUES () which Postgres rejects at parse time. Fail fast at builder time and point at sq.Expr as the escape for an all-default INSERT. Native INSERT ... DEFAULT VALUES support is tracked in #51. Caught by the fifth adversarial Codex pass.
|
Pushed Finding (round 5): if every field on a record is tagged Light fix here: reject empty-column records at builder time with a clear error pointing at if len(cols) == 0 {
return InsertBuilder{InsertBuilder: insert, err: wrapErr(fmt.Errorf(
"Map returned no columns for %T; for an all-default INSERT use sq.Expr(\"INSERT INTO %s DEFAULT VALUES\")",
record, tableName,
))}
}Tests: Native support deferred to goware/pgkit#51 — supporting Branch state (10 commits, ready for review):
17 subtests covering the new mapper + builder behavior, full pgkit test suite green against PostgreSQL 18.3, Ready for human review on the goware side. |
Closes #53. Follow-up to #50. with a build-time "record N columns differ from record 0" error. That was the conservative answer: it stopped squirrel from emitting malformed multi-row SQL where row widths didn't match. But it's restrictive. Realistic ,omitzero (#50) and legacy ,omitempty on map fields produce heterogeneous batches constantly. Forcing callers to split into per-row InsertRecord calls kills the point of batching. Replace the drift check with union-by-name: walk rows once to compute the column union and a per-row map[string]any of present columns, then emit Columns(allCols...) with each row padded to the union via sq.Expr("DEFAULT") in any slot the row skipped. PostgreSQL accepts DEFAULT in any VALUES position, so the resulting SQL is valid for every shape the union covers. The per-row "empty record" rejection from #50 also goes away — a row with no own columns can be all-DEFAULT *precisely because* another row contributes the union. Only the whole-batch empty case still errors, with a hint pointing at sq.Expr (matches the existing #50 hint shape on the InsertRecord single-row path). Tests: rewrote the drift-rejection tests to assert union+DEFAULT SQL including args; added empty-row-mixed-with-non-empty, heterogeneous map records, and a real PG round-trip against a new mixed_shape table that proves each row lands with the right mix of caller values, DB defaults, and NULLs. Planned via three Codex review rounds (per-row vs whole-batch reject flipped, map-padding by name vs positional, test plan gaps, sq.Expr hint vs unmerged #52 reference). Plan at tmp/insertrecords-mixed-shape/2026-06-02-plan.md.
Closes #53. Follow-up to #50. Replace the InsertRecords column-drift check with union-by-name: walk rows once to compute the column union and a per-row map of present columns, then emit Columns(allCols...) with each row padded to the union via DEFAULT where the row skipped a column. The per-row empty-record rejection from #50 goes away because an empty row can be all-DEFAULT when another row contributes the union. Only the whole-batch empty case still errors, now pointing callers at SQL.InsertDefaults in a loop after rebasing over the dedicated DEFAULT VALUES builder. Tests cover uniform and mixed shapes, omitzero slices, mixed maps, empty rows with a non-empty union, all-empty rejection, map records, InsertDefaults SQL construction, and PG roundtrips for both InsertDefaults and mixed-shape inserts.
2 participants
omitemptyon a[]stringfield paired withNOT NULL DEFAULT '{}'looks like a clean way to get an INSERT default, but it silently breaks the UPDATE path: pgkit treats an empty slice as zero and drops the column fromSET, so a "clear my list" call no-ops and the row keeps its stale entries. The only workarounds today are anormalizehelper that coercesnil → []string{}before Save (and droppingomitempty), or post-Update re-Get assertions in every test that touches the column.Adding
,omitzerowith the same semantics asencoding/json's Go 1.24+ option fixes this cleanly: only the type's true zero is skipped, so a non-nil empty slice/map stays in the UPDATE SET and the clear actually clears.nilstill falls through to the column DEFAULT on INSERT.omitemptyomitzeronilslice/map[]T{}/map{}nilptr""/0/falsetime.Time{}(IsZero)omitemptybehavior is untouched, so this is additive.Test plan
make db-reset test-allagainst PostgreSQL 18.3 (devbox-postgres) — all 6 packages green (pgkit/v2,db,dbtype,internal/reflectx,pgerr,tests)mapper_test.gocovers nil / empty / populated slices and maps, primitive zeros,time.IsZero, and theIncludeZeroed/IncludeNilknobs for both tag optionsgo vet ./...cleanNotes
NOT NULL DEFAULT '{}'allowed_originsTEXT[] column needs both INSERT-default and clearable-via-UPDATE semantics. The current workaround there is anormalizehelper plus droppingomitempty; once this lands and is released, that file collapses to a single,omitzerotag.encoding/json'somitzeroso callers don't have to learn pgkit-specific vocabulary.