test(request): use order-insensitive Set comparison for Allow header assertions#1380
test(request): use order-insensitive Set comparison for Allow header assertions#1380terminalchai wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThe ChangesHTTP 405 Allow Header
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/request.ts`:
- Around line 340-345: The Allow header construction can emit duplicate "HEAD"
entries when allowHead is true and expected already contains "HEAD"; modify the
code that builds the Allow header (around variables allowed, expected, allowHead
and the HTTPError construction) to deduplicate methods before joining—e.g.,
compute a set/unique array from [...allowed, allowHead ? "HEAD" : ...[]] and
then join that unique list into the Allow header string so duplicate tokens are
not emitted in the thrown HTTPError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64d1ee0d-2dbe-4087-90ea-9376b8785080
📒 Files selected for processing (3)
docs/2.utils/1.request.mdsrc/utils/request.tstest/utils.test.ts
| const allowed = Array.isArray(expected) ? expected : [expected]; | ||
| throw new HTTPError({ | ||
| status: 405, | ||
| headers: { | ||
| Allow: allowHead ? [...allowed, "HEAD"].join(", ") : allowed.join(", "), | ||
| }, |
There was a problem hiding this comment.
Deduplicate Allow methods before joining.
At Line 344, allowHead can produce duplicate HEAD tokens when expected already contains "HEAD" (for example ["GET", "HEAD"] + allowHead=true).
Suggested patch
if (!isMethod(event, expected, allowHead)) {
const allowed = Array.isArray(expected) ? expected : [expected];
+ const allowSet = new Set(allowed);
+ if (allowHead) {
+ allowSet.add("HEAD");
+ }
throw new HTTPError({
status: 405,
headers: {
- Allow: allowHead ? [...allowed, "HEAD"].join(", ") : allowed.join(", "),
+ Allow: [...allowSet].join(", "),
},
});
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const allowed = Array.isArray(expected) ? expected : [expected]; | |
| throw new HTTPError({ | |
| status: 405, | |
| headers: { | |
| Allow: allowHead ? [...allowed, "HEAD"].join(", ") : allowed.join(", "), | |
| }, | |
| const allowed = Array.isArray(expected) ? expected : [expected]; | |
| const allowSet = new Set(allowed); | |
| if (allowHead) { | |
| allowSet.add("HEAD"); | |
| } | |
| throw new HTTPError({ | |
| status: 405, | |
| headers: { | |
| Allow: [...allowSet].join(", "), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/request.ts` around lines 340 - 345, The Allow header construction
can emit duplicate "HEAD" entries when allowHead is true and expected already
contains "HEAD"; modify the code that builds the Allow header (around variables
allowed, expected, allowHead and the HTTPError construction) to deduplicate
methods before joining—e.g., compute a set/unique array from [...allowed,
allowHead ? "HEAD" : ...[]] and then join that unique list into the Allow header
string so duplicate tokens are not emitted in the thrown HTTPError.
2 participants
Problem
The
Allowheader assertions intest/utils.test.tsused strict string equality:The HTTP spec (RFC 9110 §10.2.1) doesn't mandate any particular ordering for method tokens in the
Allowheader. Pinning the test to a specific order makes it brittle — if the internal method set iteration order ever changes, the tests fail even though the response is completely valid.Fix
Convert both assertions to order-insensitive
Setcomparisons:Same values, no ordering assumption.
Summary by CodeRabbit
New Features
assertMethodfunction now includes anAllowresponse header in 405 error responses, listing the permitted HTTP methods in compliance with RFC 9110.Documentation
Allowheader behavior included in 405 error responses.Tests
Allowheader presence and contents for 405 error scenarios.