test: lock in v5 group route method-handling (405 + OPTIONS)#3003
Merged
Conversation
Adds regression tests around automatic group catch-all (404) route registration. v5 removed that auto-registration; if it is restored (e.g. PR #2996) these tests ensure it does not bring back the issues that motivated the removal: - wrong HTTP method on an existing group route must still return 405 (with Allow header), not be masked to 404 by the catch-all; - the group catch-all must not shadow concrete sibling routes, root routes, or other sibling groups' routes. All four pass on current master (v5). The 405 test fails against the restore-v4-behavior approach in #2996, pinning down that tradeoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review of the original regression file (PR #3003): its comments described an automatic group catch-all "triggered by middleware" that does not exist on master (v5), so the tests passed for the wrong reason and the no-op middleware was inert. Rewrite to assert v5's actual, verified behavior: - method mismatch on a group route -> 405 with full Allow header - OPTIONS on a registered group route -> 204 with Allow (preflight-relevant) - concrete routes resolve; group prefix does not affect root routes The 405 and OPTIONS tests are real gates: a group-level catch-all (manual or the auto-registration proposed in #2996) masks both as 404, which these tests would then catch. Drops the false premise, the inert middleware, and the in-comment PR reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous commit recorded only the deletion of the old file; this adds the rewritten suite (group_method_handling_test.go). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vishr
changed the title
Addresses final review: converts the "verified empirically" comment into an actual test. TestGroupRoute_catchAllMasksMethodHandling registers a group-wide catch-all and asserts it masks both the 405 method-mismatch and the automatic OPTIONS (204) response as 404 — the regression the 405/OPTIONS gate tests guard against. Makes the rationale self-proving in-repo. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Adds
group_method_handling_test.go: tests that lock in v5's method-handling semantics for routes registered through aGroup, and act as a regression gate for changes that would reintroduce an implicit per-group catch-all (e.g. #2996, which restores it to fix CORS-on-group preflight).Behavior asserted (verified on
master)GET-only group routePOST /api/usersAllow: OPTIONS, GETOPTIONS /api/usersAllow: OPTIONS, GET(preflight-relevant)GET /api/usersGET /health(root)Why these are real gates
A group-level catch-all — whether registered manually via
g.RouteNotFound("/*", …)or automatically (as in #2996) — matches every method, which masks both the 405 and the automatic OPTIONS response as 404. Verified empirically: with such a catch-all,POSTandOPTIONSon/api/usersboth return 404. The first two tests would catch that regression.Note
This replaces an earlier version of this PR whose comments described a middleware-triggered "auto catch-all" that does not exist on
master(v5) — the tests passed for the wrong reason. Reworked after review to assert the actual, verified behavior and drop the inert no-op middleware. All four tests pass; gofmt- and vet-clean.🤖 Generated with Claude Code