Skip to content

feat(builder): InsertDefaults for INSERT ... DEFAULT VALUES#52

Open
klaidliadon wants to merge 2 commits into
masterfrom
fix-insert-default-values
Open

feat(builder): InsertDefaults for INSERT ... DEFAULT VALUES#52
klaidliadon wants to merge 2 commits into
masterfrom
fix-insert-default-values

Conversation

@klaidliadon
Copy link
Copy Markdown
Contributor

After #50, InsertRecord rejects records whose Map returns zero columns and points callers at sq.Expr as the escape hatch — bypassing the type-safe builder. Legitimate use cases (sequence tables, audit rows, any all-DB-defaulted insert) deserve a first-class path.

DB.SQL.InsertDefaults("orders").Suffix(`RETURNING "id"`)
// → INSERT INTO orders DEFAULT VALUES RETURNING "id"

Closes #51.


Design

DefaultValuesBuilder is a separate small type, not overrides on pgkit.InsertBuilder. The plan went through three Codex review rounds before settling here:

  • v1 auto-detect rejected — silently turning bugs (forgot to set fields) into valid SQL is a one-way door we can't walk back.
  • v2 embed-and-shadow rejectedpgkit.InsertBuilder embeds sq.InsertBuilder (22 chain methods). Overriding ToSql + Suffix only handles 2; the other 20 inherited methods silently strip pgkit state. Dishonest surface area.
  • v3 → v4 separate-type — new type exposes only what it supports: ToSql, Suffix(string), Err. Codex green-light on v4.

Full plan + decision log: tmp/insert-default-values/2026-06-02-plan.md (ignored, branch-only).

Behaviour

