test(contract): enforce Vary: Accept-Encoding is declared wherever it ships#231
Merged
Conversation
… ships GzipMiddleware adds Vary: Accept-Encoding to every response that reaches a handler (#229), and the spec documents it on the 2xx responses and the shared Error response. Nothing kept that documentation honest, so a new endpoint could declare Cache-Control (already enforced by TestSpec_Every200DeclaresCacheControl) but forget Vary, and the spec would drift from the wire again, which is the same problem #228 fixed. TestSpec_VaryAccompaniesEveryGzippedResponse closes the gap, modeled on the Cache-Control test. The discriminator is wider, because gzip runs inside auth but outside the mux: Vary ships on all 2xx and on the JSON error tier, and only the 401 responses lack it (auth short-circuits before the gzip layer). The test keys on that seam rather than on 2xx-vs-non-2xx. The shared Error component and every non-401 response must declare Vary; the Unauthorized component and any literal 401 must not. The spec was already correct as of #229, so this is a forward-looking guard. Proved it is non-vacuous by deleting a Vary block and confirming the test fails naming the offending response, then restoring it. > *This was generated by AI*
The Vary spec guard keyed wantVary on `code != "401"`, treating the 401 as the only response that short-circuits before GzipMiddleware. The auth path has a second pre-gzip tier: rest/auth.go returns codeUnavailable (503) on a ValidateApiKey lookup failure, before next.ServeHTTP, so a wire 503 from auth carries no Vary either. The test passes today only because the spec folds 503 into the operation `default`, which resolves to the Vary-carrying Error shape. But the discriminator was brittle: declaring an explicit "503" with the faithful no-Vary body (mirroring how "401" is declared) would flip wantVary to true and fail the guard, blocking a more faithful spec. Key wantVary on the full auth short-circuit set (`code != "401" && code != "503"`) so an explicit 503 is expected to carry no Vary, like the 401. The `default` arm stays Vary-expecting, since the handler errors that resolve to it do. Correct the docstring and comments so they no longer call the 401 the only pre-gzip tier. Test and comments only, no behavior change. > *This was generated by AI*
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.
Closes #230. Followup to #228 / #229.
#229 added
Vary: Accept-EncodingtoGzipMiddlewareand documented it in the spec, but nothing kept the two in step the wayTestSpec_Every200DeclaresCacheControldoes forCache-Control. This adds that guard.TestSpec_VaryAccompaniesEveryGzippedResponsewalks the spec and assertsVaryis declared on every response that reaches the gzip layer (all 2xx and the sharedErrortier) and absent on the one that does not (the 401Unauthorizedtier, which short-circuits in auth before gzip). It checks the same schema rigor as the Cache-Control test: present, optional, string,const: Accept-Encoding.Test-only change. The spec was already correct as of #229, so the guard passes today; its job is to fail a future PR that documents
Cache-Controlbut forgetsVary. Proven non-vacuous against three temporary spec mutations (a missing 200Vary, a missingErrorVary, and a strayUnauthorizedVary), each restored.