From bfd0b8bc224ad5155fed65cd55ff0e5563878522 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 13:46:58 +0200 Subject: [PATCH 01/10] feat(mapper): support ,omitzero tag option 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 --- mapper.go | 49 +++++++++++---- mapper_test.go | 161 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 11 deletions(-) create mode 100644 mapper_test.go diff --git a/mapper.go b/mapper.go index ca15e08..0b20170 100644 --- a/mapper.go +++ b/mapper.go @@ -38,8 +38,21 @@ type MapOptions struct { // which can be directly passed to a query executor. This allows you to use structs/objects // to build easy insert/update queries without having to specify the column names manually. // The mapper works by reading the column names from a struct fields `db:""` struct tag. -// If you specify `,omitempty` as a tag option, then it will omit the column from the list, -// which allows the database to take over and use its default value. +// +// Two tag options control how zero values are emitted: +// +// - `,omitempty` omits the column when the field's value is empty: a nil +// pointer, an empty string, a zero number, a zero-length slice or map +// (nil OR non-nil), or any type whose IsZero() reports true. This lets +// the database fall back to a column DEFAULT on INSERT, but on UPDATE +// it silently leaves the column untouched — which prevents clearing a +// NOT NULL DEFAULT '{}' array to empty. +// - `,omitzero` omits the column only when the field holds the type's +// true zero value: a nil pointer, a nil slice or map (non-nil empty is +// INCLUDED), or any type whose IsZero() reports true. Use this on +// columns you want to be able to clear to empty via UPDATE while still +// getting the DEFAULT on INSERT when the field is nil. Mirrors the +// semantics of encoding/json's `omitzero` (Go 1.24+). func Map(record interface{}) ([]string, []interface{}, error) { return MapWithOptions(record, nil) } @@ -86,15 +99,16 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf // Field options _, tagOmitEmpty := fi.Options["omitempty"] + _, tagOmitZero := fi.Options["omitzero"] fld := reflectx.FieldByIndexesReadOnly(recordV, fi.Index) if fld.Kind() == reflect.Ptr && fld.IsNil() { - if tagOmitEmpty && !options.IncludeNil { + if (tagOmitEmpty || tagOmitZero) && !options.IncludeNil { continue } fv.fields = append(fv.fields, fi.Name) - if tagOmitEmpty { + if tagOmitEmpty || tagOmitZero { fv.values = append(fv.values, sqlDefault) } else { fv.values = append(fv.values, nil) @@ -104,20 +118,33 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf value := fld.Interface() - isZero := false + // isEmpty matches the legacy omitempty rule: nil/empty slices and + // maps both count. isStrictZero matches Go 1.24+ json's omitzero: + // only the type's true zero value (nil slice/map, not empty). + var isEmpty, isStrictZero bool if t, ok := fld.Interface().(hasIsZero); ok { if t.IsZero() { - isZero = true + isEmpty, isStrictZero = true, true } - } else if fld.Kind() == reflect.Array || fld.Kind() == reflect.Slice { + } else if fld.Kind() == reflect.Slice || fld.Kind() == reflect.Map { + if fld.IsNil() { + isEmpty, isStrictZero = true, true + } else if fld.Len() == 0 { + isEmpty = true + } + } else if fld.Kind() == reflect.Array { if fld.Len() == 0 { - isZero = true + isEmpty = true + } + if fld.IsZero() { + isStrictZero = true } } else if reflect.DeepEqual(fi.Zero.Interface(), value) { - isZero = true + isEmpty, isStrictZero = true, true } - if isZero && tagOmitEmpty && !options.IncludeZeroed { + skip := (isEmpty && tagOmitEmpty) || (isStrictZero && tagOmitZero) + if skip && !options.IncludeZeroed { continue } @@ -127,7 +154,7 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf // return nil, nil, err // } v := value - if isZero && tagOmitEmpty { + if skip { v = sqlDefault } fv.values = append(fv.values, v) diff --git a/mapper_test.go b/mapper_test.go new file mode 100644 index 0000000..0ef876d --- /dev/null +++ b/mapper_test.go @@ -0,0 +1,161 @@ +package pgkit_test + +import ( + "testing" + "time" + + sq "github.com/Masterminds/squirrel" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/goware/pgkit/v2" +) + +// mapFields runs Map and returns a column->value lookup for easier asserts. +func mapFields(t *testing.T, record any) map[string]any { + t.Helper() + cols, vals, err := pgkit.Map(record) + require.NoError(t, err) + require.Equal(t, len(cols), len(vals)) + out := make(map[string]any, len(cols)) + for i, c := range cols { + out[c] = vals[i] + } + return out +} + +func TestMap_OmitEmpty(t *testing.T) { + type Record struct { + Bare string `db:"bare"` + Str string `db:"str,omitempty"` + Slice []string `db:"slice,omitempty"` + PtrStr *string `db:"ptr_str,omitempty"` + PtrSet *string `db:"ptr_set,omitempty"` + Number int `db:"number,omitempty"` + Filled []string `db:"filled,omitempty"` + Boolean bool `db:"boolean,omitempty"` + } + + set := "value" + r := &Record{PtrSet: &set, Filled: []string{"a"}} + + got := mapFields(t, r) + + assert.Contains(t, got, "bare", "bare column always present") + assert.NotContains(t, got, "str", "zero string skipped") + assert.NotContains(t, got, "slice", "nil slice skipped") + assert.NotContains(t, got, "ptr_str", "nil pointer skipped") + assert.NotContains(t, got, "number", "zero int skipped") + assert.NotContains(t, got, "boolean", "false bool skipped") + assert.Contains(t, got, "ptr_set", "set pointer included") + assert.Equal(t, "value", *(got["ptr_set"].(*string))) + assert.Contains(t, got, "filled", "populated slice included") +} + +func TestMap_OmitEmpty_EmptyNonNilSlice(t *testing.T) { + // Regression guard: omitempty skips both nil and len-0 slices. + type Record struct { + Slice []string `db:"slice,omitempty"` + } + got := mapFields(t, &Record{Slice: []string{}}) + assert.NotContains(t, got, "slice", "omitempty drops empty-but-non-nil slice") +} + +func TestMap_OmitZero_NilSlice(t *testing.T) { + // nil slice is the zero value of a slice type: omitzero skips the column + // so the DB DEFAULT applies on INSERT and the column is left untouched on + // UPDATE. + type Record struct { + Slice []string `db:"slice,omitzero"` + } + got := mapFields(t, &Record{Slice: nil}) + assert.NotContains(t, got, "slice", "omitzero drops nil slice") +} + +func TestMap_OmitZero_EmptyNonNilSlice(t *testing.T) { + // The behavioral split versus omitempty: a non-nil empty slice is NOT + // the zero value, so omitzero includes the column. This is what lets a + // PATCH-style Update clear a NOT NULL DEFAULT '{}' array column without + // the silent no-op trap omitempty has. + type Record struct { + Slice []string `db:"slice,omitzero"` + } + got := mapFields(t, &Record{Slice: []string{}}) + require.Contains(t, got, "slice", "omitzero keeps empty-but-non-nil slice") + v, ok := got["slice"].([]string) + require.True(t, ok, "value type preserved") + assert.Len(t, v, 0) +} + +func TestMap_OmitZero_PopulatedSlice(t *testing.T) { + type Record struct { + Slice []string `db:"slice,omitzero"` + } + got := mapFields(t, &Record{Slice: []string{"a", "b"}}) + require.Contains(t, got, "slice") + assert.Equal(t, []string{"a", "b"}, got["slice"]) +} + +func TestMap_OmitZero_PrimitiveZeros(t *testing.T) { + // Primitive zero values follow the same skip rule as omitempty. + type Record struct { + Str string `db:"str,omitzero"` + Number int `db:"number,omitzero"` + Boolean bool `db:"boolean,omitzero"` + PtrStr *string `db:"ptr_str,omitzero"` + } + got := mapFields(t, &Record{}) + assert.NotContains(t, got, "str") + assert.NotContains(t, got, "number") + assert.NotContains(t, got, "boolean") + assert.NotContains(t, got, "ptr_str") +} + +func TestMap_OmitZero_TimeIsZero(t *testing.T) { + // time.Time implements IsZero(); omitzero honors the interface like + // omitempty does, so a zero-valued time is skipped. + type Record struct { + At time.Time `db:"at,omitzero"` + } + got := mapFields(t, &Record{}) + assert.NotContains(t, got, "at", "zero time skipped via IsZero") + + got = mapFields(t, &Record{At: time.Unix(1, 0)}) + assert.Contains(t, got, "at", "non-zero time included") +} + +func TestMap_OmitZero_NilMap(t *testing.T) { + type Record struct { + Tags map[string]string `db:"tags,omitzero"` + } + got := mapFields(t, &Record{}) + assert.NotContains(t, got, "tags", "nil map skipped") + + got = mapFields(t, &Record{Tags: map[string]string{}}) + assert.Contains(t, got, "tags", "empty-but-non-nil map included") +} + +func TestMapWithOptions_OmitZero_IncludeZeroed(t *testing.T) { + // IncludeZeroed surfaces the skipped column with a SQL DEFAULT marker + // instead of dropping it; same contract as omitempty. + type Record struct { + Slice []string `db:"slice,omitzero"` + } + cols, vals, err := pgkit.MapWithOptions(&Record{Slice: nil}, &pgkit.MapOptions{IncludeZeroed: true}) + require.NoError(t, err) + require.Len(t, cols, 1) + require.Equal(t, "slice", cols[0]) + assert.Equal(t, sq.Expr("DEFAULT"), vals[0]) +} + +func TestMapWithOptions_OmitZero_IncludeNil_Pointer(t *testing.T) { + // IncludeNil surfaces a nil pointer with a DEFAULT marker. + type Record struct { + Name *string `db:"name,omitzero"` + } + cols, vals, err := pgkit.MapWithOptions(&Record{}, &pgkit.MapOptions{IncludeNil: true}) + require.NoError(t, err) + require.Len(t, cols, 1) + require.Equal(t, "name", cols[0]) + assert.Equal(t, sq.Expr("DEFAULT"), vals[0]) +} From 24721e664de1c0373d5e20bd14d177315f1ca07e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 14:00:42 +0200 Subject: [PATCH 02/10] fix(mapper): preserve omitempty semantics for maps and arrays 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. --- mapper.go | 45 ++++++++++++++------------------ mapper_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 26 deletions(-) diff --git a/mapper.go b/mapper.go index 0b20170..a05f598 100644 --- a/mapper.go +++ b/mapper.go @@ -34,25 +34,14 @@ type MapOptions struct { IncludeNil bool } -// Map converts a struct object (aka record) to a mapping of column names and values -// which can be directly passed to a query executor. This allows you to use structs/objects -// to build easy insert/update queries without having to specify the column names manually. -// The mapper works by reading the column names from a struct fields `db:""` struct tag. +// Map converts a struct to (column, value) slices using `db:""` struct tags, +// suitable for feeding squirrel's Columns(...).Values(...) or similar. // -// Two tag options control how zero values are emitted: -// -// - `,omitempty` omits the column when the field's value is empty: a nil -// pointer, an empty string, a zero number, a zero-length slice or map -// (nil OR non-nil), or any type whose IsZero() reports true. This lets -// the database fall back to a column DEFAULT on INSERT, but on UPDATE -// it silently leaves the column untouched — which prevents clearing a -// NOT NULL DEFAULT '{}' array to empty. -// - `,omitzero` omits the column only when the field holds the type's -// true zero value: a nil pointer, a nil slice or map (non-nil empty is -// INCLUDED), or any type whose IsZero() reports true. Use this on -// columns you want to be able to clear to empty via UPDATE while still -// getting the DEFAULT on INSERT when the field is nil. Mirrors the -// semantics of encoding/json's `omitzero` (Go 1.24+). +// Tag options: +// - ,omitempty skips zero values, including empty (len==0) slices/maps. +// - ,omitzero skips only the type's true zero value; non-nil empty +// slices/maps stay, so a clear-to-empty UPDATE actually clears the column. +// Matches encoding/json's omitzero (Go 1.24+). func Map(record interface{}) ([]string, []interface{}, error) { return MapWithOptions(record, nil) } @@ -118,26 +107,30 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf value := fld.Interface() - // isEmpty matches the legacy omitempty rule: nil/empty slices and - // maps both count. isStrictZero matches Go 1.24+ json's omitzero: - // only the type's true zero value (nil slice/map, not empty). + // isEmpty matches the legacy omitempty rule: slices count when + // len==0 (nil OR non-nil); maps and arrays only when they equal + // the type's zero (preserving pre-omitzero DeepEqual semantics). + // isStrictZero matches Go 1.24+ json's omitzero: only the type's + // true zero (nil slice/map, all-zero array, primitive zero, or + // IsZero()==true). var isEmpty, isStrictZero bool if t, ok := fld.Interface().(hasIsZero); ok { if t.IsZero() { isEmpty, isStrictZero = true, true } - } else if fld.Kind() == reflect.Slice || fld.Kind() == reflect.Map { + } 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.Array { - 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() { - isStrictZero = true + isEmpty, isStrictZero = true, true } } else if reflect.DeepEqual(fi.Zero.Interface(), value) { isEmpty, isStrictZero = true, true diff --git a/mapper_test.go b/mapper_test.go index 0ef876d..99a846f 100644 --- a/mapper_test.go +++ b/mapper_test.go @@ -61,6 +61,38 @@ func TestMap_OmitEmpty_EmptyNonNilSlice(t *testing.T) { assert.NotContains(t, got, "slice", "omitempty drops empty-but-non-nil slice") } +func TestMap_OmitEmpty_EmptyNonNilMap(t *testing.T) { + // Regression guard: pre-omitzero behavior — omitempty only skipped a + // MAP when it was nil (DeepEqual nil-typed-nil vs non-nil-empty was + // false). A non-nil empty map stayed, so a "clear my jsonb" UPDATE + // actually cleared. Keep that. + type Record struct { + Tags map[string]string `db:"tags,omitempty"` + } + got := mapFields(t, &Record{Tags: nil}) + assert.NotContains(t, got, "tags", "nil map skipped by omitempty") + + got = mapFields(t, &Record{Tags: map[string]string{}}) + require.Contains(t, got, "tags", "empty-but-non-nil map kept by omitempty") + m, ok := got["tags"].(map[string]string) + require.True(t, ok) + assert.Len(t, m, 0) +} + +func TestMap_OmitEmpty_AllZeroArray(t *testing.T) { + // Regression guard: pre-omitzero behavior — omitempty skipped an + // all-zero array via the DeepEqual fallback (e.g. [32]byte{} hashes, + // [16]byte UUIDs). Keep that for fixed-size byte fields. + type Record struct { + Hash [16]byte `db:"hash,omitempty"` + } + got := mapFields(t, &Record{}) + assert.NotContains(t, got, "hash", "all-zero array skipped by omitempty") + + got = mapFields(t, &Record{Hash: [16]byte{1}}) + assert.Contains(t, got, "hash", "non-zero array kept by omitempty") +} + func TestMap_OmitZero_NilSlice(t *testing.T) { // nil slice is the zero value of a slice type: omitzero skips the column // so the DB DEFAULT applies on INSERT and the column is left untouched on @@ -159,3 +191,41 @@ func TestMapWithOptions_OmitZero_IncludeNil_Pointer(t *testing.T) { require.Equal(t, "name", cols[0]) assert.Equal(t, sq.Expr("DEFAULT"), vals[0]) } + +func TestMap_OmitZero_StructWithoutIsZero(t *testing.T) { + // Struct fields without their own IsZero() fall through to the + // DeepEqual-against-the-type's-zero arm, matching encoding/json's + // omitzero semantic for plain structs. + type Inner struct{ A, B int } + type Record struct { + Inner Inner `db:"inner,omitzero"` + } + got := mapFields(t, &Record{}) + assert.NotContains(t, got, "inner", "zero struct value skipped") + + got = mapFields(t, &Record{Inner: Inner{A: 1}}) + assert.Contains(t, got, "inner", "non-zero struct kept") +} + +func TestMap_BothTags_OmitEmptyWins(t *testing.T) { + // A field tagged with both options gets the broader omitempty skip + // (zero-length non-nil slice is dropped) because (isEmpty && omitempty) + // fires when (isStrictZero && omitzero) wouldn't. + type Record struct { + Slice []string `db:"slice,omitempty,omitzero"` + } + got := mapFields(t, &Record{Slice: []string{}}) + assert.NotContains(t, got, "slice", "omitempty wins on non-nil empty slice") +} + +func TestMap_MapRecord_Unchanged(t *testing.T) { + // Regression guard: the reflect.Map record path (record IS a map, not + // a struct field) is independent of the field-walking changes above + // and must keep emitting every key. The path stores values as + // reflect.Value for downstream marshaling, which is pre-existing. + record := map[string]any{"a": 1, "b": "x"} + cols, vals, err := pgkit.Map(&record) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"a", "b"}, cols) + assert.Len(t, vals, 2) +} From dbc97c6c41e137634d5413ec227ee36dd8168859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 14:04:18 +0200 Subject: [PATCH 03/10] refactor(mapper): switch on Kind instead of chained else-if 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). --- mapper.go | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/mapper.go b/mapper.go index a05f598..bf5bbb5 100644 --- a/mapper.go +++ b/mapper.go @@ -118,22 +118,27 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf if t.IsZero() { isEmpty, isStrictZero = true, true } - } 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 + } else { + switch fld.Kind() { + case reflect.Slice: + if fld.IsNil() { + isEmpty, isStrictZero = true, true + } else if fld.Len() == 0 { + isEmpty = true + } + case reflect.Map: + if fld.IsNil() { + isEmpty, isStrictZero = true, true + } + case reflect.Array: + if fld.IsZero() { + isEmpty, isStrictZero = true, true + } + default: + if reflect.DeepEqual(fi.Zero.Interface(), value) { + isEmpty, isStrictZero = true, true + } } - } else if reflect.DeepEqual(fi.Zero.Interface(), value) { - isEmpty, isStrictZero = true, true } skip := (isEmpty && tagOmitEmpty) || (isStrictZero && tagOmitZero) From 1550bde6788458fde4419c7590dd8907b9515c99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 14:14:22 +0200 Subject: [PATCH 04/10] fix(mapper): keep all-zero arrays under omitempty (legacy behavior) 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. --- mapper.go | 9 ++++++++- mapper_test.go | 25 ++++++++++++++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/mapper.go b/mapper.go index bf5bbb5..9354d41 100644 --- a/mapper.go +++ b/mapper.go @@ -131,8 +131,15 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf isEmpty, isStrictZero = true, true } case reflect.Array: + // Legacy omitempty: only [0]T counted (Len==0). Normal-length + // all-zero arrays were emitted. Keep that — switching to + // IsZero for omitempty would silently drop [16]byte UUIDs, + // [32]byte hashes, etc. omitzero gets the strict rule. + if fld.Len() == 0 { + isEmpty = true + } if fld.IsZero() { - isEmpty, isStrictZero = true, true + isStrictZero = true } default: if reflect.DeepEqual(fi.Zero.Interface(), value) { diff --git a/mapper_test.go b/mapper_test.go index 99a846f..4e69b68 100644 --- a/mapper_test.go +++ b/mapper_test.go @@ -80,17 +80,32 @@ func TestMap_OmitEmpty_EmptyNonNilMap(t *testing.T) { } func TestMap_OmitEmpty_AllZeroArray(t *testing.T) { - // Regression guard: pre-omitzero behavior — omitempty skipped an - // all-zero array via the DeepEqual fallback (e.g. [32]byte{} hashes, - // [16]byte UUIDs). Keep that for fixed-size byte fields. + // Regression guard for legacy omitempty behavior: pre-omitzero, the + // array/slice branch only set isZero on Len()==0, which is only true + // for the degenerate [0]T. The else-if chain blocked the DeepEqual + // fallback, so a normal-length all-zero array ([16]byte UUIDs, + // [32]byte hashes) was ALWAYS emitted under omitempty. Preserve that. type Record struct { Hash [16]byte `db:"hash,omitempty"` } got := mapFields(t, &Record{}) - assert.NotContains(t, got, "hash", "all-zero array skipped by omitempty") + assert.Contains(t, got, "hash", "all-zero array kept under omitempty (legacy)") got = mapFields(t, &Record{Hash: [16]byte{1}}) - assert.Contains(t, got, "hash", "non-zero array kept by omitempty") + assert.Contains(t, got, "hash", "non-zero array kept under omitempty") +} + +func TestMap_OmitZero_AllZeroArray(t *testing.T) { + // omitzero IS the strict-zero option, so an all-zero fixed-size array + // is skipped here. Distinct from omitempty's array behavior above. + type Record struct { + Hash [16]byte `db:"hash,omitzero"` + } + got := mapFields(t, &Record{}) + assert.NotContains(t, got, "hash", "all-zero array skipped under omitzero") + + got = mapFields(t, &Record{Hash: [16]byte{1}}) + assert.Contains(t, got, "hash", "non-zero array kept under omitzero") } func TestMap_OmitZero_NilSlice(t *testing.T) { From 7aa507f12f013a113fff3524f6baf987222b37fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 14:17:27 +0200 Subject: [PATCH 05/10] docs(mapper): comment sanity pass 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. --- mapper.go | 26 +++++++++----------------- mapper_test.go | 13 +++++-------- 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/mapper.go b/mapper.go index 9354d41..3b5d17c 100644 --- a/mapper.go +++ b/mapper.go @@ -34,14 +34,11 @@ type MapOptions struct { IncludeNil bool } -// Map converts a struct to (column, value) slices using `db:""` struct tags, -// suitable for feeding squirrel's Columns(...).Values(...) or similar. +// Map converts a struct to (column, value) slices using `db:""` struct tags. // -// Tag options: -// - ,omitempty skips zero values, including empty (len==0) slices/maps. -// - ,omitzero skips only the type's true zero value; non-nil empty -// slices/maps stay, so a clear-to-empty UPDATE actually clears the column. -// Matches encoding/json's omitzero (Go 1.24+). +// Both ,omitempty and ,omitzero skip zero values, but ,omitzero keeps non-nil +// empty slices/maps so a clear-to-empty UPDATE actually clears the column. +// Matches encoding/json's omitzero (Go 1.24+). func Map(record interface{}) ([]string, []interface{}, error) { return MapWithOptions(record, nil) } @@ -107,12 +104,8 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf value := fld.Interface() - // isEmpty matches the legacy omitempty rule: slices count when - // len==0 (nil OR non-nil); maps and arrays only when they equal - // the type's zero (preserving pre-omitzero DeepEqual semantics). - // isStrictZero matches Go 1.24+ json's omitzero: only the type's - // true zero (nil slice/map, all-zero array, primitive zero, or - // IsZero()==true). + // Two flags because omitempty and omitzero disagree only on + // non-nil empty slices; every other path sets both together. var isEmpty, isStrictZero bool if t, ok := fld.Interface().(hasIsZero); ok { if t.IsZero() { @@ -131,10 +124,9 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf isEmpty, isStrictZero = true, true } case reflect.Array: - // Legacy omitempty: only [0]T counted (Len==0). Normal-length - // all-zero arrays were emitted. Keep that — switching to - // IsZero for omitempty would silently drop [16]byte UUIDs, - // [32]byte hashes, etc. omitzero gets the strict rule. + // omitempty must keep all-zero arrays of normal length. + // Switching to IsZero here would silently drop [16]byte + // UUIDs, [32]byte hashes, etc. omitzero gets the strict rule. if fld.Len() == 0 { isEmpty = true } diff --git a/mapper_test.go b/mapper_test.go index 4e69b68..95eec70 100644 --- a/mapper_test.go +++ b/mapper_test.go @@ -11,7 +11,6 @@ import ( "github.com/goware/pgkit/v2" ) -// mapFields runs Map and returns a column->value lookup for easier asserts. func mapFields(t *testing.T, record any) map[string]any { t.Helper() cols, vals, err := pgkit.Map(record) @@ -62,10 +61,9 @@ func TestMap_OmitEmpty_EmptyNonNilSlice(t *testing.T) { } func TestMap_OmitEmpty_EmptyNonNilMap(t *testing.T) { - // Regression guard: pre-omitzero behavior — omitempty only skipped a - // MAP when it was nil (DeepEqual nil-typed-nil vs non-nil-empty was - // false). A non-nil empty map stayed, so a "clear my jsonb" UPDATE - // actually cleared. Keep that. + // Regression guard: legacy omitempty only skipped a nil map. A + // non-nil empty map stayed, so a "clear my jsonb" UPDATE actually + // cleared. Keep that. type Record struct { Tags map[string]string `db:"tags,omitempty"` } @@ -159,8 +157,8 @@ func TestMap_OmitZero_PrimitiveZeros(t *testing.T) { } func TestMap_OmitZero_TimeIsZero(t *testing.T) { - // time.Time implements IsZero(); omitzero honors the interface like - // omitempty does, so a zero-valued time is skipped. + // IsZero() interface takes precedence over Kind dispatch, so types + // owning their own zero rule (time.Time, decimal, etc.) win. type Record struct { At time.Time `db:"at,omitzero"` } @@ -196,7 +194,6 @@ func TestMapWithOptions_OmitZero_IncludeZeroed(t *testing.T) { } func TestMapWithOptions_OmitZero_IncludeNil_Pointer(t *testing.T) { - // IncludeNil surfaces a nil pointer with a DEFAULT marker. type Record struct { Name *string `db:"name,omitzero"` } From 587447bf7b01a9a041717bb4f392577ff841529d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 14:19:14 +0200 Subject: [PATCH 06/10] refactor(mapper): extract zeroFlags from MapWithOptions 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. --- mapper.go | 71 ++++++++++++++++++++++++++----------------------------- 1 file changed, 34 insertions(+), 37 deletions(-) diff --git a/mapper.go b/mapper.go index 3b5d17c..a068c90 100644 --- a/mapper.go +++ b/mapper.go @@ -103,43 +103,7 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf } value := fld.Interface() - - // Two flags because omitempty and omitzero disagree only on - // non-nil empty slices; every other path sets both together. - var isEmpty, isStrictZero bool - if t, ok := fld.Interface().(hasIsZero); ok { - if t.IsZero() { - isEmpty, isStrictZero = true, true - } - } else { - switch fld.Kind() { - case reflect.Slice: - if fld.IsNil() { - isEmpty, isStrictZero = true, true - } else if fld.Len() == 0 { - isEmpty = true - } - case reflect.Map: - if fld.IsNil() { - isEmpty, isStrictZero = true, true - } - case reflect.Array: - // omitempty must keep all-zero arrays of normal length. - // Switching to IsZero here would silently drop [16]byte - // UUIDs, [32]byte hashes, etc. omitzero gets the strict rule. - if fld.Len() == 0 { - isEmpty = true - } - if fld.IsZero() { - isStrictZero = true - } - default: - if reflect.DeepEqual(fi.Zero.Interface(), value) { - isEmpty, isStrictZero = true, true - } - } - } - + isEmpty, isStrictZero := zeroFlags(fld, fi.Zero.Interface()) skip := (isEmpty && tagOmitEmpty) || (isStrictZero && tagOmitZero) if skip && !options.IncludeZeroed { continue @@ -209,6 +173,39 @@ func (fv *fieldValue) Less(i, j int) bool { return fv.fields[i] < fv.fields[j] } +// Two return values because omitempty and omitzero disagree only on +// non-nil empty slices; every other path returns both flags the same. +func zeroFlags(fld reflect.Value, fieldZero any) (isEmpty, isStrictZero bool) { + if t, ok := fld.Interface().(hasIsZero); ok { + if t.IsZero() { + return true, true + } + return false, false + } + switch fld.Kind() { + case reflect.Slice: + if fld.IsNil() { + return true, true + } + if fld.Len() == 0 { + return true, false + } + case reflect.Map: + if fld.IsNil() { + return true, true + } + case reflect.Array: + // omitempty must keep all-zero arrays of normal length; switching + // to IsZero here would silently drop [16]byte UUIDs, [32]byte hashes. + return fld.Len() == 0, fld.IsZero() + default: + if reflect.DeepEqual(fieldZero, fld.Interface()) { + return true, true + } + } + return false, false +} + type hasIsZero interface { IsZero() bool } From 80b09650f1fb57a49e5b96b90c8478e499f8de07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 14:31:24 +0200 Subject: [PATCH 07/10] feat(mapper): reject ,omitempty + ,omitzero on the same field 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. --- mapper.go | 9 ++++++--- mapper_test.go | 15 +++++++++------ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/mapper.go b/mapper.go index a068c90..a7d6b5b 100644 --- a/mapper.go +++ b/mapper.go @@ -36,9 +36,9 @@ type MapOptions struct { // Map converts a struct to (column, value) slices using `db:""` struct tags. // -// Both ,omitempty and ,omitzero skip zero values, but ,omitzero keeps non-nil -// empty slices/maps so a clear-to-empty UPDATE actually clears the column. -// Matches encoding/json's omitzero (Go 1.24+). +// ,omitempty and ,omitzero (mutually exclusive) both skip zero values, but +// ,omitzero keeps non-nil empty slices/maps so a clear-to-empty UPDATE +// actually clears the column. Matches encoding/json's omitzero (Go 1.24+). func Map(record interface{}) ([]string, []interface{}, error) { return MapWithOptions(record, nil) } @@ -86,6 +86,9 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf // Field options _, tagOmitEmpty := fi.Options["omitempty"] _, tagOmitZero := fi.Options["omitzero"] + if tagOmitEmpty && tagOmitZero { + return nil, nil, fmt.Errorf("field %q has both ,omitempty and ,omitzero tags (mutually exclusive)", fi.Name) + } fld := reflectx.FieldByIndexesReadOnly(recordV, fi.Index) diff --git a/mapper_test.go b/mapper_test.go index 95eec70..ee64412 100644 --- a/mapper_test.go +++ b/mapper_test.go @@ -219,15 +219,18 @@ func TestMap_OmitZero_StructWithoutIsZero(t *testing.T) { assert.Contains(t, got, "inner", "non-zero struct kept") } -func TestMap_BothTags_OmitEmptyWins(t *testing.T) { - // A field tagged with both options gets the broader omitempty skip - // (zero-length non-nil slice is dropped) because (isEmpty && omitempty) - // fires when (isStrictZero && omitzero) wouldn't. +func TestMap_BothTags_Rejected(t *testing.T) { + // OR-composing the skip rule made the winner type-dependent (slices + // got omitempty, all-zero arrays got omitzero), silently reversing + // the array preservation we explicitly fixed earlier. No coherent + // precedence, so reject the combination. type Record struct { Slice []string `db:"slice,omitempty,omitzero"` } - got := mapFields(t, &Record{Slice: []string{}}) - assert.NotContains(t, got, "slice", "omitempty wins on non-nil empty slice") + _, _, err := pgkit.Map(&Record{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "slice") + assert.Contains(t, err.Error(), "mutually exclusive") } func TestMap_MapRecord_Unchanged(t *testing.T) { From 5a30a5fb132bf838f11eef6b92c37975b7cf3856 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 14:43:05 +0200 Subject: [PATCH 08/10] feat(mapper): omitzero + IncludeNil emits NULL, not DEFAULT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- mapper.go | 14 ++++++++++---- mapper_test.go | 5 ++++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/mapper.go b/mapper.go index a7d6b5b..85a835b 100644 --- a/mapper.go +++ b/mapper.go @@ -39,6 +39,8 @@ type MapOptions struct { // ,omitempty and ,omitzero (mutually exclusive) both skip zero values, but // ,omitzero keeps non-nil empty slices/maps so a clear-to-empty UPDATE // actually clears the column. Matches encoding/json's omitzero (Go 1.24+). +// IncludeNil surfaces nil pointers as DEFAULT under ,omitempty and as +// NULL under ,omitzero. func Map(record interface{}) ([]string, []interface{}, error) { return MapWithOptions(record, nil) } @@ -97,11 +99,15 @@ func MapWithOptions(record interface{}, options *MapOptions) ([]string, []interf continue } fv.fields = append(fv.fields, fi.Name) - if tagOmitEmpty || tagOmitZero { - fv.values = append(fv.values, sqlDefault) - } else { - fv.values = append(fv.values, nil) + // ,omitempty preserves legacy: forced-include emits DEFAULT + // so callers can fall back to the column's DB default. ,omitzero + // is the strict tag: forced-include emits literal NULL so a + // PATCH can clear a nullable column with a non-null default. + var v any + if tagOmitEmpty { + v = sqlDefault } + fv.values = append(fv.values, v) continue } diff --git a/mapper_test.go b/mapper_test.go index ee64412..3e00155 100644 --- a/mapper_test.go +++ b/mapper_test.go @@ -194,6 +194,9 @@ func TestMapWithOptions_OmitZero_IncludeZeroed(t *testing.T) { } func TestMapWithOptions_OmitZero_IncludeNil_Pointer(t *testing.T) { + // Diverges from ,omitempty's DEFAULT-on-IncludeNil: under ,omitzero + // "show the nil" means literal SQL NULL, not a fallback default. Lets + // a PATCH caller clear a nullable column with a non-null DB default. type Record struct { Name *string `db:"name,omitzero"` } @@ -201,7 +204,7 @@ func TestMapWithOptions_OmitZero_IncludeNil_Pointer(t *testing.T) { require.NoError(t, err) require.Len(t, cols, 1) require.Equal(t, "name", cols[0]) - assert.Equal(t, sq.Expr("DEFAULT"), vals[0]) + assert.Nil(t, vals[0]) } func TestMap_OmitZero_StructWithoutIsZero(t *testing.T) { From db94885ef48011b48a9e094d6d9cae258abb187f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 14:57:19 +0200 Subject: [PATCH 09/10] fix(builder): reject column drift across batch InsertRecords rows 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. --- builder.go | 14 +++++++++++ builder_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 builder_test.go diff --git a/builder.go b/builder.go index 8e8bba6..2d6ce49 100644 --- a/builder.go +++ b/builder.go @@ -3,6 +3,7 @@ package pgkit import ( "fmt" "reflect" + "slices" sq "github.com/Masterminds/squirrel" ) @@ -23,6 +24,11 @@ func (s *StatementBuilder) InsertRecord(record interface{}, optTableName ...stri return InsertBuilder{InsertBuilder: insert.Into(tableName).Columns(cols...).Values(vals...)} } +// InsertRecords builds a multi-row INSERT from a slice of records. +// +// Every record must produce the same Map column set; a drifted shape (e.g. +// mixed nil and non-nil empty slices under ,omitzero) returns a build-time +// error rather than emitting malformed multi-row SQL. func (s StatementBuilder) InsertRecords(recordsSlice interface{}, optTableName ...string) InsertBuilder { insert := sq.InsertBuilder(s.StatementBuilderType) @@ -39,6 +45,7 @@ func (s StatementBuilder) InsertRecords(recordsSlice interface{}, optTableName . tableName = optTableName[0] } + var baseCols []string for i := 0; i < v.Len(); i++ { record := v.Index(i).Interface() @@ -54,8 +61,15 @@ func (s StatementBuilder) InsertRecords(recordsSlice interface{}, optTableName . } if i == 0 { + baseCols = cols insert = insert.Columns(cols...).Values(vals...) } else { + if !slices.Equal(cols, baseCols) { + return InsertBuilder{ + InsertBuilder: insert, + err: wrapErr(fmt.Errorf("record %d columns %v differ from record 0 columns %v", i, cols, baseCols)), + } + } insert = insert.Values(vals...) } } diff --git a/builder_test.go b/builder_test.go new file mode 100644 index 0000000..8effd1d --- /dev/null +++ b/builder_test.go @@ -0,0 +1,64 @@ +package pgkit_test + +import ( + "testing" + + sq "github.com/Masterminds/squirrel" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/goware/pgkit/v2" +) + +func TestInsertRecords_ColumnDriftRejected(t *testing.T) { + // ,omitzero produces different column shapes for nil vs non-nil empty + // slices; squirrel would otherwise stitch the mismatched widths into + // malformed multi-row SQL and surface only at exec time. + type Item struct { + ID int `db:"id"` + Tags []string `db:"tags,omitzero"` + } + + sb := &pgkit.StatementBuilder{StatementBuilderType: sq.StatementBuilder.PlaceholderFormat(sq.Dollar)} + records := []Item{ + {ID: 1, Tags: nil}, + {ID: 2, Tags: []string{}}, + } + b := sb.InsertRecords(records, "items") + require.Error(t, b.Err()) + assert.Contains(t, b.Err().Error(), "differ from record 0") +} + +func TestInsertRecords_UniformShape(t *testing.T) { + // Sanity: batches with consistent column shape across rows still build. + type Item struct { + ID int `db:"id"` + Tags []string `db:"tags,omitzero"` + } + + sb := &pgkit.StatementBuilder{StatementBuilderType: sq.StatementBuilder.PlaceholderFormat(sq.Dollar)} + records := []Item{ + {ID: 1, Tags: []string{"a"}}, + {ID: 2, Tags: []string{"b"}}, + } + b := sb.InsertRecords(records, "items") + require.NoError(t, b.Err()) +} + +func TestInsertRecords_OmitEmptyMapDriftRejected(t *testing.T) { + // Latent footgun ,omitzero exposes: legacy ,omitempty on a map already + // produced shape drift (nil map skipped, non-nil empty map kept via the + // DeepEqual fallback). The validation catches this case for free. + type Item struct { + ID int `db:"id"` + Tags map[string]string `db:"tags,omitempty"` + } + + sb := &pgkit.StatementBuilder{StatementBuilderType: sq.StatementBuilder.PlaceholderFormat(sq.Dollar)} + records := []Item{ + {ID: 1, Tags: nil}, + {ID: 2, Tags: map[string]string{}}, + } + b := sb.InsertRecords(records, "items") + require.Error(t, b.Err()) +} From f1e3f71b9a87fa67e4d72dcdbb53b976ea6820f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Tue, 2 Jun 2026 15:13:38 +0200 Subject: [PATCH 10/10] fix(builder): reject empty-column records with sq.Expr hint 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 goware/pgkit#51. Caught by the fifth adversarial Codex pass. --- builder.go | 13 ++++++++++--- builder_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/builder.go b/builder.go index 2d6ce49..20a39e2 100644 --- a/builder.go +++ b/builder.go @@ -20,15 +20,19 @@ func (s *StatementBuilder) InsertRecord(record interface{}, optTableName ...stri if err != nil { return InsertBuilder{InsertBuilder: insert, err: wrapErr(err)} } + 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))} + } return InsertBuilder{InsertBuilder: insert.Into(tableName).Columns(cols...).Values(vals...)} } // InsertRecords builds a multi-row INSERT from a slice of records. // -// Every record must produce the same Map column set; a drifted shape (e.g. -// mixed nil and non-nil empty slices under ,omitzero) returns a build-time -// error rather than emitting malformed multi-row SQL. +// Every record must produce the same non-empty Map column set; a drifted +// shape (e.g. mixed nil and non-nil empty slices under ,omitzero) or an +// all-default record returns a build-time error rather than emitting +// malformed multi-row SQL. func (s StatementBuilder) InsertRecords(recordsSlice interface{}, optTableName ...string) InsertBuilder { insert := sq.InsertBuilder(s.StatementBuilderType) @@ -59,6 +63,9 @@ func (s StatementBuilder) InsertRecords(recordsSlice interface{}, optTableName . if err != nil { return InsertBuilder{InsertBuilder: insert, err: wrapErr(err)} } + if len(cols) == 0 { + return InsertBuilder{InsertBuilder: insert, err: wrapErr(fmt.Errorf("Map returned no columns for record %d (%T); for an all-default INSERT use sq.Expr", i, record))} + } if i == 0 { baseCols = cols diff --git a/builder_test.go b/builder_test.go index 8effd1d..d72df6e 100644 --- a/builder_test.go +++ b/builder_test.go @@ -45,6 +45,32 @@ func TestInsertRecords_UniformShape(t *testing.T) { require.NoError(t, b.Err()) } +func TestInsertRecord_EmptyColumnsRejected(t *testing.T) { + // All fields tagged ,omitzero (or ,omitempty) and all zero leaves + // Map with no columns. Squirrel would emit invalid INSERT INTO t + // VALUES (); fail fast at build time and point at sq.Expr as the + // escape for the all-default INSERT case. Tracked in goware/pgkit#51. + type Item struct { + Tags []string `db:"tags,omitzero"` + } + sb := &pgkit.StatementBuilder{StatementBuilderType: sq.StatementBuilder.PlaceholderFormat(sq.Dollar)} + b := sb.InsertRecord(&Item{}, "items") + require.Error(t, b.Err()) + assert.Contains(t, b.Err().Error(), "no columns") + assert.Contains(t, b.Err().Error(), "sq.Expr") +} + +func TestInsertRecords_EmptyColumnsRejected(t *testing.T) { + type Item struct { + Tags []string `db:"tags,omitzero"` + } + sb := &pgkit.StatementBuilder{StatementBuilderType: sq.StatementBuilder.PlaceholderFormat(sq.Dollar)} + records := []Item{{}, {}} + b := sb.InsertRecords(records, "items") + require.Error(t, b.Err()) + assert.Contains(t, b.Err().Error(), "no columns") +} + func TestInsertRecords_OmitEmptyMapDriftRejected(t *testing.T) { // Latent footgun ,omitzero exposes: legacy ,omitempty on a map already // produced shape drift (nil map skipped, non-nil empty map kept via the