The five open issues: a contradictory terminal, a default that fought SQL, a write set mistaken for a blast radius, and one table that could not serve two surfaces - #152
Merged
Conversation
…s so `ReadOnly` names its boundary — "unwritable through REST" — and `Immutable`, declared beside it and enforced by the same two places, read as a property of the column: "allows the column to be set at create time only". It is not one. The enforcement is `rest/item.go`'s update path and the `…Patch` body codegen omits it from; `UpdateRows[T]().Set(…)` from application code writes it, as does a hook, as does an action's write-back. That is the right design — `FieldDesc` has said "enforced by the REST layer. Go code going through the query engine directly is trusted" since the field was added — and the builder method is where somebody declaring a financial record actually reads. The generated client prose inherits the sentence and is right to drop the qualifier, because a client has only the REST door. So: one word to match its neighbour, plus where the guarantee lives if the column needs one. A `BEFORE UPDATE` trigger is the layer that sees the old row and the new one at once, which is the framing docs/concepts/domain-logic.md already uses. No behaviour change. Deliberately not: making `Immutable` police application writes, which would put the engine between an application and its own tables for a rule the database can hold properly. Closes #150 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…is refused "Give me exactly one row" and "do not produce a row on conflict" cannot both hold, and the way it used to resolve was the worst reading available: the conflict — the case the clause was added to allow — came back as `ErrNotFound`, through the same `if err != nil` as everything else, from a call whose job was to make the row exist. An idempotent insert reported failure on precisely the path it was written for. The failure profile is what makes this an API change rather than a doc fix. It inverts with state, so a test inserting into a clean database passes and only the second call fails — which is to say a test for "it inserts" passes and only a test for idempotency would catch it. And each doc was locally reasonable: `One` said nothing about conflict clauses, `OnConflictDoNothing` said nothing about terminals, and the interaction is what bites. So `One` refuses when the clause renders DO NOTHING, before the statement runs, and the message names both routes out because which one is right depends on what the caller wanted: `Exec`, whose empty slice and nil error are what "it was already there" looks like, or `OnConflictUpdate(target, target…)`, since a write that changes nothing is still a written row and a written row is a returned one. The condition is read off the same fields SQL renders on, so the two cannot drift. Broken on purpose, per ADR-0016: with the guard removed the new test does not merely fail, it fails reporting the `sqlb: no rows matched` from the issue. `pgtest`'s idempotency-key case, which pinned the old behaviour against a real database, now pins the refusal. Deliberately not: a distinct `ErrConflictSkipped` sentinel. It was the report's second option and it makes the misuse legible rather than impossible; refusing costs the same and leaves nothing to get wrong. Closes #146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A stored column reads its nullability off `NOT NULL` in the DDL, and the round trip checks it. A computed column has no DDL, so the default was doing all the work — and it pointed at the one reading an expression cannot satisfy. A correlated subquery that matches nothing is NULL, arithmetic over a nullable column is NULL, and a comparison against one is NULL. For the reporting application that was not an edge case: it was every row with no project, plus every row pointing at a deleted one, since a cross-module reference has no foreign key to prevent the second. What made it expensive is where the failure landed. `generate` had no opinion and `Diff` correctly ignores a column that is not in the database, so both gates were green; the symptom was a 500 at scan time saying `cannot scan NULL into *string`, naming the generated model rather than the `Computed` call that produced it, on data a fixture is unlikely to contain — ours only failed because one test happened to seed an entry with no project. So `Computed` defaults to nullable and `NotNull()` is the opt-in for an expression that cannot produce one: a `count(*)`, an `EXISTS`, a comparison already guarded against its own nulls. It is a claim rather than a check — nothing parses the SQL — and it fails in the direction the old default did not, since a pointer scans a non-null value fine and the reverse is the 500. `example/computed` now shows both: `is_overdue` earns `NotNull()` with the leading `due_date IS NOT NULL` it already had, and `progress` keeps the default because `NULLIF` is what it divides by. Proven against a real Postgres rather than asserted: `pgtest` reads a lookup that matches nothing into a pointer and into a plain string, and requires the second to fail naming the NULL. Broken on purpose too — with the default reverted, the schema test fails on the column the issue reported. Deliberately not: inferring nullability from the expression. It was the report's second option and its own objection stands — an incomplete inference is wrong in the unsafe direction, which is the direction this whole change is about. BREAKING: a computed column's generated field is now a pointer unless the declaration calls `NotNull()`. Recorded under *Will move* in docs/compatibility.md with the mechanical edit. Stored columns are untouched, as is the structs-first path, where the Go field's own type has always carried this. Closes #147 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r bounded it `Writes` is what the *envelope* persists — columns, on one row — and the same page of docs hands the verb a transaction it can write anything through. Two sentences that are individually right and jointly not, and the second one is not hypothetical: ADR-0043's own closing note says `completeTask` writes a comment row through `sqlb.TxFrom` and that "none of which the declaration mentions". It read as a note about an example. It was a gap in the contract. What makes it one is that `Writes` is reported as complete, by three tools, with no signal that a verb can exceed it: `sqlb impact` states it, the OpenAPI document carries it, and `--help` prints it. The CLI case is the sharp one, because ADR-0029's argument for the CLI is that `--help` answers a caller with no compile step, "such as an agent" — and a declared write set of two columns invites exactly the inference that the route is confined to one row. That inference can be wrong by ten tables. So `Touches []string` names tables beside `Writes`'s columns, and travels with it: the manifest, the contract snapshot, the OpenAPI description, the generated `Actions` doc comment, and the CLI help. A verb that declares nothing now gets a sentence saying so — the absence of a claim, not a checked bound. Unenforced, deliberately. Tracing what a Go func writes is not something the schema package can do, and the alternative to an unenforced claim was not an enforced one, it was silence: the failure mode becomes an over-broad claim rather than a confident understatement, and a test asserting the declaration against the statements the verb issued is the application's to write and can be. Validation refuses only what says nothing — an empty name, a duplicate — while an unknown table is accepted, since the cross-module write is the case the field exists for, and the table's own name is accepted because a verb writing other rows of it has no other way to say so. The lock got prose rather than code. The envelope's `FOR UPDATE` covers the row it fetched; statements issued through `TxFrom` take their own, in an order the application owns. docs/rest/actions.md says so directly under the `TxFrom` example, which is where the reader is at the moment they are handed the transaction. `example/tasks` declares `Touches: ["comments"]`, because it really does write one — which is what regenerated its CLI help, its skill and its contract baseline here. Deliberately not: growing actions to cover fan-out. ADR-0043's line — envelope declared, transition in Go — is right, and a write set naming columns on one row *should* understate an eleven-table command. What changed is that the generated documentation can now tell the two apart. Closes #149 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… serve two surfaces A headless shop reads `products` from a public storefront and from an admin panel, and the admin surface exists precisely to serve `cost_price_minor`, `supplier` and `internal_notes`. Neither lever reached it. `Hidden` is a property of the model and there is one model per table, so a column hidden for the storefront is hidden for the surface that exists to read it; `Expose` assigns `t.rest = &r`, so a second call replaces the first rather than adding a resource. The split therefore left the schema-first path for one of its halves — and a public/admin pair is neither an adoption path nor a legacy-struct case, which is what structs-first is otherwise for. The mechanism was already half-built and known to work. `rest.Options.Computed` is per-resource column reachability, added for cost (#92), and its own doc comment states the general case: a model is shared. An ordinary column is shared the same way. So `rest.Options.Columns` narrows a mount, and `filter.Options.Columns` carries it into the parser *and* into `Apply`'s default projection — both, because a resource whose parser refused a column while its projection selected it anyway would read the value out of Postgres on every request and drop it on the way out, which is a narrowing of the response and not of the query. A column not listed is unreachable: not projected, not selected, not filterable, not sortable, not searched, not nameable in `?select`, cleared off any row a body produced, refused as *unknown* by a PATCH, and absent from the list a rejection offers back. That last one is the point for a surface narrowed to conceal something — "column is not filterable" would confirm the column exists. Two refusals at mount, both startup-only because neither has a request that could report it: a name that is not a column, and a list that leaves out the primary key, which addresses a row, settles the ordering and is what a cursor is built from. One thing fell out. `listParams` and `capable` walked the model, so the OpenAPI document published a filter parameter for every computed column a resource had declined — a documented filter answering 400. They walk the binding's projection now, which closes that alongside the new case. What is *not* narrowed is recorded rather than left to be found: the response schema is the model's Go type, registered once as a component and shared by every mount of it, and the create/update body types are the caller's. Runtime behaviour is right in both; the document is wide. A test asserts the limitation and skips with instructions if it ever stops holding. Deliberately not: `Expose` appending with a `Columns` allowlist on `schema.REST`. It is the stronger answer — two generated resources, two client method sets, both on the drift gate — and it needs per-resource response types in the TypeScript and Dart emitters, a naming convention for the second resource, and restcompat keying that no longer assumes one resource per table. ADR-0050 records that, what this weaker answer costs, and what would trigger the larger one. Closes #148 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #146, #147, #148, #149, #150. Five commits, one per issue, each green on its own —
BASE=main..HEAD mise run bisect-checkpasses, sogit bisectstays trustworthy across the range.What changed
#150 —
Immutablenames its boundary.ReadOnlysays "unwritable through REST" andImmutable, enforced by the same two places, read as a property of the column. One word, plus where the guarantee lives if the column needs one. No behaviour change.#146 —
OneafterOnConflictDoNothingis refused. "Give me exactly one row" and "do not produce a row on conflict" cannot both hold, and it used to resolve asErrNotFoundon the conflict — failure reported on the exact path an idempotent insert exists to serve, from a call whose job was to make the row exist. The refusal names both routes out, because which one is right depends on whether the caller wants the row or only wants it to exist.#147 — a computed column is nullable unless it says otherwise. A stored column reads nullability off
NOT NULLand the round trip checks it; an expression has no DDL, so the default was doing all the work and pointed the wrong way.NotNull()is the opt-in. Breaking — see below.#149 —
Action.Touches.Writesis what the envelope persists; three tools reported it as the blast radius while the same page handed the verb a transaction.Touchesnames tables besideWrites's columns, unenforced by design, and travels with it throughsqlb impact, the manifest, the OpenAPI description and--help. A verb declaring nothing now says so, in those words. The lock got prose: the envelope'sFOR UPDATEcovers the row it fetched and nothing else.#148 —
rest.Options.Columns. Reachability becomes a property of the mount, generalisingOptions.Computedto stored columns, so one table serves a public surface and a privileged one. A column outside the list is unreachable — including absent from the list a rejection offers back, which for a surface narrowed to conceal something is the difference between a refusal and a disclosure.Breaking
93612a4isfeat(schema)!. A computed column's generated field is now a pointer unless the declaration callsNotNull(). Recorded under Will move in docs/compatibility.md with the mechanical edit; stored columns and the structs-first path are untouched. LeavingNotNull()off is the safe direction, so the migration can be lazy.Deliberately not done
ErrConflictSkipped(OnConflictDoNothing().One() reports ErrNotFound on the exact path an idempotent insert exists to serve #146's option 2). Makes the misuse legible rather than impossible, and refusing costs the same.Exposeappending with a schema-sideColumns(A table carries one Expose and rest.Options can narrow only computed columns, so a public surface and a privileged surface over the same table cannot both be generated #148's option 1). Columns turned out to be the small half — the public/admin split also differs by rows, ops, actions and auth, and the row half is aBeforeQueryhook, which resolves off the executor's registry and so is mount-shaped anyway. ADR-0050 records it with a revisit trigger.Records
New: ADR-0050. Revised: ADR-0041 (nullability inverted, and why inference was rejected), ADR-0043 (
Touches, and why its own 2026-07-31 revision already described the gap).Verification
preflight,lint,generate-check,impact-check,eject-check,test-pg,test-demo,test-fx,test-ts,test-cli,site-check,bisect-check— all green locally.example/tasksregenerated; its contract baseline re-recorded for the neutraltouchesaddition.Each guard was broken on purpose per ADR-0016. Two are worth naming: with #146's refusal removed the test does not merely fail, it fails reporting the reported
sqlb: no rows matched; and #147 is proved against a real Postgres, reading a subquery that matches nothing into a pointer and into a plain string, requiring the second to fail.Note for the reviewer
pagestriggers only on pushes tomain, so it will not appear on this PR. This branch adds an ADR and edits sevendocs/pages —mise run site-checkis green locally, butgh run list --commit <sha>after merge is the halfgh pr checkscannot cover. That is the trap that caught v0.7.0.🤖 Generated with Claude Code