From 006a90f89848e29b94a809f653df3347955d53a4 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 16:19:12 +0200 Subject: [PATCH] feat(builder): union columns + DEFAULT for mixed-shape InsertRecords MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #53. Follow-up to #50. #50 rejected any batch whose rows produced different Map column sets, 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. --- builder.go | 48 ++++++----- builder_test.go | 144 +++++++++++++++++++++++-------- tests/pgkit_test.go | 48 +++++++++++ tests/testdata/pgkit_test_db.sql | 10 +++ 4 files changed, 196 insertions(+), 54 deletions(-) diff --git a/builder.go b/builder.go index 20a39e2..894b631 100644 --- a/builder.go +++ b/builder.go @@ -2,6 +2,7 @@ package pgkit import ( "fmt" + "maps" "reflect" "slices" @@ -27,12 +28,8 @@ 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 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. +// InsertRecords builds a multi-row INSERT from a slice of records, unioning +// columns across rows and emitting DEFAULT for any slot a given row skipped. func (s StatementBuilder) InsertRecords(recordsSlice interface{}, optTableName ...string) InsertBuilder { insert := sq.InsertBuilder(s.StatementBuilderType) @@ -49,7 +46,8 @@ func (s StatementBuilder) InsertRecords(recordsSlice interface{}, optTableName . tableName = optTableName[0] } - var baseCols []string + rows := make([]map[string]any, 0, v.Len()) + colSet := map[string]struct{}{} for i := 0; i < v.Len(); i++ { record := v.Index(i).Interface() @@ -63,25 +61,35 @@ 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))} + byCol := make(map[string]any, len(cols)) + for j, c := range cols { + byCol[c] = vals[j] + colSet[c] = struct{}{} } + rows = append(rows, byCol) + } + + // slices.Sorted matches Map's lexical column order, so generated SQL + // lines up with what callers see when they call Map(record) directly. + allCols := slices.Sorted(maps.Keys(colSet)) + if len(allCols) == 0 { + return InsertBuilder{InsertBuilder: insert, err: wrapErr(fmt.Errorf("Map returned no columns across any of the %d records; for an all-default INSERT use sq.Expr(\"INSERT INTO %s DEFAULT VALUES\") in a loop", v.Len(), tableName))} + } - 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.Into(tableName).Columns(allCols...) + for _, row := range rows { + padded := make([]any, len(allCols)) + for i, c := range allCols { + if v, ok := row[c]; ok { + padded[i] = v + } else { + padded[i] = sqlDefault } - insert = insert.Values(vals...) } + insert = insert.Values(padded...) } - return InsertBuilder{InsertBuilder: insert.Into(tableName)} + return InsertBuilder{InsertBuilder: insert} } func (s StatementBuilder) UpdateRecord(record interface{}, whereExpr sq.Eq, optTableName ...string) UpdateBuilder { diff --git a/builder_test.go b/builder_test.go index d72df6e..4dbf778 100644 --- a/builder_test.go +++ b/builder_test.go @@ -10,10 +10,7 @@ import ( "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. +func TestInsertRecords_UniformShape(t *testing.T) { type Item struct { ID int `db:"id"` Tags []string `db:"tags,omitzero"` @@ -21,60 +18,69 @@ func TestInsertRecords_ColumnDriftRejected(t *testing.T) { sb := &pgkit.StatementBuilder{StatementBuilderType: sq.StatementBuilder.PlaceholderFormat(sq.Dollar)} records := []Item{ - {ID: 1, Tags: nil}, - {ID: 2, Tags: []string{}}, + {ID: 1, Tags: []string{"a"}}, + {ID: 2, Tags: []string{"b"}}, } b := sb.InsertRecords(records, "items") - require.Error(t, b.Err()) - assert.Contains(t, b.Err().Error(), "differ from record 0") + require.NoError(t, b.Err()) + sql, args, err := b.ToSql() + require.NoError(t, err) + assert.Equal(t, `INSERT INTO items (id,tags) VALUES ($1,$2),($3,$4)`, sql) + assert.Equal(t, []any{1, []string{"a"}, 2, []string{"b"}}, args) } -func TestInsertRecords_UniformShape(t *testing.T) { - // Sanity: batches with consistent column shape across rows still build. +func TestInsertRecords_MixedShape_UnionsAndDefaults(t *testing.T) { + // Heterogeneous batch: each row contributes a different column subset. + // The union becomes the INSERT column list; missing slots become DEFAULT. type Item struct { ID int `db:"id"` + Name string `db:"name,omitzero"` 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"}}, + {ID: 1, Name: "first"}, // cols = [id, name] + {ID: 2, Tags: []string{"foo"}}, // cols = [id, tags] + {ID: 3, Name: "third", Tags: []string{}}, // cols = [id, name, tags] (omitzero keeps non-nil empty) } b := sb.InsertRecords(records, "items") require.NoError(t, b.Err()) + sql, args, err := b.ToSql() + require.NoError(t, err) + assert.Equal(t, + `INSERT INTO items (id,name,tags) VALUES ($1,$2,DEFAULT),($3,DEFAULT,$4),($5,$6,$7)`, + sql, + ) + assert.Equal(t, []any{1, "first", 2, []string{"foo"}, 3, "third", []string{}}, args) } -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. +func TestInsertRecords_OmitZeroMixedSlices(t *testing.T) { + // #50 used to reject this with a drift error. The union-with-DEFAULT + // approach makes it valid: ,omitzero distinguishes nil (skipped → DEFAULT) + // from non-nil empty (included with empty literal). type Item struct { + ID int `db:"id"` 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{{}, {}} + records := []Item{ + {ID: 1, Tags: nil}, // tags skipped → will become DEFAULT + {ID: 2, Tags: []string{}}, // tags included → empty array literal + } b := sb.InsertRecords(records, "items") - require.Error(t, b.Err()) - assert.Contains(t, b.Err().Error(), "no columns") + require.NoError(t, b.Err()) + sql, args, err := b.ToSql() + require.NoError(t, err) + assert.Equal(t, `INSERT INTO items (id,tags) VALUES ($1,DEFAULT),($2,$3)`, sql) + assert.Equal(t, []any{1, 2, []string{}}, args) } -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. +func TestInsertRecords_OmitEmptyMixedMaps(t *testing.T) { + // Legacy footgun resolved: ,omitempty on a map has always treated nil + // and non-nil empty differently (DeepEqual sees them as distinct). + // Now the union path handles it instead of rejecting. type Item struct { ID int `db:"id"` Tags map[string]string `db:"tags,omitempty"` @@ -86,5 +92,75 @@ func TestInsertRecords_OmitEmptyMapDriftRejected(t *testing.T) { {ID: 2, Tags: map[string]string{}}, } b := sb.InsertRecords(records, "items") + require.NoError(t, b.Err()) + sql, _, err := b.ToSql() + require.NoError(t, err) + assert.Equal(t, `INSERT INTO items (id,tags) VALUES ($1,DEFAULT),($2,$3)`, sql) +} + +func TestInsertRecords_EmptyRowMixedWithNonEmpty(t *testing.T) { + // A row with all-skipped fields can still appear in a batch: another row + // contributes the column union, the empty row pads to all DEFAULT. + type Item struct { + Name string `db:"name,omitzero"` + Tags []string `db:"tags,omitzero"` + } + + sb := &pgkit.StatementBuilder{StatementBuilderType: sq.StatementBuilder.PlaceholderFormat(sq.Dollar)} + records := []Item{ + {}, // empty → all DEFAULT + {Name: "second", Tags: []string{"a"}}, // contributes the union + } + b := sb.InsertRecords(records, "items") + require.NoError(t, b.Err()) + sql, args, err := b.ToSql() + require.NoError(t, err) + assert.Equal(t, `INSERT INTO items (name,tags) VALUES (DEFAULT,DEFAULT),($1,$2)`, sql) + assert.Equal(t, []any{"second", []string{"a"}}, args) +} + +func TestInsertRecords_AllRowsEmpty_Rejected(t *testing.T) { + // Whole-batch empty union: no row contributed any column. Genuinely + // out of InsertRecords' scope — caller wants sq.Expr or per-row defaults. + type Item struct { + Name string `db:"name,omitzero"` + 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") + assert.Contains(t, b.Err().Error(), "sq.Expr") +} + +func TestInsertRecords_MapRecords(t *testing.T) { + // Map accepts records as map[string]any (mapper.go's reflect.Map case). + // Heterogeneous map batches should union just like struct batches do. + sb := &pgkit.StatementBuilder{StatementBuilderType: sq.StatementBuilder.PlaceholderFormat(sq.Dollar)} + records := []map[string]any{ + {"id": 1, "name": "first"}, + {"id": 2, "tags": "foo"}, + } + b := sb.InsertRecords(records, "items") + require.NoError(t, b.Err()) + sql, _, err := b.ToSql() + require.NoError(t, err) + // Column order is lexical (Map sorts deterministically). + assert.Equal(t, `INSERT INTO items (id,name,tags) VALUES ($1,$2,DEFAULT),($3,DEFAULT,$4)`, sql) +} + +func TestInsertRecord_EmptyColumnsRejected(t *testing.T) { + // Single-row InsertRecord rejection unchanged from #50: empty record + // has no column to fall back on, and DEFAULT VALUES is the right + // answer but lives on a separate path (sq.Expr today). + 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") } diff --git a/tests/pgkit_test.go b/tests/pgkit_test.go index c6c8530..ce0f27d 100644 --- a/tests/pgkit_test.go +++ b/tests/pgkit_test.go @@ -1165,3 +1165,51 @@ func connectToDb(conf pgkit.Config) (*pgkit.DB, error) { } return dbClient, err } + +type MixedShape struct { + ID int64 `db:"id,omitzero"` + Name string `db:"name"` + Tags []string `db:"tags,omitzero"` + Note *string `db:"note,omitempty"` + Created time.Time `db:"created_at,omitempty"` +} + +func TestInsertRecordsMixedShapeRoundTrip(t *testing.T) { + // Three rows, each contributing a different column subset. Each row + // should land with caller values where supplied and DB defaults + // (or NULL on nullable cols) where the row opted out. + truncateTable(t, "mixed_shape") + + note := "third row's note" + records := []*MixedShape{ + {Name: "first"}, // tags → DEFAULT '{}', note → NULL + {Name: "second", Tags: []string{"a", "b"}}, // note → NULL + {Name: "third", Note: ¬e}, // tags → DEFAULT '{}' + } + + _, err := DB.Query.Exec(context.Background(), DB.SQL.InsertRecords(records, "mixed_shape")) + require.NoError(t, err) + + var out []*MixedShape + err = DB.Query.GetAll( + context.Background(), + DB.SQL.Select("*").From("mixed_shape").OrderBy("id"), + &out, + ) + require.NoError(t, err) + require.Len(t, out, 3) + + assert.Equal(t, "first", out[0].Name) + assert.Empty(t, out[0].Tags) + assert.Nil(t, out[0].Note) + assert.False(t, out[0].Created.IsZero(), "created_at populated by DB default") + + assert.Equal(t, "second", out[1].Name) + assert.Equal(t, []string{"a", "b"}, out[1].Tags) + assert.Nil(t, out[1].Note) + + assert.Equal(t, "third", out[2].Name) + assert.Empty(t, out[2].Tags) + require.NotNil(t, out[2].Note) + assert.Equal(t, "third row's note", *out[2].Note) +} diff --git a/tests/testdata/pgkit_test_db.sql b/tests/testdata/pgkit_test_db.sql index 406d655..3226a82 100644 --- a/tests/testdata/pgkit_test_db.sql +++ b/tests/testdata/pgkit_test_db.sql @@ -44,3 +44,13 @@ CREATE TABLE stats ( big_num NUMERIC(78,0) NOT NULL, -- representing a big.Int runtime type rating NUMERIC(78,0) NULL -- representing a nullable big.Int runtime type ); + +-- mixed_shape proves heterogeneous batches survive end-to-end: nullable + +-- defaulted columns let different rows opt out of different fields. +CREATE TABLE mixed_shape ( + id BIGSERIAL PRIMARY KEY, + name TEXT NOT NULL, + tags TEXT[] NOT NULL DEFAULT '{}', + note TEXT NULL, + created_at TIMESTAMP WITHOUT TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL +);