refactor(server): give the List shape the query contract it serves - #7035
Open
otavio wants to merge 1 commit into
Open
refactor(server): give the List shape the query contract it serves#7035otavio wants to merge 1 commit into
otavio wants to merge 1 commit into
Conversation
otavio
force-pushed
the
refactor/list-query-contract
branch
from
September 3, 2026 21:52
7e391a3 to
c5488b1
Compare
otavio
force-pushed
the
refactor/list-query-contract
branch
from
September 5, 2026 19:14
c5488b1 to
79f5f2f
Compare
otavio
force-pushed
the
refactor/list-query-contract
branch
from
September 5, 2026 19:16
79f5f2f to
c5488b1
Compare
Fourteen routes answered with a page of a collection, and thirteen opened by hand-rolling the
same preamble: bind, normalize the paginator, normalize the sorter, unmarshal the base64 filter,
validate the filter fields, validate the sort fields, answer 400. One contract, written thirteen
times, in four different orders, with three of them skipping field validation altogether.
The order is now fixed, and it is the wrapper's: normalize the page, normalize the sort applying
the resource's default, decode the filter, hold the filter to the contract, hold the sort to the
contract, then the struct validation that was always last. Each step's failure was a 400 before
and is a 400 now, so reordering them changes no status.
The 400 gains a body: the invalid-entity error the converted device list already returned, which
the shared OpenAPI 400 component has documented all along. That is the deliberate behaviour
change. Three others ride along with the List shape and are worth naming:
- the thirteen routes now require an actor, because the shape's wrapper resolves one. Every
production caller carries X-ID; three test files did not, and now do.
- /namespaces and /users/invitations declare an unbounded scope. Both answer across namespaces
for a caller who may have selected none, so a bounded scope would 403 them.
- the SSH identity list's 401 gains a body, having been a bodiless NoContent.
ListSSHIdentities keeps a check on actor.ID that looks dead next to the wrapper's own: the
wrapper refuses a *zero* actor, and an API-key actor is not zero — it names a namespace with no
person behind it, so its user ID is empty. The route is open to API keys, so the check is live.
It reads the role through gateway.RoleFromContext rather than from the actor, because a role is a
membership's, and an actor is not yet a member of anything.
Access policies, service accounts and SSH identities set X-Total-Count to the length of the page
they were about to return, because their services discarded the count the store had already
computed. The count now comes from the store. For the first two that is the same number today;
the point is that it stops being the handler's to decide the day the route paginates. The
service-account store still derives its count from the slice, which is now the store's business
and not the route's.
list_validation_test.go is deleted in the same change. It was a linter written as a 171-line
go/ast test, and it had already gone blind: it matched handlers by their gateway-context
parameter, so the device list left its coverage the moment #6941 converted that route, and it
kept passing. The rule it enforced now reads the route table, where a route appears whatever
shape its handler has.
This breaks cloud, which reaches across the module replace for services.NamespaceFilterFields.
That is the next rung, as A-cloud was; it cannot start until shellhub-io/cloud#2534 lands.
Implements the B rung of #7032.
Fixes: #7034
otavio
force-pushed
the
refactor/list-query-contract
branch
from
September 5, 2026 19:21
c5488b1 to
552f6b8
Compare
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.
Stacked on #7030 — review that first. Base is
refactor/route-declarations, notmaster.What
The list-query contract becomes part of what the
Listshape is. A route of that shape names thefilter and sort fields its resource accepts on the line that mounts it, and the wrapper enforces
them before the handler runs:
A list handler then receives a request whose filter is decoded and whose fields are known-good, and
opens with the work it exists to do.
Why
Closes #7034. Implements the
Brung of #7032.Fourteen routes answered with a page of a collection, and thirteen hand-rolled the same preamble
before their first line of work — in four different orders, with three skipping field validation
altogether and nothing catching it. The check that should have caught it was a 171-line
go/asttest that had already gone blind: it matched handlers by their gateway-context parameter, so the
device list left its coverage the moment #6941 converted that route, and it kept passing.
Changes
query.Contract: one value per resource carrying the filter constraints, the sort field setand the default sort, replacing two loose exported field sets per resource. Thirteen contracts
cover the fourteen routes; the two membership-invitation lists share one. They stay in
services, where the field sets already lived — moving them would obscure the diff and breakcloud a second time while it is mid-stack.
gateway.Accepts: aRouteOptionin refactor(server): make route registration and declaration the same act #7030's shape. It records on the declaration andinstalls no middleware, because the wrapper is what enforces it.
resource's default, decode the filter, validate the filter, validate the sort, then the struct
validation that was always last. The default sort is applied before validation, so a default
naming a field the resource disallows is a test failure rather than a silent pass.
which the shared OpenAPI
400component has documented all along. This is the deliberatebehaviour change. It is the list-shaped slice of API: return descriptive error bodies for filter/validation errors #6467; that issue's contract-wide envelope, the
central error handler and the OpenAPI 4xx schemas are untouched, and API: return descriptive error bodies for filter/validation errors #6467 stays open.
X-Total-Countto the length of thepage they were about to return, because their services discarded the count the store had already
computed. Their signatures now carry it. Same number today; the point is that it stops being the
handler's to decide the day those routes paginate. No paginator is added — that is an API
contract change and out of scope.
int64tointat the service boundary, so thewrapper writes one header one way.
list_validation_test.godeleted, replaced by a predicate over the route table: a list-shapedroute names a contract. A route is in that table whatever shape its handler has, so the evasion
that blinded the old test cannot happen again.
CONTEXT.mdgains a query contract entry (inshellhub-io/claude, not this repo).Behaviour changes beyond the 400 body
Three ride along with the
Listshape and are worth a reviewer's explicit nod:production caller carries
X-ID; three test files did not, and now do./namespacesand/users/invitationsdeclare an unbounded scope. Both answer across namespacesfor a caller who may have selected none, so a bounded scope would 403 them. Both handlers ignore
the scope.
NoContent.No route gains or loses a permission.
Breaks cloud
cloud/internal/admin/routes/namespace.goreaches across the modulereplaceforservices.NamespaceFilterFields. That is the only cloud reference to a renamed symbol — cloud takesservices.Serviceas a parameter rather than implementing it, so the three changed servicesignatures do not reach it. Cloud follows one rung behind, as
A-clouddid, and cannot start untilshellhub-io/cloud#2534 lands.
Testing
A PR based on a feature branch runs no checks in this repository — the workflows are gated on
master. Everything below was run locally, in-container.Four seams, all pre-existing:
gateway/route_test.go): an unknown filterfield, an unknown operator, a non-base64 filter and an oversized filter each answer 400 naming
the field; a valid query reaches the handler with the filter decoded, the page normalized and the
default sort applied.
route_table_test.go), as a pure function over the declaration slice, with acompanion fed a known-bad table. Verified to fire against the real route table by removing one
Accepts.list_query_rejection_test.go): fifteen mounted entriesdriven with a refused filter field, a refused sort field, and — the direction the invariant
cannot check — a field the route's own contract allows. Verified to catch mis-wiring by
pointing the device list and then the session list at
TagQuery.deliberately disagree with the slice length, which is the only way to tell one from the other
while those lists are unpaginated.
Full server suite green apart from
TestInstallScriptRendersAValidShellScript, which fails on cleanHEADtoo (the container does not mountinstall.sh).golangci-lint: 0 issues on both modules.go mod tidyleaves both clean.