Four issues with one shape: a ceiling the mount could express, a predicate the inspection could not show, a deferral neither side could see, and a secret that is its own key - #156
Merged
Conversation
…eclaration could not `rest.Options` has five per-request cost ceilings; `schema.REST` had three. So a schema-first resource took the package default for `MaxSortTerms` and `MaxOffset` and had no way off it — and the two that were missing do not divide from the three that were there along any line: all five are per-resource cost bounds, all five are ints, all five have package defaults, and `MaxPageSize`, which is declarable, is the one a client is least able to abuse since it is capped rather than refused. `MaxOffset` is the one that matters. Its default is 100,000 and is *right* as a default precisely because it has to be safe for a table nobody described, which puts it two to four orders of magnitude above what any particular resource wants: a catalog with ten thousand products has no legitimate offset past ten thousand, and every one above it is a guaranteed empty page that still costs a scan to the end. The number is a function of the row count, which is known where the table is declared and nowhere else. Two fields, emitted like their three neighbours, carried in the manifest, and validated: a negative ceiling is refused rather than resolved, because every ceiling reads a non-positive value as "take the package default" — so a negative one reads as a tighter bound and behaves as the loosest available, which is the one direction a cost ceiling must not fail in. Two surfaces were dropping them on the way out, which is what makes this more than a missing field. The ejected exit emitted a literal `MaxSortTerms: 0`, discarding a declared value, and had no `MaxOffset` field at all — so the handlers that replace the API served `?page=50000000` while the API refused it, against a README that claims the exit refuses what the API refused, and with no `?cursor` to redirect the client to since keyset paging did not come out. And the generated skill stated the filter budget and neither of the others, which for the one budget a caller meets mid-walk is the difference between a documented ceiling and a surprise 400. Broken on purpose, per ADR-0016: with the emitted `MaxOffset` removed from `example/blog`, the new server test does not merely fail — the request that must be refused answers 200, which is the failure the package default was hiding. `example/blog` declares both, so the mount, the exit and the skill are all exercised on a number the default would not have produced. Closes #151
… the statement that runs `SQL()` renders what the caller built. On a model whose reads are confined by a `BeforeQuery` hook — which in a multi-tenant application is most of them — that is a statement with the confinement missing, and the two places a reader looks for the truth both showed it. `Explain` is the sharper half, because its own documentation claims otherwise: it answers "whether the plan regressed", and `WHERE status = $1` and `WHERE status = $1 AND org_id = $2` are different queries with different plans. The second is the one with the composite index behind it and the one production issues, so a plan-regression test written against `Explain` on a scoped model was guarding a query nobody runs — and would have stayed green through exactly the change that makes the real query seq-scan. It already had the `ctx` and the `Executor` the hooks need; it just did not call them. `SQL()`'s half is quieter and worse. The documentation offers it as how a reader confirms the safety claims made elsewhere, and the claim most worth confirming — "is the tenant predicate really on every read of this model?", which is what ADR-0008 and ADR-0030 lead with — is the one it cannot show. Someone checking that hooks work by printing `SQL()` concludes they do not. And a facet count assembled by splicing the rendered predicate into raw SQL counts rows the query would never have returned: nothing errors, the number is just too large, and in a multi-tenant application it is too large by every other tenant. So `Builder.Resolved(ctx, db)` returns a copy with the hooks and the expansion scopes applied, `Update` and `Delete` have the same method for theirs, and `Explain` and `ExplainAnalyze` compile through it. The exec paths were rewritten onto `Resolved` rather than keeping their own copy of clone-then-run-hooks, which is what stops the exported method and the wire from drifting apart — the failure being fixed is two paths disagreeing about one statement. `Insert` is deliberately not resolvable. `BeforeCreate` rewrites the rows rather than the statement, so applying it here would mutate the caller's data as a side effect of inspecting it, and what it changes are bind values, which no plan varies on. On `ExplainAnalyze` this is a correctness property rather than a reporting one: it executes, so a `BeforeDelete` scope now narrows what it removes. Broken on purpose, per ADR-0016: with the resolution removed from `explain`, the new tests fail naming the unconfined statement that reached the database rather than merely reporting a mismatch. Deliberately not: making `SQL()` itself resolve. It has no `ctx` and no executor, and giving it either would make the one method that renders a statement require a database to render it. Closes #153
`Hidden`'s doc names one property — the value must never leave the process —
and the generated facade asserts a second, by omitting the typed column: it must
never be predicated on. For a password hash the two coincide, which is presumably
why they were collapsed. You find a user by email and compare the hash in Go, and
`WHERE password_hash = $1` is a sign something has gone wrong.
For the rest of "and similar values" they come apart, and the hash *is* the
lookup key. Session tokens and API keys, password-reset and verification tokens,
webhook secrets keyed by fingerprint, idempotency keys: the client presents a
secret, the server hashes it, and the row is found by equality on the stored
hash. Every one of those must never be serialised, and `Hidden` is exactly the
right declaration for them — it just took away the operation they exist for, and
the case it does not fit is the one its own doc comment names.
So `LookupKey()` keeps the facade entry, and nothing else moves. It is not in
`Capabilities()` and adds no token to the struct tag, because there is nothing
for the engine to read — `sqlb.F("token_hash")` has always worked, which is why
this is friction rather than a blocker. What changes is that the compiler helps
at the one call site that should have it, and that the generated file says which
of the two kinds of secret each hidden column is, rather than stating a blanket
rule that is no longer true of every column in the block.
The REST side is deliberately untouched, and the doc says so where the
declaration is read. A hidden column still has no capability, so the filter
grammar still refuses `?token_hash=eq.…` with a 400 naming what would have been
accepted — a client that can probe a credential column by equality has an
oracle, and that refusal is what capabilities are for. This is a declaration
about Go, on the writer's side of a boundary the caller has already been trusted
with.
Refused without `Hidden`: there the facade carries the column anyway, so the
word would be a claim with no effect and the generated comment would be calling
an ordinary column a secret.
Broken on purpose, per ADR-0016: with `facadeFields` back to plain `!Hidden`, the
new test fails reporting the missing typed column — the compile error the issue
opens with.
Deliberately not: a `Secret()` that implies Hidden. It reads better in one line
and makes the existing spelling the odd one out, and two words that compose say
the same thing without a second way to declare the first property.
Closes #155
…ad back and diffed `DEFERRABLE INITIALLY DEFERRED` had no spelling in the DSL, which is the small half. The interesting half is that nothing noticed: the introspector did not read `condeferrable`, the differ had no field to compare, and the renderer emitted none — so a constraint altered by hand in a migration survived `sqlb check` green, and the round trip was a fixpoint *because both sides were blind to the same property*. That is ADR-0016's failure mode stated about a field rather than about an object, and it is the same shape as the v0.8.0 exclusion work the ADR already records. The consequence is narrow and points one way. A migration that recreated a deferred constraint without its clause would break every write that relies on the deferral — for the reported case, creating any multi-variant product — and the drift gate, which is the thing that exists to catch exactly that, would have stayed green through it. So `schema.Deferrable` is a field on `Unique` and `Field.Deferred()` is the column-level spelling, with `Suffix()` normalising the way `IndexOrder.Suffix` does: the diff fingerprints the rendered clause, so two spellings of one answer compare equal and an unchanged constraint is never proposed for replacement. The clause is carried in a second place as well — `ADD CONSTRAINT … USING INDEX` renders from parts rather than from the definition, and a clause dropped there produces a constraint the very next diff wants to rebuild. The reporting is the half that matters more, and it covers what the DSL cannot declare. Deferrability is now read for every constraint kind, and a deferrable primary key, foreign key, check or exclusion is a `Skip` with its `pg_get_constraintdef` attached — the registry genuinely does not describe the database, and reconciling it means keeping the hand-written migration. A single-column UNIQUE that is deferrable-but-immediate is reported too rather than rounded to either neighbour, since one reading changes when every other writer's statement is checked and the other takes the option away. The declarable case is deliberately UNIQUE alone. That is where the need is — a rule about the committed state, over rows written by more than one statement — and the alternative to a narrow declaration was not a broad one, it was silence. Broken on purpose, per ADR-0016, and the break reproduces the report exactly: with `deferralOf` returning NotDeferrable, `TestRebuiltDatabaseMatchesTheOriginal` fails naming the constraint that lost its clause while `TestRoundTripIsAFixpoint` *passes* — both registries having dropped the same thing. The fixpoint fixture now carries a deferred UNIQUE in both spellings, so the property is asserted against a real Postgres rather than assumed. Adopters should expect new report entries for databases that already defer something the DSL cannot declare. That is the change working: the entry is what turns an invisible divergence into one a person can decide about. Closes #154
…ion says what the declaration cannot, and nothing reports it #148, #151, #153, #154 and #155 read as five unrelated minor gaps, and four of the five were filed as minor with a workaround already in hand. They are one shape. The mount narrowed a resource's columns and bounded a scan; a migration deferred a constraint; a hook scoped a read; a caller predicated on a hidden column — and in each case the declaration could not say it. The missing spelling is the cheap half. The expensive half is that nothing said the gap was there: a schema-first resource silently took a package default two orders of magnitude past what its row count justified, a hand-altered constraint passed `sqlb check` because the declaration and the database were blind to the same property, and `SQL()` rendered a statement the database would never run under documentation offering it as the way to confirm that scoping works. The general form is worth stating on its own, because it is what makes these more than five small gaps: a tool reporting *no difference* is making a claim, and a tool that cannot see a property makes that claim about it whether or not a difference exists. ADR-0014 named silent dropping and ADR-0016 requires a guard to have failed on purpose; this is the same argument one level down, about a property rather than about an object. So the rule is: close the gap where that is cheap, and where it is not, make the gap visible — a refusal at the boundary, a report from the tool that reads the database, or a sentence where the reader is standing, in that order. What it rules out is the fourth option, which is what each of these had: correct behaviour, an available workaround, and the two facts documented in different files from each other. Deliberately not "declare everything". The DSL stays narrow on purpose in several places that have their own records, ADR-0050 among them; this says that where the weaker answer is taken, the gap it leaves is reported rather than left to be found. The cost is recorded too — reporting makes previously-green things red, and a report nobody reads is the failure mode of taking this too far.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four open issues, and they turned out to be one shape — the one #148 named a PR
earlier. The mount bounded a scan, a migration deferred a constraint, a hook
scoped a read, and a caller predicated on a hidden column; in each case the
declaration could not say it, and in three of the four nothing reported the
gap. The missing spelling is the cheap half.
The five ceilings, of which three were declarable (#151)
rest.Optionshas five per-request cost bounds andschema.RESThad three, andthe two that were missing do not divide from the three that were there along any
line: all five are per-resource, all five are ints, all five have package
defaults, and
MaxPageSize— which was declarable — is the one a client isleast able to abuse, since it is capped rather than refused.
MaxOffsetis the one that matters. Its default of 100,000 is right as adefault precisely because it must be safe for a table nobody described, which
puts it two to four orders of magnitude above what any particular resource
wants. The number is a function of the row count, known where the table is
declared and nowhere else.
Two surfaces were dropping the pair on the way out, which is what makes this
more than a missing field. The ejected exit emitted a literal
MaxSortTerms: 0and had no
MaxOffsetfield at all — so the handlers that replace the APIserved
?page=50000000while the API refused it, against a README claiming theexit refuses what the API refused, and with no
?cursorto redirect the clientto since keyset paging did not come out. The generated skill stated the filter
budget and neither of the others, which for the one budget a caller meets
mid-walk is the difference between a documented ceiling and a surprise 400.
The inspection points showed a statement nobody runs (#153)
SQL()renders what the caller built. On a model confined by aBeforeQueryhook — most of them, in a multi-tenant application — that is a statement with
the confinement missing, and both places a reader looks showed it.
Explainis the sharp half, because its own documentation claims otherwise:WHERE status = $1andWHERE status = $1 AND org_id = $2have differentplans, the second is the one with the composite index behind it, and a
plan-regression test written on the first stays green through exactly the change
that makes the real query seq-scan.
SQL()'s half is quieter: the docs offer itas how a reader confirms the safety claims, and the claim most worth confirming
is the one it cannot show.
So
Builder.Resolved(ctx, db), with peers onUpdateandDelete, andExplain/ExplainAnalyzecompiling through it. The exec paths were rewrittenonto
Resolvedrather than keeping their own copy of clone-then-run-hooks — thefailure being fixed is two paths disagreeing about one statement.
Insertisdeliberately not resolvable:
BeforeCreaterewrites the rows, so resolving onewould mutate the caller's data as a side effect of inspecting it.
A constraint's deferrability, on both sides (#154)
DEFERRABLE INITIALLY DEFERREDhad no spelling, which is the small half. Theinteresting half is that the round trip was a fixpoint because both sides were
blind to the same property — ADR-0016's failure mode stated about a field
rather than about an object, and the same shape as the v0.8.0 exclusion work the
ADR already records. A migration that recreated a deferred constraint without
its clause would break every write relying on the deferral, and the drift gate
would have stayed green through it.
Declarable on
UNIQUE(Unique.Deferrable,Field.Deferred()), read forevery constraint kind, and reported as a
Skipon the kinds it cannot bedeclared on, with the definition attached.
The break-on-purpose reproduces the report exactly: with
deferralOfreturningNotDeferrable,TestRebuiltDatabaseMatchesTheOriginalfails naming theconstraint that lost its clause while
TestRoundTripIsAFixpointpasses.A hidden column that is the key it is looked up by (#155)
Hiddennames one property; the facade's omission asserts a second. For apassword hash they coincide. For session tokens, API keys, reset tokens and
idempotency keys they do not — the hash is the lookup key, and
Hiddentookaway the operation the column exists for.
LookupKey()keeps the facade entry and moves nothing else: no capability, nostruct-tag token, and
?token_hash=eq.…is still a 400 naming what would havebeen accepted. Option 1 rather than the report's preferred option 2, because the
generated comment can then say which of the two kinds of secret each hidden
column is instead of stating a blanket rule that is no longer true of every
column in the block.
The rule that came out of it
ADR-0051. A tool
reporting no difference is making a claim, and a tool that cannot see a
property makes that claim about it whether or not a difference exists. Close the
gap where that is cheap; where it is not, make the gap visible — a refusal at the
boundary, a report from the tool that reads the database, or a sentence where the
reader is standing. What it rules out is the fourth option all of these had:
correct behaviour, an available workaround, and the two facts documented in
different files from each other.
Deliberately not "declare everything". The DSL stays narrow in several places
with their own records, ADR-0050 among them.
Worth knowing before a tag
Adopters whose databases already defer a foreign key, primary key, check or
exclusion will now get report entries where they got none, and
sqlb introspectexits non-zero on them. That is the change working, and it is a new obligation
for a schema that had been quietly fine.
Everything else is additive: a schema setting none of the new fields generates
what it generated before.
Checks
mise run cigreen in full, includingtest-pg,test-demoandtest-fx.bisect-checkgreen: every commit builds, vets and tests in isolation. Each ofthe four guards was broken on purpose per ADR-0016 and the failure recorded in
its commit body.
Closes #151
Closes #153
Closes #154
Closes #155
🤖 Generated with Claude Code