Conversation
…ng and performance
…proved performance and clarity
…roved accuracy and consistency
…pping flexibility
…pe safety and clarity
WalkthroughDeterministic decode splitting with strict limit checks and sentinel probing; encode treats non-Map iterables via cached indexed lists and identity checks for comma-format; slice methods now clamp ranges; Utils gains segmented encoding/decoding fast-paths and createIndexMap helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QS
participant Utils
participant EncodeExt as _$Encode
Client->>QS: encode(obj, options)
alt obj is Iterable (non-Map)
QS->>Utils: createIndexMap(iterable)
Utils-->>QS: Map<"0","1",...>
QS->>EncodeExt: _encode(map, generateArrayPrefix, commaRoundTrip)
else
QS->>EncodeExt: _encode(obj, generateArrayPrefix, commaRoundTrip)
end
EncodeExt-->>QS: Iterable<String> | String
QS-->>Client: query string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (22)
✨ 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 #33 +/- ##
==========================================
- Coverage 97.01% 95.33% -1.68%
==========================================
Files 14 14
Lines 737 815 +78
==========================================
+ Hits 715 777 +62
- Misses 22 38 +16 ☔ 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.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
lib/src/extensions/decode.dart (1)
49-71: List limit enforcement misses cumulative growth (can exceed listLimit)When splitting comma lists, the check only compares the new split length against listLimit, ignoring items already accumulated for the same key. Similarly, for single additions, the guard should check “current + 1 > limit”, not “>= limit”. This allows arrays to grow beyond listLimit across multiple occurrences.
Apply this diff to enforce cumulative limits:
static dynamic _parseListValue( dynamic val, DecodeOptions options, int currentListLength, ) { // Fast-path: split comma-separated scalars into a list when requested. if (val is String && val.isNotEmpty && options.comma && val.contains(',')) { final List<String> splitVal = val.split(','); - 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.', ); } return splitVal; } // Guard incremental growth of an existing list as we parse additional items. - if (options.throwOnLimitExceeded && - currentListLength >= options.listLimit) { + if (options.throwOnLimitExceeded && + currentListLength + 1 > options.listLimit) { throw RangeError( 'List limit exceeded. ' 'Only ${options.listLimit} element${options.listLimit == 1 ? '' : 's'} allowed in a list.', ); } return val; }
🧹 Nitpick comments (8)
lib/src/models/undefined.dart (1)
20-21: Align props type with Equatable and avoid allocationEquatable’s recommended signature is List<Object?>. Also, returning a const empty list avoids per-call allocation.
- List<Object> get props => []; + List<Object?> get props => const [];lib/src/extensions/decode.dart (2)
171-174: Numeric-entities path: helper-based join is tidyUsing a dedicated helper to join Iterables improves readability and avoids reimplementations. One tiny micro-optimisation: skip allocating tmp when val is already a String.
- final String tmp = - val is Iterable ? _joinIterableToCommaString(val) : val.toString(); - val = Utils.interpretNumericEntities(tmp); + if (val is Iterable) { + val = Utils.interpretNumericEntities(_joinIterableToCommaString(val)); + } else { + val = Utils.interpretNumericEntities(val.toString()); + }
211-219: currentListLength derivation is fragile (parsing “[idx]” from chain)Parsing the parent index via chain.slice(0, -1).join('') won’t yield a pure integer (e.g. “[0]”), so int.tryParse fails and currentListLength becomes 0 even when a parent list exists. Prefer peeking at the immediate previous segment and extracting its bracketed integer safely.
- if (chain.isNotEmpty && chain.last == '[]') { - final int? parentKey = int.tryParse(chain.slice(0, -1).join('')); - - currentListLength = parentKey != null && - val is List && - val.firstWhereIndexedOrNull((int i, _) => i == parentKey) != null - ? val.elementAt(parentKey).length - : 0; - } else { - currentListLength = 0; - } + if (chain.length >= 2 && chain.last == '[]') { + final String prev = chain[chain.length - 2]; + final bool bracketed = prev.startsWith('[') && prev.endsWith(']'); + final int? parentIndex = + bracketed ? int.tryParse(prev.substring(1, prev.length - 1)) : null; + if (parentIndex != null && + parentIndex >= 0 && + val is List && + parentIndex < val.length) { + final dynamic parent = val[parentIndex]; + currentListLength = parent is List ? parent.length : 0; + } else { + currentListLength = 0; + } + } else { + currentListLength = 0; + }lib/src/extensions/encode.dart (2)
190-195: Generate indices more idiomaticallyMinor readability nit: List.generate is clearer than allocate-and-fill.
- final int n = seqList_.length; - final List<int> idxs = List<int>.filled(n, 0, growable: false); - for (int i = 0; i < n; i++) { - idxs[i] = i; - } - keys = idxs; + keys = List<int>.generate(seqList_.length, (i) => i, growable: false);
264-267: Avoid relying on encodedKey when emitting comma-joined sentinel valueIn the comma-join branch, objKeys contains a sentinel Map with 'value'. Building keyPrefix from key.toString() is unnecessary and brittle. Use adjustedPrefix directly when in the comma mode and processing the sentinel.
- final String keyPrefix = seqList_ != null - ? generateArrayPrefix(adjustedPrefix, encodedKey) - : '$adjustedPrefix${allowDots ? '.$encodedKey' : '[$encodedKey]'}'; + final bool isCommaSentinel = + key is Map<String, dynamic> && key.containsKey('value'); + final String keyPrefix = (isCommaSentinel && + identical(generateArrayPrefix, ListFormat.comma.generator)) + ? adjustedPrefix + : (seqList_ != null + ? generateArrayPrefix(adjustedPrefix, encodedKey) + : '$adjustedPrefix${allowDots ? '.$encodedKey' : '[$encodedKey]'}');lib/src/qs.dart (1)
150-152: Minor type check clean-upUsing “String?” in an is-check is unusual here. Plain String reads better and is sufficient since map keys are String.
- if (key is! String? || (obj[key] == null && options.skipNulls)) { + if (key is! String || (obj[key] == null && options.skipNulls)) { continue; }lib/src/utils.dart (2)
68-72: Reduce duplication: extract Iterable→indexed map helperThe two blocks that convert an Iterable into a SplayTreeMap<int, dynamic> are duplicated. Extracting a small helper improves maintainability and reduces the chance of diverging behaviour.
Apply this diff in the selected ranges:
- final SplayTreeMap<int, dynamic> target_ = - SplayTreeMap<int, dynamic>(); - int i_ = 0; - for (final dynamic item in target) { - target_[i_++] = item; - } + final SplayTreeMap<int, dynamic> target_ = _toIndexedTreeMap(target);- final SplayTreeMap<int, dynamic> target_ = - SplayTreeMap<int, dynamic>(); - int j_ = 0; - for (final dynamic item in target) { - target_[j_++] = item; - } + final SplayTreeMap<int, dynamic> target_ = _toIndexedTreeMap(target);Add this helper outside the selected ranges (e.g., near the other private helpers):
static SplayTreeMap<int, dynamic> _toIndexedTreeMap(Iterable iterable) { final map = SplayTreeMap<int, dynamic>(); var i = 0; for (final v in iterable) { map[i++] = v; } return map; }Also applies to: 100-105
398-399: Nit: comments for letter ranges are swappedThe code points are correct, but the comments should read A-Z for 0x41..0x5A and a-z for 0x61..0x7A.
Apply this diff (also included in the previous fix):
- case int v when v >= 0x41 && v <= 0x5A: // a-z - case int v when v >= 0x61 && v <= 0x7A: // A-Z + case int v when v >= 0x41 && v <= 0x5A: // A-Z + case int v when v >= 0x61 && v <= 0x7A: // a-z
📜 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 (6)
lib/src/extensions/decode.dart(5 hunks)lib/src/extensions/encode.dart(8 hunks)lib/src/extensions/extensions.dart(3 hunks)lib/src/models/undefined.dart(1 hunks)lib/src/qs.dart(3 hunks)lib/src/utils.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 Static Code Analysis
🔇 Additional comments (19)
lib/src/extensions/extensions.dart (2)
48-66: List.slice: safer JS-style semantics look goodNegative indices, clamping, and non-throwing behaviour are correctly implemented. Returning an empty list when e <= s matches JS slice.
85-102: String.slice: correct JS-like behaviourIndex normalisation and clamping are sound; empty result when e <= s is consistent with JS slice semantics.
lib/src/extensions/decode.dart (3)
97-112: Deterministic split + overflow handling is a solid improvementSingle split followed by truncate-or-throw is clearer and avoids repeated iteration. The throwOnLimitExceeded branch correctly throws on overflow.
123-133: Charset sentinel scan: early break is correctBreaking after detecting the sentinel avoids needless scanning and is consistent with only a single sentinel being relevant.
177-179: Array-forcing quirk guarded by parseLists is correctGating the literal “[]=“ behaviour behind options.parseLists avoids surprising array construction when list parsing is disabled.
lib/src/extensions/encode.dart (3)
73-75: Identity check for ListFormat.comma is the right callUsing identical(...) avoids surprises if generators aren’t value-equal but are identity-stable.
146-155: Good: cache non-Map iterable as a list onceAvoids repeated enumeration and out-of-range lookups. This will also stabilise length checks and index access.
283-286: Encoder suppression in comma + encodeValuesOnly path is correctSkipping encoder here prevents double-encoding after pre-encoding during join.
lib/src/qs.dart (4)
118-119: Normalising iterables via Utils.createIndexMap is a good simplificationThis removes the asMap detour and centralises iterable handling in one utility.
122-122: Explicit List for fragments is a nice safety winClarifies intent and catches accidental non-string insertions at compile time.
154-164: Precomputing generator and comma-round-trip flag is clear and avoids recomputationUsing identical(...) for the comma format aligns with the encoder changes.
118-119: createIndexMap confirms stable String keys and preserved iteration order
The implementation usesi.toString()for all keys and builds a default DartLinkedHashMap, which maintains insertion order for bothListand generalIterableinputs. No changes required.lib/src/utils.dart (7)
187-199: Loop-based map merge is clearer and avoids transient allocationsReplacing the fold with a straightforward for-loop improves readability and avoids building intermediate structures. Looks good.
230-236: escape(): Fast-path writes via writeCharCode/hexTable are a solid micro-optimisationSwitching to writeCharCode for safe characters and hexTable lookups for escapes reduces string churn. Good change.
260-327: unescape(): Early fast-path and error-tolerant decoding are well-implemented
- The early return when there is no '%' avoids needless work.
- Using writeCharCode eliminates substring concatenations.
- Error handling gracefully treats malformed sequences as literals.
LGTM.
433-457: latin1 percent-decoder: tight, branch-light implementationThe hex nibble decoder and scan loop are efficient and easy to reason about. Nice addition.
471-478: decode(latin1): good fast-path and resilient fallback
- Early exit when no '%' keeps the hot path cheap.
- On errors, returning the plus-normalised string preserves original data.
LGTM.
615-615: interpretNumericEntities: early check avoids scanning when not neededThe fast-path on missing '&#' is a good low-cost guard. Approved.
660-678: createIndexMap(): useful utility; aligns with QS.encode iterable normalisationStraightforward implementation, returns stringified indices consistently for both List and generic Iterable. Consider documenting stability guarantees (iteration order mirrors source order), but otherwise looks good.
…in encoder for improved correctness
…s and Unicode compliance
…g for improved correctness and clarity
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
This pull request introduces significant improvements and bug fixes to the query string encoding and decoding logic, with a focus on robust handling of iterables, list formats, and utility methods. The changes enhance correctness, performance, and maintainability, especially in edge cases involving lists and string slicing.
Query String Decoding Improvements
elementAtto direct indexing, reducing unnecessary iterator overhead and clarifying intent. [1] [2]parseListsis enabled and a[]=suffix is present, matching expected query string behavior._joinIterableToCommaStringto efficiently join iterable values into comma-separated strings for numeric entity interpretation.Query String Encoding and Iterable Handling
identicalinstead of equality, and consistently cached list forms of iterables to avoid repeated enumeration and type confusion. [1] [2] [3]Utility and Extension Method Fixes
sliceextensions forListandStringto avoid reliance onclampandmin, improving negative index handling, bounds safety, and correctness for empty slices. [1] [2]dart:mathand updated documentation for clarity and style consistency.Core Model and API Consistency
Undefinedmodel for API symmetry and clarified equality semantics.QSclass to use a new utility method for converting iterables to index maps, and improved the handling of encoded keys and values for null safety and type correctness. [1] [2] [3]Internal Utility Refactoring