Skip to content

feat(server): say what was wrong with a rejected filter - #7061

Open
geovannewashington wants to merge 1 commit into
refactor/route-authority-claimsfrom
feat/filter-rejection-reasons
Open

geovannewashington wants to merge 1 commit into
refactor/route-authority-claimsfrom
feat/filter-rejection-reasons

Conversation

@geovannewashington

@geovannewashington geovannewashington commented Sep 8, 2026

Copy link
Copy Markdown
Member

What

A rejected filter now names its own reason in the fields map the error envelope already carries. Nine distinct causes get nine distinct reasons where there used to be two strings. The envelope, the 400 status and the OpenAPI schema are unchanged, so a client that only reads the status code is unaffected.

Companion PR: shellhub-io/cloud#2543 , which moves cloud's three list routes onto the same seam so they answer identically. Merge this one first.

Closes #6467

Why

ValidateFilters returned one sentinel for six causes and applyQuery collapsed seven into two strings, so a script author could not tell a typo in a field name from a filter that was merely too big. The server knew the difference at the moment it decided to refuse, and threw it away one call before anything needed it.

Before After
"filter": "is not valid" "filter": "names an unknown field: nmae"
"filter": "is not valid" "filter": "operator gt is not valid for name"
"filter": "is not valid" "filter": "value exceeds the maximum size for name"
"filter": "cannot be decoded" "filter": "is not valid base64"

Changes

pkg/api/query classifies its own refusals instead of discarding the classification. FieldConstraints.Names separates "no such field" from "that operator is not allowed here", which Allows used to answer as one. Three sentinels are added: ErrFilterNotBase64, ErrFilterTooManyItems, and ErrFilterShapeInvalid for JSON that parses but is not a filter. ErrFilterValueTooLarge splits an oversized value of an acceptable type from one of the wrong type. ErrFilterOperatorInvalid was declared but never returned by production code; it is now real. A FilterError carrier holds the offending field and operator alongside the sentinel and wraps it, so every existing errors.Is check keeps working.

server/api/pkg/gateway maps sentinel to reason in one place, at the seam every list route crosses, so no route can drift from the wording. The echoed identifier is capped at 64 runes and marks a cut, since the field name is client-supplied and only filter values are otherwise length-bounded.

Testing

Unit and HTTP-level tests pass: pkg/api/query, the gateway package, and the full server module. The gateway's rejection table went from 5 cases to 13, each asserting a distinct fields map. The one pre-existing failure, TestInstallScriptRendersAValidShellScript, is unrelated (the script is not mounted in the container).

./bin/docker-compose exec server go test ./api/pkg/gateway/ -run TestListHoldsTheQueryToTheContractItsRegistrationNamed -v
./bin/docker-compose exec server go test ./../pkg/api/query/...

Also exercised end to end against a running dev stack, since the reason strings are a client-facing contract and only a real request proves what a client receives.

TOKEN=$(curl -s -X POST http://localhost/api/login -H 'Content-Type: application/json' -d '{"username":"<you>","password":"<pw>"}' | jq -r .token)
AUTH="Authorization: Bearer $TOKEN"
b64() { echo -n "$1" | basenc --base64url -w0 | tr -d '='; }

curl -s "http://localhost/api/devices?filter=$(b64 '[{"type":"property","params":{"name":"nmae","operator":"eq","value":"x"}}]')" -H "$AUTH"
curl -s "http://localhost/api/devices?filter=$(b64 '[{"type":"property","params":{"name":"name","operator":"gt","value":"x"}}]')" -H "$AUTH"
curl -s "http://localhost/api/devices?filter=$(b64 '[{"type":"property","params":{"name":"name","operator":"eq","value":{"a":1}}}]')" -H "$AUTH"
curl -s "http://localhost/api/devices?filter=$(b64 "[{\"type\":\"property\",\"params\":{\"name\":\"name\",\"operator\":\"contains\",\"value\":\"$(printf 'A%.0s' {1..2000})\"}}]")" -H "$AUTH"
curl -s "http://localhost/api/devices?filter=$(b64 '{"nonsense":true}')" -H "$AUTH"
curl -s "http://localhost/api/devices?filter=$(b64 'not json at all')" -H "$AUTH"
curl -s "http://localhost/api/devices?filter=notbase64!!!" -H "$AUTH"

Observed, all 400:

{"message":"invalid entity","fields":{"filter":"names an unknown field: nmae"}}
{"message":"invalid entity","fields":{"filter":"operator gt is not valid for name"}}
{"message":"invalid entity","fields":{"filter":"value has the wrong type for name"}}
{"message":"invalid entity","fields":{"filter":"value exceeds the maximum size for name"}}
{"message":"invalid entity","fields":{"filter":"is valid JSON but not a filter"}}
{"message":"invalid entity","fields":{"filter":"is not valid JSON"}}
{"message":"invalid entity","fields":{"filter":"is not valid base64"}}

Controls: a valid filter still returns 200, and ?sort_by=nope still returns 400 with {"sort_by": "nope"} unchanged.

ValidateFilters answered six causes with one sentinel and applyQuery answered
seven with two strings, so a client could not tell a typo in a field name from a
filter that was merely too big.

The reason is written at the gateway rather than taken from the sentinel's own
text: a message the mapping did not write is not one a client was promised.

ErrFilterShapeInvalid and ErrFilterValueTooLarge exist because their causes were
being described by a message that was false for them. JSON that parses but is
not a filter was told it was not JSON, and a value of an acceptable type that
was merely too long was told it had the wrong type: the same misdirection this
commit removes elsewhere. Unmarshal now splits on json.SyntaxError, which is the
only failure that means the bytes are not JSON.

The echoed identifier is capped because the field name is client-supplied and
only filter values are length-bounded. 64 runes is chosen to exceed every field
any contract in either repo names, so no honest request is ever truncated. A cut
name is marked with an ellipsis, because a silently shortened one reads as a
real field the client never sent.

The switch's trailing "is not valid" is the pre-existing wording for a sentinel
the mapping does not know. Nothing forces a new ErrFilter* to be added here, so
a future one loses its message with no compiler or test signal. Left as is
rather than guarded, since the guard wants an in-package test and this package
is tested from outside.

Fixes: #6467
@geovannewashington geovannewashington added this to the 0.27.1 milestone Sep 8, 2026
@geovannewashington geovannewashington self-assigned this Sep 8, 2026
@geovannewashington
geovannewashington requested review from a team as code owners September 8, 2026 20:13
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Code Review Complete

The automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment /review.

View job

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.

API: return descriptive error bodies for filter/validation errors

1 participant