From d8e7ae3df9a2db8e0b7364f8dc4845e9922e1858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrgen=20Ryannel?= Date: Wed, 5 Aug 2026 10:04:23 +0200 Subject: [PATCH 1/5] docs(schema): Immutable is enforced at the REST boundary, and now says so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- schema/field.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/schema/field.go b/schema/field.go index ba75a0a..1ccee61 100644 --- a/schema/field.go +++ b/schema/field.go @@ -912,7 +912,22 @@ func (f *Field) ReadOnly() *Field { return f } -// Immutable allows the column to be set at create time only. +// Immutable makes the column writable through REST at create time only. +// +// It names its boundary for the reason [Field.ReadOnly] does, and the boundary +// is the same one: the create body carries the column, the generated patch body +// omits it, and the update path refuses it if a request names it anyway. +// Nothing outside REST is policed — [sqlb.UpdateRows] from application code +// writes it, as does a hook, as does an action's write-back — because the +// engine does not stand between an application and its own tables. +// +// So this is a convention in [domain logic]'s sense of the word, and it closes +// the door the generated client opens. A column that must never change after +// insert, wherever the write comes from, wants the guarantee underneath it too: +// a BEFORE UPDATE trigger, which is the layer that sees the old row and the new +// one at once. +// +// [domain logic]: https://github.com/jryannel/sqlb/blob/main/docs/concepts/domain-logic.md func (f *Field) Immutable() *Field { f.d.Immutable = true return f From aef7ba465eb048b917c05a93146a35a993a01610 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrgen=20Ryannel?= Date: Wed, 5 Aug 2026 10:04:49 +0200 Subject: [PATCH 2/5] fix(mutate): One after OnConflictDoNothing is a contradiction, so it is refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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 --- docs/queries/mutations.md | 25 ++++++++++++---- docs/special-cases.md | 25 ++++++++++------ mutate.go | 57 ++++++++++++++++++++++++++++++++++-- pgtest/smallcases_test.go | 33 +++++++++++++++------ skills/sqlb-queries/SKILL.md | 14 +++++---- sqlb_test.go | 49 +++++++++++++++++++++++++++++++ 6 files changed, 173 insertions(+), 30 deletions(-) diff --git a/docs/queries/mutations.md b/docs/queries/mutations.md index bc47f14..cb8dd9a 100644 --- a/docs/queries/mutations.md +++ b/docs/queries/mutations.md @@ -12,11 +12,26 @@ being overwritten with `""`. Every statement returns the stored rows, so generated values land back in your structs without a follow-up read. `OnConflictDoNothing(target...)` and `OnConflictUpdate(target, update...)` cover -upserts. A row skipped by do-nothing is simply absent from the result, so `One` -returns `ErrNotFound`. When any row is skipped, **no** caller struct is written -back — the returned slice is shorter than the rows that went in, so position no -longer identifies them, and writing back by position would hand one row's -generated id to another. The returned slice is the account of what was written. +upserts. A row skipped by do-nothing is simply absent from the result, so the +terminal is `Exec`: an empty slice and a nil error are what "it was already +there" looks like. `One` after `OnConflictDoNothing` is **refused**, because the +only answer it could give a conflict is `ErrNotFound` — a failure reported on +the exact path an idempotent insert exists to serve. If you want the row back +whether or not this call created it, update the conflict target to itself: + +```go +sqlb.InsertRows(&p). + OnConflictUpdate([]string{"idem_key"}, "idem_key"). + One(ctx, db) +``` + +A write that changes nothing is still a written row, and a written row is a +returned one. + +When any row is skipped, **no** caller struct is written back — the returned +slice is shorter than the rows that went in, so position no longer identifies +them, and writing back by position would hand one row's generated id to another. +The returned slice is the account of what was written. ## An upsert that assigns more than the proposed row diff --git a/docs/special-cases.md b/docs/special-cases.md index 08d82f1..54a651a 100644 --- a/docs/special-cases.md +++ b/docs/special-cases.md @@ -60,7 +60,7 @@ running Postgres, and say what it did rather than what the source implies. | Range overlap / `EXCLUDE USING gist` | 2 | **Not expressible.** No range types, no exclusion constraints | | `tsvector` | 1 | **Deliberately out** — [ADR-0037](adr/0037-search-is-ilike-until-it-cannot-be.md). The blocker is the generated column, not the type | | `DISTINCT ON` | 1 | **`Raw` only,** measured. `RawSel` reaches it, but only as the *first* projection item — a positional convention nothing enforces, so getting it wrong is a syntax error at the database rather than a build error in Go | -| Idempotency key | 28 | **Works,** measured, and not the way it reads. `OnConflictDoNothing` skips the row, so `One` returns `ErrNotFound` and the caller's struct stays zeroed — a retried payment arriving as "not found". What returns the first call's row is `OnConflictUpdate(target, target…)`: a write that changes nothing is still a written row, and a written row is a returned one | +| Idempotency key | 28 | **Works,** measured, and not the way it reads. `OnConflictDoNothing` skips the row, so `One` has no row to return. It answered `ErrNotFound` — a retried payment arriving as "not found" — and since [#146](https://github.com/jryannel/sqlb/issues/146) it refuses instead, naming both routes out. What returns the first call's row is `OnConflictUpdate(target, target…)`: a write that changes nothing is still a written row, and a written row is a returned one | | Self-referencing parent (`parent_id`) | 0 here, universal | **Not expressible.** `Ref(name, target *TableDef)` needs the target value, which does not exist yet inside its own `Table(…)` call, and there is no `AddField`. `ExternalRef` compiles but gives up the type and `?expand` — and, measured, the foreign key too: a `parent_id` naming a row that is not there is accepted | | `WITH RECURSIVE` | 0 | **`Raw`, by design** — [vision](vision.md) non-goals | | Generated column / trigger / backfill | 1 trigger, 12 backfills | **DDL not rendered.** Hand-written migrations interleave; the "one source of truth" story keeps its asterisk | @@ -126,12 +126,18 @@ for the port report's ranking than the report itself makes. **An idempotency key needs a spelling that looks like a mistake.** `OnConflictDoNothing` is the obvious call and the wrong one: a skipped row is -absent from `RETURNING`, so `One` returns `ErrNotFound` and the caller's struct -is left at its zero value — which `Insert.writeBack` documents and defends, and -which turns a retried payment into a 404. What works is -`OnConflictUpdate([]string{"key"}, "key")`, updating the conflict target to -itself: a write that changes nothing is still a write, and a written row is -returned. It is correct, it is one line, and it reads like a typo. +absent from `RETURNING`, so `One` had no row to return and answered +`ErrNotFound`, leaving the caller's struct at its zero value — which +`Insert.writeBack` documents and defends, and which turns a retried payment into +a 404. What works is `OnConflictUpdate([]string{"key"}, "key")`, updating the +conflict target to itself: a write that changes nothing is still a write, and a +written row is returned. It is correct, it is one line, and it reads like a typo. + +*Updated after [#146](https://github.com/jryannel/sqlb/issues/146):* the pairing +is now refused at the terminal rather than answered, with a message naming +`Exec` for "make sure it exists" and the line above for "give me the row either +way". The finding stands; what changed is that it is no longer discovered from +production. Two further numbers the census did not have. `InsertRows` renders one statement, so bulk insert has a hard ceiling at 65535 bind parameters divided by the columns @@ -349,8 +355,9 @@ point where an HTTP status has to be chosen or a batch has to be sized. - **Idempotency key** — 28 lines in the corpus. Written, and the assumption in this line was wrong: `OnConflictDoNothing` does not return the first call's - row, it returns `ErrNotFound`. `OnConflictUpdate(target, target…)` does. See - [What the tests changed](#what-the-tests-changed). + row. `OnConflictUpdate(target, target…)` does. It returned `ErrNotFound`, and + after [#146](https://github.com/jryannel/sqlb/issues/146) the pairing is + refused outright. See [What the tests changed](#what-the-tests-changed). - **Optimistic concurrency** — a version column, `Update…Where(version.Eq(n))`, and the zero-rows-affected path. Written, and the mechanism is entirely there. What is not is the distinction a 409 needs: a stale version and a missing row diff --git a/mutate.go b/mutate.go index 5013fcc..e116483 100644 --- a/mutate.go +++ b/mutate.go @@ -47,6 +47,13 @@ type conflictSet struct { value Expr } +// skipsRows reports whether the clause renders DO NOTHING, which is the one +// shape where a row can be absent from RETURNING. The condition is the same one +// SQL renders on, and it is read from the same fields, so the two cannot drift. +func (c *conflictClause) skipsRows() bool { + return c != nil && len(c.doUpdate) == 0 && len(c.sets) == 0 +} + // InsertRows starts an INSERT for one or more rows. The rows are pointers so // that hooks and returned database values can be written back into them. func InsertRows[T any](rows ...*T) *Insert[T] { @@ -112,7 +119,14 @@ func (i *Insert[T]) checkColumns(method string, columns []string) { // Because a skipped row cannot be told apart from its neighbours in what // comes back, a statement that skips any row leaves every caller struct // untouched — the returned slice is then the only account of what was -// written. See Exec. +// written. So the terminal is [Insert.Exec], whose empty slice and nil error +// are what "it was already there" looks like. +// +// [Insert.One] is refused after this call rather than answering ErrNotFound on +// the conflict, which is the case an idempotent insert exists to serve (#146). +// If the row itself is wanted whether or not this call created it, the spelling +// is OnConflictUpdate with the target as its own update column — a write that +// changes nothing is still a written row, and a written row is a returned one. func (i *Insert[T]) OnConflictDoNothing(target ...string) *Insert[T] { i.conflict = &conflictClause{target: target} return i @@ -383,19 +397,58 @@ func (i *Insert[T]) writeBack(stored []T) { } // One inserts a single row and returns it. +// +// It is refused over ON CONFLICT DO NOTHING. "Give me exactly one row" and "do +// not produce a row on conflict" are a contradiction, and the way it used to +// resolve was the worst 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. The failure +// also inverts with state, so a test that inserts into a clean database passes +// and only the second call fails (#146). func (i *Insert[T]) One(ctx context.Context, db Executor) (T, error) { var zero T + if err := i.refuseSkippingTerminal(); err != nil { + return zero, err + } stored, err := i.Exec(ctx, db) if err != nil { return zero, err } if len(stored) == 0 { - // Reachable via ON CONFLICT DO NOTHING. + // Unreachable now that DO NOTHING is refused above, and kept because + // One's contract is "one row or an error" and a silent index panic is + // not the way to discover a statement that returned none. return zero, ErrNotFound } return stored[0], nil } +// refuseSkippingTerminal rejects One over a clause that can return no row. +// +// Refused at the terminal rather than at OnConflictDoNothing, because the +// clause is fine and it is the pairing that is not — and the terminal is the +// call the author is about to get wrong. +func (i *Insert[T]) refuseSkippingTerminal() error { + if !i.conflict.skipsRows() { + return nil + } + alt := "OnConflictUpdate with the conflict target as its own update column" + if t := i.conflict.target; len(t) > 0 { + quoted := make([]string, len(t)) + for n, name := range t { + quoted[n] = fmt.Sprintf("%q", name) + } + list := strings.Join(quoted, ", ") + alt = fmt.Sprintf("OnConflictUpdate([]string{%s}, %s)", list, list) + } + return fmt.Errorf( + "sqlb: One after OnConflictDoNothing on %s: a skipped insert returns no row, "+ + "so a conflict would answer ErrNotFound — the case the clause exists to allow;\n"+ + " call Exec instead, whose empty slice and nil error are what \"it was already there\" looks like,\n"+ + " or %s if the row is wanted whether or not this call created it", + i.model.Table, alt) +} + // Update is an UPDATE statement over model T. type Update[T any] struct { model *Model diff --git a/pgtest/smallcases_test.go b/pgtest/smallcases_test.go index 1c01319..b9fa276 100644 --- a/pgtest/smallcases_test.go +++ b/pgtest/smallcases_test.go @@ -47,15 +47,17 @@ func paymentsDB(t *testing.T) *sqlb.DB { // RETURNING. // // The finding is that the obvious spelling does not do it. OnConflictDoNothing -// skips the row, and a skipped row is *absent* from RETURNING — so One reports -// ErrNotFound and the caller's struct is left at its zero value, which is the -// deliberate choice Insert.writeBack documents. A retried payment arriving as -// "not found" is the opposite of idempotent. +// skips the row, and a skipped row is *absent* from RETURNING — so One used to +// report ErrNotFound with the caller's struct at its zero value, which is a +// retried payment arriving as "not found". Since #146 the pairing is refused +// outright, at the terminal, before the statement runs; this test now pins the +// refusal, because what the caller needs is a message rather than a sentinel +// that reads as a real database answer. // // What does do it is OnConflictUpdate with the conflict target as its own // update column. `DO UPDATE SET key = EXCLUDED.key` is a write that changes // nothing, and a written row is a returned row. That is the whole trick, and it -// is written down nowhere else in the repository. +// is what the refusal now names. // // Deliberately not: a claim about concurrent retries of the *same* key. Two // simultaneous inserts on one index serialise, and the loser takes the update @@ -75,14 +77,27 @@ func TestIdempotencyKeyMakesASecondCallReturnTheFirstCallsRow(t *testing.T) { t.Fatal("first call returned no generated id") } - // The spelling that looks right and is not. + // The spelling that looks right and is not — now refused rather than + // answered. retry := Payment{Key: "charge-7", Amount: 250} _, err = sqlb.InsertRows(&retry).OnConflictDoNothing("key").One(ctx, db) - if !errors.Is(err, sqlb.ErrNotFound) { - t.Errorf("do-nothing retry: err = %v, want ErrNotFound", err) + switch { + case err == nil: + t.Error("do-nothing retry was accepted; it answers ErrNotFound on the idempotent path") + case errors.Is(err, sqlb.ErrNotFound): + t.Errorf("do-nothing retry still reports a missing row: %v", err) + case !strings.Contains(err.Error(), "OnConflictUpdate"): + t.Errorf("the refusal should name the spelling that works, got: %v", err) } if retry.ID != 0 { - t.Errorf("do-nothing retry wrote back id %d; a skipped row must leave the caller's struct alone", retry.ID) + t.Errorf("do-nothing retry wrote back id %d; a refused statement must not run", retry.ID) + } + + // And it is refused before the statement runs, so the row count is + // untouched — the assertion at the end of this test would pass either way, + // since DO NOTHING inserts nothing anyway. + if _, _, err := sqlb.InsertRows(&retry).OnConflictDoNothing("key").SQL(); err != nil { + t.Errorf("the clause itself is fine and only the pairing is refused; SQL() = %v", err) } // The spelling that is. diff --git a/skills/sqlb-queries/SKILL.md b/skills/sqlb-queries/SKILL.md index 946952b..ad24fab 100644 --- a/skills/sqlb-queries/SKILL.md +++ b/skills/sqlb-queries/SKILL.md @@ -147,17 +147,21 @@ q.Where(sqlb.F("at").Gte(start), sqlb.F("at").Lt(end)) // → WHERE ("at" >= $1) AND ("at" < $2) ``` -### Trap 4 — `OnConflictDoNothing` + `One` returns `ErrNotFound` +### Trap 4 — `OnConflictDoNothing` picks the terminal for you An idempotency key does not behave the way it reads. `DO NOTHING` skips the -row, so nothing is returned and the caller's struct stays zeroed — a retried -payment arriving as "not found". +row, so nothing is returned and there is no row for `One` to give back. Since +#146 the pairing is refused at the terminal rather than answered with +`ErrNotFound`, but the choice is still yours to make: ```go -// Returns ErrNotFound on the retry: +// Refused, with a message naming both of the following: sqlb.InsertRows(&p).OnConflictDoNothing("idem_key").One(ctx, db) -// Returns the first call's row — a write that changes nothing is still a +// "Make sure it exists" — empty slice, nil error, on the conflict: +sqlb.InsertRows(&p).OnConflictDoNothing("idem_key").Exec(ctx, db) + +// "Give me the row either way" — a write that changes nothing is still a // written row, and a written row is a returned one: sqlb.InsertRows(&p).OnConflictUpdate([]string{"idem_key"}, "idem_key").One(ctx, db) ``` diff --git a/sqlb_test.go b/sqlb_test.go index 0eeba57..3942ecf 100644 --- a/sqlb_test.go +++ b/sqlb_test.go @@ -512,6 +512,55 @@ func TestInsertDoesNotWriteBackWhenAConflictSkippedARow(t *testing.T) { } } +// One after OnConflictDoNothing is refused, because the alternative is that the +// conflict — the case the clause exists to allow — arrives as ErrNotFound from +// a call whose job was to make the row exist (#146). +// +// The harness returns no rows, which is what the database does on the second +// call: with the refusal removed this test does not merely fail, it fails with +// the ErrNotFound the issue reported. +func TestOneIsRefusedAfterOnConflictDoNothing(t *testing.T) { + h := newHarness(t, storedUserColumns, nil) + defer h.close() + + u := &User{Email: "ada@example.com", Name: "Ada", OrgID: "acme"} + _, err := sqlb.InsertRows(u).OnConflictDoNothing("email").One(context.Background(), h.db) + if err == nil { + t.Fatal("One after OnConflictDoNothing was accepted; it answers ErrNotFound on the idempotent path") + } + if errors.Is(err, sqlb.ErrNotFound) { + t.Errorf("the refusal must not be ErrNotFound, which is the confusion it exists to remove: %v", err) + } + // ADR-0011: a rejection names what would have been accepted. Both routes + // out are here, because which one is right depends on whether the caller + // wants the row or only wants it to exist. + for _, want := range []string{"Exec", `OnConflictUpdate([]string{"email"}, "email")`} { + if !contains(err.Error(), want) { + t.Errorf("the error should name %s, got: %v", want, err) + } + } +} + +// The refusal is about DO NOTHING specifically. A conflict clause that updates +// something returns a row on every path, so One over it is exactly right — and +// it is the spelling the refusal recommends, so breaking it would leave the +// error pointing at a call that does not work. +func TestOneIsAllowedAfterOnConflictUpdate(t *testing.T) { + h := newHarness(t, storedUserColumns, [][]any{storedUser("gen-ada", "ada@example.com")}) + defer h.close() + + u := &User{Email: "ada@example.com", Name: "Ada", OrgID: "acme"} + got, err := sqlb.InsertRows(u). + OnConflictUpdate([]string{"email"}, "email"). + One(context.Background(), h.db) + if err != nil { + t.Fatalf("One after OnConflictUpdate: %v", err) + } + if got.ID != "gen-ada" { + t.Errorf("returned id = %q, want gen-ada", got.ID) + } +} + // storedUserColumns is the RETURNING order writeReturning emits for User. var storedUserColumns = []string{"id", "email", "name", "age", "org_id", "password_hash", "created_at"} From 93612a4a4f054de303301e8070fec712afc867d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrgen=20Ryannel?= Date: Wed, 5 Aug 2026 10:05:25 +0200 Subject: [PATCH 3/5] feat(schema)!: a computed column is nullable unless it says otherwise MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- codegen/computed_test.go | 12 ++++- codegen/override_test.go | 2 +- docs/adr/0041-computed-fields.md | 21 +++++++++ docs/compatibility.md | 11 +++++ docs/schema/README.md | 21 +++++++++ example/computed/declared.go | 11 ++++- pgtest/computed_test.go | 81 ++++++++++++++++++++++++++++++++ schema/computed_test.go | 40 ++++++++++++++++ schema/field.go | 41 ++++++++++++++++ 9 files changed, 235 insertions(+), 5 deletions(-) diff --git a/codegen/computed_test.go b/codegen/computed_test.go index f9f22ed..6ed4152 100644 --- a/codegen/computed_test.go +++ b/codegen/computed_test.go @@ -16,12 +16,15 @@ func computedFixture() *schema.Registry { schema.Date("due_date").Nullable().Filterable().Sortable(), schema.Int("open_tasks").Filterable(), + // Nullable, and not by an oversight: due_date is nullable, so the + // comparison is NULL for every row without one. schema.Computed("is_overdue", schema.TypeBool, schema.FromSQL("due_date < current_date AND open_tasks > 0")).Filterable(), + // EXISTS is true or false and never NULL, which is what NotNull claims. schema.Computed("is_starred", schema.TypeBool, schema.FromSQL("EXISTS (SELECT 1 FROM stars s "+ "WHERE s.project_id = projects.id AND s.member_id = ?)")). - Needs("viewer").Filterable(), + NotNull().Needs("viewer").Filterable(), ).Expose(schema.REST{Ops: schema.CRUD | schema.OpList}) return r } @@ -29,11 +32,16 @@ func computedFixture() *schema.Registry { // One declaration, and the field is in the row type with the expression beside // it — the method rather than the tag, because SQL does not fit in a // comma-separated list. +// +// The two derived fields also pin the nullability default from #147: a computed +// column is a pointer unless the declaration claims NotNull, because an +// expression can be NULL and there is no DDL to read the answer off. func TestGeneratedModelCarriesTheExpression(t *testing.T) { models := generate(t, computedFixture())["models_gen.go"] for _, want := range []string{ - `IsOverdue bool ` + "`" + `db:"is_overdue" json:"is_overdue" sqlb:"type:bool,filter,readonly"` + "`", + `IsOverdue *bool ` + "`" + `db:"is_overdue" json:"is_overdue" sqlb:"type:bool,filter,readonly"` + "`", + `IsStarred bool ` + "`" + `db:"is_starred" json:"is_starred" sqlb:"type:bool,filter,readonly"` + "`", "func (Project) ComputedColumns() []sqlb.Computed {", `{Name: "is_overdue", Expr: "due_date < current_date AND open_tasks > 0"},`, `Needs: []string{"viewer"}`, diff --git a/codegen/override_test.go b/codegen/override_test.go index e567ac3..adbb569 100644 --- a/codegen/override_test.go +++ b/codegen/override_test.go @@ -161,7 +161,7 @@ func TestOverriddenComputedColumnStillImportsSqlb(t *testing.T) { schema.UUIDv7("id").PrimaryKey(), schema.Int("open_tasks"), schema.Computed("is_overdue", schema.TypeBool, - schema.FromSQL("open_tasks > 0")).Filterable(), + schema.FromSQL("open_tasks > 0")).NotNull().Filterable(), ) // The override has to match the computed column, which is what puts it // behind the guard. Matching by type is the narrowest way to say so. diff --git a/docs/adr/0041-computed-fields.md b/docs/adr/0041-computed-fields.md index a40b657..55241f3 100644 --- a/docs/adr/0041-computed-fields.md +++ b/docs/adr/0041-computed-fields.md @@ -301,3 +301,24 @@ reversible, a filter grammar is not. made a computed column opt-in per reader, which removed the cost argument for keeping derived work out of SQL. The taxonomy keeps four rows because a record that quietly drops the option it rejected is not a record of the decision. + +- 2026-08-05 — **Nullability inverted.** A computed column is now nullable + unless the declaration writes `NotNull()`, where a stored one stays not-null + unless it writes `Nullable()`. The record never stated a default, and the one + it inherited was the one an expression cannot honour: a correlated subquery + matching nothing is `NULL`, which for the reporting application was not an + edge case but every row with no project plus every row pointing at a deleted + one — a cross-module reference having no foreign key to prevent the second + ([#147](https://github.com/jryannel/sqlb/issues/147)). + + What made it expensive is where it landed. A stored column reads its + nullability off `NOT NULL` and the round trip checks it; a computed column has + no DDL, so `generate` had no opinion and `Diff` correctly ignored a column that + is not in the database. Both gates were green and the failure was a 500 at scan + time — `cannot scan NULL into *string`, naming the generated model rather than + the declaration that produced it — on data a fixture is unlikely to contain. + + Inference over the expression was considered and rejected for the reason the + report gave: it is wrong in the unsafe direction wherever it is incomplete. + `NotNull()` is a claim rather than a check, and it fails the other way, which + is the direction this record already prefers everywhere else. diff --git a/docs/compatibility.md b/docs/compatibility.md index 35b898d..e77b60f 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -103,6 +103,17 @@ Named in advance, so the break is a documented plan rather than a surprise. failure this prevents is silent, so its migration must not be. The mechanical edits are `On[T]()` → `On[T](reg)`, `OnIn[T](reg)` → `On[T](reg)`, `PublishChangesIn` → `PublishChanges`, and a `WithHooks(reg)` on the handle. +- ~~**A computed column's nullability.**~~ Landed: `schema.Computed` now + defaults to nullable, so the generated field is a pointer unless the + declaration calls `NotNull()`. It moved because the old default was the one + reading the expression cannot satisfy — a correlated subquery that matches + nothing is `NULL`, and the failure was a 500 at scan time on rows a fixture is + unlikely to contain, from a declaration `generate` and the drift gate were + both happy with ([#147](https://github.com/jryannel/sqlb/issues/147)). The + mechanical edit is `NotNull()` on every computed column whose expression + genuinely cannot produce a `NULL`; leaving it off is the safe direction, since + a pointer scans a non-null value fine. Stored columns are untouched, as is the + structs-first path, where the Go field's own type has always carried this. - **Terminal call signatures**, when Go 1.27 arrives. `sqlb.Collect[R](ctx, db, b)`, `filter.Apply(b, q)` and the `db` threaded through every terminal call all gain method forms, because a method on a concrete type cannot introduce a diff --git a/docs/schema/README.md b/docs/schema/README.md index e29abea..88cd2d9 100644 --- a/docs/schema/README.md +++ b/docs/schema/README.md @@ -173,6 +173,27 @@ declaration reaches the projection, `?filter=is_overdue.eq.true` and `?sort=-progress` at once. The projection aliases it back to the column name, which is what lets the row scan into the field. +**A computed column is nullable unless it says otherwise**, which is the +opposite of a stored one and the same as SQL. A correlated subquery that matches +nothing is `NULL`, arithmetic over a nullable column is `NULL`, and a comparison +against one is `NULL` — and there is no `NOT NULL` in any DDL for the generator +to read the answer off, because there is no DDL. So the three declarations above +generate `*bool`, `*int32` and `*bool`, and `NotNull()` is how an expression +that cannot produce one says so: + +```go +schema.Computed("total_tasks", schema.TypeInt, + schema.FromSQL("(SELECT count(*) FROM tasks t WHERE t.project_id = projects.id)")). + NotNull(), // count(*) is 0, never NULL +``` + +It is a claim rather than a check — nothing parses the SQL — so it belongs on +the `count(*)`, the `EXISTS`, and the comparison already guarded against its own +nulls. The default runs the other way because that is the direction that fails +safely: a pointer scans a non-null value fine, and the reverse is a 500 saying +`cannot scan NULL into *string`, on rows a fixture is unlikely to contain +([#147](https://github.com/jryannel/sqlb/issues/147)). + **A subquery is projection-only unless you say otherwise.** Writing `Filterable()` on one is the acknowledgement that a subquery in a `WHERE` runs once per candidate row. `Searchable()` says the same thing about `?search`, and diff --git a/example/computed/declared.go b/example/computed/declared.go index 95ab2ed..d683403 100644 --- a/example/computed/declared.go +++ b/example/computed/declared.go @@ -41,11 +41,18 @@ func declaredRegistry() *schema.Registry { // expression reads current_date — may not be sorted. The refusal is at // declaration time, not at request time: a keyset pages on the sort // column, and this one is a different value on the next page. + // + // NotNull is what the leading `due_date IS NOT NULL` earns. Without the + // guard the comparison would be NULL for every row with no due date, + // and a computed column is nullable unless it says otherwise (#147). schema.Computed("is_overdue", schema.TypeBool, schema.FromSQL("(due_date IS NOT NULL AND due_date < current_date AND open_tasks > 0)")). - Filterable(), + NotNull().Filterable(), // Arithmetic over two stored columns. Stable, so it may be sorted. + // NULLIF is there to avoid dividing by zero, and what it divides by + // instead is NULL — so this one keeps the default, and Nullable says so + // out loud rather than relying on it. schema.Computed("progress", schema.TypeInt, schema.FromSQL("(completed_tasks * 100 / NULLIF(total_tasks, 0))")). Nullable().Filterable().Sortable(), @@ -58,7 +65,7 @@ func declaredRegistry() *schema.Registry { schema.Computed("is_starred", schema.TypeBool, schema.FromSQL("EXISTS (SELECT 1 FROM project_stars s "+ "WHERE s.project_id = projects.id AND s.member_id = ?)")). - Needs("viewer").Filterable(), + NotNull().Needs("viewer").Filterable(), ).Expose(schema.REST{Ops: schema.CRUD | schema.OpList}) return r } diff --git a/pgtest/computed_test.go b/pgtest/computed_test.go index 9611e72..789e904 100644 --- a/pgtest/computed_test.go +++ b/pgtest/computed_test.go @@ -262,3 +262,84 @@ func TestComputedInReturningRunsAgainstPostgres(t *testing.T) { t.Error("is_starred should not be answered by a write") } } + +// A correlated subquery that matches nothing is NULL, which is why a computed +// column declared through the schema DSL is nullable unless it says otherwise +// (#147). +// +// The two models below project the same expression into a pointer and into a +// plain string. Both are legal Go and only one of them survives the row with +// nothing to match — and which rows those are is not visible from the +// declaration, so the report that produced this arrived as a 500 in production +// rather than as anything `sqlb generate` or the drift gate could have said. +type CompLookup struct { + ID int64 `db:"id" sqlb:"pk,default"` + Name string `db:"name"` + Owner *string `db:"owner_name" sqlb:"readonly"` +} + +func (CompLookup) TableName() string { return "compprojects" } + +func (CompLookup) ComputedColumns() []sqlb.Computed { + return []sqlb.Computed{{Name: "owner_name", Expr: compLookupExpr}} +} + +// CompLookupNotNull is the same projection typed the way the old default +// generated it. +type CompLookupNotNull struct { + ID int64 `db:"id" sqlb:"pk,default"` + Name string `db:"name"` + Owner string `db:"owner_name" sqlb:"readonly"` +} + +func (CompLookupNotNull) TableName() string { return "compprojects" } + +func (CompLookupNotNull) ComputedColumns() []sqlb.Computed { + return []sqlb.Computed{{Name: "owner_name", Expr: compLookupExpr}} +} + +// LIMIT 1 because a scalar subquery returning two rows is an error of its own, +// and that is not the failure under test. +const compLookupExpr = `(SELECT s.member_id::text FROM compstars s + WHERE s.project_id = compprojects.id LIMIT 1)` + +func TestAComputedLookupThatMatchesNothingIsNull(t *testing.T) { + t.Parallel() + ctx := context.Background() + raw := computedDB(t) + seedComputedRows(t, raw) + + rows, err := sqlb.Query[CompLookup](). + WithComputed("owner_name"). + OrderBy(sqlb.F("name").Asc()). + All(ctx, sqlb.New(raw)) + if err != nil { + t.Fatalf("the nullable projection did not run: %v", err) + } + if len(rows) != 3 { + t.Fatalf("got %d rows, want 3", len(rows)) + } + // apollo is the only starred row; the other two correlate to nothing. + if rows[0].Owner == nil || *rows[0].Owner != "7" { + t.Errorf("apollo owner = %v, want 7", rows[0].Owner) + } + for _, row := range rows[1:] { + if row.Owner != nil { + t.Errorf("%s owner = %q, want NULL — no row matched the subquery", row.Name, *row.Owner) + } + } + + // And the same read against the non-null spelling, which is the failure the + // default now avoids. Asserted rather than assumed: without it this test + // would pass on a fixture where every row happens to match, which is + // exactly how the bug survived. + _, err = sqlb.Query[CompLookupNotNull](). + WithComputed("owner_name"). + All(ctx, sqlb.New(raw)) + if err == nil { + t.Fatal("scanning NULL into a non-pointer string succeeded; the whole reason for the nullable default is that it does not") + } + if !strings.Contains(err.Error(), "NULL") { + t.Errorf("the scan failure should name the NULL, got: %v", err) + } +} diff --git a/schema/computed_test.go b/schema/computed_test.go index 8d3d76e..0a1a0d1 100644 --- a/schema/computed_test.go +++ b/schema/computed_test.go @@ -161,6 +161,46 @@ func TestComputedSortableIsAllowedWhenStable(t *testing.T) { } } +// A computed column is nullable unless it says otherwise, which is the opposite +// of a stored one (#147). +// +// The default that assumed non-null was not merely unhelpful: a correlated +// subquery matching nothing produced a 500 at scan time, from a declaration +// `sqlb generate` accepted and the drift gate ignored, on rows a fixture is +// unlikely to contain. Nullable is the direction that fails safely — a pointer +// scans a non-null value fine, and the reverse is the 500. +func TestComputedIsNullableUnlessItSaysOtherwise(t *testing.T) { + r := schema.NewRegistry() + projects := r.Table("projects", + schema.UUIDv7("id").PrimaryKey(), + schema.Int("open_tasks"), + + // The shape from the issue: a cross-module lookup with no foreign key, + // so a row pointing at nothing is ordinary rather than exceptional. + schema.Computed("project_name", schema.TypeText, + schema.FromSQL("(SELECT p.name FROM projects p WHERE p.id = time_entries.project_id)")), + // count(*) over a subquery is 0, never NULL. + schema.Computed("total_tasks", schema.TypeInt, + schema.FromSQL("(SELECT count(*) FROM tasks t WHERE t.project_id = projects.id)")). + NotNull(), + ) + if err := r.Validate(); err != nil { + t.Fatalf("valid schema rejected: %v", err) + } + + if !projects.Field("project_name").Desc().Nullable { + t.Error("a computed column defaulted to NOT NULL; an expression that matches nothing is NULL, and the model has to be able to hold it") + } + if projects.Field("total_tasks").Desc().Nullable { + t.Error("NotNull did not take on a computed column") + } + // The default runs the other way for storage, where the DDL carries the + // answer and the round trip checks it. + if projects.Field("open_tasks").Desc().Nullable { + t.Error("a stored column defaulted to nullable") + } +} + // The manifest is what a program reads to answer "what does this endpoint // serve, and what did the server have to do to serve it". func TestComputedInManifest(t *testing.T) { diff --git a/schema/field.go b/schema/field.go index 1ccee61..ebac779 100644 --- a/schema/field.go +++ b/schema/field.go @@ -373,6 +373,28 @@ func Vector(name string, dim int) *Field { // storage: it emits no DDL in either direction, Diff does not see it, no insert // names it and no update assigns it. ADR-0041 has the shape and the reasons. // +// # Nullability runs the other way +// +// A computed column is [Field.Nullable] unless it says otherwise, which is the +// opposite of a stored one and the same as SQL: a correlated subquery that +// matches nothing is NULL, an arithmetic expression over a nullable column is +// NULL, and a comparison against one is NULL. A stored column reads its +// nullability off `NOT NULL` in the DDL and the round trip checks it; an +// expression has no DDL, so the default is doing all the work, and the default +// that assumed otherwise failed at scan time on the first row with nothing to +// match — a 500 with `cannot scan NULL into *string`, from a declaration +// `sqlb generate` and the drift gate were both happy with (#147). +// +// [Field.NotNull] is the opt-in for an expression that cannot produce one: +// +// schema.Computed("total_tasks", schema.TypeInt, +// schema.FromSQL("(SELECT count(*) FROM tasks t WHERE t.project_id = projects.id)")). +// NotNull() +// +// It is a claim, not a check — nothing parses the SQL — and it fails in the +// direction the default does not: a nullable column typed as a pointer scans a +// non-null value fine, where the reverse is the 500. +// // # What each form may claim // // The expression is rendered as written, so a name in it resolves the way @@ -432,6 +454,9 @@ func Computed(name string, t Type, e ComputedExpr) *Field { // every write path to check is what keeps the generated create and update // bodies correct without knowing this feature exists. f.d.ReadOnly = true + // Nullable by default, which is the opposite of a stored column and the + // same as SQL. See the doc comment above for the argument. + f.d.Nullable = true return f } @@ -734,6 +759,22 @@ func (f *Field) Nullable() *Field { return f } +// NotNull is the opposite claim, and the one [Computed] needs. +// +// A stored column is not null unless it says otherwise, so writing this on one +// restates the default and is harmless. A computed column defaults the other +// way — an expression can be NULL and Postgres has no NOT NULL to read it from +// — so this is where the author says the expression cannot produce one, and it +// is a claim rather than a check: nothing parses the SQL (#147). +// +// Worth it when the expression is a `count(*)`, an `EXISTS`, or a comparison +// already guarded against its own nulls, because those are the ones where a +// pointer in the generated model is noise. +func (f *Field) NotNull() *Field { + f.d.Nullable = false + return f +} + // Unique adds a single-column unique constraint. func (f *Field) Unique() *Field { f.d.Unique = true From 2dab3aeddf6c0db2f2aed30175dedc480edc5efc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrgen=20Ryannel?= Date: Wed, 5 Aug 2026 10:05:48 +0200 Subject: [PATCH 4/5] feat(schema): an action declares what it Touches, because Writes never bounded it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- codegen/action.go | 29 ++++++-- codegen/action_test.go | 62 +++++++++++++++-- codegen/cliaction.go | 22 +++++- codegen/skill.go | 12 ++-- docs/adr/0043-declared-actions.md | 35 ++++++++++ docs/rest/actions.md | 47 +++++++++++++ .../tasks/.claude/skills/sqlb-schema/SKILL.md | 8 ++- example/tasks/cli/cli_gen.go | 5 +- example/tasks/rest_gen.go | 10 ++- example/tasks/restcontract.json | 3 + example/tasks/sqlb.json | 3 + example/tasks/taskschema/schema.go | 6 ++ rest/action.go | 36 +++++++++- rest/action_test.go | 46 +++++++++++++ restcompat/action.go | 13 +++- restcompat/action_test.go | 22 ++++++ schema/action.go | 69 +++++++++++++++++-- schema/action_test.go | 53 +++++++++++++- schema/manifest.go | 10 ++- 19 files changed, 458 insertions(+), 33 deletions(-) diff --git a/codegen/action.go b/codegen/action.go index 6091002..14112be 100644 --- a/codegen/action.go +++ b/codegen/action.go @@ -170,8 +170,15 @@ func renderActions(b *bytes.Buffer, defs []actionDef) { fmt.Fprintf(b, "\t//\n\t// %s\n", desc) } if w := d.action.Writes; len(w) > 0 { - fmt.Fprintf(b, "\t//\n\t// The envelope persists %s afterwards, and nothing else.\n", - quoteList(w)) + fmt.Fprintf(b, "\t//\n\t// The envelope persists %s off this row afterwards, and nothing\n"+ + "\t// else — which bounds the envelope and not the func: the transaction\n"+ + "\t// is yours through sqlb.TxFrom, and statements issued there take\n"+ + "\t// their own locks, in an order this code owns.\n", quoteList(w)) + } + if tt := d.action.Touches; len(tt) > 0 { + fmt.Fprintf(b, "\t//\n\t// Declared reach beyond that row: %s. Nothing checks it; it is\n"+ + "\t// what the route tells `sqlb impact`, the OpenAPI document and the\n"+ + "\t// CLI's --help, so a change here belongs in the schema.\n", quoteList(tt)) } if d.action.IsCollection() { fmt.Fprintf(b, "\t%s func(context.Context, %s) error\n", d.goName(), d.inputName()) @@ -201,11 +208,10 @@ func renderActionCalls(b *bytes.Buffer, optsVar string, defs []actionDef) { fmt.Fprintf(b, "\t\tDescription: %q,\n", s) } if w := d.action.Writes; len(w) > 0 { - quoted := make([]string, len(w)) - for i, name := range w { - quoted[i] = fmt.Sprintf("%q", name) - } - fmt.Fprintf(b, "\t\tWrites: []string{%s},\n", strings.Join(quoted, ", ")) + fmt.Fprintf(b, "\t\tWrites: []string{%s},\n", quotedList(w)) + } + if tt := d.action.Touches; len(tt) > 0 { + fmt.Fprintf(b, "\t\tTouches: []string{%s},\n", quotedList(tt)) } if len(d.action.Body) > 0 { fmt.Fprintf(b, "\t\tHasBody: true,\n") @@ -214,6 +220,15 @@ func renderActionCalls(b *bytes.Buffer, optsVar string, defs []actionDef) { } } +// quotedList renders a name set as Go source: "status", "closed_at". +func quotedList(names []string) string { + quoted := make([]string, len(names)) + for i, name := range names { + quoted[i] = fmt.Sprintf("%q", name) + } + return strings.Join(quoted, ", ") +} + // quoteList renders a column set for a doc comment: `status` and `closed_at`. func quoteList(names []string) string { quoted := make([]string, len(names)) diff --git a/codegen/action_test.go b/codegen/action_test.go index 65023fe..700f098 100644 --- a/codegen/action_test.go +++ b/codegen/action_test.go @@ -30,14 +30,18 @@ func actionFixture() *schema.Registry { schema.Timestamp("completed_at"), ), Writes: []string{"status", "closed_at"}, + // The verb also writes a comment row through the transaction, + // which the write set above cannot say and used to leave unsaid. + Touches: []string{"comments"}, }). Action(schema.Action{ Name: "archive", Writes: []string{"status"}, }). Action(schema.Action{ - Name: "purge-archived", - Path: "/purge-archived", + Name: "purge-archived", + Path: "/purge-archived", + Touches: []string{"tasks", "comments"}, }) return r } @@ -106,6 +110,37 @@ func TestActionsStructNamesEveryVerbWithItsSignature(t *testing.T) { } } +// The declared reach reaches the runtime spec and the doc comment above the +// func the application writes. +// +// Both matter and for different readers: the spec is what puts the sentence in +// the OpenAPI document, and the comment is what the author of the verb sees at +// the moment they are deciding whether to reach for sqlb.TxFrom (#149). +func TestTheDeclaredReachReachesTheGeneratedCode(t *testing.T) { + src := generate(t, actionFixture())["rest_gen.go"] + + if !strings.Contains(specOf(t, src, "complete"), `Touches: []string{"comments"}`) { + t.Errorf("the action's spec does not carry its reach:\n%s", specOf(t, src, "complete")) + } + // A verb with no declared reach emits no field, so a schema that never uses + // this generates what it generated before. + if strings.Contains(specOf(t, src, "archive"), "Touches") { + t.Error("a verb with no declared reach emitted a Touches field") + } + + for _, want := range []string{ + "// Declared reach beyond that row: `comments`.", + // And the write set now says what it bounds, which is the envelope and + // not the transaction the func is handed. + "the transaction", + "is yours through sqlb.TxFrom", + } { + if !strings.Contains(src, want) { + t.Errorf("the Actions doc comment is missing %q:\n%s", want, src) + } + } +} + // A schema with no verbs must generate exactly what it generated before this // feature existed. The parameter is additive; the absence of it is the promise. func TestRegisterKeepsItsSignatureWhenNothingDeclaresAnAction(t *testing.T) { @@ -230,13 +265,32 @@ func TestActionsReachTheCLI(t *testing.T) { // round trip less than relaying the server's 422. `_ = cmd.MarkFlagRequired("completed-at")`, // The write set is in the help, because "what does this touch" is the - // question an operator has at the prompt. - "This writes status, closed_at, and no other column.", + // question an operator has at the prompt. It is stated as what comes + // back on the row rather than as the blast radius, which it is not. + "The response row carries status, closed_at, and no other column the server changed on it.", + // And the declared reach beside it, which is the half the write set + // understates. --help is the surface ADR-0029 argues about hardest: a + // caller with no compile step reads this instead of making a request. + "Beyond that row the route writes: comments.", + "The schema declares that set; nothing enforces it.", + // A collection verb has no row at all, so the reach is the only thing + // it can state. + "Beyond that row the route writes: tasks, comments.", } { if !strings.Contains(src, want) { t.Errorf("the CLI does not contain %q\n%s", want, src) } } + // An undeclared reach is not a bound. `archive` names no Touches, and its + // help says so rather than leaving the write set to be read as the whole + // of it — which is the misreading the issue reported. + archiveHelp := src[strings.Index(src, "newTasksArchiveCommand(c *client.Client)"):] + if end := strings.Index(archiveHelp, "return cmd"); end > 0 { + archiveHelp = archiveHelp[:end] + } + if !strings.Contains(archiveHelp, "the absence of a claim rather") { + t.Errorf("a verb with no declared reach should say so:\n%s", archiveHelp) + } // A verb with no body sends none, rather than posting `{}` at an operation // that does not declare one. archive := src[strings.Index(src, "newTasksArchiveCommand(c *client.Client)"):] diff --git a/codegen/cliaction.go b/codegen/cliaction.go index 5d88953..4e175dc 100644 --- a/codegen/cliaction.go +++ b/codegen/cliaction.go @@ -125,15 +125,35 @@ func cliActionLong(r cliResource, a schema.Action) string { } if a.IsCollection() { b.WriteString("A verb on the collection: it addresses no single row, and a successful call\nwrites nothing to print.") + writeCLIReach(&b, a) return b.String() } b.WriteString("A verb on one row. The server fetches it, runs the transition, and answers\nwith the row as it now stands.") if len(a.Writes) > 0 { - fmt.Fprintf(&b, "\n\nThis writes %s, and no other column.", strings.Join(a.Writes, ", ")) + fmt.Fprintf(&b, "\n\nThe response row carries %s, and no other column the server changed on it.", + strings.Join(a.Writes, ", ")) } + writeCLIReach(&b, a) return b.String() } +// writeCLIReach states what the verb writes beyond the row it answers with. +// +// This is the surface ADR-0029's argument is sharpest about: --help is what a +// caller with no compile step reads instead of a request, and a caller reading +// a write set of two columns concludes the verb is confined to one row. The +// sentence goes in whether or not the schema declared a reach, because the +// absence of a declaration is not the absence of a reach — an undeclared verb +// still holds the transaction. +func writeCLIReach(b *strings.Builder, a schema.Action) { + if len(a.Touches) > 0 { + fmt.Fprintf(b, "\n\nBeyond that row the route writes: %s.\nThe schema declares that set; nothing enforces it.", + strings.Join(a.Touches, ", ")) + return + } + b.WriteString("\n\nThe route declares nothing written beyond that row. A verb holds the\ntransaction and may write more, so this is the absence of a claim rather\nthan a checked bound.") +} + // cliActionExample writes one runnable invocation, filling every required flag. func cliActionExample(r cliResource, a schema.Action) string { var args []string diff --git a/codegen/skill.go b/codegen/skill.go index a346794..0c7bf6a 100644 --- a/codegen/skill.go +++ b/codegen/skill.go @@ -212,12 +212,16 @@ func skillResource(b *strings.Builder, t schema.TableManifest) { b.WriteString("**Declared actions.** Domain verbs this resource owns. " + "Reaching the same outcome by PATCHing a column is the mistake these exist to " + "prevent — the verb owns the transition.\n\n") - b.WriteString("| Verb | Route | Writes |\n|---|---|---|\n") + b.WriteString("| Verb | Route | Writes | Also writes |\n|---|---|---|---|\n") for _, a := range r.Actions { - fmt.Fprintf(b, "| `%s` | `%s %s` | %s |\n", - a.Name, a.Method, a.Path, orNone(joinCode(a.Writes, ", "))) + fmt.Fprintf(b, "| `%s` | `%s %s` | %s | %s |\n", + a.Name, a.Method, a.Path, + orNone(joinCode(a.Writes, ", ")), orNone(joinCode(a.Touches, ", "))) } - b.WriteString("\n") + b.WriteString("\n*Writes* is the columns the envelope persists on the addressed row. " + + "*Also writes* is the tables the verb declares it reaches through its " + + "transaction — declared, not enforced, and *none* there means no claim was " + + "made rather than that none are written.\n\n") } if len(t.CollectedBy) > 0 { diff --git a/docs/adr/0043-declared-actions.md b/docs/adr/0043-declared-actions.md index 501b71c..1c0baad 100644 --- a/docs/adr/0043-declared-actions.md +++ b/docs/adr/0043-declared-actions.md @@ -334,3 +334,38 @@ them is this record arguing with itself. is load-bearing. `completeTask` writes a comment row through `sqlb.TxFrom`, the `BeforeCreate` hook supplies its `workspace_id`, and a refused completion rolls the comment back with it — none of which the declaration mentions. + +- 2026-08-05 — **`Touches` added**, because the record's escape hatch turned out + to be invisible from every surface that describes a route + ([#149](https://github.com/jryannel/sqlb/issues/149)). This record already + knew the shape of it — *"`completeTask` writes a comment row through + `sqlb.TxFrom` … none of which the declaration mentions"* is the last line of + the 2026-07-31 revision — and read it as a note about an example rather than + as a gap in the contract. + + What made it one is that `Writes` is *reported* as complete, by three tools, + with nothing to say 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](0029-go-cli.md)'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 verb is + confined to one row. That inference can be wrong by ten tables, and the + repository already treats a plausible wrong answer as worse than a warning. + + So `Touches []string` names tables beside `Writes`'s columns, and it is + documentation with no enforcement behind it. Enforcing it would mean tracing + application code the schema package cannot see; the alternative to an + unenforced claim was not an enforced one, it was silence. Validation refuses + only what says nothing — an empty name, a duplicate — and an unknown table is + accepted, since the cross-module write is the case the field exists for. + + **Not a change to what an action covers.** The envelope stays declared and the + transition stays in Go; the eleven-table command is still a hand-written + command layer, and `Writes` naming columns on one row *should* understate it. + What changed is that the generated documentation can now tell the two apart. + + The lock got the same treatment in prose rather than in code: the envelope's + `FOR UPDATE` covers the row it fetched, and statements issued through + `sqlb.TxFrom` take their own, in an order the application owns. + [docs/rest/actions.md](../rest/actions.md) says so under the `TxFrom` example, + which is where the reader is at the moment they are handed the transaction. diff --git a/docs/rest/actions.md b/docs/rest/actions.md index aa6a841..125d022 100644 --- a/docs/rest/actions.md +++ b/docs/rest/actions.md @@ -90,11 +90,58 @@ _, err := sqlb.InsertRows(&tasks.Comment{TaskID: task.ID, Body: *in.Note}).One(c The comment and the completion commit together or neither does. +## `Writes` is not the blast radius, and `Touches` says so + +Read those two sections together and the gap is obvious: `Writes` is what the +*envelope* persists — columns, on one row — and the paragraph above hands the +same verb a transaction it can write anything through. The three surfaces that +print `Writes` cannot see the difference, and a caller with no compile step, +which is the caller [ADR-0029](../adr/0029-go-cli.md) has in mind, +reads `status, completed_at` and concludes the route is confined to one row +([#149](https://github.com/jryannel/sqlb/issues/149)). + +`Touches` is how a wide route says it is wide: + +```go +Task.Action(schema.Action{ + Name: "complete", + Writes: []string{"status", "completed_at"}, + Touches: []string{"comments"}, +}) +``` + +Table names, not columns. It travels with `Writes` everywhere `Writes` goes — +`sqlb impact`, the manifest, the OpenAPI description, the generated `Actions` +doc comment, and `--help`: + +``` +Beyond that row the route writes: comments. +The schema declares that set; nothing enforces it. +``` + +**Nothing checks the claim**, and that is the design rather than a gap. Tracing +what a Go func writes is not something the schema package can do, and the +alternative on offer was silence. So the failure mode is an over-broad claim +instead of a confident understatement, and a test asserting the declaration +against the statements the verb actually issued is yours to write — which, from +inside the application, it can be. + +A verb that declares nothing gets a sentence saying so, in those words: the +absence of a claim, not a checked bound. + +## The lock covers one row + `Writes` also decides the lock. Every one of these is a read-modify-write across a round trip, so a declared write set makes the fetch `SELECT … FOR UPDATE` — without it, two concurrent completions read the same row and the second overwrites the first. Nobody has to remember it per route. +**The row it locks is the one the envelope fetched, and nothing else.** +Statements issued through `sqlb.TxFrom` take their own locks, in an order this +application owns — which is where deadlocks between two wide verbs come from, +and which no declaration can arrange for you. The lock is a guarantee about the +transition on one row, not about the transaction around it. + ## Scoping comes with the fetch The envelope's fetch runs the model's `BeforeQuery` hooks, so an action on a diff --git a/example/tasks/.claude/skills/sqlb-schema/SKILL.md b/example/tasks/.claude/skills/sqlb-schema/SKILL.md index da604e5..5caa8d6 100644 --- a/example/tasks/.claude/skills/sqlb-schema/SKILL.md +++ b/example/tasks/.claude/skills/sqlb-schema/SKILL.md @@ -94,9 +94,11 @@ Values: `status` is one of `todo` `in_progress` `blocked` `done`; `priority` is **Declared actions.** Domain verbs this resource owns. Reaching the same outcome by PATCHing a column is the mistake these exist to prevent — the verb owns the transition. -| Verb | Route | Writes | -|---|---|---| -| `complete` | `POST /tasks/{id}/complete` | `status`, `completed_at` | +| Verb | Route | Writes | Also writes | +|---|---|---|---| +| `complete` | `POST /tasks/{id}/complete` | `status`, `completed_at` | `comments` | + +*Writes* is the columns the envelope persists on the addressed row. *Also writes* is the tables the verb declares it reaches through its transaction — declared, not enforced, and *none* there means no claim was made rather than that none are written. ### `users` diff --git a/example/tasks/cli/cli_gen.go b/example/tasks/cli/cli_gen.go index afc78d2..aead529 100644 --- a/example/tasks/cli/cli_gen.go +++ b/example/tasks/cli/cli_gen.go @@ -1298,7 +1298,10 @@ Marks the task done and stamps its completion time. A task that is already done A verb on one row. The server fetches it, runs the transition, and answers with the row as it now stands. -This writes status, completed_at, and no other column.`, +The response row carries status, completed_at, and no other column the server changed on it. + +Beyond that row the route writes: comments. +The schema declares that set; nothing enforces it.`, Example: " taskctl tasks complete ", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/example/tasks/rest_gen.go b/example/tasks/rest_gen.go index 0f5b6f7..9374ba5 100644 --- a/example/tasks/rest_gen.go +++ b/example/tasks/rest_gen.go @@ -308,7 +308,14 @@ type Actions struct { // // Marks the task done and stamps its completion time. A task that is already done is refused with a 409. // - // The envelope persists `status` and `completed_at` afterwards, and nothing else. + // The envelope persists `status` and `completed_at` off this row afterwards, and nothing + // else — which bounds the envelope and not the func: the transaction + // is yours through sqlb.TxFrom, and statements issued there take + // their own locks, in an order this code owns. + // + // Declared reach beyond that row: `comments`. Nothing checks it; it is + // what the route tells `sqlb impact`, the OpenAPI document and the + // CLI's --help, so a change here belongs in the schema. CompleteTask func(context.Context, *Task, CompleteTaskInput) error } @@ -379,6 +386,7 @@ func Register(api huma.API, db sqlb.Executor, actions Actions) error { Summary: "Complete a task", Description: "Marks the task done and stamps its completion time. A task that is already done is refused with a 409.", Writes: []string{"status", "completed_at"}, + Touches: []string{"comments"}, HasBody: true, }, actions.CompleteTask); err != nil { return err diff --git a/example/tasks/restcontract.json b/example/tasks/restcontract.json index 7a37f78..7b3c989 100644 --- a/example/tasks/restcontract.json +++ b/example/tasks/restcontract.json @@ -347,6 +347,9 @@ "writes": [ "status", "completed_at" + ], + "touches": [ + "comments" ] } ] diff --git a/example/tasks/sqlb.json b/example/tasks/sqlb.json index 92c48ef..a3d2ff6 100644 --- a/example/tasks/sqlb.json +++ b/example/tasks/sqlb.json @@ -759,6 +759,9 @@ "writes": [ "status", "completed_at" + ], + "touches": [ + "comments" ] } ], diff --git a/example/tasks/taskschema/schema.go b/example/tasks/taskschema/schema.go index 1eec546..0b867ae 100644 --- a/example/tasks/taskschema/schema.go +++ b/example/tasks/taskschema/schema.go @@ -272,6 +272,11 @@ var Task = schema.Table("tasks", // do not disagree. ReadOnly says no *request* may set it; the envelope // writes it from the row the verb mutated, on the server, which is the same // standing the BeforeUpdate hook has. + // + // Touches is the other half, and this verb is why it exists: a note in the + // body becomes a comment row through sqlb.TxFrom, and the write set above + // cannot say so. Two columns on one row is what the envelope persists, not + // what the route reaches (#149). Action(schema.Action{ Name: "complete", Body: schema.Body( @@ -279,6 +284,7 @@ var Task = schema.Table("tasks", Comment("Recorded as a comment on the task, in the same transaction."), ), Writes: []string{"status", "completed_at"}, + Touches: []string{"comments"}, Description: "Marks the task done and stamps its completion time. A task that is already done is refused with a 409.", }) diff --git a/rest/action.go b/rest/action.go index 723419e..6cdb53f 100644 --- a/rest/action.go +++ b/rest/action.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "reflect" + "strings" "github.com/danielgtaylor/huma/v2" "github.com/jryannel/sqlb" @@ -63,6 +64,12 @@ type ActionSpec struct { // write through the transaction, which it has. Writes []string + // Touches names the tables the verb writes through that transaction, as + // the schema declared them. Nothing here enforces it and nothing here + // could; it is carried so the operation's description can state a reach + // the write set understates (#149). + Touches []string + // HasBody reports whether the action declared any body properties. // // The input type is generated either way, so that adding the first property @@ -84,6 +91,31 @@ type ActionSpec struct { var ErrNoTransaction = errors.New( "rest: this action needs the transaction, and the resource runs its writes under autocommit (Options.DisableTransactions)") +// describe is the operation's description: what the schema wrote, plus what the +// route reaches. +// +// The reach is appended here rather than baked into Description by codegen so +// that a hand-written mount gets it from the same field a generated one does — +// and so that the sentence stays one sentence, in one place, when it needs +// rewording. +// +// Only Touches is appended. Writes is in the response schema and in the CLI's +// help already, and repeating it here would put the understated number in front +// of a reader twice for every once that the correction appears. +func (s ActionSpec) describe() string { + if len(s.Touches) == 0 { + return s.Description + } + reach := fmt.Sprintf( + "Beyond the row in the response, this operation writes: %s. "+ + "That set is declared rather than enforced — see the schema for what it claims.", + strings.Join(s.Touches, ", ")) + if s.Description == "" { + return reach + } + return s.Description + "\n\n" + reach +} + func (s ActionSpec) validate(resource string) error { switch { case s.Name == "": @@ -242,7 +274,7 @@ func Action[T, In any](api huma.API, db sqlb.Executor, opts Options, spec Action Method: http.MethodPost, Path: spec.Path, Summary: spec.Summary, - Description: spec.Description, + Description: spec.describe(), Tags: []string{opts.tag()}, Security: opts.Security, RejectUnknownQueryParameters: true, @@ -305,7 +337,7 @@ func CollectionAction[In any](api huma.API, db sqlb.Executor, opts Options, spec Method: http.MethodPost, Path: spec.Path, Summary: spec.Summary, - Description: spec.Description, + Description: spec.describe(), Tags: []string{opts.tag()}, Security: opts.Security, DefaultStatus: statusNoBody, diff --git a/rest/action_test.go b/rest/action_test.go index ef5915f..e967ebf 100644 --- a/rest/action_test.go +++ b/rest/action_test.go @@ -343,3 +343,49 @@ func TestACollectionActionFetchesNothingAndAnswers204(t *testing.T) { } } } + +// The operation's description states what the verb reaches beyond the row it +// answers with. +// +// This is the surface the report singled out (#149): a write set of two columns +// is what the envelope persists, and a reader given only that concludes the +// route is confined to one row. The correction has to be in the document the +// client generator and the agent read, not only in the schema file. +func TestTheOperationDescriptionCarriesTheDeclaredReach(t *testing.T) { + db := newFakeDB(t) + spec := completeSpec() + spec.Description = "Close the post and note why." + spec.Touches = []string{"comments", "audit_log"} + + api := mountAction(t, db.db, spec, func(context.Context, *Post, CompletePost) error { return nil }) + + op := api.OpenAPI().Paths["/posts/{id}/complete"].Post + if op == nil { + t.Fatal("the action is not in the document") + } + for _, want := range []string{ + // The schema's own prose survives; the reach is appended to it. + "Close the post and note why.", + "comments, audit_log", + "declared rather than enforced", + } { + if !strings.Contains(op.Description, want) { + t.Errorf("description is missing %q:\n%s", want, op.Description) + } + } +} + +// A verb that declares no reach gets its description back unchanged, rather +// than a paragraph of hedging on every operation in the document. +func TestAnUndeclaredReachAddsNothingToTheDescription(t *testing.T) { + db := newFakeDB(t) + spec := completeSpec() + spec.Description = "Close the post." + + api := mountAction(t, db.db, spec, func(context.Context, *Post, CompletePost) error { return nil }) + + op := api.OpenAPI().Paths["/posts/{id}/complete"].Post + if op.Description != "Close the post." { + t.Errorf("description = %q, want the schema's own text untouched", op.Description) + } +} diff --git a/restcompat/action.go b/restcompat/action.go index 0cc076a..1ac4493 100644 --- a/restcompat/action.go +++ b/restcompat/action.go @@ -30,6 +30,12 @@ type ActionSnap struct { // so a change here is neutral — but it widens or narrows what one route // can mutate, which is exactly the blast-radius question this tool is for. Writes []string `json:"writes,omitempty"` + // Touches names the tables the verb writes through its transaction, as + // declared. Also neutral, and for a sharper reason than Writes: the + // declaration is unenforced, so a change here is a change in what the route + // *claims* — which is the only thing a diff can see, and the thing a + // reviewer most wants shown when a verb's reach grows. + Touches []string `json:"touches,omitempty"` } // ActionPropSnap is one property of an action's request body. @@ -50,7 +56,7 @@ func (p ActionPropSnap) required() bool { return !p.Nullable && !p.HasDefault } func captureActions(t *schema.TableDef, path string) []ActionSnap { var out []ActionSnap for _, a := range t.Actions() { - snap := ActionSnap{Name: a.Name, Path: a.FullPath(path), Writes: a.Writes} + snap := ActionSnap{Name: a.Name, Path: a.FullPath(path), Writes: a.Writes, Touches: a.Touches} for _, f := range a.Body { d := f.Desc() snap.Body = append(snap.Body, ActionPropSnap{ @@ -97,6 +103,11 @@ func diffActions(path string, o, n map[string]ActionSnap, add func(Break)) { fmt.Sprintf("write set changed from %v to %v; no client breaks, but the route now touches different columns", ov.Writes, nv.Writes)}) } + if !sameStrings(ov.Touches, nv.Touches) { + add(Break{LevelNeutral, path, FacetAction, name, + fmt.Sprintf("declared reach changed from %v to %v; no client breaks, but the route claims to write different tables", + ov.Touches, nv.Touches)}) + } } } diff --git a/restcompat/action_test.go b/restcompat/action_test.go index 2850cb3..5220eee 100644 --- a/restcompat/action_test.go +++ b/restcompat/action_test.go @@ -153,6 +153,28 @@ func TestAChangedWriteSetIsReportedAsNeutral(t *testing.T) { } } +// A verb whose declared reach grows is the change a reviewer most wants shown, +// and the one the diff could otherwise not see at all: no column moves, no +// route moves, and the only evidence is the claim itself (#149). +func TestAChangedReachIsReportedAsNeutral(t *testing.T) { + before := complete() + before.Touches = []string{"comments"} + after := complete() + after.Touches = []string{"comments", "inventory_reservations", "payments"} + + breaks := restcompat.Diff(withActions(before), withActions(after)) + b := find(t, breaks, "declared reach changed") + if b.Level != restcompat.LevelNeutral { + t.Errorf("level = %s, want neutral", b.Level) + } + if !strings.Contains(b.Summary, "payments") { + t.Errorf("the summary should name the tables, got %q", b.Summary) + } + if len(restcompat.Breaking(breaks)) != 0 { + t.Errorf("a reach change failed the strict gate: %v", restcompat.Breaking(breaks)) + } +} + // Reordering declarations in a schema file must not show up as a contract // change, or every `-write` becomes a diff nobody can review. func TestActionOrderIsNotContract(t *testing.T) { diff --git a/schema/action.go b/schema/action.go index a091ac2..2163abc 100644 --- a/schema/action.go +++ b/schema/action.go @@ -66,14 +66,50 @@ type Action struct { // written, from the row the verb mutated. // // A verb that has to touch anything else has the transaction and can issue - // the statement itself. What this buys is that the blast radius of a route - // is something the OpenAPI document and `sqlb impact` can state — and that - // the envelope knows to take the row lock, since a declared write set is - // exactly the case where a read-modify-write can be lost. + // the statement itself — see Touches, which is where the route says so. It + // is worth being precise about the scope of this field, because three + // surfaces print it and none of them can widen it: Writes is what the + // *envelope* persists, not a bound on the transaction. + // + // What it does buy is the row lock. A declared write set is exactly the + // case where a read-modify-write can be lost, so the envelope's fetch takes + // SELECT … FOR UPDATE on this row — and on this row only. // // It must be empty on a collection action, which has no row. Writes []string + // Touches names the tables the verb writes through the transaction, beyond + // the row the envelope persists. It is documentation with no enforcement + // behind it, and that is the whole design: the alternative was a route that + // prints a two-column write set while opening eleven tables' worth of + // transaction, with nothing in the generated surfaces to say so (#149). + // + // Order.Action(schema.Action{ + // Name: "place", + // Writes: []string{"status", "placed_at"}, + // Touches: []string{"order_lines", "inventory_reservations", "payments"}, + // }) + // + // `sqlb impact`, the OpenAPI description and the CLI's --help carry it + // beside Writes, which is what makes a wide route say it is wide. The + // caller ADR-0029 has in mind — one with no compile step, "such as an + // agent" — reads a declared write set of two columns and concludes the verb + // is confined to one row; that inference is the one the surface invites, + // and this is how a route declines it. + // + // Nothing checks the claim. A test asserting it against the statements the + // verb actually issued is the application's to write, and can be; a checker + // here would have to trace application code the schema package cannot see. + // So a stale Touches is possible, and it is still strictly better than the + // silence it replaces — the failure mode is an over-broad claim rather than + // a confident understatement. + // + // Unlike Writes it is legal on a collection action, which does all of its + // work through the transaction and has no row of its own at all. Naming + // this table is legal too, and means what it says: the envelope writes one + // row of it, and a verb that writes others has no other way to declare them. + Touches []string + // Summary is the one-line description in the OpenAPI document. // // Left empty it is filled in downstream, as "Complete a task", rather than @@ -187,6 +223,31 @@ func (r *Registry) validateActions(t *TableDef, report func(string, string, stri r.validateActionBody(t, a, report) r.validateActionWrites(t, a, report) + r.validateActionTouches(t, a, report) + } +} + +// validateActionTouches checks the declared blast radius is a set of table +// names, which is as far as checking can go: the tables are the application's +// to write and may live in another module or another schema entirely, so a name +// this registry does not know is a legitimate declaration rather than a typo. +// +// What it does refuse is a claim that says nothing: an empty name, or the same +// table twice. The table's own name is allowed and is not redundant — the +// envelope writes one row of it, and a verb that writes *other* rows of the +// same table has no other way to say so. +func (r *Registry) validateActionTouches(t *TableDef, a Action, report func(string, string, string, ...any)) { + seen := make(map[string]bool, len(a.Touches)) + for _, name := range a.Touches { + switch { + case name == "": + report(t.name, "", "action %q: Touches has an empty table name", a.Name) + continue + case seen[name]: + report(t.name, "", "action %q: Touches names %q twice", a.Name, name) + continue + } + seen[name] = true } } diff --git a/schema/action_test.go b/schema/action_test.go index 473ee55..04e754c 100644 --- a/schema/action_test.go +++ b/schema/action_test.go @@ -152,11 +152,54 @@ func TestAnItemActionNeedsAPrimaryKey(t *testing.T) { // The manifest is what an agent reads to learn the API. A verb missing from it // reads as a CRUD-only resource, and the transition gets guessed at. +// Touches is unenforced by design — the tables it names may belong to another +// module, and tracing what a Go func writes is not something this package can +// do. So validation refuses only what says nothing (#149). +func TestTouchesRefusesAClaimThatSaysNothing(t *testing.T) { + refusal(t, tasksWith(schema.Action{ + Name: "complete", + Touches: []string{"comments", "comments"}, + }), "Touches names \"comments\" twice") + + refusal(t, tasksWith(schema.Action{ + Name: "complete", + Touches: []string{""}, + }), "Touches has an empty table name") +} + +// A table this registry has never heard of is the ordinary case, not a typo: +// the point of the field is the cross-module write, and refusing an unknown +// name would refuse exactly the declaration worth making. +func TestTouchesAcceptsATableThisRegistryDoesNotHave(t *testing.T) { + r := tasksWith(schema.Action{ + Name: "complete", + Writes: []string{"status"}, + Touches: []string{"billing.invoices", "tasks"}, + }) + if err := r.Validate(); err != nil { + t.Fatalf("a cross-module reach was refused: %v", err) + } +} + +// Unlike Writes, which needs a row, a collection action is the shape most +// likely to have a reach: it does all of its work through the transaction. +func TestACollectionActionMayDeclareAReach(t *testing.T) { + r := tasksWith(schema.Action{ + Name: "purge-archived", + Path: "/purge-archived", + Touches: []string{"tasks", "comments"}, + }) + if err := r.Validate(); err != nil { + t.Fatalf("a collection action's reach was refused: %v", err) + } +} + func TestTheManifestCarriesTheVerbs(t *testing.T) { r := tasksWith(schema.Action{ - Name: "complete", - Body: schema.Body(schema.Text("note").Nullable()), - Writes: []string{"status"}, + Name: "complete", + Body: schema.Body(schema.Text("note").Nullable()), + Writes: []string{"status"}, + Touches: []string{"comments"}, }) var rest *schema.RESTManifest for _, tm := range r.BuildManifest().Tables { @@ -179,5 +222,9 @@ func TestTheManifestCarriesTheVerbs(t *testing.T) { t.Errorf("body = %+v", a.Body) case len(a.Writes) != 1 || a.Writes[0] != "status": t.Errorf("writes = %v", a.Writes) + // The manifest is what a program reads instead of making a request, so the + // half the write set understates has to be in it too. + case len(a.Touches) != 1 || a.Touches[0] != "comments": + t.Errorf("touches = %v", a.Touches) } } diff --git a/schema/manifest.go b/schema/manifest.go index 82330f0..14ee891 100644 --- a/schema/manifest.go +++ b/schema/manifest.go @@ -218,9 +218,14 @@ type ActionManifest struct { // happen to be optional. Body []ActionProperty `json:"body,omitempty"` // Writes names the columns the envelope persists after the verb returns. - // It is what makes the blast radius of a route readable rather than - // something to be inferred from a handler. + // It is not the blast radius: it is one row of this table, and a verb may + // write anything else through the transaction it holds. Writes []string `json:"writes,omitempty"` + // Touches names the tables the verb writes beyond that row, as declared. + // Nothing enforces it — see schema.Action.Touches — and it is here because + // a reader that had only Writes would conclude the route is confined to one + // row, which is what this field exists to contradict. + Touches []string `json:"touches,omitempty"` } // ActionProperty is one property of an action's request body. @@ -422,6 +427,7 @@ func (a Action) manifest(resourcePath string) ActionManifest { Method: "POST", Summary: a.Summary, Writes: a.Writes, + Touches: a.Touches, } for _, f := range a.Body { d := f.Desc() From 8ce87fe236ff067564132adeb0ffff6280d75386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrgen=20Ryannel?= Date: Wed, 5 Aug 2026 10:06:16 +0200 Subject: [PATCH 5/5] feat(rest): reachability is a property of the mount, so one table can serve two surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CLAUDE.md | 2 +- ...reachability-is-a-property-of-the-mount.md | 117 ++++++++ docs/adr/README.md | 1 + docs/schema/capabilities.md | 52 ++++ docs/start/structs-first.md | 16 ++ filter/computed_test.go | 70 +++++ filter/filter.go | 82 ++++-- rest/binding.go | 92 +++++- rest/columns_test.go | 271 ++++++++++++++++++ rest/item.go | 13 +- rest/list.go | 1 + rest/params.go | 23 +- rest/rest.go | 57 ++++ 13 files changed, 764 insertions(+), 33 deletions(-) create mode 100644 docs/adr/0050-reachability-is-a-property-of-the-mount.md create mode 100644 rest/columns_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 619702d..53ab9d6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,7 +18,7 @@ Read in this order, and stop as soon as you have what you need: Go way instead, as `// Command sqlb …` at the head of `cmd/sqlb/main.go`. 2. **[docs/architecture.md](docs/architecture.md)** for how the pieces fit and why the seams are where they are. -3. **[docs/adr/](docs/adr/)** — 50 records, and they are *load-bearing rather +3. **[docs/adr/](docs/adr/)** — 51 records, and they are *load-bearing rather than historical*. A decision here is usually the answer to "why is this not simpler", and reversing one without reading it is the most common way to spend an afternoon rediscovering a rejected alternative. Each carries a diff --git a/docs/adr/0050-reachability-is-a-property-of-the-mount.md b/docs/adr/0050-reachability-is-a-property-of-the-mount.md new file mode 100644 index 0000000..e20719b --- /dev/null +++ b/docs/adr/0050-reachability-is-a-property-of-the-mount.md @@ -0,0 +1,117 @@ +# ADR-0050: Reachability is a property of the mount + +- **Status:** Working +- **Confidence:** Medium +- **Decided:** 2026-08-05 +- **Last reviewed:** 2026-08-05 + +## Context + +A table has one model and one `Expose`, and every capability is declared on the +column: `Filterable`, `Sortable`, `Hidden`. That is the right shape for a table +served one way, which is nearly all of them. + +It is the wrong shape for a table served two ways. A headless shop reads +`products` from a public storefront and from an admin panel behind a shared +secret; the admin surface exists precisely to serve `cost_price_minor`, +`supplier` and `internal_notes`, and the storefront must not. `Hidden` hides a +column from both, because it is a property of the model and there is one model. +`Expose` cannot add a second resource, because `schema/table.go` assigns +`t.rest = &r` and a second call replaces the first. So the split left the +schema-first path for one of its two halves — and the public/admin pair is not +an adoption path or a legacy-struct case, which is what +[structs-first](../start/structs-first.md) is otherwise for +([#148](https://github.com/jryannel/sqlb/issues/148)). + +The mechanism was already half-built and known to work. `rest.Options.Computed` +is per-resource column reachability with the general rationale written into its +own doc comment — *"a model is shared"* — and it was added for cost rather than +for disclosure ([#92](https://github.com/jryannel/sqlb/issues/92)). An ordinary +column is shared the same way. + +## Decision + +Reachability is a property of the mount, and `rest.Options.Columns` says it. + +A resource that names columns serves those and no others: absent from the +response, absent from the `SELECT`, not filterable, not sortable, not searched, +not nameable in `?select`, not settable by a body, and not named in the list a +rejection offers back. `filter.Options.Columns` carries it into the parser and +into `Apply`'s default projection, so what a request may name and what the +database is asked for cannot disagree. Empty means every column, which is what +a generated resource emits and what every existing mount relies on. + +`Expose` stays singular and the emitters keep one resource per table. The +privileged half is a hand-written `rest.Resource` call over the generated model. + +## Consequences + +**Buys.** A public and a privileged surface over one table, in one process, over +one model. The narrowed half keeps the generated model, the typed column facade, +the manifest and the drift gate; only the mount is hand-written, where the +alternative — a second `Describe`d struct — gives up all four. And the OpenAPI +parameters now follow the resource rather than the model, which also closes a +gap `Options.Computed` had left: a resource that declined a computed column was +still publishing a filter parameter for it. + +**Costs.** Three, and they are the shape of the boundary rather than bugs. + +The **response schema** in the OpenAPI document is the model's Go type, +registered once as a component and shared by every mount of it, so it still +lists the columns the narrowed resource does not serve. Runtime responses omit +them; a client generated from the document carries optional fields that are +always absent. Narrowing it needs a per-resource Go type. + +The **create and update body types** are the caller's, so a narrowed mount +reusing the wide resource's bodies documents fields it will not write. It does +not write them — a column outside the list is cleared off the row a body +produced, exactly as a `ReadOnly` one is — but a resource narrowed for +disclosure usually wants `Ops` without the write operations. + +And the narrowing is **a mount-time argument rather than a schema property**, +which is the thing the report asked not to be true. A model with no field for +`cost_price_minor` has no code path that can return it, this year or next; a +model that has the field and a mount that declines it is one `Options` value +away from serving it. That is a weaker guarantee, honestly weaker, and the +schema-side version is what would replace it. + +## What would change our mind + +- **A second surface routinely wants its own clients.** If narrowed mounts + accumulate and each one grows a hand-written TypeScript or Dart client beside + it, the emitters are the thing that should have changed, and `Expose` + appending with a `Columns` allowlist on `schema.REST` is the shape — two + generated resources, both on the drift gate. +- **The response schema's width causes a real disclosure.** It has not yet: the + values are absent and the parameters are gone. If a document reader treats the + schema as the surface — an agent deciding what to request, a generator + emitting a client somebody reads — the per-resource schema stops being a + nicety. +- **`Columns` gets used for cost rather than disclosure.** If resources narrow + to avoid reading wide rows, this is a projection feature wearing a visibility + hat, and `?select` with a server-side default is the thing being asked for. + +## Cost of change + +**Widening is free.** Dropping `Columns` from a mount restores the wide surface, +and a schema that never sets it generates exactly what it generated before. + +**Narrowing an existing resource is a wire break**, in the way +[ADR-0039](0039-a-schema-edit-is-an-api-edit.md) means: a deployed client +holding a filter or a response field loses it with no DDL in sight. `sqlb +impact` sees it only for generated resources, since the narrowed mount is +hand-written and not in the contract snapshot — which is the one place this +decision costs the gate something real. + +**Replacing it with the schema-side version is additive.** `schema.REST` gaining +a `Columns` and `Expose` appending would leave `rest.Options.Columns` as the +thing codegen writes into the generated mount, which is the arrangement +`Options` already has with `schema.REST` everywhere else. + +## Revisions + +- 2026-08-05 — Written, against [#148](https://github.com/jryannel/sqlb/issues/148). + The record exists mostly to name what was *not* built and why the weaker + answer was taken first: the reporter had no view on which of the three options + was right, and this is the one whose cost is a paragraph rather than a + redesign of every emitter. diff --git a/docs/adr/README.md b/docs/adr/README.md index b59210b..13d5dd1 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -156,6 +156,7 @@ the section it bears on, and move on. | [0047](0047-no-default-hook-registry.md) | There is no default hook registry, and the short name takes the registry | Working | High | | [0048](0048-auto-incrementing-keys.md) | An auto-incrementing key is a property of the column, and both of Postgres's spellings are declarable | Working | High | | [0049](0049-the-skill-is-generated.md) | The agent skill is generated where it can be gated, and static only where no check is possible | Working | Medium | +| [0050](0050-reachability-is-a-property-of-the-mount.md) | Reachability is a property of the mount, so one table can serve a public surface and a privileged one | Working | Medium | † **Deliberately not in 1.0.** The decision is recorded; the feature is out of scope for the first tag. [The road to 1.0](../release-1.0.md) says why for each. diff --git a/docs/schema/capabilities.md b/docs/schema/capabilities.md index d39d0cc..4a57d56 100644 --- a/docs/schema/capabilities.md +++ b/docs/schema/capabilities.md @@ -112,6 +112,58 @@ The check proves a hook *exists*, not that it is right. That is worth knowing before relying on it, and it catches the case that actually happens: the table somebody added last week ([ADR-0030](../adr/0030-declared-scope-is-required.md)). +## One table, two surfaces + +Every capability above is a property of the *column*, and a column belongs to a +model, and a table has one model. That is the right shape for almost everything +— and it is the wrong shape for the case most applications with an admin panel +have: a public surface and a privileged surface over the same table, differing +in which columns each may see. + +`Hidden()` cannot say it. A column hidden for the storefront is hidden for the +admin panel, which is the surface that exists to read it. `Expose` cannot say +it either: a table carries one, and a second call replaces the first rather than +adding a resource. + +What can say it is the **mount**. `rest.Options.Columns` narrows one resource to +the columns it names, the way `rest.Options.Computed` narrows it to the derived +columns it is willing to pay for +([#148](https://github.com/jryannel/sqlb/issues/148)): + +```go +// The generated one, over every column the schema declares. +if err := catalog.Register(api, db); err != nil { … } + +// And a public one beside it, over the same generated model. +err := rest.Resource[catalog.Product, rest.None[catalog.Product], rest.None[catalog.Product]]( + api, db, rest.Options{ + Path: "/storefront/products", + Name: "storefront-product", + Ops: rest.OpList | rest.OpRead, + Columns: []string{"id", "title", "handle", "status", "price_minor"}, + }) +``` + +A column not listed is not reachable from that resource: absent from the +response, absent from the `SELECT` the database sees, not filterable, not +sortable, not searched, not nameable in `?select`, and — the part that matters +for a surface narrowed to conceal something — not named in the list a rejection +offers back. + +**What you give up is the second resource's generated half.** The models, the +typed column facade, the manifest and the drift gate all still cover it, because +there is still one model; the mount, and any client for it, are hand-written. +Two further things stay wide, because they come from a Go type rather than from +the mount: the response schema in the OpenAPI document is the model's, and the +create and update body types are whatever you pass for `C` and `U`. A public +surface is usually read-only, which is why `Ops` above names only two — and if +it is not, give it body types of its own. + +The alternative — a second `Describe`d struct over the same table — is stronger +in one respect, since a model with no field for a column has no code path that +can return it, and gives up all four of the generated halves. See +[structs-first](../start/structs-first.md) for that table. + ## Next - [References and relations](references.md) — `Expandable` and its inverse diff --git a/docs/start/structs-first.md b/docs/start/structs-first.md index d8be1b6..b2fd106 100644 --- a/docs/start/structs-first.md +++ b/docs/start/structs-first.md @@ -80,6 +80,22 @@ generate from: The query builder, the filter grammar, the capabilities, the hooks and the pagination are identical. What moves is who writes the boilerplate around them. +**One case that used to land here and no longer has to.** A public surface and a +privileged surface over the same table — a storefront and an admin panel over +`products` — is neither an adoption path nor a legacy-struct case, and it used +to arrive here anyway, because a table carries one `Expose` and a column hidden +for one surface is hidden for both. `rest.Options.Columns` narrows a *mount* +instead, so the second surface is a hand-written `rest.Resource` call over the +generated model rather than a second model: only the last row of the table above +is given up, and the drift gate still covers the half you would otherwise be +writing by hand. [Capabilities](../schema/capabilities.md#one-table-two-surfaces) +has the shape and what stays wide. + +A second `Describe`d struct is still the stronger answer when the point is that +no code path can return the column *at all* — a model with no field for +`cost_price_minor` cannot serve it by accident, this year or next — and the +table above is what that costs. + You can also start here and move: `Describe` and the DSL declare the same metadata by two routes, so adopting the DSL later is a schema file plus a codegen program, not a rewrite. diff --git a/filter/computed_test.go b/filter/computed_test.go index dc754bc..d0b073b 100644 --- a/filter/computed_test.go +++ b/filter/computed_test.go @@ -106,3 +106,73 @@ func TestComputedBindIsSentOnce(t *testing.T) { t.Errorf("args = %v, want the viewer first", args) } } + +// Options.Columns is the same per-resource reachability, generalised to stored +// columns: one model, two surfaces, and the second one may not see everything +// the first does (#148). +// +// Tested here as well as in rest because Parse and Apply are the layer that +// decides it — a resource whose parser refused a column while Apply projected +// it anyway would read the value on every request and drop it on the way out, +// which is a narrowing of the response and not of the query. +func TestColumnsNarrowsTheSurfaceParseAndApplyAgreeOn(t *testing.T) { + narrow := filter.Options{ + Model: sqlb.ModelOf[Report](), + Columns: []string{"id", "title"}, + } + + // Not projected. The default projection is built in Apply, so this is the + // half a response-only narrowing would have missed. + q, err := filter.Parse(url.Values{}, narrow) + if err != nil { + t.Fatalf("Parse: %v", err) + } + sql, _, err := filter.Apply(sqlb.Query[Report](), q).SQL() + if err != nil { + t.Fatalf("SQL: %v", err) + } + if strings.Contains(sql, "due_days") { + t.Errorf("the narrowed projection reads a column outside its surface:\n%s", sql) + } + for _, want := range []string{`"id"`, `"title"`} { + if !strings.Contains(sql, want) { + t.Errorf("the narrowed projection dropped %s:\n%s", want, sql) + } + } + + // Not filterable, not sortable, not nameable — and not offered back in the + // rejection, which for a resource narrowed to conceal a column is the + // difference between a refusal and a disclosure. + for _, query := range []string{"due_days=eq.3", "sort=due_days", "select=id,due_days"} { + values, err := url.ParseQuery(query) + if err != nil { + t.Fatal(err) + } + _, err = filter.Parse(values, narrow) + if err == nil { + t.Errorf("%s: accepted against a resource that does not serve the column", query) + continue + } + if !strings.Contains(err.Error(), "unknown") { + t.Errorf("%s: the refusal should read as unknown: %v", query, err) + } + // The message carries the allowed list, and due_days must not be in it. + // The name the caller typed is echoed before it, which is not a + // disclosure — the caller typed it. + _, allowed, found := strings.Cut(err.Error(), "(allowed: ") + if !found { + t.Errorf("%s: the refusal names nothing that would have been accepted: %v", query, err) + continue + } + if strings.Contains(allowed, "due_days") { + t.Errorf("%s: the allowed list offers the column the resource does not serve: %v", query, err) + } + } + + // An empty Columns is no narrowing at all, which is what every existing + // resource relies on. + wide := filter.Options{Model: sqlb.ModelOf[Report]()} + if _, err := filter.Parse(url.Values{"due_days": {"eq.3"}}, wide); err != nil { + t.Errorf("an un-narrowed resource lost a filter: %v", err) + } +} diff --git a/filter/filter.go b/filter/filter.go index 843f18c..7a2e7b8 100644 --- a/filter/filter.go +++ b/filter/filter.go @@ -95,22 +95,50 @@ type Options struct { // would have. Computed []string + // Columns narrows this resource to the columns it names. Empty means every + // column the model has, which is the default and what almost every resource + // wants. + // + // It is the same per-resource reachability Computed has, generalised to + // stored columns, and it is here because a model is shared in the other + // direction too: one table, two surfaces, and the privileged one is the + // reason the sensitive column exists (#148). A public catalogue and an + // admin panel over the same products differ in which columns each may see, + // and Hidden cannot say that — Hidden is a property of the model, and there + // is one model. + // + // A column not listed is not reachable from this resource at all: not + // projected, not filterable, not sortable, not nameable in ?select, not + // searched by ?search, and not named in the list a rejection offers. That + // last one matters — a narrowed resource that advertised the column it is + // about to refuse would leak the schema it was narrowed to hide. + // + // Names are column names, as Computed's are. The rest package checks them + // against the model at startup, where a typo is a resource missing a column + // rather than a request-time surprise. + Columns []string + // DisableSearch rejects ?search even when columns are searchable. DisableSearch bool } -// computedAllowed reports whether a column may be reached from this resource. -// Stored columns always may; a computed one has to be named in Options. -func (o Options) computedAllowed(col *sqlb.ColumnInfo) bool { - if col == nil || !col.Computed() { +// reachable reports whether a column may be reached from this resource. +// +// Two independent narrowings, and a column has to pass both: Columns, which is +// the surface this mount serves at all, and Computed, which is the derived +// columns it is willing to pay for. Empty means "no narrowing" in each case, +// so the default is every stored column and no computed one. +func (o Options) reachable(col *sqlb.ColumnInfo) bool { + if col == nil { return true } - for _, name := range o.Computed { - if name == col.Name { - return true - } + if len(o.Columns) > 0 && !contains(o.Columns, col.Name) { + return false } - return false + if !col.Computed() { + return true + } + return contains(o.Computed, col.Name) } func (o Options) defaultPageSize() int { @@ -180,6 +208,14 @@ type Query struct { // Options so that Apply projects exactly what parsing validated against. Computed []string + // Columns is the resource's surface, copied from Options for the same + // reason: the default projection is built in Apply, and a narrowed resource + // whose parser refused a column while its projection selected it anyway + // would read the value out of the database on every request and drop it on + // the way out — which is a narrowing in the response only, and not the one + // Options.Columns describes. + Columns []string + // Cursor is the keyset position `?cursor=` asked to resume from, empty for // the first page. It is the alternative to Page and Offset rather than an // addition to them: a request carrying both is refused, since the two @@ -222,6 +258,12 @@ func Apply[T any](b *sqlb.Builder[T], q *Query) *sqlb.Builder[T] { selects[name] = true } for _, col := range b.Model().Selectable() { + // Both narrowings, in the order Options.reachable applies them. + // A column outside Columns is not this resource's to read at all + // (#148); a computed one it did not ask for is a cost it declined. + if len(q.Columns) > 0 && !contains(q.Columns, col.Name) { + continue + } if col.Computed() && !selects[col.Name] { continue } @@ -329,7 +371,7 @@ func Parse(values url.Values, opts Options) (*Query, error) { return nil, fmt.Errorf("filter: Options.Model is required") } p := &parser{opts: opts, model: opts.Model} - q := &Query{PageSize: opts.defaultPageSize(), Computed: opts.Computed} + q := &Query{PageSize: opts.defaultPageSize(), Computed: opts.Computed, Columns: opts.Columns} // Before anything is read, because what follows reads only the first // occurrence of each of these and the rest would vanish unremarked. @@ -464,7 +506,7 @@ func (p *parser) filterableColumn(name string) *sqlb.ColumnInfo { // that its existence cannot be probed by reading the rejection. A computed // column this resource does not select is unknown in the plainer sense: // it is declared on the model, and this endpoint does not have it (#92). - if col == nil || col.Hidden || !p.opts.computedAllowed(col) { + if col == nil || col.Hidden || !p.opts.reachable(col) { p.errAllowed(name, "", "unknown parameter", p.capable(capFilter)) return nil } @@ -492,7 +534,7 @@ func (p *parser) capable(c capability) []string { // surface, so it is absent from the "allowed" lists too — naming it in // a rejection would advertise a column every request for it is about // to be refused for (#92). - if col.Hidden || !p.opts.computedAllowed(col) { + if col.Hidden || !p.opts.reachable(col) { continue } // Wire, not Name: this list is what a caller is told it may type, and @@ -1032,7 +1074,7 @@ func (p *parser) parseSearch(term string) (sqlb.Pred, bool) { } var preds []sqlb.Pred for _, col := range p.model.Columns { - if col.Searchable && !col.Hidden && p.opts.computedAllowed(col) { + if col.Searchable && !col.Hidden && p.opts.reachable(col) { preds = append(preds, sqlb.F(col.Name).Contains(term)) } } @@ -1077,7 +1119,7 @@ func (p *parser) parseSort(raw string) []sqlb.Order { col := p.model.ColumnByWire(term) switch { - case col == nil || col.Hidden || !p.opts.computedAllowed(col): + case col == nil || col.Hidden || !p.opts.reachable(col): p.errAllowed("sort", term, "unknown column", p.capable(capSort)) continue case !col.Sortable: @@ -1130,15 +1172,19 @@ func (p *parser) parseSelect(raw string) []string { continue } col := p.model.ColumnByWire(name) - if col == nil || col.Hidden || !p.opts.computedAllowed(col) { + if col == nil || col.Hidden || !p.opts.reachable(col) { p.errAllowed("select", name, "unknown column", p.selectableNames()) continue } out = append(out, col.Name) } // A projection that dropped the primary key cannot address its own rows, - // so it is added back rather than surprising the client later. - if len(out) > 0 && p.model.PK != nil && !contains(out, p.model.PK.Name) { + // so it is added back rather than surprising the client later — unless the + // resource narrowed itself out of the key, in which case adding it back + // would put the one column Options.Columns excluded into every response + // that named any other (#148). + if len(out) > 0 && p.model.PK != nil && !contains(out, p.model.PK.Name) && + p.opts.reachable(p.model.PK) { out = append([]string{p.model.PK.Name}, out...) } return out @@ -1461,7 +1507,7 @@ func contains(list []string, s string) bool { func (p *parser) selectableNames() []string { out := make([]string, 0, len(p.model.Columns)) for _, col := range p.model.Selectable() { - if !p.opts.computedAllowed(col) { + if !p.opts.reachable(col) { continue } // The wire spelling, because a 400 that lists names the caller cannot diff --git a/rest/binding.go b/rest/binding.go index c88b607..130b810 100644 --- a/rest/binding.go +++ b/rest/binding.go @@ -44,6 +44,15 @@ type binding[T any] struct { // real false (#92). selectable []*sqlb.ColumnInfo + // served is selectable as a set, for the places that ask about one column + // rather than walking the projection — the OpenAPI parameter list, mostly. + // + // Derived from selectable rather than recomputed from Options, because the + // document and the parser disagreeing about what a request may name is the + // failure both #92 and #148 are shaped like: a filter parameter published + // for a column every request naming it is about to be refused for. + served map[string]bool + // writable is what a request body may set. Read-only columns are excluded // because the database or a hook owns them, and hidden ones because a // column that never leaves the process should not be settable from @@ -107,7 +116,19 @@ func bind[T any](opts Options) (*binding[T], error) { opts: opts, model: m, jsonKey: make(map[string][]byte, len(m.Columns)), - selectable: selectableFor(m, opts.Computed), + selectable: selectableFor(m, opts.Computed, opts.Columns), + } + + // Checked before the loop below, because that loop decides what a request + // body may write and an unchecked name there is a resource quietly serving + // the wrong surface. + if err := checkColumns(m, opts); err != nil { + return nil, err + } + reachable := reachableSet(m, opts.Columns) + b.served = make(map[string]bool, len(b.selectable)) + for _, col := range b.selectable { + b.served[col.Name] = true } // The unrendered names, for the diagnostic below. Serialising wants the @@ -128,7 +149,13 @@ func bind[T any](opts Options) (*binding[T], error) { } b.jsonKey[col.Name] = key } - if col.ReadOnly { + // A column outside Options.Columns is treated exactly as a read-only one + // on the write path: cleared off the row a request produced, rather than + // merely absent from the generated body. The generated body is shared + // with the wide resource, so "absent" is not something the narrowed + // mount can arrange — and a CreateBody.Row that sets the column would + // otherwise write it through a resource that cannot even read it (#148). + if col.ReadOnly || !reachable(col) { b.readOnly = append(b.readOnly, col.Index) continue } @@ -577,19 +604,74 @@ func (b *binding[T]) relationsFor(names []string) []expansion { return out } +// checkColumns validates Options.Columns against the model. +// +// Both refusals are startup-only on purpose. An unknown name would leave the +// resource serving one column fewer than somebody meant, with no request able +// to report it; a missing primary key would leave a resource that cannot +// address, order or page its own rows, and the symptoms of that arrive one at +// a time and in the wrong place. +func checkColumns(m *sqlb.Model, opts Options) error { + if len(opts.Columns) == 0 { + return nil + } + for _, name := range opts.Columns { + if m.Column(name) == nil { + return fmt.Errorf( + "rest: %s declares Columns %q, but %s has no such column (have: %s)", + opts.name(), name, m.Type, strings.Join(m.ColumnNames(), ", ")) + } + } + if m.PK != nil && !containsName(opts.Columns, m.PK.Name) { + return fmt.Errorf( + "rest: %s narrows to Columns that leave out the primary key %q; "+ + "the key addresses a row, settles the ordering and is what a cursor is built from, "+ + "so a resource without it cannot page — add %q, or drop Columns and use Hidden if the key must never be served", + opts.name(), m.PK.Name, m.PK.Name) + } + return nil +} + +// reachableSet answers "is this column part of this resource" once, so the +// binding loop does not walk the allowlist per column. +func reachableSet(m *sqlb.Model, columns []string) func(*sqlb.ColumnInfo) bool { + if len(columns) == 0 { + return func(*sqlb.ColumnInfo) bool { return true } + } + in := make(map[string]bool, len(columns)) + for _, name := range columns { + in[name] = true + } + return func(col *sqlb.ColumnInfo) bool { return col != nil && in[col.Name] } +} + +func containsName(list []string, s string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false +} + // selectableFor is the resource's projection: every non-hidden column, minus -// the computed ones it did not ask for. +// the computed ones it did not ask for and the ones outside its surface. // // Model.Selectable cannot answer this on its own — it is model-wide, and the // same model may be mounted twice with different computed sets, which is the -// case that made a shared model expensive to read (#92). -func selectableFor(m *sqlb.Model, computed []string) []*sqlb.ColumnInfo { +// case that made a shared model expensive to read (#92), and twice with +// different column sets, which is the public-and-privileged pair (#148). +func selectableFor(m *sqlb.Model, computed, columns []string) []*sqlb.ColumnInfo { wanted := make(map[string]bool, len(computed)) for _, name := range computed { wanted[name] = true } + reachable := reachableSet(m, columns) out := make([]*sqlb.ColumnInfo, 0, len(m.Columns)) for _, col := range m.Selectable() { + if !reachable(col) { + continue + } if col.Computed() && !wanted[col.Name] { continue } diff --git a/rest/columns_test.go b/rest/columns_test.go new file mode 100644 index 0000000..25581e4 --- /dev/null +++ b/rest/columns_test.go @@ -0,0 +1,271 @@ +package rest_test + +import ( + "fmt" + "net/http" + "strings" + "testing" + + "github.com/danielgtaylor/huma/v2" + "github.com/danielgtaylor/huma/v2/humatest" + "github.com/jryannel/sqlb" + "github.com/jryannel/sqlb/rest" +) + +// Options.Columns is the answer to two surfaces over one table (#148). +// +// A storefront and an admin panel read the same products and differ in which +// columns each may see, and Hidden cannot say that: Hidden is a property of the +// model and there is one model per table. So the narrowing moved to the mount, +// where Computed had already put it for a different reason (#92). +// +// The Post model stands in for the shape. `public` serves three columns; the +// wide mount below serves everything, which is the half that has to keep +// working — a narrowing that narrowed the model rather than the resource would +// pass every test in this file except the last one. + +// publicOptions is the narrow surface: id, title, status, and nothing else. +func publicOptions() rest.Options { + o := postOptions() + o.Path = "/public/posts" + o.Name = "public-post" + o.Columns = []string{"id", "title", "status"} + return o +} + +// The projection the database sees, the keys the response carries, and the +// parameters the document publishes all follow the resource rather than the +// model. +func TestANarrowedResourceReadsAndServesOnlyItsColumns(t *testing.T) { + db := newFakeDB(t, reply{ + cols: []string{"id", "title", "status"}, + rows: [][]any{{"p1", "Hello", "draft"}}, + }) + api := mount(t, db.db, publicOptions()) + + resp := api.Get("/public/posts") + if resp.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", resp.Code, resp.Body) + } + + // Read, not merely hidden on the way out. A resource that selected the + // column and dropped it during serialisation would still put the value on + // the wire between Postgres and this process, and into every query log + // along the way. + stmt := db.lastStatement() + for _, gone := range []string{`"body"`, `"excerpt"`, `"view_count"`, `"org_id"`, `"created_at"`} { + if strings.Contains(stmt, gone) { + t.Errorf("the narrowed resource selected %s:\n%s", gone, stmt) + } + } + for _, want := range []string{`"id"`, `"title"`, `"status"`} { + if !strings.Contains(stmt, want) { + t.Errorf("the narrowed resource did not select %s:\n%s", want, stmt) + } + } + + body := resp.Body.String() + for _, gone := range []string{"body", "excerpt", "view_count", "org_id", "created_at"} { + if strings.Contains(body, `"`+gone+`"`) { + t.Errorf("the response carries %q:\n%s", gone, body) + } + } +} + +// A request naming an excluded column is refused as *unknown*, and the list of +// what would have been accepted does not name it either. +// +// Both halves matter. ADR-0011 makes the rejection part of the contract, and a +// narrowed resource that answered "column is not filterable" — or listed +// view_count among the allowed names — would confirm the existence of the +// column it was narrowed to conceal. Echoing back the name the client sent is +// not a disclosure: the client typed it. +func TestANarrowedResourceRefusesAnExcludedColumnAsUnknown(t *testing.T) { + db := newFakeDB(t) + api := mount(t, db.db, publicOptions()) + + for _, q := range []string{ + "/public/posts?view_count=gte.3", + "/public/posts?sort=-created_at", + "/public/posts?select=id,body", + } { + resp := api.Get(q) + if resp.Code != http.StatusBadRequest { + t.Fatalf("%s: status = %d, want 400: %s", q, resp.Code, resp.Body) + } + problem := decode(t, resp.Body.Bytes()) + errs, _ := problem["errors"].([]any) + if len(errs) == 0 { + t.Fatalf("%s: no errors in %v", q, problem) + } + for _, raw := range errs { + detail, _ := raw.(map[string]any) + if msg, _ := detail["message"].(string); !strings.Contains(msg, "unknown") { + t.Errorf("%s: message = %q, want it to read as unknown", q, msg) + } + for _, name := range detail["allowed"].([]any) { + switch name { + case "view_count", "created_at", "body", "excerpt", "org_id": + t.Errorf("%s: the allowed list names %q, which the resource does not serve: %v", + q, name, detail["allowed"]) + } + } + } + } +} + +// The operation's parameters come from the same surface the parser enforces. A +// published parameter for a column every request naming it is about to be +// refused for is the failure both #92 and #148 are shaped like. +func TestANarrowedResourceDocumentsOnlyItsParameters(t *testing.T) { + db := newFakeDB(t) + api := mount(t, db.db, publicOptions()) + + list := api.OpenAPI().Paths["/public/posts"].Get + if list == nil { + t.Fatal("the narrowed resource has no list operation") + } + names := map[string]bool{} + enums := map[string]bool{} + for _, p := range list.Parameters { + names[p.Name] = true + if p.Schema != nil && p.Schema.Items != nil { + for _, v := range p.Schema.Items.Enum { + enums[strings.TrimPrefix(fmt.Sprint(v), "-")] = true + } + } + } + for _, gone := range []string{"view_count", "created_at", "excerpt", "org_id", "body"} { + if names[gone] { + t.Errorf("the document publishes a %q filter the resource will refuse", gone) + } + if enums[gone] { + t.Errorf("the document offers %q in a sort or select enum", gone) + } + } + // And it does publish what it serves, or the assertions above would pass on + // an operation with no parameters at all. + if !names["title"] || !names["status"] { + t.Errorf("the narrowed resource lost its own filters: %v", names) + } + if !enums["title"] { + t.Errorf("the narrowed resource lost its sort and select enums: %v", enums) + } +} + +// The limitation, asserted rather than left to be discovered: the *response +// schema* is the model's Go type, registered once as a component and shared by +// every mount of it, so it still lists the columns this resource does not +// serve. The runtime response omits them — TestANarrowedResourceReadsAndServes +// OnlyItsColumns is the assertion that matters — but a client generated from +// the document will carry optional fields that are always absent. +// +// It is recorded here because the honest reading of it is a scope boundary, +// not a bug to be fixed inside Options: a per-resource response schema needs a +// per-resource Go type, which is the generated second resource this option +// deliberately did not build. See rest.Options.Columns. +func TestTheResponseSchemaStillDescribesTheModel(t *testing.T) { + db := newFakeDB(t) + api := mount(t, db.db, publicOptions()) + + schema := api.OpenAPI().Components.Schemas.Map()["Post"] + if schema == nil { + t.Fatal("the model's schema is not registered under its own name") + } + if _, ok := schema.Properties["view_count"]; !ok { + t.Skip("the response schema is now narrowed per resource; delete this test and the paragraph in Options.Columns that predicts it") + } +} + +// A PATCH naming an excluded column is refused, as unknown rather than as +// read-only, and the writable list does not name it. +func TestANarrowedResourceRefusesAWriteToAnExcludedColumn(t *testing.T) { + db := newFakeDB(t) + api := mount(t, db.db, publicOptions()) + + resp := api.Patch("/public/posts/p1", map[string]any{"body": "rewritten"}) + if resp.Code != http.StatusUnprocessableEntity { + t.Fatalf("status = %d, want 422: %s", resp.Code, resp.Body) + } + body := resp.Body.String() + if !strings.Contains(body, "unknown column") { + t.Errorf("the rejection should read as unknown, got %s", body) + } + for _, stmt := range db.statements() { + if strings.Contains(stmt, "UPDATE") { + t.Errorf("a refused patch still issued an update:\n%s", stmt) + } + } +} + +// Two refusals at mount, both startup-only because neither has a request that +// could report it. +func TestColumnsIsCheckedAgainstTheModel(t *testing.T) { + db := newFakeDB(t) + + // A typo would otherwise be a resource serving one column fewer than + // somebody meant, silently and forever. + opts := publicOptions() + opts.Columns = []string{"id", "titel"} + err := mountErr(t, db.db, opts) + if err == nil { + t.Fatal("a Columns entry naming no column was accepted") + } + for _, want := range []string{"titel", "title"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal should name %q: %v", want, err) + } + } + + // The key addresses a row, settles the ordering and is what a cursor is + // built from, so a surface without it cannot page. + opts = publicOptions() + opts.Columns = []string{"title", "status"} + err = mountErr(t, db.db, opts) + if err == nil { + t.Fatal("a surface with no primary key was accepted") + } + for _, want := range []string{"primary key", "cursor"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal should explain itself, mentioning %q: %v", want, err) + } + } +} + +// The point of the feature, and the assertion the rest of this file cannot +// make on its own: both surfaces exist at once, over one model, and the +// privileged one is unchanged. +func TestTheWideResourceIsUnaffectedByTheNarrowOne(t *testing.T) { + db := newFakeDB(t, + reply{cols: postCols(), rows: [][]any{postRow("p1", "Hello")}}, + reply{cols: postCols(), rows: [][]any{postRow("p1", "Hello")}}, + ) + _, api := humatest.New(t, huma.DefaultConfig("Test", "1.0.0")) + for _, opts := range []rest.Options{publicOptions(), postOptions()} { + if err := rest.Resource[Post, PostCreate, PostUpdate](api, db.db, opts); err != nil { + t.Fatalf("mounting %s: %v", opts.Path, err) + } + } + + // The admin surface filters on the column the public one does not have. + resp := api.Get("/posts?view_count=gte.3") + if resp.Code != http.StatusOK { + t.Fatalf("the wide resource lost a filter: %d %s", resp.Code, resp.Body) + } + if !strings.Contains(resp.Body.String(), `"view_count"`) { + t.Errorf("the wide resource stopped serving view_count:\n%s", resp.Body) + } + // And the narrow one still refuses it, from the same process and the same + // model. + if code := api.Get("/public/posts?view_count=gte.3").Code; code != http.StatusBadRequest { + t.Errorf("the narrow resource answered %d for a column it does not serve", code) + } +} + +// mountErr registers the resource and returns the mounting error rather than +// failing, for the refusals above. +func mountErr(t *testing.T, db sqlb.Executor, opts rest.Options) error { + t.Helper() + _, api := humatest.New(t, huma.DefaultConfig("Test", "1.0.0")) + return rest.Resource[Post, PostCreate, PostUpdate](api, db, opts) +} diff --git a/rest/item.go b/rest/item.go index 7536316..bf13c0e 100644 --- a/rest/item.go +++ b/rest/item.go @@ -159,7 +159,12 @@ func (b *binding[T]) expansions(ctx context.Context, names []string) ([]string, } q, err := filter.Parse( url.Values{"expand": {strings.Join(names, ",")}}, - filter.Options{Model: b.model, Expandable: b.opts.Expandable, Computed: b.opts.Computed}, + filter.Options{ + Model: b.model, + Expandable: b.opts.Expandable, + Computed: b.opts.Computed, + Columns: b.opts.Columns, + }, ) if err != nil { return nil, asHumaError(ctx, err, b.opts.name()) @@ -336,13 +341,15 @@ func registerDelete[T any](api huma.API, w writer, b *binding[T]) { // // A hidden column is reported as unknown rather than as unwritable, and never // appears in the allow-list, so that the rejection cannot be used to enumerate -// what the resource is concealing. +// what the resource is concealing. A column outside Options.Columns is treated +// the same way and for the same reason: from this resource it does not exist, +// and "column is read-only" would confirm that it does (#148). func (b *binding[T]) rejectUnwritable(names []string) *Problem { var details []*ProblemDetail for _, name := range names { col := b.model.Column(name) switch { - case col == nil || col.Hidden: + case col == nil || col.Hidden || !b.served[name]: details = append(details, &ProblemDetail{ Message: "unknown column", Location: "body." + name, diff --git a/rest/list.go b/rest/list.go index 94314f6..956327f 100644 --- a/rest/list.go +++ b/rest/list.go @@ -84,6 +84,7 @@ func registerList[T any](api huma.API, db sqlb.Executor, b *binding[T]) { MaxOffset: opts.MaxOffset, Expandable: opts.Expandable, Computed: opts.Computed, + Columns: opts.Columns, DisableSearch: opts.DisableSearch, }) if err != nil { diff --git a/rest/params.go b/rest/params.go index ba32a04..9a63e4e 100644 --- a/rest/params.go +++ b/rest/params.go @@ -28,7 +28,12 @@ func listParams[T any](b *binding[T]) []*huma.Param { var params []*huma.Param for _, col := range b.model.Columns { - if col.Hidden || !col.Filterable { + // served, not merely !Hidden: a column this mount does not serve — one + // outside Options.Columns, or a computed one it declined — is refused + // by the parser, and publishing a parameter for it would document a + // filter that answers 400 and name a column the resource was narrowed + // to conceal. + if !b.served[col.Name] || !col.Filterable { continue } params = append(params, &huma.Param{ @@ -48,7 +53,7 @@ func listParams[T any](b *binding[T]) []*huma.Param { }) } - if sortable := capable(b.model, func(c *sqlb.ColumnInfo) bool { return c.Sortable }); len(sortable) > 0 { + if sortable := capable(b.selectable, func(c *sqlb.ColumnInfo) bool { return c.Sortable }); len(sortable) > 0 { terms := make([]any, 0, len(sortable)*2) for _, name := range sortable { terms = append(terms, name, "-"+name) @@ -87,7 +92,7 @@ func listParams[T any](b *binding[T]) []*huma.Param { }) if !b.opts.DisableSearch { - if cols := capable(b.model, func(c *sqlb.ColumnInfo) bool { return c.Searchable }); len(cols) > 0 { + if cols := capable(b.selectable, func(c *sqlb.ColumnInfo) bool { return c.Searchable }); len(cols) > 0 { params = append(params, &huma.Param{ Name: "search", In: "query", @@ -312,10 +317,16 @@ func isJSON(t reflect.Type) bool { } // capable lists the non-hidden columns satisfying want, for documentation. -func capable(m *sqlb.Model, want func(*sqlb.ColumnInfo) bool) []string { +// capable names the columns of a projection that carry a capability. +// +// It takes the binding's projection rather than the model, because the model is +// shared and the projection is this resource's: the same Product is mounted +// publicly and privileged, and only the second one's document may say +// cost_price_minor is sortable (#148). +func capable(cols []*sqlb.ColumnInfo, want func(*sqlb.ColumnInfo) bool) []string { var out []string - for _, col := range m.Columns { - if !col.Hidden && want(col) { + for _, col := range cols { + if want(col) { // Wire, because every caller of this builds an enum a client is // typed against or a message a caller reads. out = append(out, col.Wire) diff --git a/rest/rest.go b/rest/rest.go index a117699..db40a1f 100644 --- a/rest/rest.go +++ b/rest/rest.go @@ -155,6 +155,63 @@ type Options struct { // and one that does not select it no longer has to care. Computed []string + // Columns narrows this resource to the columns it names. Empty — the + // default, and what a generated resource emits — is every column the model + // has. + // + // This is the answer to two surfaces over one table (#148). A storefront and + // an admin panel read the same products, and they differ in which columns + // each may see: `cost_price_minor` and `internal_notes` are the reason the + // admin resource exists and must not be within a mile of the public one. + // Hidden cannot express that, because Hidden is a property of the model and + // there is one model per table; Computed already established that + // reachability is a property of the *mount*, and this is the same idea + // applied to stored columns. + // + // A column not listed is not reachable from this resource at all: absent + // from the response, absent from the SELECT the database sees, not + // filterable, not sortable, not searched, not nameable in ?select, not + // settable by a create or update body, and not named in the list a rejection + // offers — that last one because a narrowed resource that advertised the + // column it is about to refuse would leak the schema it was narrowed to + // hide. + // + // Every name must be a column of the model, and the list must include the + // primary key: it addresses rows, settles the ordering, and is what a cursor + // is built from, so a resource without it cannot page. Both are checked at + // startup, where the failure is a resource that will not mount rather than + // one serving a surface nobody meant. + // + // What this does not do is generate the second resource. Codegen emits one + // mount per exposed table, so the narrowed half is a hand-written + // rest.Resource call over the generated model — the models, the typed column + // facade, the manifest and the drift gate all still cover it, and only the + // mount is yours. The alternative, a second model over the same table, gives + // up all four. + // + // # Two things it does not narrow, and why + // + // **The response schema in the OpenAPI document.** It is the model's Go type, + // registered once as a component and shared by every mount of it, so it + // still lists the columns this resource does not serve. Runtime responses + // omit them and every parameter follows this list; what a client generated + // from the document gets is optional fields that are always absent. Narrowing + // it needs a per-resource Go type, which is the generated second resource + // that is a larger change than this one. + // + // **The create and update body types.** They are the caller's — C and U — so + // a narrowed mount reusing the wide resource's bodies documents fields it + // will not write. It will not write them: a column outside this list is + // cleared off the row a body produced, the same way a ReadOnly one is, and a + // PATCH naming one is refused as unknown. But the document says otherwise, + // so a resource narrowed for disclosure usually wants Ops without the write + // operations, or body types of its own. + // + // Both are worth reading as the shape of the boundary: this narrows what a + // resource *does*, and the parts of the document that come from a Go type + // still describe that type. + Columns []string + // DisableSearch rejects ?search even when columns are searchable. DisableSearch bool