OnConflictDoNothing().One() reports ErrNotFound on the exact path an idempotent insert exists to serve

[jryannel]

Porting an INSERT … ON CONFLICT DO NOTHING and hit the trap that sqlb-queries already documents. Filing it anyway, because the skill catching it is an argument that the API should.

The shape

-- what it was
INSERT INTO billing_subscriptions (org_id) VALUES ($1)
ON CONFLICT (org_id) DO NOTHING;

The function exists to be idempotent: something needs the row to exist, and does not care whether it already did.

_, err := sqlb.InsertRows(&row).Only("org_id").
    OnConflictDoNothing("org_id").
    One(ctx, db)      // <-- wrong, and reads as right

One is the natural reach for a single-row insert, and every other single-row insert in the port uses it. Here it is a bug: DO NOTHING means a conflicting insert returns no row, One maps no row to ErrNotFound, and the function reports failure on precisely the case it was written for. The first call succeeds and every subsequent one errors.

Exec is correct — empty slice, nil error — and that is what the code now does. No complaint about the semantics; the complaint is that nothing says so at the call site.

Why it is worth an API change rather than only a doc

The failure has an unusually bad profile:

• It inverts with state. Tests that insert into a clean database pass. The failure needs the row to already exist, which is the second call, which is the case a test for idempotency would cover and a test for "it inserts" would not.
• The error is plausible. ErrNotFound from an insert is strange enough to notice if you are looking, but it arrives through the same if err != nil { return fmt.Errorf(...) } as everything else, and "not found" from a function whose job is to make something exist reads as a real database problem rather than as a category error.
• One's doc does not mention conflict clauses, and OnConflictDoNothing's does not mention terminals. Each is locally reasonable; the interaction is what bites, and neither doc is where you are looking when you write the other.

What would help, roughly in order

1. Refuse the combination. One after OnConflictDoNothing is, as far as I can construct, never what anyone means — "give me exactly one row, and also do not produce a row on conflict" is a contradiction. Failing it at build time with a message naming Exec would have cost me nothing and saved the lookup.
2. A distinct sentinel. If refusing is too strong, ErrConflictSkipped (or similar) instead of ErrNotFound would at least make the error say what happened. Callers who genuinely want "did it insert" get a clean branch, and the misuse stops looking like a missing row.
3. Cross-reference the docs. Cheapest, and worth doing regardless: one sentence on OnConflictDoNothing saying that a skipped insert returns no row and therefore that One will report ErrNotFound.

Note on the skill

sqlb-queries lists this among its four traps, and it is the first of the four this port has hit — which reads as evidence the trap list was chosen from real failures rather than imagined ones. It did not help me here, because the skill is in sqlb's repo rather than in the consumer's, and what loads in this repo is the generated sqlb-schema (see #142, #143). Not a request to move it — just a data point that a well-chosen trap list only pays off where the code is being written.

[jryannel]

Fixed in aef7ba4, on claude/github-issues-c69665. Took option 1 — refuse the combination — plus option 3.

One after a clause that renders DO NOTHING is now refused at the terminal, before the statement runs:

sqlb: One after OnConflictDoNothing on billing_subscriptions: a skipped insert returns no row,
so a conflict would answer ErrNotFound — the case the clause exists to allow;
  call Exec instead, whose empty slice and nil error are what "it was already there" looks like,
  or OnConflictUpdate([]string{"org_id"}, "org_id") if the row is wanted whether or not this call created it

Both routes out are named because which one is right depends on what the caller wanted, and the second is not obvious — it is the trick pgtest/smallcases_test.go documents and nothing else did. The condition is read off the same fields SQL() renders on, so the refusal and the rendered statement cannot drift.

Option 2 was not taken. A distinct ErrConflictSkipped makes the misuse legible rather than impossible, and refusing costs the same. Your point about the profile — it inverts with state, so the clean-database test passes — is what decided it: a sentinel still needs somebody to have branched on it.

On the docs: OnConflictDoNothing and One now cross-reference each other, docs/queries/mutations.md has the Exec-vs-OnConflictUpdate fork, and skills/sqlb-queries' Trap 4 is rewritten as "the clause picks the terminal for you". docs/special-cases.md keeps its finding and notes what changed rather than being rewritten — the census is dated on purpose.

Per ADR-0016 the guard was broken deliberately: with the refusal removed the new test does not merely fail, it fails reporting the sqlb: no rows matched from this report.

Noted separately, and not addressed here: the skill living in sqlb's repo rather than the consumer's. That is #142/#143's ground.