Seven from the issue sweep: #88, #84, #90, #92, #93, #97, and #17 cut - #99
Merged
Conversation
…sor carries it (#88) The builder could say NULLS LAST and the REST sort grammar could not, so a resource whose natural ordering needs it had no way to get it. `?sort=-published_at` compiled to a bare DESC, Postgres's default for which is NULLS FIRST, and a feed ordered by a column that is NULL until a row is published lifted every draft to the top. Declared on the column rather than spelled per request, which is the direction the issue argued for and the one that fits: where NULLs belong is a property of what the column *means* — a NULL published_at means "not published", which belongs last however the feed is sorted — not of what a caller wants. It also means the generated TypeScript and Dart clients need no new syntax to get it right, and that the grammar stays one character of prefix. schema.Timestamp("published_at").Nullable().Sortable(schema.NullsLast) It reuses the schema.Nulls the index orders already use (#64), so a resource sorted `published_at DESC NULLS LAST` and the index declared to serve it are legible as the pair they are. The placement applies in both directions. A rule that only bit on DESC would be a rule with an invisible exception, and the default it exists to escape is itself direction-following — which is the whole defect. Two things fell out of it: - **the cursor had to carry it.** nullsAfter used to be a pure function of desc, so validating the direction validated the placement for free. It is not one any more. cursorTerm now encodes the *declared* placement, not the resolved one: a column with no declaration encodes nothing, so cursors issued before this field existed still decode, and a column that gains a declaration refuses them with the ordinary "drop the cursor when the sort changes" error instead of paging under an ordering they were not built for; - **restcompat had to see it.** A placement change removes no parameter and rejects no request, so the capability diff is blind to it by construction — the sort key exists on both sides. It is reported breaking, because lists come back in a different order and outstanding cursors stop working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Expanding a relation whose target had a date column answered 500.
json_build_object serialises a date as "2026-07-01", the Go field for it is a
time.Time, and encoding/json parses that strictly as RFC 3339 — so the decode
failed on every non-null date, at request time, only under ?expand.
Fixed in the SQL rather than the decoder, because the two representations were
already inconsistent and the expansion held the side nothing expected. A direct
read of the same column scans through pgx into a time.Time and Go marshals it
RFC 3339; codegen/dartclient.go says so in a comment written for exactly this
confusion ("a date column does not arrive as YYYY-MM-DD, whatever the column
type suggests"), and the TS client types it string | Date. So the cast does not
choose a wire format, it stops the expansion from contradicting the one already
in effect.
::timestamp AT TIME ZONE 'UTC', not ::timestamptz
That is the whole correctness of it. ::timestamptz resolves through the
session's TimeZone, so under Europe/Berlin the date 2026-07-01 comes back as
2026-06-30T22:00:00Z and the column loses a day. UTC midnight is what a direct
read produces.
What blocked it was that the compiler could not tell a date from a timestamptz:
both are time.Time in ColumnInfo.Type. So the logical type is now carried
through the struct tag as `type:date` and read back into ColumnInfo.PGType —
for every column, not only the ones a bug has been found on, because the next
question of this shape would otherwise need its own marker. Describe.SQLType is
the hand-written half.
Composed in codegen rather than added to FieldDesc.Capabilities: a type is not a
capability, and putting it in that list made the schema's own documentation
print it twice on one line.
One sibling deliberately not fixed: schema.TypeTime has the same defect on
paper — json_build_object emits "14:30:00" — but no test covers a TIME column
round trip and pgx's mapping for it could not be verified here, so casting it
would be a guess. Left for its own issue rather than fixed blind.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hich row it means (#90) An upsert could only assign the proposed row's own value. `updated_at = now()`, `count = count + 1` and `x = COALESCE(EXCLUDED.x, table.x)` had no spelling, and the workaround for the first is not neutral: computing the timestamp in Go moves its source from the database clock to the application clock and forces the column into the INSERT list so EXCLUDED can echo it back, so one column on the row ends up on a different clock from the rest. OnConflictUpdate([]string{"key"}, "payload"). OnConflictSet("updated_at", sqlb.Now()). OnConflictSet("hits", sqlb.Add(sqlb.Current("hits"), sqlb.Val(1))) Named OnConflictSet rather than Set: an Insert.Set would read as setting a column on the insert, and this is only ever about the conflict branch. The qualifier is required, which is the part worth arguing. Both rows are in scope inside DO UPDATE, so `count = count + 1` reads like an accumulation whichever side it resolves to, and SQL picks the stored one silently. Excluded and Current name the two, and a bare Field in an assignment is refused with an error that offers both — the expression is walked to find one, and Raw is exempt for the reason Raw is always exempt. Names are checked against the model on both sides of the assignment, so a typo is an error from this package naming the column rather than a 42703 at request time — the check the bare-column form already did, extended to where a column can now also be named. New vocabulary, kept to what the three cases need: Excluded, Current, Now, Val, Add, Sub. COALESCE needed nothing — Coalesce already existed and Selection.Expr makes it usable here. Assignments render after the bare columns and share the statement's bind numbering, so a parameterised assignment is an ordinary $n rather than a separate sequence that happens to line up. An assignment with no bare columns is still a DO UPDATE, not a DO NOTHING — the updated_at-only upsert is exactly the case the issue opened with. docs/release-1.0.md moves this from Before 1.0 to Done. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…to read (#92) A computed column is declared on the model and wanted by one screen. Everything reading the model paid for it: three aggregates declared for a list attached a correlated subquery each to every read, including an existence check by id, and a column declaring Needs made those reads *fail* — asking a query that only wanted to know whether a row exists for a "viewer" bind it had no business supplying. Nothing projects a computed column now unless it asks: sqlb.Query[Project]().WithComputed("total_tasks", "is_starred") rest.Options{Computed: []string{"total_tasks", "is_starred"}} Declaring stays global — the expression, its type, its binds. Selecting is per reader, which is where the cost is decided. For a resource it is a boundary rather than a projection setting: a computed column a resource does not select is not filterable, sortable, or nameable in ?select there either, and is absent from the "allowed" list in a rejection. A filter on a correlated subquery costs what the projection would have, so merely not projecting it would not have made it cheap. Three things followed: - **the obligation moved with the selection.** rest refused to mount any resource whose *model* declared a Needs column. It now asks only of the resources that render one — an obligation every mount inherited was an obligation with no failure behind it for most of them; - **the response shape got honest.** binding.selectable drives the JSON keys as well as the SELECT list, so a column the mount does not select is absent from the body rather than present holding its zero value, which for a bool would have been indistinguishable from a real false; - **generated resources are unchanged.** codegen emits Computed listing the table's own computed columns, because a computed column declared on an exposed table is one the schema meant that resource to serve. What the opt-in changes is everything else reading the model, which is where the bug was. example/computed gains Exists — the query that could not be written before, and now measures as costing nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ach past the row (#93) ?search fanned out over the resource's own columns and only those, which is right for most resources and silently wrong for any whose natural search question is about something it points at. A chat is named in the UI by whoever is in it — a direct message has no name column at all — so "type a colleague's name to find the conversation" found nothing for exactly those rows, with a 200. The workaround was closed: a computed column rendering the related names into one text value could not be Searchable. Two places refused it, and both gave the same reason — "?search fans out over text columns with ILIKE and an expression has no reading there". That reason is a claim about *type*, and the schema already makes it directly: "Searchable requires a text column" has always applied to computed columns too. So the blanket refusal added exactly one thing — the refusal of a computed column whose declared type *is* text, which has a perfectly good ILIKE reading and was the only way to search across a relation. Removed; the type rule does the work, and a Searchable bool expression is still refused, by the rule that was always the real one. The other half of the objection was cost: ?search over a correlated subquery runs it per candidate row. That is now answered where cost belongs. Since #92 a computed column reaches a resource only if the resource selected it, so the fan-out is gated on the same opt-in — a mount that does not select the expression does not search it either. Without that gate the opt-in would have governed the projection and leaked on the one path that runs the subquery most. This is the third of the issue's directions, and it covers both cases the issue raised, including the participant one — a jsonb array of ids, which is not a Ref and so would have stayed out of a relation-based spelling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 1, 2026
…mports it (#97) A Go program that wanted the typed client — the request encoder, the filter vocabulary, Do, the typed problem bodies — had to import a package carrying spf13/cobra and a whole command tree with it. A sync job or a server-to-server caller took a command-line framework to make one HTTP request, and an admin tool that already had its own command tree could not take the encoder at all. Two packages now: cli/client/client_gen.go Client, Request, Transport, Do, Run, Problem — the standard library, and nothing else cli/cli_gen.go the cobra tree, importing the above Nothing about Client or Do ever needed cobra. Three things did: run, which took a *cobra.Command, and the two flag helpers. run became an exported Run taking a context and an io.Writer — which is the whole of what the client needed to compile alone — and the command tree keeps a four-line runRequest that passes cmd.Context and cmd.OutOrStdout into it. This file argued for one file, and that argument is answered rather than dropped. It was about a different split — invariant runtime versus per-table commands, both of which need cobra — but its real point stands: an import set that depends on which operations a schema exposes fails at the *consumer's* build, because gofmt parses an unused import happily. So the CLI's imports are now derived from the rendered body rather than written down, which makes that class of mistake unrepresentable rather than merely avoided. A create-only schema no longer imports net/url, and a test asserts every import is used. Two knobs, and they compose: ClientDir emits the client alone, which is the server-to-server case; CLIDir emits both, defaulting the client to a client/ subdirectory, so an existing consumer keeps working. ClientImportPath is the CLI's import of the client, derived from the nearest go.mod and Dir, with an error naming the knob when it cannot be — an absolute Dir, or generating outside a module. Named runRequest rather than run because a generated unexported identifier shares a package with whatever the consumer writes beside it, and example/tasks had a hand-written run in exactly that package. The issue also records that three external readings concluded sqlb "emits no Go client", every mention having described it only as a CLI. docs/cli/README.md is now "Go client and CLI" and leads with the table of what lands where; README and the architecture fan-out name the client half too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…elf (#17) ADR-0041 named FromGo the tier most likely to be cut and wrote the condition under which to cut it: "if the first two applications express everything in SQL, cut it and keep the record honest about why." Both have, so it goes rather than sitting in the tracker as work nobody asked for. The evidence, recorded in the ADR rather than only here: - example/computed works the six values this record was written against. Three are stored counters; the other three are schema.Computed and every one is FromSQL. next_due_date — the value the proposal wrote FromGo for — is not among them and no example carries it; - the multi-app adoption declared five, described in #92 as "one per ADR-0041 tier: three aggregates over another table, one row-local predicate, and one that depends on who is asking". Tiers 1 to 3 again, working against a real database; - there is no FromGo in the tree, and codegen/schemasrc.go — which renders a schema back out of an introspected database — only knows how to emit FromSQL. A tier the round trip cannot express is a tier nothing can adopt. Two changes on the same day narrowed the space it would have occupied rather than widening it: #93 made a text expression over a related table declarable, which is the shape a "render related values into one field" case would have reached for Go to do, and #92 made computed columns opt-in per reader, which removed the cost argument for keeping derived work out of SQL. What the record does not claim: nobody tried FromGo and found it wanting. The finding is that nobody reached for it, which is weaker — and is exactly the evidence the trigger asked for. next_due_date over a recurrence rule is still the case with the best claim to needing Go, and it came from a codebase that has not adopted sqlb. That is written down too. The taxonomy keeps four rows. A record that quietly drops the option it rejected is not a record of the decision. docs/review-adoption-existing-app.md is deliberately untouched: it is a dated review, and rewriting what it proposed would falsify it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five failures, one per gate that runs something `go test ./...` at the root does not. pgtest, the two example modules and golangci-lint are separate modules or separate tools, so none of this was reachable from where the earlier commits were verified. **pgtest/computed_test.go — three failures from #92.** A computed column is opt-in now, and these tests read three of them without asking: the projection came back nil, and the filter and sort parameters came back "unknown". Same update the engine's own computed tests took, one module over, where the root suite never compiled it. **pgtest/expand_date_test.go — two failures, and the fix's fault was mine not the product's.** The models I wrote carried no json tags. An expanded row is built by Postgres as a JSON object keyed by *column* name, and encoding/json matches a tagless field case-insensitively without ignoring underscores — so "due_on" never reached DueOn and the date came back null. Tagged, and both tests now assert what they were written to assert: the date survives the expansion, it survives it as the same day, and the direct and expanded reads agree. That is #84 verified against a database rather than argued. **filter: columnNames was left unused** when #92 replaced its only caller with selectableNames. Removed. **example/fxapp/store was not regenerated** after #84 put the logical type in the struct tag. Regenerated; it is the fifth schema package and the only one outside the two modules the earlier commits touched. And one thing the pgtest run surfaced that no gate would have: census_test.go carried TestUpsertCannotExpressAnIncrement, whose own assertion said "if this now reports 8, an expression form was added and this test should become its demonstration". #90 added it. So it is that now — renamed, the raw-SQL workaround dropped because leaving the builder is no longer the price of an atomic counter, and a second case added for the insert branch, where Current reads a row that is not there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #88.
Closes #84.
Closes #90.
Closes #92.
Closes #93.
Closes #97.
The builder could say
NULLS LASTand the REST sort grammar could not, so a resource whose natural ordering needs it had no way to get it.?sort=-published_atcompiled to a bareDESC, whose Postgres default isNULLS FIRST, and a feed ordered by a column that is NULL until a row is published lifted every draft to the top.The direction
Declared on the column, which is the one the issue argued for:
Where NULLs belong is a property of what the column means — a NULL
published_atmeans "not published", which belongs last however the feed is sorted — not of what a caller wants. The grammar is untouched, so the generated TypeScript and Dart clients need no new syntax to get it right, and?sort=stays one character of prefix.It reuses the
schema.Nullsthe index orders already use (#64), so a resource sortedpublished_at DESC NULLS LASTand the index declared to serve it read as the pair they are.Description.SortNullsLast/SortNullsFirstare the hand-written-model half.The placement applies in both directions. A rule that only bit on
DESCwould be a rule with an invisible exception, and the default it exists to escape is itself direction-following.Two things that fell out
The cursor had to carry it.
nullsAfterused to be a pure function ofdesc, so validating the direction validated the placement for free. It is not a pure function any more, so a cursor issued before a redeploy that changed a declaration would be interpreted under a placement it was not built for — silently wrong pages, the one failure keyset paging exists to prevent.cursorTermnow encodes the declared placement rather than the resolved one, which is what keeps it backward compatible: a column with no declaration encodes nothing, so cursors issued before the field existed still decode, while a column that gains one refuses them with the existingdrop the cursor when the sort changeserror.restcompat had to see it. A placement change removes no parameter and rejects no request, so the capability diff is blind to it by construction — the sort key exists on both sides. It is now
LevelBreakingonFacetSort: lists come back in a different order and outstanding cursors stop working. Same shape of gap as #68, one facet over.Also carried into
ColumnManifest.SortNulls, besideCapabilitiesrather than in it — a request can neither ask for it nor decline it.Tests
filter/sortnulls_test.go— both directions, the undeclared column still renders noNULLSclause, the grammar still refuses a spellingschema/sortnulls_test.go— tag round-trip, both panics, the manifestcodegen/sortnulls_test.go— the declaration reaches the generated struct tag; without this the fix stops at the schema packagecursor_nulls_test.go— the old-cursor and new-declaration cases in both directionsrestcompat/restcompat_test.go— the break, and that it is not reported as a capability deltago test ./...andgo vet ./...green.pgtestis a separate module and vets clean, but I could not run it — no Docker here — so the live-database half is unverified.Not included
A resource still has no
DefaultSort, so a caller must send?sort=to get the ordering the resource was designed around. That was the issue's third direction and is separate work.🤖 Generated with Claude Code
06d0c2f— an embedded date is the same shape as a direct one (#84)Expanding a relation whose target had a date column answered 500:
json_build_objectserialises a date as"2026-07-01"and the Go field is atime.Time, which parses strictly as RFC 3339.Fixed in the SQL rather than the decoder, because the issue's reason for not doing that turned out not to hold. A direct read of the same column scans through pgx into a
time.Timeand Go marshals it RFC 3339 —codegen/dartclient.godocuments exactly this ("a date column does not arrive as YYYY-MM-DD, whatever the column type suggests") and the TS client types itstring | Date. So the two representations already disagreed and the expansion held the side nothing expected; the cast removes an inconsistency rather than introducing one.::timestamp AT TIME ZONE 'UTC', not::timestamptz— the latter resolves through the session'sTimeZone, so underEurope/Berlinthe date would come back a day earlier. There is a test whose only job is to fail if someone shortens it.Cost worth reviewing: the compiler could not tell a date from a timestamptz (both are
time.TimeinColumnInfo.Type), so the logical type now rides the struct tag astype:dateand lands inColumnInfo.PGType— for every column. That regeneratesmodels_gen.goacross the repo and grows every tag.Describe.SQLTypeis the hand-written half.One of the test updates was a real catch:
TestNoOverridesChangesNothingasserted!contains(models, "uuid")to mean "no uuid import", and every uuid column now saysuuidin its tag — it would have passed for the wrong reason forever. It looks for the import path now.Not fixed:
schema.TypeTimehas the same defect on paper, but nothing round-trips a TIME column and pgx's mapping for it could not be verified here, so casting it would be a guess.ad1e164— ON CONFLICT DO UPDATE assigns an expression (#90)Both constraints the issue asked to settle in the same change:
DO UPDATE, socount = count + 1reads like an accumulation whichever side it resolves to.ExcludedandCurrentname them, and a bareFieldis refused — the expression is walked to find one,Rawexempt as always;Assignments share the statement's bind numbering, so a parameterised assignment is an ordinary
$n.Not in the issue and would have been a bug:
DO NOTHINGwas chosen onlen(doUpdate) == 0, so an upsert carrying only an assignment — theupdated_at = now()case the issue opened with — would have compiled to a statement that writes nothing.New vocabulary kept to what the three cases need:
Excluded,Current,Now,Val,Add,Sub. COALESCE needed nothing —Selection.Expr()already madeCoalesceusable here.docs/release-1.0.mdmoves this from Before 1.0 to Done.34184e1— a computed column is opt-in (#92)A computed column is declared on the model and wanted by one screen; everything reading the model paid for it. Three aggregates declared for a list attached a correlated subquery each to every read — including an existence check by id — and a column declaring
Needsmade those reads fail, asking a query that only wanted to know whether a row exists for aviewerbind it had no business supplying.The issue proposed this at
rest.Options. That alone would not have fixed it: the query that failed is hand-written and never goes throughrest, so the default projection is where it moved.For a resource it is a boundary rather than a projection setting — a column the resource does not select is not filterable, sortable or nameable in
?selectthere either. A filter on a correlated subquery costs what the projection would have.Three consequences:
checkObligationsread the model, so every mount of a model declaring aNeedscolumn inherited the hook requirement. It now asks only of resources that render one;binding.selectabledrives the JSON keys as well as the SELECT list, so an unselected computed column is absent from the body rather than present holding its zero value;Computedlisting the table's own computed columns. Worth reviewing as a deliberate deviation from "defaulting to none": none for a hand-written mount, everything-declared for a generated one.e0bd206— a text expression may be Searchable (#93)?searchfanned out over the resource's own columns only, so a chat named in the UI by its participants — a direct message has nonameat all — found nothing for exactly the rows a search is for, and answered 200.The refusal that closed the workaround gave this reason in both places it appeared: "?search fans out over text columns with ILIKE, and an expression has no reading there." That is a claim about type, and
schema/registry.go:229already makes it directly —Searchable requires a text columnhas always applied to computed columns too. So the blanket refusal added exactly one thing: the refusal of a computed column whose declared type is text, which is the only way to search across a relation. Removed; aSearchablebool expression is still refused by the general rule.The cost half is answered by #92 landing first: the fan-out is gated on the same per-resource opt-in, so a mount that does not select the expression does not search it. That gate is load-bearing — without it the opt-in would govern the projection and leak on the path that runs the subquery most.
641ac0d— the Go client is its own package (#97)A Go program that wanted the typed client had to import a package carrying cobra and a whole command tree. Two packages now:
Three functions were the entire coupling:
run, which took a*cobra.Command, and the two flag helpers.runbecame an exportedRun(ctx, io.Writer, req, all), and the CLI keeps a four-linerunRequestbridging cobra to it.Two knobs, composing.
ClientDiralone emits the client and no command tree;CLIDiremits both, defaulting the client to aclient/subdirectory so an existing consumer keeps working.ClientImportPathoverrides the derived import path, which comes from the nearest go.mod plusDir.The old file's argument is answered, not dropped. It argued for one file on the grounds that a split import set would depend on which operations a schema exposes — and fail at the consumer's build, since gofmt parses an unused import happily. That is real, so the CLI's imports are now derived from the rendered body rather than written down, with a test that fails on any import the body never refers to.
Worth reviewing:
runhad to becomerunRequest, becauseexample/tasks/cli/cli_test.gohas a hand-writtenrunand the first build after the split failed on the redeclaration. Any short generated identifier is a hazard in a package the consumer also writes in.Docs:
docs/cli/README.mdis retitled Go client and CLI with a Just the client section, and README +docs/architecture.mdname the client half — the issue's point that three external readers concluded sqlb emits no Go client because every mention described only a CLI.96a43a5—FromGois cut (#17)Docs only. ADR-0041 named
FromGothe tier most likely to be cut and wrote the condition for cutting it — "if the first two applications express everything in SQL" — and both have:example/computed's three computed values are allFromSQL, and the multi-app adoption's five are tiers 1–3 (#92).codegen/schemasrc.gocan only emitFromSQL, so the round trip could not have carried the tier anyway.Two changes in this same PR narrowed the space it would have occupied: #93 made a text expression over a related table declarable, and #92 removed the cost argument for keeping derived work out of SQL.
The ADR gains a
FromGois cut section carrying the evidence, a struck-through trigger with the date it fired, and a note on what the cut does not claim — nobody tried it and found it wanting; nobody reached for it, which is weaker and is the evidence the trigger asked for. The taxonomy keeps four rows.Verification across all six
go test ./...,go vet ./...andgofmt -lare clean.pgtestis a separate module: it vets clean but could not be run here (no Docker), so the six live-database tests added across these commits are unverified. They are the ones that actually prove #84 and #90 — worth a CI run before merging.