Conversation
…ot decoding in documentation
…d bracket depth correctly
…C# parity and option consistency
… logic; add decodeKey/decodeValue helpers
…ior in DecodeOptions
|
Warning Rate limit exceeded@techouse has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 30 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughReplaces the single decoder with kind-aware key/value decoding (Decoder + deprecated LegacyDecoder), adds depth-aware top-level dot-to-bracket conversion, refactors key segmentation/bracket handling and parameter-splitting limits, removes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant QS as QS.decode
participant DO as DecodeOptions
participant DEC as Decoder
participant LDEC as LegacyDecoder
participant Utils as Utils.decode
Client->>QS: decode(query, options)
QS->>DO: decodeKey(rawKey, charset)
alt Decoder present
DO->>DEC: call(rawKey, charset, kind=KEY)
DEC-->>DO: decodedKey
else if LegacyDecoder present
DO->>LDEC: call(rawKey, charset)
LDEC-->>DO: decodedKey
else
DO->>Utils: decode(rawKey, charset)
Utils-->>DO: decodedKey
end
QS->>QS: _dotToBracketTopLevel(decodedKey)
QS->>QS: _splitKeyIntoSegments(...)
QS->>DO: decodeValue(rawValue, charset)
alt Decoder present
DO->>DEC: call(rawValue, charset, kind=VALUE)
DEC-->>DO: decodedValue
else if LegacyDecoder present
DO->>LDEC: call(rawValue, charset)
LDEC-->>DO: decodedValue
else
DO->>Utils: decode(rawValue, charset)
Utils-->>DO: decodedValue
end
QS-->>Client: parsed object (with depth/list rules)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ 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 #37 +/- ##
==========================================
+ Coverage 94.85% 94.87% +0.02%
==========================================
Files 14 14
Lines 835 878 +43
==========================================
+ Hits 792 833 +41
- Misses 43 45 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ction; improve charsetSentinel and decoder behavior descriptions
…invocation and legacy overloads
…egacy dynamic invocation cases
… decode.dart accordingly
![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 (2)
test/unit/decode_test.dart (1)
2182-2193: “Dynamic fallback” test does not actually miss named parametersThe helper
_Loose1.callcurrently accepts{Encoding? charset, DecodeKind? kind}, which matches the library’s typedefs, so the dynamic fallback isn’t exercised as the comment claims. If the goal is to force the untyped “(value) only” path, change the named parameter names so they don’t match, or remove them entirely.-class _Loose1 { +class _Loose1 { final List<String?> sink; _Loose1(this.sink); - dynamic call(String? v, {Encoding? charset, DecodeKind? kind}) { + // Intentionally mismatched named parameters to exercise the dynamic fallback. + // Do not rename to `charset`/`kind`. + dynamic call(String? v, {Encoding? cs, Object? kd}) { sink.add(v); return v == null ? null : 'X$v'; } }Also applies to: 2564-2568
lib/src/extensions/decode.dart (1)
43-53: List limit enforcement misses combined length when splitting comma valuesWhen
valis a comma-separated string, the check only comparessplitVal.lengthtolistLimit. It should also account forcurrentListLengthso appending"d,e"to an existing list near the limit throws correctly whenthrowOnLimitExceededis true.- if (val is String && val.isNotEmpty && options.comma && val.contains(',')) { - final List<String> splitVal = val.split(','); - if (options.throwOnLimitExceeded && splitVal.length > options.listLimit) { + if (val is String && val.isNotEmpty && options.comma && val.contains(',')) { + final List<String> splitVal = val.split(','); + if (options.throwOnLimitExceeded && + (currentListLength + splitVal.length) > options.listLimit) { throw RangeError( 'List limit exceeded. ' 'Only ${options.listLimit} element${options.listLimit == 1 ? '' : 's'} allowed in a list.', ); } return splitVal; } // Guard incremental growth of an existing list as we parse additional items. - if (options.throwOnLimitExceeded && - currentListLength >= options.listLimit) { + if (options.throwOnLimitExceeded && currentListLength >= options.listLimit) { throw RangeError( 'List limit exceeded. ' 'Only ${options.listLimit} element${options.listLimit == 1 ? '' : 's'} allowed in a list.', ); }Follow-up: add a test where an existing list has
listLimit - 1items and a comma-value with 2 more items is appended (should throw).Also applies to: 55-62, 150-159
🧹 Nitpick comments (14)
test/unit/example.dart (1)
900-916: Decoder signature update looks correct; minor clean-up opportunityThe Shift-JIS decoder matches the new
(String? str, {Encoding? charset, DecodeKind? kind})shape and returnsnullsafely. You can simplify the loop to avoidstr!assertions and repeatedfirstMatchre-allocations.- Match? parts; - while ((parts = reg.firstMatch(str!)) != null && parts != null) { - result.add(int.parse(parts.group(1)!, radix: 16)); - str = str.substring(parts.end); - } + for (Match? m = reg.firstMatch(str); m != null; m = reg.firstMatch(str)) { + result.add(int.parse(m.group(1)!, radix: 16)); + str = str.substring(m.end); + }test/unit/models/decode_options_test.dart (1)
169-221: Key decoding semantics: add “leading dot” coverageThe tests thoroughly cover encoded dot handling in and out of brackets. Consider adding a leading-dot case to lock in the “preserve literal leading dot” behaviour the PR description promises.
For example:
expect(const DecodeOptions(allowDots: true).decodeKey('.a'), equals('.a')); expect(const DecodeOptions(allowDots: true).decodeKey('..a'), equals('..a'));Want me to push a test for this?
test/unit/uri_extension_test.dart (1)
1309-1332: Custom decoder including kind: good, but consider negative-path testThe custom Shift-JIS decoder now supports the
kindparameter. To strengthen coverage, consider asserting that the decoder is invoked withDecodeKind.keyon the key side for at least one pair in this test file (similar to what’s done elsewhere), to ensure kind flows here too.test/unit/decode_test.dart (1)
2195-2553: C# parity suite is strong; consider adding “leading dot” and “double dot” casesYou covered trailing dots, bracket-before-dot, and encoded-dot flows. To fully capture the PR’s stated behaviour (“.a”, “a..b”), add:
- Leading dot preserved:
QS.decode('.a=x', allowDots: true) → {'.a': 'x'}.- Double dots: first dot literal, second splits:
QS.decode('a..b=x', allowDots: true) → {'a.': {'b': 'x'}}.lib/src/extensions/decode.dart (3)
255-261: Doc/implementation drift: extra%2Ehandling afterdecodeKeyComments state “keys have already been percent‑decoded earlier by
decodeKey”, yet this block still replaces%2E/%2e. IfdecodeKeyguarantees percent-decoding (as the tests imply), this is dead code; if not, it creates a split responsibility.
- Either remove the
%2Ereplacement here and centralise percent-decoding inDecodeOptions.decodeKey, or- Update the comment to reflect that
decodeKeymay return raw tokens for custom decoders, and we normalise encoded dots inside bracket segments here.If you choose the first option:
- final String decodedRoot = options.decodeDotInKeys - ? cleanRoot.replaceAll('%2E', '.').replaceAll('%2e', '.') - : cleanRoot; + final String decodedRoot = cleanRoot;
379-397: Remainder-wrapping behaviour is correct but deserves an inline commentUsing
'[${key.substring(open)}]'yields a double-bracket token (e.g.,[[g][h]]), which subsequently becomes a single literal segment"[g][h]"after trimming in_parseObject. This matches the tests; a one-line comment would prevent future “fixes”.- segments.add('[${key.substring(open)}]'); + // Wrap the remaining bracket groups as a single literal segment. + // Example: key="a[b][c][d]", depth=2 → segment="[[c][d]]" which becomes "[c][d]" later. + segments.add('[${key.substring(open)}]');
91-107: Parameter limit logic is sound; slight simplification possibleYou can compute overflow first and short-circuit without materialising
limit + 1entries. Purely optional.- if (limit != null && limit > 0) { - final int takeCount = options.throwOnLimitExceeded ? limit + 1 : limit; - final int count = - allParts.length < takeCount ? allParts.length : takeCount; - parts = allParts.sublist(0, count); - if (options.throwOnLimitExceeded && allParts.length > limit) { + if (limit != null && limit > 0) { + if (options.throwOnLimitExceeded && allParts.length > limit) { throw RangeError( 'Parameter limit exceeded. Only $limit parameter${limit == 1 ? '' : 's'} allowed.', ); - } + } + parts = allParts.sublist(0, allParts.length < limit ? allParts.length : limit); } else { parts = allParts; }lib/src/models/decode_options.dart (7)
19-21: Docs say “will throw”, but implementation uses asserts (won’t run in release builds)As written, this will not throw in release modes; it’s enforced via asserts only. Please reword to avoid promising a runtime exception.
Apply this doc tweak:
-/// set `allowDots: false` — which is an invalid combination and will throw at construction time. +/// set `allowDots: false` — which is an invalid combination and is checked +/// via asserts at construction time (debug/profile builds).
76-88: Validation via asserts is coherent, but consider tightening edge cases
- Good: charset whitelist, allowDots/decodeDotInKeys coupling, positive parameterLimit.
- Potential follow‑ups:
- Depth/listLimit invariants aren’t validated; consider asserting they’re positive to catch misconfig early.
- parameterLimit is a num; if only integers are meaningful downstream, consider int to prevent fractional inputs.
Example tightening (non‑breaking; asserts only):
assert( charset == utf8 || charset == latin1, 'Invalid charset', ), assert( !(decodeDotInKeys ?? false) || allowDots != false, 'decodeDotInKeys requires allowDots to be true', ), assert( parameterLimit > 0, 'Parameter limit must be positive', - ); + ), + assert(depth > 0, 'Depth must be positive'), + assert(listLimit > 0, 'List limit must be positive');If you want hard runtime failures, we’ll need a non‑const constructor or a factory that performs runtime checks — happy to sketch that if desired.
129-136: Re‑word to avoid promising a throw in release buildsSame note as earlier: this invalid combination is guarded by asserts, not guaranteed throws.
Doc tweak:
-/// Passing `decodeDotInKeys: true` while forcing `allowDots: false` is an -/// invalid combination and will throw *at construction time*. +/// Passing `decodeDotInKeys: true` while forcing `allowDots: false` is an +/// invalid combination and is rejected via asserts at construction time +/// (debug/profile builds).
184-204: Unify charset fallback in legacy pathdecode() falls back to this.charset only for Utils.decode; the legacy decoder currently receives a potentially null charset. For symmetry and back‑compat, pass the effective charset to the legacy decoder too.
Apply:
- if (_legacyDecoder != null) { - return _legacyDecoder!(value, charset: charset); - } + if (_legacyDecoder != null) { + return _legacyDecoder!(value, charset: charset ?? this.charset); + }
228-236: Deprecated decoder(...) shim is helpful for migrationClear deprecation message pointing to the new decode() signature. Consider adding an @internal TODO to remove in the next major.
255-257: Expose throwOnLimitExceeded in copyWith; preserve decodersDecoders are carried over correctly. However, copyWith can’t toggle throwOnLimitExceeded today. Suggest adding it for parity with the constructor and props.
Proposed change:
DecodeOptions copyWith({ bool? allowDots, bool? allowEmptyLists, int? listLimit, Encoding? charset, bool? charsetSentinel, bool? comma, bool? decodeDotInKeys, Pattern? delimiter, int? depth, Duplicates? duplicates, bool? ignoreQueryPrefix, bool? interpretNumericEntities, num? parameterLimit, bool? parseLists, bool? strictNullHandling, bool? strictDepth, + bool? throwOnLimitExceeded, Decoder? decoder, LegacyDecoder? legacyDecoder, }) => DecodeOptions( allowDots: allowDots ?? this.allowDots, allowEmptyLists: allowEmptyLists ?? this.allowEmptyLists, listLimit: listLimit ?? this.listLimit, charset: charset ?? this.charset, charsetSentinel: charsetSentinel ?? this.charsetSentinel, comma: comma ?? this.comma, decodeDotInKeys: decodeDotInKeys ?? this.decodeDotInKeys, delimiter: delimiter ?? this.delimiter, depth: depth ?? this.depth, duplicates: duplicates ?? this.duplicates, ignoreQueryPrefix: ignoreQueryPrefix ?? this.ignoreQueryPrefix, interpretNumericEntities: interpretNumericEntities ?? this.interpretNumericEntities, parameterLimit: parameterLimit ?? this.parameterLimit, parseLists: parseLists ?? this.parseLists, strictNullHandling: strictNullHandling ?? this.strictNullHandling, strictDepth: strictDepth ?? this.strictDepth, + throwOnLimitExceeded: + throwOnLimitExceeded ?? this.throwOnLimitExceeded, decoder: decoder ?? _decoder, legacyDecoder: legacyDecoder ?? _legacyDecoder, );Also applies to: 276-278
280-299: Include throwOnLimitExceeded in toString for diagnosticsIt’s in props and constructor but omitted from toString, which hinders debugging.
Suggested addition:
String toString() => 'DecodeOptions(\n' ' allowDots: $allowDots,\n' ' allowEmptyLists: $allowEmptyLists,\n' ' listLimit: $listLimit,\n' ' charset: $charset,\n' ' charsetSentinel: $charsetSentinel,\n' ' comma: $comma,\n' ' decodeDotInKeys: $decodeDotInKeys,\n' ' delimiter: $delimiter,\n' ' depth: $depth,\n' ' duplicates: $duplicates,\n' ' ignoreQueryPrefix: $ignoreQueryPrefix,\n' ' interpretNumericEntities: $interpretNumericEntities,\n' ' parameterLimit: $parameterLimit,\n' ' parseLists: $parseLists,\n' ' strictDepth: $strictDepth,\n' - ' strictNullHandling: $strictNullHandling\n' + ' strictNullHandling: $strictNullHandling,\n' + ' throwOnLimitExceeded: $throwOnLimitExceeded\n' ')';
📜 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 (7)
lib/src/extensions/decode.dart(11 hunks)lib/src/models/decode_options.dart(10 hunks)lib/src/qs.dart(0 hunks)test/unit/decode_test.dart(8 hunks)test/unit/example.dart(1 hunks)test/unit/models/decode_options_test.dart(2 hunks)test/unit/uri_extension_test.dart(4 hunks)
💤 Files with no reviewable changes (1)
- lib/src/qs.dart
⏰ 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 (14)
test/unit/models/decode_options_test.dart (1)
144-167: Constructor/copyWith guardrails are well-coveredGood assertions for inconsistent allowDots/decodeDotInKeys combinations at construction and via copyWith. The anyOf matcher keeps the suite resilient across platforms/assertion modes.
test/unit/uri_extension_test.dart (2)
1160-1177: Dual decoder paths exercised correctly (legacy + new)Great separation of
legacyDecoder(2-arg) anddecoder(3-arg with kind). This meaningfully asserts backward compatibility while validating the new API.
1463-1471: Null-returning decoder with latin1 + interpretNumericEntitiesThis is a good edge-case test confirming null-propagation and numeric entity interpretation precedence. No action needed.
test/unit/decode_test.dart (2)
206-224: Comma + listLimit throwing path: nice targeted failure messageClear assertion on the thrown message; it will catch regressions in the limit check.
1270-1281: Number-aware decoder (3-arg) path looks goodThe 3-arg decoder with kind parameter behaves correctly and is used in both 0 and 1 numeric cases. No changes needed.
lib/src/models/decode_options.dart (9)
1-1: File‑level ignore is justifiedThe file defines and consumes a deprecated public typedef for back‑compat. The scoped ignore is reasonable here.
43-49: Deprecation path for LegacyDecoder looks goodClear migration guidance and a dedicated typedef minimise friction for adopters.
92-95: allowDots defaulting clarifiedGood clarification that decodeDotInKeys implies allowDots unless explicitly set false (and that such a combination is invalid).
111-114: Charset sentinel “first occurrence” semantics documentedThe explicit “first occurrence” rule and omission from the result map reduce ambiguity. Nice.
179-183: Private decoder fields are well factoredKeeping the primary and legacy decoders separate simplifies the selection logic and equatability.
206-216: decodeKey() coercion to String via toString() is acceptableThis keeps the API forgiving even if a custom decoder returns a non‑String. Documented behaviour is implicit; all good.
217-227: decodeValue() helper is straightforwardThin wrapper keeps call sites tidy and encourages kind‑aware decoders.
320-321: Equatable props extended with _legacyDecoder — goodEnsures equality reflects decoder identity changes across both paths.
54-57: No matches found — manual verification requiredI ran the provided ripgrep checks and did not locate any occurrences of:
- The instance method
.decoder(...)- The
LegacyDecodertype orlegacyDecoder:parameter- Any instantiation of
DecodeOptions(However, absence of output is not evidence of absence. Please manually confirm that no callers—particularly in test suites,
example/directories, or downstream packages—still rely on the removed multi-shapedecoder(...)typedef or thelegacyDecoderparameter. Ensuring these call sites are updated will guarantee the change remains safe and fully back-compatible.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/src/extensions/decode.dart (1)
401-464: Top-level dot splitter preserves degenerate cases and fixes the leading-dot issueDepth-aware conversion, leading-dot preservation, duplicate/trailing dot handling, and “.[” degeneracy are all handled cleanly. This addresses the prior concern about leading '.' being preserved as a literal.
🧹 Nitpick comments (5)
lib/src/extensions/decode.dart (4)
45-61: Guard comma-split lists against cumulative overflow, not only per-token lengthWhen
comma == true, the limit check only considerssplitVal.length. This can be bypassed when the same key appears multiple times with comma-separated values, cumulatively exceedinglistLimit. Use the already-availablecurrentListLengthto enforce the projected size.Apply:
- if (options.throwOnLimitExceeded && splitVal.length > options.listLimit) { + if (options.throwOnLimitExceeded && + currentListLength + splitVal.length > options.listLimit) { throw RangeError( 'List limit exceeded. ' 'Only ${options.listLimit} element${options.listLimit == 1 ? '' : 's'} allowed in a list.', ); }
94-104: Avoid allocating extra items when throwing on parameter overflowMinor: when
throwOnLimitExceededis true, you can detectallParts.length > limitand throw before slicing, avoiding thelimit + 1sublist. This is micro-optimisation and simplifies reasoning.Suggested change:
- final int takeCount = options.throwOnLimitExceeded ? limit + 1 : limit; - final int count = - allParts.length < takeCount ? allParts.length : takeCount; - parts = allParts.sublist(0, count); - if (options.throwOnLimitExceeded && allParts.length > limit) { + if (options.throwOnLimitExceeded && allParts.length > limit) { throw RangeError( 'Parameter limit exceeded. Only $limit parameter${limit == 1 ? '' : 's'} allowed.', ); } + parts = allParts.sublist(0, limit);
252-261: Redundant%2Ereplacement likely unnecessary post-decodingBy this stage, keys have already been percent-decoded via
decodeKey, socleanRootshould not contain%2E/%2e. Keeping the replacement is harmless (supports custom decoders that skip%2E), but consider documenting this rationale inline to avoid confusion for future maintainers.
379-396: Strict-depth branch: add an explicit test for “text after depth + more groups”You already cover many depth cases. Add a test where the key has trailing text and additional groups beyond
maxDepth(e.g.,a[b][c]tail[d]at depth 2 withstrictDepth: true) to validate this throw path.I can draft an additional unit test if useful.
test/unit/decode_test.dart (1)
30-92: Anchoring tests to specific line numbers is brittleThe comment “targets lines 154–156 in decode.dart” will drift quickly. Consider replacing the line reference with a brief behavioural description only.
- // This test targets lines 154-156 in decode.dart + // This test exercises nested list handling inside _parseObject.
📜 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(11 hunks)test/unit/decode_test.dart(8 hunks)
🔇 Additional comments (12)
lib/src/extensions/decode.dart (5)
142-159: Key/value decoding order and null-to-empty coercion look correctDecoding keys with
decodeKey(...)??: ''and values withdecodeValueafter_parseListValuealigns with the documented semantics and keeps kind-awareness intact.
262-281: Index handling matches qs semantics and testsThe bracketed-numeric test (
root != decodedRootgating) correctly disambiguates bare numeric keys, andindex <= listLimitenforces the upper bound as expected.
318-399: Depth, unterminated groups, and trailing text handling are solid
depth == 0returns a single segment (qs parity).- Unterminated
[returns the whole key as a literal segment.- Trailing text after the last balanced group becomes one final bracket segment (ignores lone trailing
.).- For extra groups beyond
maxDepth, throwing understrictDepthis correct; otherwise collapsing remainder into a single bracket segment preserves content (e.g.,[[g][h]]).
470-511: Fast-path normalisation of %5B/%5D is clear and efficientSingle-pass rewrite for bracket tokens (upper/lower case) improves downstream splitting without over-decoding. Good balance of correctness and performance.
139-159: slice extension confirmed – no action requiredThe
slicemethod is provided by theStringExtensioninlib/src/extensions/extensions.dart(extension declared on line 69,String slice(int start, [int? end])), covering all uses indecode.dart.test/unit/decode_test.dart (7)
16-28: Parameter-limit test now tolerates platform/throw variationsAccepting
ArgumentError | StateError | AssertionErroris pragmatic and should reduce false negatives across environments.
206-224: Good coverage for comma + list-limit throw pathThe assertion message matches the implementation (“Only 3 elements allowed in a list.”). This aligns with the refactor in
_parseListValue.
2195-2261: C# parity: top-level encoded dot and bracket-segment behaviours are well exercisedThese cases comprehensively verify:
- top-level
%2E/%2esplitting withallowDots=true(regardless ofdecodeDotInKeys),- bracket
%2E/%2emapping to '.' across charsets,- the invalid combo
allowDots=false+decodeDotInKeys=true.
2462-2470: Lower-case%2etop-level split is explicitly testedNice catch to ensure case-insensitive behaviour remains intact for encoded dots.
2496-2516: Leading dot and double-dot degeneracy tests validate the new splitterThe “leading dot preserved” and “double dot” expectations match the refined
_dotToBracketTopLevel. Great edge coverage.
2042-2052: Legacy 2‑arg decoder compatibility test is concise and effectiveEnsures
legacyDecoderintegration still functions without impacting the new 3‑arg path.
2586-2590: Dynamic callable fallback coverage is valuableThis test ensures the dynamic ladder in
DecodeOptions.decoderremains robust when named parameters don’t match.
…ing comma-separated values
…bracket groups as single segment
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/unit/decode_test.dart (2)
30-92: Make this test resilient to code refactors; remove brittle file/line references.The comment “targets lines 154-156 in decode.dart” will rot quickly. Also, the note “Decode it with the existing result as the input” is misleading: the code decodes
queryString4independently. Consider tightening comments to describe behaviour, not implementation line numbers.Apply this diff to clarify:
- // This test targets lines 154-156 in decode.dart - // We need to create a scenario where val is a List and parentKey exists in the list + // Exercise the _parseObject branch that handles nested lists and parent indices. + // Create scenarios where `val` is a List and verify index-based insertion/compaction. @@ - // Decode it with the existing result as the input + // Decode it separately; ensure compaction yields only the provided element
1502-1518: Percent-decoder in this test ignores non-encoded bytes; make it robust.The loop only consumes “%XX” fragments and drops any intervening plain text. For safety and clarity, parse sequentially so both encoded and literal bytes survive. This improves future-proofing if the fixture ever contains mixed content.
Apply this diff:
-String? decode(String? str, {Encoding? charset, DecodeKind? kind}) { - if (str == null) { - return null; - } - - final RegExp reg = RegExp(r'%([0-9A-F]{2})', caseSensitive: false); - final List<int> result = []; - Match? parts; - while ((parts = reg.firstMatch(str!)) != null && parts != null) { - result.add(int.parse(parts.group(1)!, radix: 16)); - str = str.substring(parts.end); - } - return ShiftJIS().decode( - Uint8List.fromList(result), - ); -} +String? decode(String? s, {Encoding? charset, DecodeKind? kind}) { + if (s == null) return null; + final bytes = <int>[]; + for (int i = 0; i < s.length; ) { + final c = s.codeUnitAt(i); + if (c == 0x25 /* '%' */ && i + 2 < s.length) { + final h1 = s.codeUnitAt(i + 1), h2 = s.codeUnitAt(i + 2); + int d(int u) => (u >= 0x30 && u <= 0x39) ? u - 0x30 + : (u | 0x20) >= 0x61 && (u | 0x20) <= 0x66 ? (u | 0x20) - 0x61 + 10 + : -1; + final hi = d(h1), lo = d(h2); + if (hi >= 0 && lo >= 0) { + bytes.add((hi << 4) | lo); + i += 3; + continue; + } + } + bytes.add(c); + i++; + } + return ShiftJIS().decode(Uint8List.fromList(bytes)); +}lib/src/extensions/decode.dart (2)
101-105: Avoid building the fullallPartslist when a small positiveparameterLimitis set.
cleanStr.split(delimiter)materialises all parts, then slices; for very large strings this doubles memory. Consider a limited split that stops afterlimitparts whenthrowOnLimitExceededis false, or counts tolimit+1when true.Example approach:
- For string delimiter: manual scan and push until
limit(orlimit+1to detect overflow).- For RegExp delimiter: iterate
allMatcheslazily and stop atlimit.Keeps behaviour identical while reducing peak allocations on long inputs.
201-204: Doc/implementation mismatch around percent-decoded dots.Comment states keys arrive percent‑decoded before this phase, so
%2E/%2eshouldn’t be present. That makes the%2Ehandling below redundant and potentially misleading. Align the code or the comment.Apply this diff to simplify and match the documented pipeline:
- final String decodedRoot = options.decodeDotInKeys - ? cleanRoot.replaceAll('%2E', '.').replaceAll('%2e', '.') - : cleanRoot; + // `decodeKey` already percent-decodes; `%2E` should not appear here. + final String decodedRoot = cleanRoot;If you prefer to keep a safeguard, add an assertion in debug mode rather than conditional replacement.
📜 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(12 hunks)test/unit/decode_test.dart(7 hunks)
🔇 Additional comments (12)
test/unit/decode_test.dart (4)
16-28: Good defence around invalid parameterLimit (multiple exception types accepted).Covers ctor-time/runtime variants cleanly; aligns with stricter options validation.
206-224: List limit enforcement for comma-splitting is precise and user-facing error is clear.Message matches library behaviour; good job asserting the exact string.
2042-2052: Decoder API migration tests (legacy 2‑arg and new 3‑arg) are crisp.They verify backwards compatibility and that kind-aware decoding is now the default path. Nice.
2181-2561: Excellent dot-notation parity coverage (top-level, bracketed, case-insensitive, charset).These tests lock in nuanced behaviour: leading/trailing dots, duplicate dots, encoded dots before/after bracket segments, invalid option combinations, and kind propagation. Strong guardrails for future refactors.
lib/src/extensions/decode.dart (8)
20-23: Good contract: avoid mutating caller containers.Clear and important guarantee; matches how objects/lists are freshly allocated and merged.
46-62: List-limit checks are now correct for comma-splits and incremental growth.
- Split-time: throws when current + new > limit.
- Incremental: throws when current >= limit.
Matches tests and avoids off-by-one.
265-281: Index semantics correctly restrict list indices to bracketed numerics and<= listLimit.Prevents accidental list creation from bare numeric keys; mirrors
qs. Good attention to detail.
325-333: Depth=0 short-circuit is correct and cheap.Returning the whole key ensures parity with
qs; avoids unnecessary scanning.
346-399: Balanced-group splitter: robust handling of unterminated brackets, trailing text, and strictDepth.
- Unterminated group returns the entire key as a literal (matches
qs).- Trailing text is wrapped as one segment (except a lone '.').
- Remainder beyond
maxDepththrows whenstrictDepth.Well-structured and readable.
404-468: Top-level dot splitter preserves degenerate cases correctly.Leading dot literal, “a..b” partial literal, trailing dot ignored by splitter, and “.[” normalised — all covered. Matches the new tests.
469-517: Fast-path bracket normalisation is a nice win.Single pass, case-insensitive %5B/%5D rewrite keeps downstream parsing simple and fast.
140-161: String.slice Extension VerifiedThe JS-style
String.sliceextension is implemented inlib/src/extensions/extensions.dart(lines 69–85), so all calls topart.slice(…)will compile successfully.
No further changes are required.
…and improve byte extraction
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/decode_test.dart (1)
1093-1132: Clarify negativelistLimitsemantics vsthrowOnLimitExceededThe existing tests show that:
- With
listLimit: -1andthrowOnLimitExceeded: false(the default), numeric bracket indices are always treated as map keys (i.e. “disable list growth/force map”) – see the “allows overriding list limit” tests at lines 1093–1132.- With
listLimit: -1andthrowOnLimitExceeded: true, any list growth (e.g. empty-bracket pushes or comma-splits) immediately throws aRangeError– see the “handles negative list limit correctly” test at lines 2012–2019.These semantics are intentional but aren’t currently documented, which can be surprising. I recommend updating the inline API docs to spell out that:
- A negative
listLimitdisables numeric-index parsing (all numbered brackets become map keys).- However, when
throwOnLimitExceededistrue, any list-growth operation—empty brackets (a[]), comma-separated values, or nested pushes—will throw if the (implicit) limit of –1 is exceeded.Please update:
lib/src/models/decode_options.dart
• In the doc comment forlistLimit, note that negative values disable numeric-indexed lists.
• In the doc forthrowOnLimitExceeded, clarify that it applies to all list-growth paths, even whenlistLimitis negative.lib/src/extensions/decode.dart
• In the extension’s method docs, call out howthrowOnLimitExceededinteracts with negativelistLimitfor empty-bracket and comma-split list growth.This makes the contract crystal-clear and aligns the documentation with the verified test coverage.
🧹 Nitpick comments (6)
test/unit/decode_test.dart (6)
30-32: Avoid coupling test names to private implementation detailsThe test name references a private implementation detail (
_parseObject). Prefer behaviour-focused wording to keep tests resilient to internal refactors.Apply this diff to rename the test:
- test('Nested list handling in _parseObject method', () { + test('Nested list handling preserves nested lists and compaction behaviour', () {
216-221: Make error-message assertion less brittleAsserting the exact error message string is fragile and can break on minor copy changes. Match a stable substring instead.
- isA<RangeError>().having( - (e) => e.message, - 'message', - 'List limit exceeded. Only 3 elements allowed in a list.', - ), + isA<RangeError>().having( + (e) => e.message, + 'message', + contains('List limit exceeded'), + ),
1297-1313: Remove duplicated test blockThis block duplicates the preceding test case (same input, same expectation). It increases runtime and maintenance without adding coverage.
- expect( - QS.decode('foo[]=1,2,3&foo[]=', const DecodeOptions(comma: true)), - equals({ - 'foo': [ - ['1', '2', '3'], - '' - ] - }), - );
1053-1059: Reduce work in the “very long list” generatorInside the loop,
utf8.encode(str.toString())re-encodes the growing buffer every iteration, which is quadratic in practice. This is fine functionally, but you can keep the test fast and deterministic by growing an ordinaryStringfirst, then wrapping it in aStringBufferif needed.Example alternative:
var s = 'a[]=a'; while (utf8.encode(s).length < 128 * 1024) { s = '$s&$s'; } expect(() => QS.decode(s), returnsNormally);
1502-1526: Handle “+” as space in the custom Shift-JIS decoderURL-encoded query semantics typically treat “+” as a space for x-www-form-urlencoded. Your custom decoder ignores it; harmless for this test input, but easy to make correct.
for (int i = 0; i < s.length;) { final c = s.codeUnitAt(i); if (c == 0x25 /* '%' */ && i + 2 < s.length) { final h1 = s.codeUnitAt(i + 1), h2 = s.codeUnitAt(i + 2); int d(int u) => switch (u) { >= 0x30 && <= 0x39 => u - 0x30, >= 0x61 && <= 0x66 => u - 0x61 + 10, >= 0x41 && <= 0x46 => u - 0x41 + 10, _ => -1, }; final hi = d(h1), lo = d(h2); if (hi >= 0 && lo >= 0) { bytes.add((hi << 4) | lo); i += 3; continue; } } + if (c == 0x2B /* '+' */) { + bytes.add(0x20); // space + i++; + continue; + } bytes.add(c); i++; }
2399-2408: Deduplicate overlapping C#‑parity dot‑handling testsThese four tests materially overlap with earlier cases in the same group (which already cover plain dot, encoded dot upper/lower, and bracket segments). Removing them keeps the suite lean without reducing coverage.
Remove the redundant cases:
- test( - 'top-level encoded dot splits when allowDots=true, decodeDotInKeys=true', - () { - const opt = DecodeOptions(allowDots: true, decodeDotInKeys: true); - expect( - QS.decode('a%2Eb=c', opt), - equals({ - 'a': {'b': 'c'} - })); - });- test( - 'top-level encoded dot also splits when allowDots=true, decodeDotInKeys=false', - () { - const opt = DecodeOptions(allowDots: true, decodeDotInKeys: false); - expect( - QS.decode('a%2Eb=c', opt), - equals({ - 'a': {'b': 'c'} - })); - });- test( - 'top-level lowercase encoded dot splits when allowDots=true (decodeDotInKeys=false)', - () { - const opt = DecodeOptions(allowDots: true, decodeDotInKeys: false); - expect( - QS.decode('a%2eb=c', opt), - equals({ - 'a': {'b': 'c'} - })); - });- test( - 'bracket segment: encoded dot mapped to \'.\' (allowDots=true, decodeDotInKeys=true)', - () { - const opt = DecodeOptions(allowDots: true, decodeDotInKeys: true); - expect( - QS.decode('a[%2E]=x', opt), - equals({ - 'a': {'.': 'x'} - })); - expect( - QS.decode('a[%2e]=x', opt), - equals({ - 'a': {'.': 'x'} - })); - });If you prefer to keep them, consider collapsing these into a parameterised helper to avoid repetition.
Also applies to: 2410-2419, 2456-2465, 2513-2527
📜 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)
test/unit/decode_test.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: Test (chrome)
🔇 Additional comments (3)
test/unit/decode_test.dart (3)
2051-2054: Legacy 2‑arg decoder coverage looks goodNice, this confirms
DecodeOptions.legacyDecoderstill routes both key and value through the legacy signature without involvingDecodeKind.
2548-2569: Align decoder “key” expectations with documented order (percent‑decode before splitting)The PR summary states keys are percent‑decoded before bracket/dot splitting. This test currently allows either the raw
a%2Ebor the bracketiseda[b]. If the intended contract is “decoder sees percent‑decoded key”, we should also accepta.bhere.Proposed adjustment:
- expect( - calls.any((it) => - it[1] == DecodeKind.key && (it[0] == 'a%2Eb' || it[0] == 'a[b]')), - isTrue, - ); + expect( + calls.any((it) => + it[1] == DecodeKind.key && + (it[0] == 'a%2Eb' || it[0] == 'a[b]' || it[0] == 'a.b')), + isTrue, + );If, instead, the decoder is intentionally invoked pre‑decode or post‑segmentation, the summary or doc comments may need a small tweak for clarity. I can update docs/tests accordingly once you confirm the desired sequence.
2230-2240: Invalid option combination is correctly enforcedGood to see
allowDots: false+decodeDotInKeys: truethrowing early and the test accepting multiple error types for cross‑platform consistency.
…interaction in decode options comments
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/extensions/decode.dart (1)
268-275: Off‑by‑one in list index bound (allows listLimit+1 elements).The check
index <= options.listLimitletsindex == listLimitpass, and you then allocateindex + 1slots — effectively permittinglistLimit + 1elements. Elsewhere (e.g., Line 57) the limit is enforced as a count (>= listLimitblocks further growth), so this path is inconsistent and can exceed the configured cap.Recommend making the bound strict so the highest admissible index is
listLimit - 1.Apply this diff:
- } else if (index != null && + } else if (index != null && index >= 0 && root != decodedRoot && index.toString() == decodedRoot && options.parseLists && - index <= options.listLimit) { + index < options.listLimit) {Please add/adjust tests to cover:
listLimit: 1witha[1]=x→ should throw (strict) or be rejected (non‑throwing mode).listLimit: 2witha[2]=x→ should be out of bounds.
♻️ Duplicate comments (1)
lib/src/extensions/decode.dart (1)
436-443: Leading.preservation in_dotToBracketTopLevel— LGTM and matches prior review.The new guard keeps a leading dot literal (except the
".[..."degenerate), aligning with the documented “degenerate cases” behaviour and our previous review ask.
🧹 Nitpick comments (3)
lib/src/extensions/decode.dart (3)
46-47: Non‑throwing path can silently exceedlistLimitfor comma‑split values; clamp instead of ignoring.When
throwOnLimitExceededis false,_parseListValuereturns the fullsplitValeven if it pushes the list length pastlistLimit. Consider clamping to the remaining capacity to enforce limits consistently across throw/non‑throw paths.Apply this diff:
- return splitVal; + final int remaining = options.listLimit - currentListLength; + if (remaining <= 0) return const <String>[]; + return splitVal.length <= remaining + ? splitVal + : splitVal.sublist(0, remaining);This mirrors the intent described in the PR (“enforce limits correctly for comma‑separated values”) without hard throwing. Please ensure tests cover the non‑throwing path.
Also applies to: 55-63
101-104: Minor: favourtake(limit)over manualsublistbounds logic.Using
take(limit).toList()is clearer and avoids computing the min yourself.- parts = allParts.sublist( - 0, - allParts.length < limit ? allParts.length : limit, - ); + parts = allParts.take(limit).toList();
201-204: Docs around encoded dots are internally inconsistent; align commentary with behaviour.
- Line 201 suggests keys arrive “already decoded (top‑level encoded dots become literal '.')”.
- Lines 254–265 then say percent‑encoded dots “may still be present here”, and conditionally map
%2E→.based ondecodeDotInKeys.- Lines 414–416 state encoded dots are “not handled here because keys have already been decoded”.
These statements contradict. Given the code, the intended flow appears to be:
DecodeOptions.decodeKeymay choose to leave%2Eencoded whendecodeDotInKeysis false (so top‑level dot splitting won’t accidentally trigger)._dotToBracketTopLevelonly splits literal.at depth 0.- Inside bracket segments, you optionally map
%2E→.here whendecodeDotInKeysis true.Recommend tightening the comments to reflect this exact flow.
Apply this doc tweak:
- /// - Percent‑encoded dots are not handled here because keys have already been decoded by - /// `DecodeOptions.decodeKey` (defaulting to percent‑decoding). + /// - Percent‑encoded dots may be left encoded by `DecodeOptions.decodeKey` when + /// `decodeDotInKeys` is false; only literal `.` are considered for splitting here.And at Line 201, consider:
- /// - Keys arrive already decoded (top‑level encoded dots become literal `.` before we get here). + /// - Keys have been decoded per `DecodeOptions.decodeKey`; top‑level splitting applies to + /// literal `.` only. Encoded dots can remain encoded depending on `decodeDotInKeys`.Please also add/confirm tests covering:
allowDots: true, decodeDotInKeys: falsewhere%2Eat top level must not split.allowDots: true, decodeDotInKeys: truewhere a top‑level%2Ebehaves like a literal dot.Also applies to: 254-265, 414-416
📜 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(12 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 Security Scan
🔇 Additional comments (3)
lib/src/extensions/decode.dart (3)
383-393: Edge case: trailing remainder"."is ignored — confirm multi‑dot tails are preserved as intended.You special‑case
remainder == '.'to drop it. For inputs likea[b].this matches the comment. Fora[b]..the remainder becomes".."and will be wrapped as"[..]"(literal), which seems correct; just ensure tests lock this in so future refactors don’t regress double‑dot handling.Suggested tests:
QS.decode('a.b.=x', allowDots: true)→{'a': {'b': 'x'}}(single trailing dot ignored).QS.decode('a.b..=x', allowDots: true)→{'a': {'b.': 'x'}}(double dot: first is literal).
145-150: No changes needed:sliceextension is in scopeThe
StringExtension.slicemethod is defined inlib/src/extensions/extensions.dartandlib/src/qs.dartimports this file, makingsliceavailable in all its part files, includingdecode.dart. Therefore, replacingslicewithsubstringis unnecessary.• Extension definition:
lib/src/extensions/extensions.dart, lines 69–86
• Import in main library:lib/src/qs.dartimportspackage:qs_dart/src/extensions/extensions.dartLikely an incorrect or invalid review comment.
300-312: ValuesParsed flag is correctly applied; no changes neededAll call sites to
_parseKeysoriginate fromQS.decodein lib/src/qs.dart, where it’s invoked as:
_$Decode._parseKeys(entry.key, entry.value, options, input is String)(lib/src/qs.dart lines 86–88).
Sinceinput is Stringhere,valuesParsedis alwaystruefor raw query‐string inputs, preventing any double list parsing.No other direct calls to
_parseKeysor_parseObjectexist in the codebase, so thevaluesParsedguard fully covers all paths.
…ching, and long input parsing; fix percent-decoding to handle '+' as space
…th negative listLimit in decode logic comments
1 participant
This pull request refines the query-string decoding logic to better match the reference
qslibrary semantics and clarifies option behaviors, particularly around dot notation and bracketed keys. The changes improve how keys are split and normalized, enforce stricter option validation, and update documentation for clarity. The most important changes are grouped below.Decoder logic and key splitting improvements
_splitKeyIntoSegmentsto exactly matchqssemantics: unterminated brackets now cause the whole key to be treated as a single segment, and trailing or excess segments are handled more robustly. Introduced_dotToBracketTopLevelto convert top-level dots to bracket segments, preserving degenerate cases and ensuring percent-decoded dots are handled correctly._parseObjectand related areas to clarify that keys are decoded before bracket/dot splitting, and that numeric indices are only honored for bracketed segments. [1] [2]Option validation and documentation
DecodeOptions: settingdecodeDotInKeys: truewhile forcingallowDots: falsenow throws at construction time, preventing invalid configurations. Documentation for these options was expanded to clarify their interaction and effects. [1] [2] [3]DecodeOptionsfields, especially around charset sentinel handling and dot notation, to clarify subtle behaviors and edge cases. [1] [2]Decoder API modernization
Decodertype, and provided clear migration guidance in comments.Parameter and list limit enforcement
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17]