Skip to content

refactor(server): give the List shape the query contract it serves - #7035

Open
otavio wants to merge 1 commit into
refactor/route-declarationsfrom
refactor/list-query-contract
Open

refactor(server): give the List shape the query contract it serves#7035
otavio wants to merge 1 commit into
refactor/route-declarationsfrom
refactor/list-query-contract

Conversation

@otavio

@otavio otavio commented Sep 3, 2026

Copy link
Copy Markdown
Member

Stacked on #7030 — review that first. Base is refactor/route-declarations, not master.

What

The list-query contract becomes part of what the List shape is. A route of that shape names the
filter and sort fields its resource accepts on the line that mounts it, and the wrapper enforces
them before the handler runs:

gateway.GET(publicAPI, GetDeviceListURL, gateway.List(handler.GetDeviceList),
    gateway.Accepts(services.DeviceQuery), gateway.Guard(routesmiddleware.Authorize))

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 B rung 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/ast
test 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 set
    and 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 break
    cloud a second time while it is mid-stack.
  • gateway.Accepts: a RouteOption in refactor(server): make route registration and declaration the same act #7030's shape. It records on the declaration and
    installs no middleware, because the wrapper is what enforces it.
  • Fixed validation order, in the wrapper: normalize the page, normalize the sort applying the
    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.
  • 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. This is the deliberate
    behaviour 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.
  • 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. 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.
  • The membership-invitation count narrows from int64 to int at the service boundary, so the
    wrapper writes one header one way.
  • list_validation_test.go deleted, replaced by a predicate over the route table: a list-shaped
    route 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.md gains a query contract entry (in shellhub-io/claude, not this repo).

Behaviour changes beyond the 400 body

Three ride along with the List shape and are worth a reviewer's explicit nod:

  • 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. Both handlers ignore
    the scope.
  • The SSH identity list's 401 gains a body, having been a bodiless NoContent.

No route gains or loses a permission.

Breaks cloud

cloud/internal/admin/routes/namespace.go reaches across the module replace for
services.NamespaceFilterFields. That is the only cloud reference to a renamed symbol — cloud takes
services.Service as a parameter rather than implementing it, so the three changed service
signatures do not reach it. Cloud follows one rung behind, as A-cloud did, and cannot start until
shellhub-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:

  1. The mechanism, once, through a mounted route (gateway/route_test.go): an unknown filter
    field, 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.
  2. The invariant (route_table_test.go), as a pure function over the declaration slice, with a
    companion fed a known-bad table. Verified to fire against the real route table by removing one
    Accepts.
  3. The wiring, per route, through HTTP (list_query_rejection_test.go): fifteen mounted entries
    driven 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.
  4. The count, for the three services whose signature changed: the store mocks return counts that
    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 clean
HEAD too (the container does not mount install.sh). golangci-lint: 0 issues on both modules.
go mod tidy leaves both clean.

@otavio
otavio requested review from a team as code owners September 3, 2026 21:39
@otavio
otavio force-pushed the refactor/list-query-contract branch from 7e391a3 to c5488b1 Compare September 3, 2026 21:52
@otavio otavio added this to the 0.27.1 milestone Sep 3, 2026
@otavio
otavio force-pushed the refactor/list-query-contract branch from c5488b1 to 79f5f2f Compare September 5, 2026 19:14
@otavio
otavio requested a review from a team as a code owner September 5, 2026 19:14
@otavio
otavio force-pushed the refactor/list-query-contract branch from 79f5f2f to c5488b1 Compare September 5, 2026 19:16
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
otavio force-pushed the refactor/list-query-contract branch from c5488b1 to 552f6b8 Compare September 5, 2026 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(server): the list-query contract belongs to the List shape

1 participant