feat: implement strict merge option and update related tests#11
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR extends the query string decoder with two features: explicit handling of bracket-notation duplicate parameters (always combining them into arrays regardless of the ChangesDuplicate-Aware Bracket Parsing and Legacy Merge Behavior
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 2 |
🟢 Coverage 93.42% diff coverage · -0.08% coverage variation
Metric Results Coverage variation ✅ -0.08% coverage variation (-1.00%) Diff coverage ✅ 93.42% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (9ccd566) 4997 4891 97.88% Head commit (94bf559) 5053 (+56) 4942 (+51) 97.80% (-0.08%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#11) 76 71 93.42% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
==========================================
- Coverage 97.87% 97.80% -0.08%
==========================================
Files 37 37
Lines 4997 5053 +56
==========================================
+ Hits 4891 4942 +51
- Misses 106 111 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 `@README.md`:
- Around line 211-225: The README legacy-merge example violates the decode
contract by expecting Value::Bool(true); update the example so decode (called as
decode(..., &DecodeOptions::new().with_strict_merge(false))) only yields
permitted types — replace the Value::Bool(true) expectation with a String
variant (e.g., Value::String("").to_owned() or the appropriate string
representation your decoder emits for flags) so the asserted result for
legacy_merge.get("a") uses only Null, String, Array, or Object types.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6072467e-b9a8-4d84-bb19-cfa99e41b553
📒 Files selected for processing (13)
README.mdsrc/decode/accumulate/insert.rssrc/decode/accumulate/insert/tests.rssrc/decode/accumulate/process.rssrc/decode/tests/duplicates.rssrc/merge.rssrc/merge/tests.rssrc/options/decode.rstests/comparison/js/case.jstests/porting_ledger.mdtests/regressions.rstests/support/cases/node_parse.rstests/support/mod.rs
There was a problem hiding this comment.
Pull request overview
This PR refactors decode-time duplicate handling and merge conflict behavior to better match Node qs, adding a strict_merge decode option and making per-key duplicate strategy selection explicit (notably forcing bracketed array syntax to always combine duplicates).
Changes:
- Added
DecodeOptions::strict_merge(defaulttrue) and threaded it into the merge pipeline to support legacy object/scalar conflict behavior when disabled. - Introduced per-part duplicate strategy selection during decode accumulation (bracketed
[]keys forceDuplicates::Combine) and updated insertion helpers to accept an explicitDuplicates. - Expanded Node-backed parity coverage and updated README examples to document bracketed-duplicate and legacy-merge behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/support/mod.rs | Passes strictMerge into the Node parity harness options JSON. |
| tests/support/cases/node_parse.rs | Adds new Node parity cases for bracket-notation duplicates and strictMerge behavior. |
| tests/regressions.rs | Locks in strict_merge default and builder toggling behavior. |
| tests/porting_ledger.md | Updates the ledger to reflect ported duplicates + strictMerge parse coverage. |
| tests/comparison/js/case.js | Normalizes strictMerge defaulting on the JS side for parity runs. |
| src/options/decode.rs | Introduces the strict_merge option field + public getter/builder and default true. |
| src/merge/tests.rs | Adds unit tests validating strict vs legacy merge behavior. |
| src/merge.rs | Implements legacy merge handling when strict_merge is disabled (including boolean true key insertion). |
| src/decode/tests/duplicates.rs | Adds regression tests ensuring bracket notation combines regardless of global duplicates strategy. |
| src/decode/accumulate/process.rs | Selects “effective duplicates” per-part and threads explicit Duplicates through insertion paths. |
| src/decode/accumulate/insert/tests.rs | Updates tests to use the new insertion helpers that accept explicit Duplicates. |
| src/decode/accumulate/insert.rs | Adds *_with_duplicates insertion helpers and keeps old wrappers for default behavior. |
| README.md | Adds examples for bracketed-duplicate behavior and legacy merge via strict_merge(false). |
This pull request refactors how duplicate key handling is implemented during query string decoding. The main change is to ensure that bracketed array syntax (e.g.,
b[]=1&b[]=2) always uses the "combine" duplicate strategy, regardless of the global setting, while also making the duplicate handling logic more explicit and testable. Several helper functions are renamed or extended to accept the duplicate strategy as a parameter, and tests and documentation are updated accordingly.Duplicate handling improvements:
effective_duplicates_for_partfunction to select the correct duplicate handling strategy for each key, ensuring bracketed keys always useDuplicates::Combineeven if the global option is different.insert_value,insert_occupied_value,insert_default_value) to take an explicitDuplicatesparameter, and refactored their usage throughout the codebase. [1] [2] [3] [4] [5] [6]Test and documentation updates:
README.mddemonstrating the behavior of duplicate handling for bracketed and legacy merge syntax.Internal code cleanup:
process.rsto consistently use the new duplicate strategy logic and explicit parameters. [1] [2] [3] [4] [5] [6] [7]These changes make the handling of duplicate keys more robust, predictable, and easier to maintain.
Summary by CodeRabbit
Release Notes
New Features
strict_mergeoption to enable legacy object/scalar merge behavior (defaults to strict mode).Documentation