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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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 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/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 ba75a0a..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 @@ -912,7 +953,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 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() 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"}