Skip to content

fix(table): populate caller records on multi-record Insert (#24) - #61

Open
VojtechVitek wants to merge 2 commits into
masterfrom
fix-insert-multi-populate
Open

fix(table): populate caller records on multi-record Insert (#24)#61
VojtechVitek wants to merge 2 commits into
masterfrom
fix-insert-multi-populate

Conversation

@VojtechVitek

Copy link
Copy Markdown
Member

Related to #24.

TL;DR

Issue #24's original symptom — SQLSTATE 42601 (VALUES lists of differing length) on a batch insert with mixed ,omitempty fields — is already fixed on master by #54 (union columns + DEFAULT). Investigating it surfaced a second, adjacent bug in the same batch-insert path, which this PR fixes.

The bug

Table.Insert with multiple records did not populate the caller's records with DB-generated ids/defaults when the records were passed as individual variadic args:

db.Accounts.Insert(ctx, a, b)   // a.ID, b.ID stay 0  ❌
db.Accounts.Insert(ctx, recs...) // recs[i].ID populated ✔ (spread slice)

insertAll relied on GetAll rewriting the slice backing array. GetAll (scany) resets the slice and appends freshly-allocated structs, so the DB values reach the caller only when they spread a slice — the backing array is shared. Passing individual records builds a fresh internal slice, so the caller's named pointers never see the ids. This is inconsistent with:

  • single Insert (insertOneGetOne scans into the record), and
  • Save (saveAll already copies DB values back into the originals).

The fix

Preserve the original pointers and copy the RETURNING * values back into them after GetAll, exactly as saveAll does. Now both calling styles populate ids/defaults.

Commits (red → green)

  1. test(table) — end-to-end Postgres regression reproducing omitempty bug in InsertRecords #24 (mixed ,omitempty batch). The 42601 assertion already passes; the populate-back assertions fail. → CI red.
  2. fix(table) — the copy-back. → CI green.

This is a real bug fix, so CI is intentionally red on commit 1 (proving the defect) and green on commit 2.

Testing

  • New TestInsertMultiPopulatesRecords_Issue24: red before, green after.
  • Full suite incl. Postgres-backed ./tests/... passes.
  • go build ./..., go vet ./ clean.

Once merged, #24 can be closed (both halves addressed: 42601 by #54, populate-back here).

🤖 Generated with Claude Code

VojtechVitek and others added 2 commits August 2, 2026 22:46
Passing records to Table.Insert as individual variadic args (not a
spread slice) does not populate the caller's records with DB-generated
ids/defaults on a multi-record insert, unlike a single Insert and unlike
Save. insertAll relies on GetAll rewriting the slice backing array, which
only reflects back to the caller when they spread a slice.

This test reproduces #24 end to end
against Postgres. The SQLSTATE 42601 part is already fixed (PR #54 union +
DEFAULT); the populate-back assertions fail. The fix follows next commit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
insertAll relied on GetAll rewriting the slice backing array, so
DB-generated ids/defaults reached the caller only when they spread a
slice (Insert(ctx, recs...)); passing individual records
(Insert(ctx, a, b)) left them at their zero values, unlike a single
Insert and unlike Save.

Preserve the original pointers and copy the DB-returned values back into
them after GetAll (RETURNING *), matching insertOne/GetOne and saveAll.

Fixes the populate-back half of #24; the SQLSTATE 42601 half was already
fixed by PR #54 (union + DEFAULT). Verified by
TestInsertMultiPopulatesRecords_Issue24 (red in the previous commit,
green now) and the full Postgres-backed suite.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
require.NoError(t, err, "batch insert with mixed ,omitempty created_at")

require.NotZero(t, withTime.ID, "id populated on caller record")
require.NotZero(t, withoutTime.ID, "id populated on caller record")

@VojtechVitek VojtechVitek Aug 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was failing with:

=== RUN   TestInsertMultiPopulatesRecords_Issue24
    issue24_repro_test.go:34: 
        	Error Trace:	/home/runner/work/pgkit/pgkit/tests/issue24_repro_test.go:34
        	Error:      	Should not be zero, but was 0
        	Test:       	TestInsertMultiPopulatesRecords_Issue24
        	Messages:   	id populated on caller record

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant