✨ qs.js 6.15.3 parity#58
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughList-limit handling is updated across decoding, merging, and combining. Decode now honors ChangesList limit enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|---|---|
| Complexity | -4 |
| 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 (f843c85) 1931 1931 100.00% Head commit (088d918) 1929 (-2) 1929 (-2) 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 (#58) 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 #58 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 1931 1929 -2
=========================================
- Hits 1931 1929 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates qs_codec’s decoder/merge internals to more closely match Node qs 6.15.3 around cumulative list-limit enforcement (including earlier rejection of oversized comma payloads), and adds regression tests for bracket-parsing edge cases and cycle-safe compaction.
Changes:
- Introduces a shared list-limit enforcement helper and applies it across
Utils.merge/Utils.combine, including raising behavior whenraise_on_limit_exceeded=True. - Adjusts decode parsing behavior for comma limits (especially around bracket-array assignments) and removes a local
parse_listsoverride to keep caller options consistent. - Expands regression coverage for list-limit overflow combinations, unbalanced bracket keys, and multi-step cycles during compaction; documents the change in the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/qs_codec/utils/utils.py |
Adds _enforce_list_limit and refactors merge/combine logic to enforce list limits consistently. |
src/qs_codec/decode.py |
Removes local parse_lists override and tweaks comma-limit enforcement behavior. |
tests/unit/utils_test.py |
Adds focused tests for list-limit growth behavior across merge/combine and cyclic compaction. |
tests/unit/decode_test.py |
Adds regression tests for unbalanced bracket keys and list-limit overflow scenarios. |
CHANGELOG.md |
Documents the parity fixes and new regression coverage. |
Comments suppressed due to low confidence (1)
src/qs_codec/utils/utils.py:200
- When merging a scalar into an existing list/tuple target,
Undefinedsources should be ignored (not appended). Currently the scalar path appendsUndefined, which can leak the sentinel into results and can also trigger list-limit enforcement erroneously.
candidate = [*current_target, current_source]
enforced = _enforce_list_limit(candidate, frame.options)
if isinstance(enforced, OverflowDict):
stack.pop()
last_result = enforced
continue
mutable_target = list(current_target) if isinstance(current_target, tuple) else current_target
mutable_target.append(current_source)
![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
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)
63-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring: the "disable list parsing on token count" note no longer reflects the code.
The invocation-local
parse_lists_effectiveswitch was removed and_parse_keysis now always called withparse_lists=opts.parse_lists. The Notes bullet still claims the parser "temporarily disables list parsing for this invocation" when top-level tokens exceedlist_limit, which is now inaccurate and misleading for callers.As per coding guidelines ("Keep docstrings descriptive and parity-focused").
📝 Proposed docstring fix
Notes ----- - Empty/falsey ``value`` returns an empty dict. - - When the *number of top-level tokens* exceeds ``list_limit`` and ``parse_lists`` is enabled, the parser temporarily **disables list parsing** for this invocation to avoid quadratic work. This mirrors the behavior of other ports and keeps large flat query strings efficient. + - List parsing follows ``options.parse_lists`` for the whole invocation; ``list_limit`` is + enforced incrementally during comma-splitting and merge/combine rather than by disabling + list parsing based on token count.🤖 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 63 - 66, The Notes docstring in decode.py is stale and still describes the removed invocation-local list-parsing disable behavior. Update the Notes text near the decode/parse logic to match the current flow where _parse_keys is always called with opts.parse_lists, and remove or rewrite the bullet about temporarily disabling list parsing when top-level token count exceeds list_limit so it reflects the actual behavior.Source: Coding guidelines
🧹 Nitpick comments (2)
src/qs_codec/utils/utils.py (1)
350-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsrc/qs_codec/utils/utils.py:355 — Avoid mutating the caller list here
frame.target[:] = enforcedrewrites the input list in place; the merged list is already available asenforced, so return that instead.🤖 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 350 - 359, The list iteration completion path in the stack unwinding logic is mutating the caller’s list in place via frame.target[:] = enforced; update the list handling in the phase == "list_iter" branch to avoid rewriting the original input and instead use the already computed merged result from _enforce_list_limit, assigning last_result to that value directly. Keep the change localized around the list_iter/frame.target handling in utils.py.Source: Coding guidelines
tests/unit/utils_test.py (1)
611-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting no caller-input mutation, like the sibling test does.
test_merge_raises_when_list_growth_exceeds_limit(Lines 582-589) explicitly assertstarget == original_targetafter a raise. This test reusessourceacross twoUtils.merge()calls (one non-raising, one raising) without any equivalent check thattarget/sourceremain unmutated, so a caller-input mutation regression here would go undetected.♻️ Suggested addition
def test_merge_enforces_limit_after_nested_list_merge(self) -> None: target = [{"a": 1}] source = [{"b": 2}, {"c": 3}] + original_source = [dict(item) for item in source] result = Utils.merge(target, source, DecodeOptions(list_limit=1)) assert result == {"0": {"a": 1, "b": 2}, "1": {"c": 3}} assert isinstance(result, OverflowDict) + assert source == original_source with pytest.raises(ValueError, match="List limit exceeded"): Utils.merge([{"a": 1}], source, DecodeOptions(list_limit=1, raise_on_limit_exceeded=True)) + + assert source == original_sourceAs per coding guidelines,
**/*.py: "Do not mutate caller inputs—copy/normalize before traversal", the test suite should verify this contract wherever list-limit merges are exercised, consistent with the pattern already used in the adjacent test.🤖 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 611 - 621, Add an assertion in test_merge_enforces_limit_after_nested_list_merge that the caller inputs are unchanged after Utils.merge runs, mirroring the adjacent test’s mutation check. Use the existing target/source variables around Utils.merge and verify they still match their original contents after both the non-raising and raise_on_limit_exceeded calls, so any future caller-input mutation in Utils.merge/merge_enforces_limit_after_nested_list_merge is caught.Source: Coding guidelines
🤖 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 `@tests/unit/decode_test.py`:
- Around line 2023-2028: The `decode_test.py` expectation for the `pytest.param`
covering `0=y&[]=x` is incorrect; update the asserted result to match `qs`
6.15.3 behavior where `decode` keeps the flat key and empty-bracket root
separate. Locate the failing case by the
`flat-zero-combines-leading-bracket-root` test id and change the expected value
from a merged list under `0` to separate entries for `0` and the empty key.
---
Outside diff comments:
In `@src/qs_codec/decode.py`:
- Around line 63-66: The Notes docstring in decode.py is stale and still
describes the removed invocation-local list-parsing disable behavior. Update the
Notes text near the decode/parse logic to match the current flow where
_parse_keys is always called with opts.parse_lists, and remove or rewrite the
bullet about temporarily disabling list parsing when top-level token count
exceeds list_limit so it reflects the actual behavior.
---
Nitpick comments:
In `@src/qs_codec/utils/utils.py`:
- Around line 350-359: The list iteration completion path in the stack unwinding
logic is mutating the caller’s list in place via frame.target[:] = enforced;
update the list handling in the phase == "list_iter" branch to avoid rewriting
the original input and instead use the already computed merged result from
_enforce_list_limit, assigning last_result to that value directly. Keep the
change localized around the list_iter/frame.target handling in utils.py.
In `@tests/unit/utils_test.py`:
- Around line 611-621: Add an assertion in
test_merge_enforces_limit_after_nested_list_merge that the caller inputs are
unchanged after Utils.merge runs, mirroring the adjacent test’s mutation check.
Use the existing target/source variables around Utils.merge and verify they
still match their original contents after both the non-raising and
raise_on_limit_exceeded calls, so any future caller-input mutation in
Utils.merge/merge_enforces_limit_after_nested_list_merge is caught.
🪄 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: f5ca638a-f3ab-4ff6-8f85-af14f8b2469b
📒 Files selected for processing (5)
CHANGELOG.mdsrc/qs_codec/decode.pysrc/qs_codec/utils/utils.pytests/unit/decode_test.pytests/unit/utils_test.py
There was a problem hiding this comment.
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)
355-358: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid writing the merged list back into
target
frame.target[:] = enforcedmutates the caller’s list on successful list merges. Return the merged copy directly instead soUtils.mergestays side-effect free.🤖 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 355 - 358, The list-merge path in Utils.merge is mutating the caller’s list via frame.target, which breaks side-effect-free behavior. Update the branch around _enforce_list_limit so it does not assign back into frame.target after a successful merge. Instead, keep the merged list in the local result flow by returning the enforced list directly and using last_result to hold that merged copy, while preserving the existing behavior for frame.list_merged and list handling.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/qs_codec/utils/utils.py`:
- Around line 355-358: The list-merge path in Utils.merge is mutating the
caller’s list via frame.target, which breaks side-effect-free behavior. Update
the branch around _enforce_list_limit so it does not assign back into
frame.target after a successful merge. Instead, keep the merged list in the
local result flow by returning the enforced list directly and using last_result
to hold that merged copy, while preserving the existing behavior for
frame.list_merged and list handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df772d45-42e7-408c-bd3b-b38c4c234726
📒 Files selected for processing (2)
src/qs_codec/decode.pysrc/qs_codec/utils/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/qs_codec/decode.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/qs_codec/utils/utils.py:202
- When merging a scalar into a list/tuple target,
mergeappendscurrent_sourceunconditionally. Ifcurrent_sourceis the internalUndefinedsentinel, this will insert a sparse-hole placeholder as an actual element and also consumelist_limitcapacity, which is inconsistent with othermergebranches that treatUndefinedas “skip / no-op”.
Consider short-circuiting when current_source is Undefined so list targets are left unchanged and the limit check does not count the skipped sentinel.
mutable_target = list(current_target) if isinstance(current_target, tuple) else current_target
mutable_target.append(current_source)
Enforce cumulative limits across duplicate and mixed list merges. Reject oversized flat comma values before splitting or decoding, and add upstream parity regressions.
Describe cumulative enforcement, numeric-key overflow, ValueError semantics, and bracketed comma groups across the changelog, public docs, option docstrings, and qs-codec skill.
This pull request implements stricter and more consistent enforcement of list limits during query string decoding and merging, aligning behavior with Node
qs6.15.3. It also improves error handling for oversized list values and updates internal logic for list merging and compaction. Additionally, regression tests and documentation updates are included.List limit enforcement and error handling:
qs6.15.3 cumulative enforcement. Oversized lists raiseValueErrorifraise_on_limit_exceededis enabled, otherwise they are converted toOverflowDict. (src/qs_codec/utils/utils.py,src/qs_codec/decode.py) [1] [2] [3] [4] [5] [6] [7]raise_on_limit_exceededis set, preventing unnecessary processing. (src/qs_codec/decode.py)Decoding and merging logic updates:
parse_listsis always honored, andlist_limitis enforced during list construction and merging. (src/qs_codec/decode.py) [1] [2] [3]src/qs_codec/utils/utils.py) [1] [2] [3] [4] [5] [6] [7] [8]Testing and documentation:
qs6.15.3. Documentation and changelog have been updated to reflect these changes. (CHANGELOG.md)Summary by CodeRabbit
list_limitconsistently across duplicate keys and mixed list merges, including cumulative growth scenarios.