✨ enhance decoding options and tests for strict merge behavior, bracket notation, and list limits#55
Conversation
…et notation, and list limits
… for Node `qs` compatibility
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
🟢 Coverage 100.00% diff coverage · +0.06% coverage variation
Metric Results Coverage variation ✅ +0.06% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (05b1506) 1391 1359 97.70% Head commit (26c5945) 1430 (+39) 1398 (+39) 97.76% (+0.06%) 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 (#55) 62 62 100.00% 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 97.69% 97.76% +0.06%
==========================================
Files 20 20
Lines 1391 1430 +39
==========================================
+ Hits 1359 1398 +39
Misses 32 32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds ChangesQueryString Decoder strictMerge Option and List-Limit Parity
Sequence Diagram(s)sequenceDiagram
participant Parser as Decoder
participant Comma as _parseListValue
participant Promote as _promoteCommaListIfNeeded
participant Merge as Utils.merge
participant Mark as Utils.markOverflow
Parser->>Comma: Parse comma value
Comma->>Promote: Split and return values
Promote->>Merge: Combine iterable with listLimit check
Merge->>Mark: Mark overflow if limit exceeded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the Dart qs decoder to more closely match modern Node qs behavior, focusing on strict object/scalar merge conflicts, bracket-notation duplicate handling, and list-limit edge cases. It also expands unit coverage and updates end-user docs/changelog to reflect the new semantics.
Changes:
- Added
DecodeOptions.strictMerge(defaulttrue) and updated merge behavior for object/scalar conflicts, with a legacy mode (strictMerge: false) that restores scalar-key merging. - Adjusted decoding semantics for bracket notation duplicates and refined list-limit behavior (indexed brackets and comma-splitting overflow/promotion).
- Updated and expanded unit tests, plus README/CHANGELOG documentation for the new behaviors.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/models/decode_options.dart | Adds strictMerge option, wires it through copyWith, toString, and equality. |
| lib/src/utils.dart | Updates core merge/combine/overflow utilities to support new strict-merge and list-limit semantics. |
| lib/src/extensions/decode.dart | Adjusts decode pipeline for bracket-duplicate combining and post-decode comma-list promotion/limit handling. |
| test/unit/decode_test.dart | Adds/updates decoding tests for strict-merge conflicts, bracket duplicates, list-limit boundaries, and strict overflow behavior. |
| test/unit/utils_test.dart | Updates merge/overflow expectations and adds targeted tests for strict vs legacy merge behavior. |
| test/unit/models/decode_options_test.dart | Verifies strictMerge is preserved/overridden via copyWith and appears in toString. |
| test/unit/uri_extension_test.dart | Updates URI extension expectations to match revised list-limit/indexed bracket parsing semantics. |
| README.md | Documents bracket-duplicate combining behavior and introduces strictMerge behavior and examples. |
| CHANGELOG.md | Notes the new option and parity fixes (list limits, bracket duplicates, unlimited parameter parsing). |
Comments suppressed due to low confidence (1)
lib/src/utils.dart:117
Utils.mergenow treats several non-null scalar values ('',0,false,NaN) as a no-op via_isFalsyMergeSource, returning the target unchanged. This causes data loss for decode merge conflicts likea[b]=c&a=under the defaultstrictMerge: true(the empty string is dropped instead of being wrapped in a conflict list). Consider limiting the early-return check to onlynull/Undefined, and handling the “ignore empty/missing scalars” behavior only inside thestrictMerge: falselegacy branch (e.g., by using_legacyScalarMergeKeythere).
if (frame.phase == MergePhase.start) {
final dynamic currentTarget = frame.target;
final dynamic currentSource = frame.source;
if (_isFalsyMergeSource(currentSource)) {
stack.removeLast();
frame.onResult(currentTarget);
continue;
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/decode_test.dart (1)
2487-2592: ⚡ Quick winAdd a regression case for
parseLists: falsewitha[]under strict list limits.These new boundary tests are solid, but please also pin the learned guardrail:
a[]should stay literal whenparseListsis disabled and should not trigger list-limit strict throws.Suggested test addition
+ test('parseLists=false keeps [] literal and bypasses strict list-limit growth checks', () { + expect( + QS.decode( + 'a[]=1', + const DecodeOptions( + parseLists: false, + listLimit: -1, + throwOnLimitExceeded: true, + ), + ), + equals({ + 'a': {'0': '1'} + }), + ); + });Based on learnings: In
lib/src/extensions/decode.dart, list growth gating for array-like keys must usecombiningDuplicates || (options.parseLists && rawKey.endsWith('[]')), and this should be explicitly covered by tests.🤖 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 `@test/unit/decode_test.dart` around lines 2487 - 2592, Update the list-growth gating logic in lib/src/extensions/decode.dart so that array-like keys only count as list growth when either combiningDuplicates is true OR options.parseLists is true and the raw key ends with '[]' (i.e., change the condition to combiningDuplicates || (options.parseLists && rawKey.endsWith('[]'))); adjust the code path that checks/increments list size (the block that inspects rawKey, combiningDuplicates, and options.parseLists) and add a unit test asserting that with parseLists: false an input like 'a[]=' does not trigger list-limit strict throws (it remains literal), ensuring strict listLimit behavior only applies when parseLists is enabled.
🤖 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.
Nitpick comments:
In `@test/unit/decode_test.dart`:
- Around line 2487-2592: Update the list-growth gating logic in
lib/src/extensions/decode.dart so that array-like keys only count as list growth
when either combiningDuplicates is true OR options.parseLists is true and the
raw key ends with '[]' (i.e., change the condition to combiningDuplicates ||
(options.parseLists && rawKey.endsWith('[]'))); adjust the code path that
checks/increments list size (the block that inspects rawKey,
combiningDuplicates, and options.parseLists) and add a unit test asserting that
with parseLists: false an input like 'a[]=' does not trigger list-limit strict
throws (it remains literal), ensuring strict listLimit behavior only applies
when parseLists is enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d7d6f1b-0e22-4ffc-bd1b-0b9399acd453
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdlib/src/extensions/decode.dartlib/src/models/decode_options.dartlib/src/utils.darttest/unit/decode_test.darttest/unit/models/decode_options_test.darttest/unit/uri_extension_test.darttest/unit/utils_test.dart
This pull request introduces significant improvements to the query string decoding logic to better match the behavior of Node's
qslibrary, particularly regarding list limits, bracket notation, duplicate handling, and object/scalar merge conflicts. The main change is the addition of theDecodeOptions.strictMergeoption, which defaults to modernqsbehavior for merging objects and scalars. The PR also fixes several edge cases to ensure parity with recent versions ofqs, and updates the documentation accordingly.Decode behavior improvements:
DecodeOptions.strictMerge(default: true) to control how object/scalar merge conflicts are handled, matching modern Nodeqsbehavior by wrapping conflicts in a list, with an option to restore legacy merging. [1] [2] Fb87a85fL247R247, [3]qs6.15.0.qs6.14.2, including correct overflow and unlimited parsing behavior. [1] [2] [3] [4] [5]strictMergeis false, including protection against unsafe object keys. (Fb87a85fL247R247, lib/src/utils.dartR918-R953)Documentation updates:
README.mdandCHANGELOG.mdto document the newstrictMergeoption, improved duplicate handling, and clarified list limit semantics. [1] [2] [3] [4]Other codebase improvements:
These changes ensure that the Dart implementation closely tracks the behavior of Node's
qslibrary, improving compatibility and predictability for users porting code or expecting parity.