🐛 bug: Fix handling of wildcard matching in acceptsOffer#3880
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Negotiator as negotiation routine
participant Helpers as acceptsOffer(spec, offer)
Note over Helpers: Handles trailing "*" as prefix pattern (case-insensitive)
Negotiator->>Helpers: acceptsOffer(spec, offer)
alt spec ends with "*"
Helpers->>Helpers: prefix = spec[:-1]
alt prefix == ""
Helpers-->>Negotiator: true
else if len(offer) < len(prefix)
Helpers-->>Negotiator: false
else
Helpers-->>Negotiator: return strings.HasPrefix(strings.ToLower(offer), strings.ToLower(prefix))
end
else
Helpers-->>Negotiator: return strings.EqualFold(spec, offer)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
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 |
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of content negotiation by refining the logic for wildcard matching in offer specifications. It ensures that wildcard patterns like Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in wildcard matching for acceptsOffer and adds a comprehensive set of tests to cover various wildcard scenarios, including prefix matching and complex content negotiation with quality weights. The changes are solid, and the added tests are thorough. I have one suggestion to simplify the implementation in acceptsOffer for better readability and maintainability.
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3880 +/- ##
==========================================
+ Coverage 91.68% 91.71% +0.03%
==========================================
Files 115 115
Lines 9967 9970 +3
==========================================
+ Hits 9138 9144 +6
+ Misses 530 527 -3
Partials 299 299
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This pull request expands test coverage for wildcard pattern matching in content negotiation, specifically for Accept-Charset header handling. The implementation correctly handles prefix-based wildcard matching with quality weight negotiation.
Key Changes
- Enhanced wildcard matching logic to properly validate prefix matches rather than accepting all wildcards unconditionally
- Added comprehensive test coverage for basic wildcard prefix matching scenarios
- Added complex negotiation tests with overlapping quality weights to ensure correct selection priority
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
helpers.go |
Fixed wildcard matching in acceptsOffer to verify that offers actually start with the wildcard prefix, preventing false positive matches for patterns like utf-* |
helpers_test.go |
Added 11 new test cases covering wildcard prefix validation (basic cases, prefix mismatch scenarios) and complex quality-weighted negotiation with multiple competing patterns |
gaby
changed the title
renanbastos93
left a comment
renanbastos93
left a comment
There was a problem hiding this comment.
Suggested Improvement
Great fix! The logic is now correctly aligned with the expected behavior for prefix wildcards (e.g., utf-*).
I’d like to suggest one improvement to increase readability, maintainability, and test quality: extract the wildcard-matching logic into a dedicated helper function.
Right now, acceptsOffer is correct, but it handles two responsibilities at once (exact matching and wildcard matching). Splitting this into smaller helpers makes the intent clearer and makes unit testing far easier, especially for edge cases. This also reduces the chance of future regressions.
Example:
func acceptsOffer(spec, offer string, _ headerParams) bool {
if hasWildcardSuffix(spec) {
return matchWildcardOffer(spec, offer)
}
return utils.EqualFold(spec, offer)
}
func hasWildcardSuffix(s string) bool {
return len(s) > 0 && s[len(s)-1] == '*'
}
func matchWildcardOffer(spec, offer string) bool {
// spec is ASCII-based (e.g. "utf-*"), last byte is always '*'
prefix := spec[:len(spec)-1]
// '*' alone accepts anything
if prefix == "" {
return true
}
if len(offer) < len(prefix) {
return false
}
return utils.EqualFold(prefix, offer[:len(prefix)])
}Benefits of extracting the logic:
- Improved readability: the flow becomes easier to understand at a glance.
- Better testability:
matchWildcardOfferandhasWildcardSuffixcan be tested independently. - More granular test coverage: easier to cover edge cases for prefix matching vs. exact matching.
- Simpler future maintenance: if new wildcard patterns need to be supported later, the structure already accommodates this cleanly.
Overall, the fix is great — this is just an incremental improvement to make it even more robust. If it helps, I’m happy to open a follow-up PR with the refactor.
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a bug in the wildcard matching logic within the acceptsOffer function. The previous implementation, which incorrectly accepted any offer for a wildcard specification, has been replaced with proper prefix-based matching. The change is clear, correct, and well-contained. Furthermore, the addition of extensive test cases in helpers_test.go provides solid validation for the fix, covering various wildcard scenarios and complex content negotiation cases. Overall, this is an excellent contribution that improves the robustness of the framework.
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary