✨ upsteam parity#53
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 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 (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
===========================================
- Coverage 100.00% 99.68% -0.32%
===========================================
Files 22 22
Lines 1904 1932 +28
===========================================
+ Hits 1904 1926 +22
- Misses 0 6 +6 ☔ 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 8 complexity · 0 duplication
Metric Results Complexity 8 Duplication 0
🟢 Coverage 93.55% diff coverage · -0.31% coverage variation
Metric Results Coverage variation ✅ -0.31% coverage variation (-1.00%) Diff coverage ✅ 93.55% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (96140c2) 1904 1904 100.00% Head commit (47ca334) 1932 (+28) 1926 (+22) 99.69% (-0.31%) 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) 62 58 93.55% 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
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