diff --git a/builder.go b/builder.go index 8e8bba6..20a39e2 100644 --- a/builder.go +++ b/builder.go @@ -3,6 +3,7 @@ package pgkit import ( "fmt" "reflect" + "slices" sq "github.com/Masterminds/squirrel" ) @@ -19,10 +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 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) @@ -39,6 +49,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() @@ -52,10 +63,20 @@ 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 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..d72df6e --- /dev/null +++ b/builder_test.go @@ -0,0 +1,90 @@ +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 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 + // 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()) +} diff --git a/mapper.go b/mapper.go index ca15e08..85a835b 100644 --- a/mapper.go +++ b/mapper.go @@ -34,12 +34,13 @@ 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. -// 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. +// Map converts a struct to (column, value) slices using `db:""` struct tags. +// +// ,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) } @@ -86,38 +87,34 @@ 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) if fld.Kind() == reflect.Ptr && fld.IsNil() { - if tagOmitEmpty && !options.IncludeNil { + if (tagOmitEmpty || tagOmitZero) && !options.IncludeNil { continue } fv.fields = append(fv.fields, fi.Name) + // ,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 { - fv.values = append(fv.values, sqlDefault) - } else { - fv.values = append(fv.values, nil) + v = sqlDefault } + fv.values = append(fv.values, v) continue } value := fld.Interface() - - isZero := false - if t, ok := fld.Interface().(hasIsZero); ok { - if t.IsZero() { - isZero = true - } - } else if fld.Kind() == reflect.Array || fld.Kind() == reflect.Slice { - if fld.Len() == 0 { - isZero = true - } - } else if reflect.DeepEqual(fi.Zero.Interface(), value) { - isZero = true - } - - if isZero && tagOmitEmpty && !options.IncludeZeroed { + isEmpty, isStrictZero := zeroFlags(fld, fi.Zero.Interface()) + skip := (isEmpty && tagOmitEmpty) || (isStrictZero && tagOmitZero) + if skip && !options.IncludeZeroed { continue } @@ -127,7 +124,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) @@ -185,6 +182,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 } diff --git a/mapper_test.go b/mapper_test.go new file mode 100644 index 0000000..3e00155 --- /dev/null +++ b/mapper_test.go @@ -0,0 +1,249 @@ +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" +) + +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_OmitEmpty_EmptyNonNilMap(t *testing.T) { + // 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"` + } + 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 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.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 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) { + // 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) { + // 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"` + } + 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) { + // 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"` + } + 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.Nil(t, 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_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"` + } + _, _, 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) { + // 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) +}