✨ add strict_merge option and align list-limit semantics with Node.js qs v6.15#53
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPR adds DecodeOptions.strict_merge, redefines list_limit as a maximum element count, forces bracket-array keys to combine, converts over-limit comma-splits to CommaOverflowDict or raises, preserves OverflowDict subclasses on copy, and updates tests/docs accordingly. ChangesQuery String Decoding: Strict Merge, List Limits, and Bracket-Array Semantics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the decoder to better match Node.js qs v6.15+ behavior, mainly around merge semantics, duplicate handling for bracket-arrays, and list_limit enforcement/overflow behavior.
Changes:
- Add
DecodeOptions.strict_merge(defaultTrue) to wrap object/scalar merge conflicts in a list, with a legacy mode when disabled. - Adjust decoding semantics for bracket-array duplicates (always combine) and refine
list_limitbehavior + overflow handling (including comma-split overflow marking). - Update docs, comparison fixtures, and unit tests to cover the new parity behaviors.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/qs_codec/decode.py |
Implements new list-limit message helper, bracket-array duplicate behavior, comma-overflow handling, and updated list/index limit logic. |
src/qs_codec/utils/utils.py |
Updates merge semantics for strict merge conflicts and refines overflow combination behavior (including comma overflow nesting). |
src/qs_codec/models/decode_options.py |
Adds strict_merge option (default True). |
src/qs_codec/models/overflow_dict.py |
Preserves subclass type across copy/deepcopy and introduces CommaOverflowDict. |
tests/unit/decode_test.py |
Expands coverage for strict merge behavior, bracket-array duplicate combining, revised list-limit semantics, and comma overflow cases. |
tests/unit/utils_test.py |
Updates merge tests to reflect strict merge default behavior and legacy mode behavior. |
tests/unit/example_test.py |
Updates examples for bracket-array duplicates, strict merge behavior, and revised list-limit semantics wording. |
tests/unit/decode_options_test.py |
Asserts DecodeOptions.strict_merge default value. |
tests/comparison/test_cases.json |
Adds/adjusts parity fixtures for strict merge and list-limit semantics. |
docs/README.rst |
Documents strict merge, bracket-array duplicate semantics, and revised list-limit semantics. |
CHANGELOG.md |
Notes the new option and behavior fixes. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 1904 1939 +35
=========================================
+ Hits 1904 1939 +35 ☔ 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.
🧹 Nitpick comments (1)
tests/unit/utils_test.py (1)
604-618: ⚡ Quick winAssert copy-on-write in the new scalar-conflict merge tests.
Line 608 and Line 616 validate result shape, but they don’t verify that
targetstays unmodified (and unaliased) across strict/legacy paths. Add non-mutation assertions so these tests enforce the repo contract directly.Suggested test hardening
def test_merge_mapping_target_with_scalar_source_wraps_with_strict_merge(self) -> None: target = {"a": "b"} source = "scalar" result = Utils.merge(target, source) # type: ignore[arg-type] assert result == [{"a": "b"}, "scalar"] + assert target == {"a": "b"} + assert result[0] is not target def test_merge_mapping_target_with_scalar_source_uses_legacy_strict_merge_false(self) -> None: target = {"a": "b"} source = "scalar" result = Utils.merge(target, source, DecodeOptions(strict_merge=False)) # type: ignore[arg-type] assert result == {"a": "b", "scalar": True} + assert target == {"a": "b"} + assert result is not targetAs per coding guidelines:
**/*.py: Do not mutate caller inputs—copy/normalize before traversal.🤖 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 `@tests/unit/utils_test.py` around lines 604 - 618, Update the two tests test_merge_mapping_target_with_scalar_source_wraps_with_strict_merge and test_merge_mapping_target_with_scalar_source_uses_legacy_strict_merge_false to assert copy-on-write behavior: after calling Utils.merge(target, source, ...) verify the original target dict remains unchanged (e.g., equals {"a": "b"}) and is not aliased into the result (ensure result does not reference target object). Use the same symbols (target, result, Utils.merge, DecodeOptions) to locate where to add non-mutation assertions.
🤖 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 `@tests/unit/utils_test.py`:
- Around line 604-618: Update the two tests
test_merge_mapping_target_with_scalar_source_wraps_with_strict_merge and
test_merge_mapping_target_with_scalar_source_uses_legacy_strict_merge_false to
assert copy-on-write behavior: after calling Utils.merge(target, source, ...)
verify the original target dict remains unchanged (e.g., equals {"a": "b"}) and
is not aliased into the result (ensure result does not reference target object).
Use the same symbols (target, result, Utils.merge, DecodeOptions) to locate
where to add non-mutation assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f8603af-a717-4309-85eb-9ed498d78546
📒 Files selected for processing (11)
CHANGELOG.mddocs/README.rstsrc/qs_codec/decode.pysrc/qs_codec/models/decode_options.pysrc/qs_codec/models/overflow_dict.pysrc/qs_codec/utils/utils.pytests/comparison/test_cases.jsontests/unit/decode_options_test.pytests/unit/decode_test.pytests/unit/example_test.pytests/unit/utils_test.py
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 19 complexity · 0 duplication
Metric Results Complexity 19 Duplication 0
🟢 Coverage 100.00% diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (96140c2) 1904 1904 100.00% Head commit (ac97103) 1939 (+35) 1939 (+35) 100.00% (+0.00%) 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 (#53) 70 70 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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qs_codec/utils/utils.py (1)
539-549: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCopy
bbefore inserting it into the overflow result.This branch preserves
a, but it still stores the originalbobject inside the returnedOverflowDict. Any later in-place cleanup of the merged structure can mutate the caller's sequence/mapping through that alias.Suggested normalization
if not isinstance(b, Undefined): - a_copy[str(idx)] = b + if isinstance(b, list): + a_copy[str(idx)] = list(b) + elif isinstance(b, tuple): + a_copy[str(idx)] = list(b) + elif isinstance(b, ABCMapping): + a_copy[str(idx)] = dict(b) + else: + a_copy[str(idx)] = b return a_copyAs per coding guidelines "Do not mutate caller inputs—copy/normalize before traversal (shallow copy for mappings; deep-copy only when a callable filter may mutate; index-projection for sequences)".
🤖 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/qs_codec/utils/utils.py` around lines 539 - 549, The branch that appends b into the new OverflowDict (in the Utils.is_overflow branch) must insert a copy of b instead of the original to avoid aliasing caller data; before the line a_copy[str(idx)] = b, create a copied version (e.g., shallow-copy mappings/sequences or deepcopy when callers may mutate via callables) and assign a_copy[str(idx)] = b_copy; reference: Utils.is_overflow, OverflowDict, Undefined, _numeric_key_pairs, variables a, b, a_copy.
🤖 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/qs_codec/decode.py`:
- Around line 242-245: The check enforcing options.list_limit inside
_parse_array_value is being applied to plain key=value assignments and causes
DecodeOptions(list_limit=-1, raise_on_limit_exceeded=True) to raise for non-list
tokens; change the guard so the current_list_length >= options.list_limit check
only runs when we are actually building a list (i.e., when the call path
indicates comma/bracket array parsing or when a list container is present), not
for simple scalar assignments in _parse_array_value; update the condition that
references current_list_length and options.list_limit (and any early-return
paths) to first verify we are in list-building mode (for example by checking a
list context flag or the presence of an existing list value) before enforcing
the limit so decode("a=b", ...) no longer triggers negative-limit logic.
---
Outside diff comments:
In `@src/qs_codec/utils/utils.py`:
- Around line 539-549: The branch that appends b into the new OverflowDict (in
the Utils.is_overflow branch) must insert a copy of b instead of the original to
avoid aliasing caller data; before the line a_copy[str(idx)] = b, create a
copied version (e.g., shallow-copy mappings/sequences or deepcopy when callers
may mutate via callables) and assign a_copy[str(idx)] = b_copy; reference:
Utils.is_overflow, OverflowDict, Undefined, _numeric_key_pairs, variables a, b,
a_copy.
🪄 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: 7a9241a2-6c3b-41dd-8ccd-0e644fe5712b
📒 Files selected for processing (5)
src/qs_codec/decode.pysrc/qs_codec/models/decode_options.pysrc/qs_codec/utils/utils.pytests/unit/decode_test.pytests/unit/utils_test.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
techouse
changed the title
strict_merge option and align list-limit semantics with Node.js qs v6.15
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qs_codec/decode.py (1)
401-407:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOverflow detection lost after bracket-array wrapping.
Line 388 correctly detects overflow on the inner comma-split list, but line 403 overwrites
list_limit_exceededafter wrapping. Sinceval = [val]produces a single-element outer list,len(val) > options.list_limitbecomes1 > list_limit, which is False for any positive limit.Example:
a[]=1,2,3withlist_limit=2produceslist_limit_exceeded=Trueat line 388 (3 > 2), but line 403 resets it to False (1 > 2), so the 3-element inner list passes through without overflow handling.🐛 Proposed fix: preserve original overflow detection
if bracket_array_assignment and isinstance(val, (list, tuple)): val = [val] - list_limit_exceeded = len(val) > options.list_limit + # Preserve list_limit_exceeded from line 388; the inner list length was already checked if list_limit_exceeded and isinstance(val, (list, tuple)):🤖 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/qs_codec/decode.py` around lines 401 - 407, The overflow flag gets overwritten when wrapping a comma-split list into an outer list for bracket_array_assignment: compute or preserve the inner-list overflow before doing val = [val] so the original detection isn't lost. Concretely, in the block that handles bracket_array_assignment and sets val = [val] (referencing bracket_array_assignment, val, and options.list_limit), capture the inner length or preserve list_limit_exceeded (e.g., compute inner_len = len(val) and set list_limit_exceeded = list_limit_exceeded or inner_len > options.list_limit) before wrapping, then proceed to the existing raise_on_limit_exceeded/CommaOverflowDict logic (options.raise_on_limit_exceeded, CommaOverflowDict) so overflow handling still triggers for the original inner list.
🤖 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/qs_codec/decode.py`:
- Around line 503-508: The overflow check is done after wrapping the existing
list into a single-element list so len(leaf) is always 1; adjust the logic in
the bracket-array/comma handling (where bracket_array_comma_value is used and
leaf is manipulated) to first, if isinstance(leaf, (list, tuple)), validate
len(leaf) against options.list_limit and either raise
ValueError(_list_limit_exceeded_message(options.list_limit)) when
options.raise_on_limit_exceeded is true or convert to CommaOverflowDict({str(i):
item for i, item in enumerate(leaf)}) on overflow, and only if no overflow wrap
leaf = [leaf] for the bracket-array comma case.
---
Outside diff comments:
In `@src/qs_codec/decode.py`:
- Around line 401-407: The overflow flag gets overwritten when wrapping a
comma-split list into an outer list for bracket_array_assignment: compute or
preserve the inner-list overflow before doing val = [val] so the original
detection isn't lost. Concretely, in the block that handles
bracket_array_assignment and sets val = [val] (referencing
bracket_array_assignment, val, and options.list_limit), capture the inner length
or preserve list_limit_exceeded (e.g., compute inner_len = len(val) and set
list_limit_exceeded = list_limit_exceeded or inner_len > options.list_limit)
before wrapping, then proceed to the existing
raise_on_limit_exceeded/CommaOverflowDict logic
(options.raise_on_limit_exceeded, CommaOverflowDict) so overflow handling still
triggers for the original inner list.
🪄 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: 03990e3e-e27c-4ff5-840d-2edc15d32a03
📒 Files selected for processing (5)
src/qs_codec/decode.pysrc/qs_codec/utils/utils.pytests/unit/decode_test.pytests/unit/thread_safety_test.pytox.ini
🚧 Files skipped from review as they are similar to previous changes (2)
- src/qs_codec/utils/utils.py
- tests/unit/decode_test.py
techouse
force-pushed
the
feat/upstream-fixes-v2
branch
from
1a3b029 to
ac97103
Compare
This pull request introduces several improvements and bug fixes to the query string decoder, aligning its behavior more closely with Node.js
qsv6.15, especially around list and object merging, duplicate handling, and array limits. The most significant changes are the addition of a newstrict_mergeoption, improved handling of list limits, and enhanced duplicate key semantics. Documentation and tests have also been updated to reflect these changes.Decoder behavior and options:
DecodeOptions.strict_mergeoption (defaultTrue) to control how object/scalar conflicts are merged: when enabled, conflicting values are wrapped in a list (Node.jsqs6.15+ parity); when disabled, legacy behavior is restored, adding non-empty string scalars as object keys. [1] [2] [3] [4]list_limitoption to act as a maximum element count (matching Node.jsqsarrayLimit), not a maximum index. Now, only indices below the limit are included in lists; higher indices are converted to dicts. [1] F167923eL310R342, [2] [3] [4]CommaOverflowDictto mark over-limit comma-split values. [1] [2] [3] [4]Documentation and tests:
README.rstto describe the newstrict_mergeoption, bracket-array duplicate behavior, and revised list limit semantics. [1] [2]Changelog:
CHANGELOG.mdto document the new features and fixes.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests