refactor(server): give routes three shapes and hand handlers their inputs - #6941
Open
otavio wants to merge 7 commits into
Open
refactor(server): give routes three shapes and hand handlers their inputs#6941otavio wants to merge 7 commits into
otavio wants to merge 7 commits into
Conversation
otavio
force-pushed
the
refactor/pure-handlers
branch
from
August 24, 2026 16:49
9cbc40f to
ebffaa5
Compare
…orter A request type that embeds both query.Paginator and query.Sorter promotes two Normalize methods at the same depth, and they cancel each other out: neither is reachable through the outer type, and no interface over that name can be satisfied. Every list handler works around this by naming the embedded fields, which only works when the handler knows the concrete request type. GetPaginator and GetSorter are reachable because their names are unique, so code holding any request can normalize the page and the sort order without knowing what it is holding.
…er is The device list handler read a raw query parameter and appended two filter entries of its own. It did so after validation, so the pair escaped the filter-count limit and a caller could buy two extra entries by asking for containers. It also branched on the parameter being present rather than on its value, which made connector=false mean the opposite of what it says. The request now carries the caller's intent and the service builds the filter, following the precedent the sorter's tiebreak field already set. The appended pair counts against the limit like any other, and the intent reads the value. The result is a new filter set rather than an edit of the request, because a request is an input and reusing one must not compound the filter. The alias states its intent ahead of the caller's query string. The binder reads the first value of a repeated parameter, so /api/containers?connector=false would otherwise have returned exactly the devices the container endpoint exists to exclude. The route test that covered this asserted only that an operator preceded a property, never which devices the comparison kept. It is replaced by one that asserts the intent reaching the service, and by service tests that assert the filter in both directions.
…ne place The authenticator stamps seven headers through Identity.WriteTo, and everything downstream took them apart again one accessor at a time. Nothing held the two spellings of that header set together. IdentityFrom is the read side of that write, and a round-trip test is what keeps them naming the same headers: a field added to one and forgotten in the other passes review and the compiler, and shows up only as an identity that quietly loses part of itself on the way to a handler. An Actor is the identity narrowed to what a handler may see. Role and admin stay behind, because they decide what the caller may do and the middleware answers that first. An actor is not always a person: an API key and a device token name a namespace principal with no user behind them, which is why the type carries the credential rather than a user ID alone.
Eighty-five handlers each rewrote the same five blocks, and the copies had already drifted apart. X-Total-Count is written before the error check in two places and after it in ten, and two sites report the length of the page rather than the count the service returned. Three idioms answered "which namespace is this bounded to?", so a cross-tenant read could be introduced in any handler and reviewing for it meant reading all of them. Nothing enforced one answer, because no module held it. One module holds it now. A route registers under One, List or None, and the wrapper binds the request, normalizes the paginator and sorter, validates, resolves the namespace scope and the actor, calls the handler and encodes the result. Each shape produces an echo.HandlerFunc, so per-route middleware composes exactly as it does today. Every route is bounded and needs an actor unless its registration says why not, and both reasons are required arguments. The two claims are independent: a device authenticating with its own token is bounded to a namespace and still carries no actor. Each registration records what it claimed, which is what lets a test refuse a reason left empty -- Echo does not expose a route's handler, so there is nothing else to enumerate. Normalization runs before validation rather than after. Validating first turns an out-of-range page or an unknown sort order into a 400, where every list route today corrects it and carries on. The wrapper refuses a request when the gateway context is not installed, so a wiring mistake fails closed rather than serving unscoped data. It keeps stashing that context in the request context, which is how the service layer's tenant, username and identity lookups still reach it; dropping that would break them silently, with no compile error. The ceremony is asserted once, here, rather than re-tested per entity.
The first route to convert, and the one that proves both reason mechanisms: it reports on the instance, which belongs to no namespace, and a load balancer asks it before any credential exists. Neither claim is inferable, so both are typed at the route table where a reviewer reads them. The handler stops knowing about HTTP, so its test stops building a context to call it with.
…List shapes Both services already take a namespace scope, which is what makes these two the worked examples. The handlers keep only what is specific to listing devices -- the sort field and the caller's encoded filter -- and receive everything else already resolved. Two wire changes follow from the shapes and are intended. A failed list no longer carries X-Total-Count, which is the inconsistency the shapes exist to settle. A refused filter or sort field answers 400 with a body rather than an empty one, because a handler that does not know about HTTP cannot write an empty response and the global error handler now writes one for it. Each refusal logs its cause, which the status alone cannot carry. The device and MCP route tests sent a tenant and a role and no identity, which no authenticated request looks like: the authenticator stamps a user ID for a token and a key for an API key. They now send what the credential would carry, which is what the routes now require.
A required argument makes a reason impossible to omit. Only an inventory of the claims makes an empty one impossible to merge, so this reads back what the route table declared while it was built and refuses a claim that says nothing. A second test proves that check bites rather than passing because it looks at nothing. The exempt set names the routes that are not resource operations and so keep a direct registration. Echo does not expose a route's handler, so no test can prove that a route outside the set went through a wrapper; what the set buys is the other direction, where joining it is a visible edit next to the reason each member is exempt. Each member is cross-checked against the router, so an entry that outlives its route fails. The three converted routes are pinned to both their shape and their address, which rules out a claim recorded by a wrapper nothing mounted. Fixes: #6940
otavio
force-pushed
the
refactor/pure-handlers
branch
from
August 24, 2026 17:04
ebffaa5 to
f2cd7bb
Compare
Member
Author
|
/review |
|
Claude finished @otavio's task in 6m 4s —— View job Code Review CompleteReviewed 23 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is. Notable things I verified rather than flagged:
To request another review round, comment |
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.
What
Route registration owns the HTTP ceremony. A route registers under one of three shapes and the
wrapper binds the request, normalizes the paginator and sorter, validates, resolves the namespace
scope and the actor, calls the handler and encodes the result. A handler becomes a function of its
inputs and no longer knows that HTTP exists.
Three routes convert as worked examples. The other 82 keep the existing adapter, which stays until
the conversion.
Why
Closes #6940. Implements ADRs 0001 through 0005.
Eighty-five handlers each rewrote the same five blocks, and the copies had drifted.
X-Total-Countis written before the error check in two places and after it in ten, and two sites report the length
of the page rather than the count the service returned. Three idioms answered "which namespace is
this bounded to?", so a cross-tenant read could be introduced in any handler and reviewing for it
meant reading all of them.
Changes
server/api/pkg/gateway:One,ListandNone, each producing anecho.HandlerFuncsoper-route middleware composes exactly as before.
Listwrites the total count after checking theerror, from the count the handler returned. The wrapper refuses a request when the gateway context
is not installed, and keeps stashing that context in the request context, which is how the service
layer's tenant, username and identity lookups still reach it.
Unbounded(reason)andAnonymous(reason)both take a requiredreason. The two are independent — a device authenticating with its own token is bounded to a
namespace and still carries no actor. Each registration records a
Declaration, which is whatlets a test refuse a claim whose reason is empty; Echo does not expose a route's handler, so there
is nothing else to enumerate.
IdentityFrom/Identity.Actor: the authenticator stamps seven headers throughIdentity.WriteTo, and everything downstream took them apart one accessor at a time.IdentityFromis the read side of that write, andActornarrows the result to what a handlermay see. A round-trip test holds the two sides to the same header set.
gateway.Actor: the authenticated identity, which is not always a person. An API key and adevice token name a namespace principal with no user behind them, so requiring a user ID would
refuse every API-key and MCP caller. ADR 0004's wording implies user;
CONTEXT.mddoes not, andthe code follows
CONTEXT.md.the filter. The appended pair now counts against the filter limit instead of escaping it, and the
intent branches on the value rather than on the parameter being present. The handler stays the
only place that validates what a caller sent; the service checks the one thing appending can
break, which is the item count.
reads the first value of a repeated parameter, so
/api/containers?connector=falsepreviouslyreturned non-container devices.
query.Paginated/query.Sorted: a request embedding both aPaginatorand aSorterpromotes two
Normalizemethods at the same depth, which cancel out. These accessors are how thewrapper reaches them without reflection.
Testing
Three seams. The wrapper suite drives all three shapes through a router with faked identity headers
and asserts the ceremony as a table. The route-table test asserts every claim states a reason, that
the exempt set is registered, and that each converted route is both declared and mounted. The
service test asserts which comparison the connector intent produces, in both directions — the route
test it replaces only checked that an operator preceded a property, never whether connector devices
were included or excluded.
Worth a reviewer's attention:
X-Total-Count, and a 400 from thedevice-list guards now carries a JSON body instead of an empty one. Both follow from ADR 0001 and
its landed prerequisite.
connectoris now a bool, soconnector=xyzreturns 422; only therewrite sends that parameter.
first turns
order_by=garbageinto a 400 instead of correcting it todesc, which every listroute does today.
sent only a tenant and a role, which no authenticated request looks like; they now send the
identity header the credential would carry.
Merge together with shellhub-io/cloud#2509. Between them, cloud does not compile against
shellhub master.