Call Result
InsertDefaults("t").ToSql() INSERT INTO t DEFAULT VALUES
InsertDefaults("t").Suffix(\RETURNING "id"`)` INSERT INTO t DEFAULT VALUES RETURNING "id"
chained .Suffix(...) accumulates left-to-right
InsertDefaults("") build-time error via Err()
InsertRecord(allDefault) (existing #50 path) hints at SQL.InsertDefaults("t") instead of sq.Expr

Suffix takes literal SQL only. Placeholder-bearing suffixes (ON CONFLICT ... SET col = ?) fall back to sq.Expr at the call site — placeholder rebinding without sq.StatementBuilderType's PlaceholderFormat getter is non-trivial and defer-able until concrete demand surfaces.

Error from InsertDefaults("") is raw (no pgkit: prefix). Querier.wrapErr at querier.go:23,47,71 adds the prefix at use time — double-wrapping would surface pgkit: pgkit: ....


Test plan

  • make db-reset test-all against PostgreSQL 18.3 — all 6 packages green.
  • 6 unit tests: plain SQL, RETURNING, chained Suffix, EXCLUDED upsert, empty-table rejection, InsertRecord hint update.
  • Integration TestInsertDefaultsRoundTrip: exec INSERT INTO default_only DEFAULT VALUES RETURNING id against the new default_only table (every column DB-defaulted, isolates the contract).
  • go vet ./... clean.

Out of scope (tracked separately)

Codex's review surfaced two latent state-loss bugs in pgkit.InsertBuilder's embed-and-inherit pattern that pre-date this PR:

  1. pgkit.InsertBuilder.Suffix drops err — inherited from sq.InsertBuilder, returns the squirrel type, loses the pgkit wrapper.
  2. table.go:93 chain InsertRecord(r).Into(t).Suffix("...") loses pgkit state via .Into(...).

Both are symptoms of the same disease — embedding-and-inheriting chain methods that return the parent type. The fix is to wrap every chain method on pgkit's side. Out of scope here; will land as a follow-up issue titled "pgkit.InsertBuilder loses state through inherited squirrel methods."

Closes #51. Follow-up to #50.

After #50, InsertRecord on an all-zero record errored out and pointed
callers at sq.Expr as the escape hatch — bypassing the type-safe
builder. The legitimate use cases (sequence tables, audit rows, any
all-DB-defaulted insert) deserve a first-class path.

DefaultValuesBuilder is a separate small type rather than overrides on
pgkit.InsertBuilder. Earlier drafts tried to embed sq.InsertBuilder and
shadow ToSql + Suffix, but that pretends to support 22 squirrel chain
methods while only handling 2 — every other inherited method silently
strips pgkit state. The new type exposes exactly what it supports:
ToSql, Suffix(string), Err.

  DB.SQL.InsertDefaults("orders").Suffix(`RETURNING "id"`)
  // INSERT INTO orders DEFAULT VALUES RETURNING "id"

Empty table is a build-time error via Err (raw, no pgkit: prefix —
Querier.wrapErr adds it at use time). Suffix takes literal SQL only;
placeholder-bearing suffixes (ON CONFLICT ... SET col = ?) fall back
to sq.Expr at the call site.

InsertRecord's empty-cols error now hints at SQL.InsertDefaults rather
than sq.Expr; tableName empty case prints a generic placeholder.

Tests:
- 6 unit tests covering plain SQL, RETURNING, chained Suffix, EXCLUDED
  upsert, empty-table rejection, and the InsertRecord hint update
- new default_only PG table in test schema + integration round-trip
  test exec'ing DEFAULT VALUES RETURNING id

Planned via three Codex review iterations (auto-detect rejected,
embed-and-shadow rejected, separate-type green-lit on v4). Plan at
tmp/insert-default-values/2026-06-02-plan.md.

Pre-existing pgkit.InsertBuilder state-loss bugs surfaced during
review (Suffix drops err; table.go:93 chain returns sq.InsertBuilder
mid-chain) are out of scope here and will land as a follow-up issue.
@david-littlefarmer
Copy link
Copy Markdown
Member

Code Review

One confirmed correctness bug, three low-severity design notes.


🔴 #1 — Unquoted table identifier (builder.go:111)

b.table is concatenated verbatim into SQL with no identifier quoting:

return "INSERT INTO " + b.table + " DEFAULT VALUES" + b.suffix, nil, nil

Every other builder in this repo delegates to squirrel's .Into() which handles quoting. This bypasses that entirely:

  • InsertDefaults("user")INSERT INTO user DEFAULT VALUESuser is a PG reserved word, syntax error at runtime
  • InsertDefaults("MyTable") → silently targets mytable (Postgres lowercases unquoted identifiers)

The fix is straightforward — use pq.QuoteIdentifier(table) (or squirrel's quoting) at construction time.


🟡 #2 — Pointer receiver on InsertDefaults (builder.go:92)

InsertDefaults uses *StatementBuilder while InsertRecords, UpdateRecord, and UpdateRecordColumns all use value receivers. No mutation justifies the pointer receiver here — a value receiver would be consistent with the majority of the API and avoid a footgun for callers that hold a StatementBuilder by value.

(Note: InsertRecord also uses *StatementBuilder, so this isn't a new pattern — but widening it further without a reason is worth flagging.)


🟡 #3 — Exported zero value bypasses the empty-table guard (builder.go:101)

DefaultValuesBuilder is an exported value type. A caller that zero-initialises it directly (var b pgkit.DefaultValuesBuilder) gets err == nil and table == "". ToSql() then returns "INSERT INTO DEFAULT VALUES" — invalid SQL — with no error, so the failure only surfaces as a cryptic Postgres parse error at exec time rather than a build-time message.


#4Suffix("") injects a trailing space (builder.go:119)

Calling Suffix("") produces "INSERT INTO t DEFAULT VALUES " (trailing space). Postgres ignores it, but it breaks string-equality assertions in snapshot tests and query-log comparisons. Common pattern where this bites: building suffix conditionally — b.Suffix(returningClause) where returningClause is "" on the non-returning path.


Review generated with Claude Code

Three of David's four findings on #52:

- Value receiver on InsertDefaults to match the majority of sibling
  methods (UpdateRecord, UpdateRecordColumns, InsertRecords).
  InsertRecord stays *StatementBuilder for now; widening the
  inconsistency isn't justified.
- Defensive table check in DefaultValuesBuilder.ToSql so a direct
  zero-value construction (var b pgkit.DefaultValuesBuilder) errors
  cleanly instead of emitting invalid SQL at exec time.
- Suffix("") no-op so conditional-suffix callers don't get a trailing
  space that breaks SQL-string snapshot tests.

Push back on the fourth finding (table identifier quoting) — the bug
exists pgkit-wide (squirrel doesn't quote either, see Insert / Into
across the repo). Fixing only InsertDefaults would create asymmetric
API behavior. Will track as a separate pgkit-wide issue.
@klaidliadon
Copy link
Copy Markdown
Contributor Author

Pushed 514707f addressing 3 of 4. Pushing back on #1.

#1 — Table identifier quoting: Pushing back. The premise that "every other builder delegates to squirrel's .Into() which handles quoting" is empirically not the case — grep for Quote across pgkit returns zero hits, and squirrel's .Into() just writes the table name verbatim into the SQL (verified: sq.Insert("items").ToSql() returns INSERT INTO items …, no quotes). InsertRecord(rec, "user") would break at runtime just as badly as InsertDefaults("user"). Adding quoting to only InsertDefaults makes the API asymmetric and worse to reason about (InsertRecord and InsertDefaults would treat the same string differently). The right fix is pgkit-wide identifier quoting in a separate PR — probably a major version bump, since some callers may be pre-quoting strings today to work around the current behavior. Happy to file an issue.

#2 — Pointer receiver: Accepted. InsertDefaults now uses a value receiver, matching UpdateRecord / UpdateRecordColumns / InsertRecords. InsertRecord kept on *StatementBuilder for now — widening the inconsistency isn't justified, narrowing it is its own scoped change.

#3 — Zero-value bypass: Accepted. DefaultValuesBuilder.ToSql now re-validates table == "" defensively, so a direct var b pgkit.DefaultValuesBuilder errors cleanly instead of emitting INSERT INTO DEFAULT VALUES at exec time. Test: TestInsertDefaults_ZeroValueErrors.

#4Suffix("") trailing space: Accepted. Empty suffix is now a no-op; b.Suffix("") returns the same builder unchanged, no trailing whitespace. Test: TestInsertDefaults_EmptySuffixIsNoop.

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.

Support INSERT ... DEFAULT VALUES for all-defaulted records

2 participants