Skip to content

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

Description

@otavio

Problem Statement

Fourteen routes answer with a page of a collection, and thirteen of them hand-roll the same
sequence before the first line of work happens: bind, normalize the paginator, normalize the sorter,
unmarshal the base64 filter, validate the filter fields, validate the sort fields, log what failed,
answer 400. It is one contract, written out thirteen times, and it has already drifted.

The order differs. The public-keys list validates the filter before normalizing the paginator;
the tags list unmarshals, validates the filter, then normalizes; the API-key and install-key lists
normalize, apply a default sort, then validate. Nothing makes one of these the right one, and a
fourteenth route would pick a fourth order.

The failure differs. Thirteen answer a bodiless 400 that tells the client nothing. The one
already-converted route, the device list, answers a JSON body naming the offending field. Two
contracts for the same failure, on the same API — and every list-validation test in the repository
asserts the status alone, so nothing notices.

The count differs. Eleven read a real count from the service. The membership-invitation lists
format an int64 where their neighbours format an int. Three — access policies, service accounts
and SSH identities — set X-Total-Count to the length of the slice they are about to return,
because their services discard the count the store already handed back. Today the two agree, since
none of those three paginate. The moment any of them does, the header silently becomes the page
size, which contradicts both written definitions of it: CONTEXT.md defines the list shape as
"a page of values with the size of the whole collection", and the shared OpenAPI header component
every list path references calls it "Total number of items matching the query."

Three skip field validation entirely, and nothing catches it, because the thing that would catch
it is a 171-line go/ast test. That test is the shape of the problem: it parses the requests
package for structs embedding a filter or sorter, parses the routes package for functions taking a
gateway context, and errors when a handler binds one without calling the validator. It is a linter
written in test form, because the contract has no home in the code. It also has a blind spot that
has already opened — it matches handlers by their gateway-context parameter, so the device list left
its coverage the moment that route was converted to a pure shape. The test still passes. It stopped
checking.

#7030 gave routes somewhere to put this. The wrapper that runs before every
converted handler already normalizes the paginator and the sorter when the request embeds them. What
it does not know is which fields that particular resource allows.

Solution

The list-query contract becomes part of what the List shape is, stated on the line that mounts the
route:

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

Accepts is a RouteOption in the shape #7030 established: it records the claim on the declaration
and installs no middleware, because the wrapper is what enforces it. The wrapper gains the two steps
it is missing — unmarshal the filter, then validate the filter and the sorter against the contract
the registration named — in one fixed order, beside the normalization it already performs.

The per-resource field sets, today two loose exported values per resource referenced by name from
inside a handler, collapse into one named contract per resource carrying both, plus the default sort
the resource wants when the client asks for none.

A handler then receives a request whose filter is decoded and whose fields are known-good. It does
not open with a validation preamble, because there is no preamble left.

The invariant that replaces the go/ast test reads the route table rather than the source text: a
route of the list shape names a query contract.
It cannot be evaded by changing a handler's
signature, which is exactly how the test it replaces went blind.

User Stories

  1. As a maintainer, I want the list-query contract written once, so that fourteen routes cannot
    drift into fourteen orderings of the same steps.
  2. As a maintainer, I want a route's accepted filter and sort fields stated on the line that mounts
    it, so that reading the registration tells me what the endpoint accepts.
  3. As a maintainer, I want a list handler to receive a decoded, validated request, so that its first
    line is the work it exists to do.
  4. As an API client, I want the same failure body from every list endpoint when my filter or sort is
    rejected, so that I write error handling once.
  5. As an API client, I want a rejected filter to tell me which field was wrong, so that I can fix
    the request without reading the server's source.
  6. As a maintainer, I want the total-count header written by the wrapper from a count the service
    returned, so that no handler decides where the header goes or what it counts.
  7. As an API consumer, I want the total count to mean the size of the collection, as the glossary
    and the OpenAPI header both say it does, so that paging through a list terminates correctly.
  8. As a maintainer, I want the three lists that substitute a slice length to carry the count the
    store already computed, so that the header does not quietly become wrong the day the route
    paginates.
  9. As a reviewer, I want a test that fails when a list route names no query contract, so that a new
    list endpoint cannot accept unvalidated fields by omission.
  10. As a reviewer, I want each list route's accepted fields pinned by a test that goes through HTTP,
    so that a route wired to the wrong resource's contract is caught rather than assumed.
  11. As a maintainer, I want the go/ast test deleted, so that the rule is enforced by the route
    table rather than by parsing the source — and cannot go blind when a handler changes shape.
  12. As a contributor adding a list endpoint, I want the compiler and the tests to demand the
    contract, so that the accepted field set cannot arrive by omission.
  13. As a newcomer, I want "query contract" defined in the glossary, so that the term on the
    registration line means something I can look up.

