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. |
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.