Conversation
WalkthroughKey-splitting and decode logic updated: depth==0 returns the original key; top-level dot→bracket normalisation runs only for depth>0 with refined degenerate-dot and ident-start rules; parse logic adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant QS as QS.decode
participant SK as _splitKeyIntoSegments
participant DT as _dotToBracketTopLevel
participant PO as _parseObject
Client->>QS: decode(queryString, options)
QS->>SK: split key (originalKey, maxDepth, allowDots, decodeDotInKeys)
alt maxDepth <= 0
SK-->>QS: return [originalKey] (no dot/bracket normalisation)
else maxDepth > 0
SK->>DT: apply top-level dot→bracket normalisation
note right of DT #eef8f0: Handle leading dot as bracketed segment\nSkip degenerate ".[" dot\nCheck ident-start after dot
DT-->>SK: normalized segments
SK-->>QS: segments
end
QS->>PO: parse segments (compute wasBracketed)
note right of PO #eef3ff: If wasBracketed && options.parseLists\nand decodedRoot == index → treat as list index\nElse normalise numeric-looking keys back to strings\nList-limit checks emit dynamic messages
PO-->>QS: constructed object/list
QS-->>Client: decoded object
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
- Coverage 94.87% 94.52% -0.35%
==========================================
Files 14 14
Lines 878 895 +17
==========================================
+ Hits 833 846 +13
- Misses 45 49 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/src/extensions/decode.dart (1)
276-301: Use an explicit ‘wasBracketed’ flag instead of ‘root != decodedRoot’Minor readability nit: the list-index branch relies on “root != decodedRoot” as a proxy for “token was bracketed”. This is slightly indirect (and could be confused with future transformations). A small refactor makes intent explicit without changing behaviour.
- final String cleanRoot = root.startsWith('[') && root.endsWith(']') - ? root.slice(1, root.length - 1) - : root; + final bool wasBracketed = root.startsWith('[') && root.endsWith(']'); + final String cleanRoot = + wasBracketed ? root.slice(1, root.length - 1) : root; final String decodedRoot = options.decodeDotInKeys ? cleanRoot.replaceAll('%2E', '.').replaceAll('%2e', '.') : cleanRoot; final int? index = int.tryParse(decodedRoot); if (!options.parseLists && decodedRoot == '') { obj = <String, dynamic>{'0': leaf}; - } else if (index != null && - index >= 0 && - root != decodedRoot && + } else if (index != null && + index >= 0 && + wasBracketed && index.toString() == decodedRoot && options.parseLists && index <= options.listLimit) {test/unit/decode_test.dart (2)
2486-2494: Rename test: the title contradicts the new expectationThe body now expects ".a=x" to decode to {'a': 'x'} (leading dot ignored/splits), but the title says “preserved”. Rename for clarity.
- test('leading dot preserved when allowDots=true', () { + test('leading dot splits to a new segment when allowDots=true', () {
2566-2734: Great coverage for encoded-dot semantics; add two micro-cases and consider dedupingThe new group thoroughly exercises:
- top-level encoded vs plain dots across option combinations,
- bracket-segment decoding with/without decodeDotInKeys,
- guardrails (leading/trailing/double dots),
- depth=0 bypass.
Two small gaps to cement behaviour:
- Start-of-key bracket degenerate: add ".[a]=x" with allowDots=true to ensure the “.[” branch is covered at position 0.
- Depth=0 + encoded dot: add “a%2Eb=c” with allowDots=true, depth=0 to assert no splitting despite a decoded '.'.
Also, there’s overlap with the earlier “C# parity” block that asserts similar cases (e.g., bracket %2E mapping and top-level encoded dots). You might fold the duplicate assertions into one place to reduce maintenance load.
Proposed additions:
@@ group('encoded dot behavior in keys (%2E / %2e)', () { + test('leading dot before bracket: skip the dot (.[a]=x)', () { + const opt = DecodeOptions(allowDots: true, decodeDotInKeys: true); + expect(QS.decode('.[a]=x', opt), equals({'a': 'x'})); + }); + + test('depth=0 with encoded dot: do not split key', () { + expect( + QS.decode('a%2Eb=c', const DecodeOptions(allowDots: true, depth: 0)), + equals({'a.b': 'c'}), + ); + });If you’d like, I can consolidate the overlapping tests and open a follow-up PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/src/extensions/decode.dart(3 hunks)test/unit/decode_test.dart(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (5)
lib/src/extensions/decode.dart (5)
346-349: Depth=0: early return with original key is correctReturning the unmodified originalKey when maxDepth <= 0 faithfully matches the intended “no splitting, no normalisation” semantics and unblocks the new test expectations.
351-354: Deferring dot normalisation until after the depth guard is soundNormalising top-level dots only when depth > 0 prevents accidental splitting at depth 0 and aligns with qs. No issues spotted.
429-433: Documentation for degenerate top-level dots is clear and matches behaviourThe notes for “.[”, leading/trailing dots, and double dots accurately reflect the implementation that follows. This will save future readers time.
456-459: Edge case handled: skip dot before a bracket (".[")The “hasNext && next == '['” branch ensures inputs like "a.[b]" behave like "a[b]". Consider adding a dedicated unit test for the start-of-key case ".[a]=x" to lock this in.
Would you like me to add such a test to the encoded-dot groups?
464-474: Top-level “.segment” → “[segment]” conversion looks correctThe scanner collects the next token until '.' or '[' and emits a single bracketed segment. This makes “.a” and “a.b” both reduce to “[a]” and “[b]” respectively at depth 0. Behaviour matches the updated tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/src/extensions/decode.dart (2)
276-301: Bracket-only numeric index semantics: correct and saferGating list-index handling on a bracketed token via wasBracketed eliminates accidental list creation for bare numeric keys and aligns with qs semantics. The added decodedRoot matching and listLimit guard are sound.
Two minor follow‑ups:
- Consider parsing index lazily only when wasBracketed && options.parseLists to avoid unnecessary int.tryParse on every segment.
- Add a quick test asserting that bare numeric keys like "0=foo" stay as {'0': 'foo'} while "a[0]=b" still forms a list when parseLists is true (there are similar tests, but this explicit pair would lock in the new invariant).
Optional micro-refactor to avoid unnecessary int parsing:
- final int? index = int.tryParse(decodedRoot); + final int? index = (wasBracketed && options.parseLists) + ? int.tryParse(decodedRoot) + : null; @@ - } else if (index != null && + } else if (index != null && index >= 0 && wasBracketed && index.toString() == decodedRoot && options.parseLists && index <= options.listLimit) { @@ - } else { - obj[index?.toString() ?? decodedRoot] = leaf; - } + } else { + // Normalise numeric-looking keys back to their canonical string form when not a list index + obj[index?.toString() ?? decodedRoot] = leaf; + }
456-475: Top-level dot normaliser: degenerate cases handled well; add hardening around Unicode and digitsThe ".[", leading ".", trailing/double dots are covered and produce intuitive results. Consider two small robustness tweaks:
- Treat letters, digits, and underscore as valid starts for a bracketed segment on a “normal split” to avoid creating empty segments if the next token is punctuation.
- Use code units (int) comparisons consistently for tiny speed wins in hot paths.
Here is a focused tweak to guard the “normal split” branch:
- } else { + } else { // Normal split: convert top-level ".a" or "a.b" into a bracket segment. final int start = ++i; int j = start; - while (j < s.length && s[j] != '.' && s[j] != '[') { + // Accept [A-Za-z0-9_] at the start of a segment; otherwise, keep '.' literal. + bool isIdentStart(int cu) => + (cu >= 0x41 && cu <= 0x5A) || // A-Z + (cu >= 0x61 && cu <= 0x7A) || // a-z + (cu >= 0x30 && cu <= 0x39) || // 0-9 + cu == 0x5F; // _ + if (start >= s.length || !isIdentStart(s.codeUnitAt(start))) { + sb.write('.'); // keep as literal if next char isn't an ident start + continue; + } + while (j < s.length && s[j] != '.' && s[j] != '[') { j++; }test/unit/decode_test.dart (1)
2567-2746: Excellent coverage of encoded-dot edge cases; consider deduplication and a couple of extra assertionsThe new group thoroughly exercises:
- depth=0 bypass,
- allowDots/decodeDotInKeys combinations,
- leading/trailing/double dots,
- bracketed segments and case-insensitive %2E,
- bare-key paths.
Suggestions:
- Some scenarios duplicate earlier “C# parity” tests (e.g., a[%2E]=x) — dedup to trim runtime and maintenance.
- Add two quick tests to lock in guardrails:
- A sole key of "." remains literal with allowDots=true ('.=x' => {'.': 'x'}).
- "a%2E[b]=x" splits top-level then brackets when allowDots=true (already covered for lowercase; assert upper-case too if not yet).
If you want, I can propose a minimal set by extracting shared cases into a helper and reusing it across both groups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/src/extensions/decode.dart(5 hunks)test/unit/decode_test.dart(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (2)
lib/src/extensions/decode.dart (1)
347-354: Depth==0 bypass now returns the original key without dot normalisationDeferring dot-to-bracket handling until after the maxDepth <= 0 check precisely matches qs’s “no splitting at depth 0” behaviour. Good change; it keeps keys intact for shallow parsing.
Please confirm DecodeOptions docs mention that allowDots has no effect at depth == 0, to prevent surprising consumers.
test/unit/decode_test.dart (1)
2486-2494: Expectation change for leading dot is correctUpdating the test from preserving ".a" to normalising it to "a" under allowDots=true matches the new top-level normaliser logic. Nice coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/unit/decode_test.dart (1)
2567-2746: Excellent coverage of encoded-dot semantics; add a few edge casesThe new group validates most combinations (top-level/bracketed, upper/lower, depth=0, bare keys). Consider adding:
- Unicode/ASCII-extended segment starts after a dot (e.g., 'µ.b=x', 'a.-b=x') to ensure the ident-start check in _dotToBracketTopLevel doesn’t block legitimate splits.
- Triple dots and mixed forms: 'a...b=x', '.%2Eb=x', '%2E%2Ea=x'.
- Encoded dot immediately before percent-encoded bracket: 'a%2E%5Bb%5D=x' (case variants).
lib/src/extensions/decode.dart (4)
283-297: Synthetic remainder normalisation: avoid per-call RegExp scansThe opens/closes count with RegExp inside the hot path can be micro-optimised. A single pass is cheaper and clearer.
Apply:
- final int opens = RegExp(r'\[').allMatches(decodedRoot).length; - final int closes = RegExp(r'\]').allMatches(decodedRoot).length; + int opens = 0, closes = 0; + for (int k = 0; k < decodedRoot.length; k++) { + final cu = decodedRoot.codeUnitAt(k); + if (cu == 0x5B) opens++; + if (cu == 0x5D) closes++; + }
479-503: Top-level dot splitting ident-start check is too restrictiveSegment starts are limited to [A-Za-z0-9_]; this prevents splitting on common identifiers like '-k' or Unicode letters. If intentional, document it; otherwise, at least allow hyphen.
Apply:
- bool isIdentStart(int cu) => switch (cu) { + bool isIdentStart(int cu) => switch (cu) { >= 0x41 && <= 0x5A => true, // A-Z >= 0x61 && <= 0x7A => true, // a-z >= 0x30 && <= 0x39 => true, // 0-9 - 0x5F => true, // _ + 0x5F => true, // _ + 0x2D => true, // - _ => false, };Also consider a follow-up to permit general Unicode letters using Character.isAlphabetic if you adopt a broader policy.
279-282: Normalising %2E inside bracket segments: make it unconditionalTests assert that inside [ ... ] a percent-encoded dot becomes '.' regardless of decodeDotInKeys. Relying on the upstream decoder to ensure this is brittle.
Apply:
- String decodedRoot = options.decodeDotInKeys - ? cleanRoot.replaceAll('%2E', '.').replaceAll('%2e', '.') - : cleanRoot; + // Inside bracket segments, percent-encoded dots should be literal '.' + // regardless of `decodeDotInKeys`, since bracket groups never split on dots. + String decodedRoot = cleanRoot.replaceAll('%2E', '.').replaceAll('%2e', '.');Please confirm DecodeOptions.decodeKey doesn’t already guarantee this in all cases; if it does, this change simply hardens the behaviour.
52-76: List-limit messaging for negative limits could be clearerThe logic correctly enforces “growth capped at zero” or throws when listLimit<0, but the error text “Only -1 elements allowed” is confusing.
Apply:
- throw RangeError( - 'List limit exceeded. ' - 'Only ${options.listLimit} element${options.listLimit == 1 ? '' : 's'} allowed in a list.', - ); + final msg = options.listLimit < 0 + ? 'List parsing is disabled (listLimit < 0).' + : 'List limit exceeded. Only ${options.listLimit} ' + 'element${options.listLimit == 1 ? '' : 's'} allowed in a list.'; + throw RangeError(msg);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/src/extensions/decode.dart(6 hunks)test/unit/decode_test.dart(2 hunks)
🔇 Additional comments (8)
test/unit/decode_test.dart (3)
2486-2493: Rename + expectation change for leading dot is correctChanging the test to expect ".a=x" → {'a': 'x'} under allowDots=true matches the new degenerate leading-dot normalisation. Good safeguard.
2573-2578: Depth=0 “no split” assertion is on pointConfirming that encoded dots don’t split at depth=0 prevents regressions when normalisation must be fully disabled.
2713-2746: Guardrails for leading/trailing/double dots look solidThese expectations align with the documented degenerate-handling: leading dot ignored, trailing dot literal (no empty segment), only the second of a double-dot splits. Nice.
lib/src/extensions/decode.dart (5)
366-374: Depth==0: early return preserves original key (no normalisation)Returning the originalKey untouched when maxDepth<=0 correctly avoids dot→bracket normalisation and matches qs parity. Good.
276-321: Bracket-aware numeric index gating prevents false list indicesUsing wasBracketed to only treat “[n]” as indices (and never bare numeric) fixes long-standing ambiguity with numeric-looking keys and interplays well with listLimit/parseLists. LGTM.
407-413: Unterminated group handling matches intended semanticsWrapping the raw remainder as a single synthetic bracket segment and returning early (even under strictDepth) aligns with tests like 'a[b[c]=x' and avoids hard-failures. Nice.
424-446: Depth overflow vs trailing remainder: re-check throw conditionYou only throw when open>=0 (i.e., more groups remain) but never for trailing non-group remainder. That matches qs, but please add an explicit test for something like 'a[b]cd=e' with strictDepth=true and depth=1 (should not throw; 'cd' must become a single bracket segment).
I can craft a test if helpful.
96-105: Parameter limit: explicit positive check is welcomeThrowing on non‑positive finite limits removes ambiguity and matches the tests around parameterLimit=0. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lib/src/extensions/decode.dart (5)
54-58: De-duplicate list-limit error message construction; keep semantics in one placeNice improvement to the error text, but the identical message-building logic is duplicated. Let’s centralise it to avoid drift and keep copy consistent if we tweak wording again.
Apply this diff within these ranges:
- final String msg = options.listLimit < 0 - ? 'List parsing is disabled (listLimit < 0).' - : 'List limit exceeded. Only ${options.listLimit} ' - 'element${options.listLimit == 1 ? '' : 's'} allowed in a list.'; - throw RangeError(msg); + throw RangeError(_listLimitErrorMessage(options));- final String msg = options.listLimit < 0 - ? 'List parsing is disabled (listLimit < 0).' - : 'List limit exceeded. Only ${options.listLimit} ' - 'element${options.listLimit == 1 ? '' : 's'} allowed in a list.'; - throw RangeError(msg); + throw RangeError(_listLimitErrorMessage(options));Add this helper (outside the selected ranges, e.g., near other private statics in this extension):
static String _listLimitErrorMessage(DecodeOptions options) { final int n = options.listLimit; return n < 0 ? 'List parsing is disabled (listLimit < 0).' : 'List limit exceeded. Only $n element${n == 1 ? '' : 's'} allowed in a list.'; }Also applies to: 70-75
325-326: Avoid coercing non-list numeric-looking keys to canonical numericsWhen not treated as a list index,
"[01]"becomes key"1"due toindex?.toString() ?? decodedRoot. That’s a behavioural change unrelated to the dot-handling objective and likely diverges fromqsparity, where non-index bracketed numerics are just strings.Proposed change:
- // Normalise numeric-looking keys back to their canonical string form when not a list index - obj[index?.toString() ?? decodedRoot] = leaf; + // Preserve original key when not a list index + obj[decodedRoot] = leaf;Request: please confirm existing tests (or add one) for keys like
a[01]=x,a[000]=xto ensure we don’t accidentally normalise.
312-312: RedundantwasBracketedcheck
indexis only computed whenwasBracketed && options.parseLists; repeatingwasBracketedin theelse ifcondition is unnecessary.- } else if (index != null && - index >= 0 && - wasBracketed && + } else if (index != null && + index >= 0 && index.toString() == decodedRoot && options.parseLists && index < options.listLimit) {
458-461: Doc nit: reflect the actual ident-start setThe code allows
A–Z,a–z, digits, underscore, and hyphen after a dot; the comment currently implies onlyA–Z,a–z, digits, and underscore. Let’s mention the hyphen for clarity.Would you like me to propose a wording update to these comment lines?
493-507: Switch expression forisIdentStartlikely requires Dart 3; consider compatibility
switchexpressions with relational patterns are Dart 3+ only. If the package still supports older SDKs, this will break consumers. A simple boolean check reads fine here and avoids bumping the lower SDK.Proposed replacement (SDK-agnostic), preserving the same character set:
- // Accept [A-Za-z0-9_] at the start of a segment; otherwise, keep '.' literal. - bool isIdentStart(int cu) => switch (cu) { - (>= 0x41 && <= 0x5A) || // A-Z - (>= 0x61 && <= 0x7A) || // a-z - (>= 0x30 && <= 0x39) || // 0-9 - 0x5F || // _ - 0x2D => // - - true, - _ => false, - }; + // Accept [A-Za-z0-9_-] at the start of a segment; otherwise, keep '.' literal. + bool isIdentStart(int cu) { + final bool isAZ = cu >= 0x41 && cu <= 0x5A; + final bool isaz = cu >= 0x61 && cu <= 0x7A; + final bool is09 = cu >= 0x30 && cu <= 0x39; + return isAZ || isaz || is09 || cu == 0x5F /* _ */ || cu == 0x2D /* - */; + }Also, if the intent is to mimic
qsmore closely, consider whether$should be accepted as a start character (and add tests either way).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/src/extensions/decode.dart(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
lib/src/extensions/decode.dart (6)
278-284: Bracketed root handling and%2E→.underdecodeDotInKeyslook soundThe
wasBracketed/cleanRoot/decodedRootsequence is clear and correctly scopes%2E/%2edecoding to bracketed segments only when opted in. Nice.
285-304: Synthetic remainder normalisation is cautious and matches intentThe guard
root.startsWith('[[')plus bracket counting ensures we only trim a trailing unmatched]that originated from an unterminated group. This should keep literal]in legit keys intact.Please ensure tests cover:
a[b[c]=x,a[[b]c]=x, and their%2Evariants under bothstrictDepth=true/false.
372-374: Depth==0 returns the original key unchanged (no dot normalisation)This aligns with
qsdepth semantics and the PR goal to preserve keys atdepth == 0. Good.
377-380: Run dot→bracket normalisation only when depth > 0The reordering avoids accidental normalisation at depth 0. Correct and matches the updated docs/tests.
413-419: Unterminated bracket group handling is consistent with “treat as a literal”Wrapping the raw remainder into one synthetic bracket segment and returning early avoids throwing even under
strictDepth. Matches the documented behaviour.Please ensure there’s a test asserting no throw with
strictDepth=truefor an unterminated group (e.g.,a[b=c).
485-488: Degenerate `".[": skip-dot behaviour is sensibleTreating
a.[b]likea[b]removes a surprising empty segment. This is a good UX improvement and aligns with the PR’s degenerate-dot goals.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This pull request refines the query string decoding logic to handle dot (
.) and percent-encoded dot (%2E/%2e) behavior in keys more accurately and predictably. The changes clarify how dots are interpreted at different parsing depths, improve handling of edge cases (like leading or trailing dots), and introduce comprehensive tests for various dot-related scenarios.Improvements to dot and bracket splitting logic
depth == 0and preserving the original key in those cases.Test coverage and edge case handling
allowDots,decodeDotInKeys, and parsing depth, ensuring correct handling for both literal and encoded dots in various scenarios.allowDots=true, matching the new parsing behavior.