Implementation Decisions

Accepts is a route option, not an argument to the list shape. It records on the declaration
and returns no middleware, exactly as the unbounded and anonymous claims do. A second argument to
List would put the claim somewhere the declaration audits cannot read, and would not compose with
the option list every other claim already uses.

One contract value per resource, replacing the loose field sets. Each resource's sort field set,
filter constraints and default sort become a single named value. Thirteen such values cover the
fourteen list routes — the two membership-invitation lists share one. They stay in the services
package
, where the field sets live today: cloud already reaches across the module replace to
reference them by name, and relocating them in the same change would both obscure the diff and break
cloud a second time while it is mid-stack.

The default sort moves into the contract. The API-key list picks one default and the install-key
list another, each with a conditional in the handler body. That is the resource's policy, not the
handler's logic, and it belongs beside the fields the resource allows. It is applied during sorter
normalization — before validation, so a default is held to the same field set a client-supplied
sort is, and a default naming a field the resource does not allow is a test failure rather than a
silent pass.

Validation order is fixed, and it is the order the converted device list already uses: normalize
the paginator, normalize the sorter applying the default, unmarshal the filter, validate the filter,
validate the sorter, then the existing struct validation. Where a handler used a different order the
resulting status is unchanged, because each step's failure is a 400 either way.

The 400 gains a body, and this is the one deliberate behaviour change. Thirteen routes move from
a bodiless 400 to the invalid-entity error the device list already returns, which names the offending
field. The status is unchanged, so a client checking the status is unaffected; a client reading the
body currently reads nothing. This is the narrow, list-shaped slice of #6467 —
it does not attempt that issue's contract-wide error envelope, does not touch the central error
handler or the OpenAPI 4xx schemas, and #6467 stays open.

The three unpaginated lists propagate the store's count. Access policies, service accounts and
SSH identities each return a slice and an error while discarding the count their store call already
produces; the handler then substitutes the slice length. Their service signatures change to return
that count, and the routes become list handlers. Their request types gain no paginator here — adding
pagination to those endpoints is an API contract change and is out of scope — but the header stops
being derived from the page. The written basis for this is the shape definition in CONTEXT.md
and the shared total-count header component in the OpenAPI spec, both of which already say
collection size.

The membership-invitation count is converted at the service boundary. The wrapper writes the
header one way, so an int64 count becomes an int where the service returns it rather than the
handler formatting it differently from its eleven neighbours.

A list route that accepts no query still declares a contract. Where a resource takes no filter
and no sort, Accepts names a contract that allows nothing, rather than being omitted — an omission
is indistinguishable from a forgotten one, which is the failure mode the invariant exists to catch.

Only list handlers convert. A route cannot take a query contract while its handler still binds
and validates its own request, so the thirteen list handlers move to the list handler signature
here. Their single-value and no-content siblings in the same files do not, even though it leaves
those files half-converted for a rung or two. Taking each file to completion would pull the deferred
per-resource pass forward and turn a legible change into a large one.

CONTEXT.md gains a "query contract" entry. The glossary defines actor, shape, route
declaration
and unbounded scope, but nothing for filter, sorter, paginator, or contract. This
change introduces the term on the registration line, so it defines it in the same change, per
domain-modeling. No ADR is written: the repository's ADR directory does not yet exist, and this
decision is the tracking issue's B rung rather than a standalone one.

Testing Decisions

Four seams, all of which already exist. No new harness is built.

Seam 1 — the contract mechanism, once, through a mounted route. The gateway's route tests
already drive a mounted list route over HTTP through the wrapper, with helpers that build a router
with the binder, validator and error handler installed, and a probe handler that records what it
saw. That test is extended rather than joined: one probe route with a known contract, asserting that
an unknown filter field, an unknown sort field, a filter that is not valid base64, and an oversized
filter each answer 400 with the field named in the body; and that a valid query reaches the handler
with the filter decoded, the paginator normalized and the default sort applied. The same file
already pins the total-count header, which is where that behaviour stays.

Seam 2 — the invariant, as a pure function over declarations. The route-table test added by
#7030 holds every mounted route to its claims through predicates that are pure functions over the
declaration slice, each with a companion test fed a hand-built known-bad table so a passing run
proves the predicate looked. The new predicate — a list-shaped route names a contract — takes the
same shape, in the same file, against the same authenticated-router helper. list_validation_test.go
is deleted in the same commit, so the tree is never without the rule.

Seam 3 — the wiring, per route, through HTTP. The existing list-query rejection tests already
drive real endpoints with an unapproved filter field and an unapproved sort field, asserting the
status and that the service was never called; they cover three resources today and are extended to
all fourteen, and gain body assertions now that the 400 names the field. This seam earns its keep
because seam 2 cannot cover it: the invariant proves a route names a contract, not the right
one, and mis-wiring is precisely the risk this change introduces by moving the contract reference off
the handler body and onto the registration line.

Seam 4 — the count, for the three services whose signature changes. The device list's service
test already asserts the returned slice, count and error as one value, with the store mock returning
a count alongside the slice. The three converted services follow it, with the mock returning a count
that deliberately disagrees with the slice length — the only way to tell the store's count from the
slice's length.

Deliberately not tested separately: the thirteen contract values as data. Seam 3 pins each
route's accepted fields through HTTP and seam 1 pins the mechanism; a table test over the field
lists would assert the same thing a third time, one layer lower.

Go, mockery-generated service and store mocks, no testcontainers, no build tags — the routes and
gateway tests are untagged today. Run in-container via ./bin/docker-compose.

Out of Scope

  • The cloud copies. Cloud follows one rung behind, as A-cloud did, and cannot start until
    shellhub-io/cloud#2534 lands.
  • API: return descriptive error bodies for filter/validation errors #6467's contract-wide error envelope. This change gives list validation
    failures the body the converted device route already gives them; the central error handler and the
    OpenAPI 4xx schemas are untouched.
  • Adding pagination to the access-policy, service-account and SSH-identity lists.
  • Permissions. A′ is the security review; no route gains or loses a permission here.
  • Non-list handlers, including the single-value and no-content conversion of these files'
    siblings.
  • The remaining ~160 handlers — the per-resource conversion pass this stack defers.

Further Notes

The MCP surface reads the total-count header back off a recorder after re-entering the route through
an in-process request, so the header's format is load-bearing there as well as for clients. It keeps
working unchanged: the wrapper writes the same decimal string the handlers do.

The go/ast test's blind spot is worth recording, because it is the argument for the invariant that
replaces it rather than a tidiness complaint. It matches handlers by their gateway-context
parameter, so the device list dropped out of its coverage the moment #6941 converted that route. A
check over the route table cannot fail that way, because a route is in the table whatever shape its
handler has.

Note for CI: a pull request based on a feature branch runs no checks in this repository — the
workflows are gated on the default branch. Lint and tests must be run locally, in-container, before
this is called green.

Implements the B rung of #7032. Depends on #7030.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions