✨ decode: add throwOnLimitExceeded option
#26
Conversation
WalkthroughThe changes in this pull request involve significant enhancements to the parsing and filtering functionalities across various extensions and classes. Key modifications include renaming and expanding the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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 preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
- Coverage 97.70% 97.32% -0.38%
==========================================
Files 14 14
Lines 653 674 +21
==========================================
+ Hits 638 656 +18
- Misses 15 18 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
techouse
changed the title
techouse
changed the title
throwOnLimitExceeded option
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
lib/src/extensions/extensions.dart (2)
9-17: Consider improving readability of the index calculations.The implementation is functionally correct, but the nested ternary operations could be clearer.
Consider this more readable alternative:
- List<T> slice([int start = 0, int? end]) => sublist( - (start < 0 ? length + start : start).clamp(0, length), - (end == null ? length : (end < 0 ? length + end : end)) - .clamp(0, length), - ); + List<T> slice([int start = 0, int? end]) { + final adjustedStart = start < 0 ? length + start : start; + final boundedStart = adjustedStart.clamp(0, length); + + final rawEnd = end ?? length; + final adjustedEnd = rawEnd < 0 ? length + rawEnd : rawEnd; + final boundedEnd = adjustedEnd.clamp(0, length); + + return sublist(boundedStart, boundedEnd); + }
9-13: Add usage examples to the documentation.Whilst the MDN reference is helpful, inline examples would make the usage clearer.
Consider adding examples like:
/// Modeled after JavaScript's `Array.prototype.slice()` method. /// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice + /// + /// Examples: + /// ```dart + /// final fruits = ['apple', 'banana', 'orange']; + /// fruits.slice(1, 2); // ['banana'] + /// fruits.slice(-2); // ['banana', 'orange'] + /// fruits.slice(1, -1); // ['banana'] + /// ```test/unit/extensions/extensions_test.dart (1)
23-37: Well-structured test cases for slice functionalityThe test cases comprehensively cover various slicing scenarios including positive indices, negative indices, and ranges. Consider adding these edge cases:
- Empty list
- Single-element list
- Out-of-bounds indices
- start > end scenarios
Here's a suggested addition:
test('slice', () { const List<String> animals = [ 'ant', 'bison', 'camel', 'duck', 'elephant', ]; expect(animals.slice(2), ['camel', 'duck', 'elephant']); expect(animals.slice(2, 4), ['camel', 'duck']); expect(animals.slice(1, 5), ['bison', 'camel', 'duck', 'elephant']); expect(animals.slice(-2), ['duck', 'elephant']); expect(animals.slice(2, -1), ['camel', 'duck']); expect(animals.slice(), ['ant', 'bison', 'camel', 'duck', 'elephant']); + // Edge cases + expect(<String>[].slice(), []); + expect(['single'].slice(0), ['single']); + expect(animals.slice(10), []); + expect(animals.slice(3, 2), []); });lib/src/qs.dart (1)
Line range hint
1-150: Well-structured implementation of the new featureThe implementation of the
throwOnLimitExceededoption follows good architectural principles by:
- Keeping the main
QSclass interface unchanged- Encapsulating the new functionality in the appropriate part files
- Using the
DecodeOptionsclass for configurationThis approach maintains clean separation of concerns and makes the feature easily discoverable through the options pattern.
lib/src/models/decode_options.dart (2)
114-116: Consider enhancing the documentation with examples.Whilst the documentation explains the purpose clearly, it would be more helpful to include:
- Example scenarios when this option should be enabled
- Code examples showing the behaviour with and without this option
- Related properties that work in conjunction with this option (e.g.,
listLimitandparameterLimit)
Line range hint
129-186: Critical: Update supporting methods to include the new property.The following methods need to be updated to handle the new
throwOnLimitExceededproperty:
copyWith: Add parameter to support cloning with modified valueprops: Add property to ensure proper value comparisontoString: Add property to ensure complete string representationApply these changes:
DecodeOptions copyWith({ bool? allowDots, bool? allowEmptyLists, // ... other parameters ... bool? strictDepth, Decoder? decoder, + bool? throwOnLimitExceeded, }) => DecodeOptions( allowDots: allowDots ?? this.allowDots, // ... other assignments ... strictDepth: strictDepth ?? this.strictDepth, decoder: decoder ?? _decoder, + throwOnLimitExceeded: throwOnLimitExceeded ?? this.throwOnLimitExceeded, ); @override String toString() => 'DecodeOptions(\n' // ... other properties ... ' strictDepth: $strictDepth,\n' - ' strictNullHandling: $strictNullHandling\n' + ' strictNullHandling: $strictNullHandling,\n' + ' throwOnLimitExceeded: $throwOnLimitExceeded\n' ')'; @override List<Object?> get props => [ // ... other properties ... strictDepth, strictNullHandling, _decoder, + throwOnLimitExceeded, ];lib/src/extensions/decode.dart (1)
143-145: Add unit tests to cover list length calculation in_parseObjectThe logic on lines 143-145 calculates
currentListLengthfor lists with indexed keys. This code is not currently covered by tests, which may lead to unnoticed bugs in list limit enforcement.Would you like assistance in generating unit tests to cover this scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 143-145: lib/src/extensions/decode.dart#L143-L145
Added lines #L143 - L145 were not covered by teststest/unit/uri_extension_test.dart (5)
Line range hint
442-455: Suggestion: Add tests forthrowOnLimitExceededwhen list limits are exceededCurrently, the test cases verify the behaviour when
listLimitis exceeded without throwing an error. To ensure the new error-handling logic functions correctly, please add tests wherethrowOnLimitExceededis set totrueand thelistLimitis exceeded, to confirm that an exception is thrown as expected.
Line range hint
867-881: Suggestion: Add tests forthrowOnLimitExceededwith parameter limitsThe existing tests check that parameters exceeding
parameterLimitare ignored. To fully validate the new error-handling behaviour, consider adding tests wherethrowOnLimitExceededis set totrueandparameterLimitis exceeded, ensuring that an appropriate exception is raised.
Line range hint
1300-1324: Consider handling unsupported charsets gracefullyAn assertion failure occurs when an unknown charset is provided. It might be beneficial to handle unsupported charsets by throwing a more descriptive exception or by defaulting to a standard charset, improving the robustness of the code.
Line range hint
1570-1585: Avoid potential null pointer exceptions in custom decodersIn the test where a custom decoder returns
null, ensure that subsequent code can handlenullvalues without throwing exceptions. This prevents potential runtime errors when dealing with unexpected or malformed input.
Line range hint
1700-1715: Enhance test coverage for numeric entity interpretationCurrently, tests verify numeric entity interpretation in
iso-8859-1charset wheninterpretNumericEntitiesis enabled. To ensure comprehensive coverage, add tests for cases whereinterpretNumericEntitiesis disabled, and when using different charsets likeutf-8, to confirm behaviour remains consistent.test/unit/decode_test.dart (1)
1791-1799: Consider documenting silent truncation behaviourWhen
throwOnLimitExceededis not specified, and the parameter limit is exceeded, the decoder silently truncates excess parameters. It would be beneficial to clearly document this behaviour to prevent confusion for users who might expect an error or warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
lib/src/extensions/decode.dart(4 hunks)lib/src/extensions/extensions.dart(1 hunks)lib/src/models/decode_options.dart(2 hunks)lib/src/qs.dart(1 hunks)lib/src/utils.dart(5 hunks)test/unit/decode_test.dart(3 hunks)test/unit/extensions/extensions_test.dart(2 hunks)test/unit/uri_extension_test.dart(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
lib/src/extensions/decode.dart
[warning] 143-145: lib/src/extensions/decode.dart#L143-L145
Added lines #L143 - L145 were not covered by tests
lib/src/utils.dart
[warning] 123-123: lib/src/utils.dart#L123
Added line #L123 was not covered by tests
[warning] 127-127: lib/src/utils.dart#L127
Added line #L127 was not covered by tests
[warning] 389-389: lib/src/utils.dart#L389
Added line #L389 was not covered by tests
[warning] 391-391: lib/src/utils.dart#L391
Added line #L391 was not covered by tests
🔇 Additional comments (17)
lib/src/extensions/extensions.dart (1)
4-5: LGTM! Note the breaking change.
The replacement of whereNotUndefined() with a more generic whereNotType<Q>() is a good improvement that provides more flexibility. However, this is a breaking change that will require updates in consuming code.
Let's verify the usage of the old method to ensure all occurrences are updated:
✅ Verification successful
The change is properly reflected in tests
The test file shows that the tests have already been updated to use the new whereNotType<Undefined>() method instead of the old whereNotUndefined(). The tests demonstrate that the functionality remains the same whilst being more generic. No further action is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of whereNotUndefined
rg "whereNotUndefined" --type dart
Length of output: 188
Script:
#!/bin/bash
# Let's check the test file content to understand the context
cat test/unit/extensions/extensions_test.dart
Length of output: 1713
test/unit/extensions/extensions_test.dart (2)
9-9: Excellent improvement to type safety!
The change from whereNotUndefined to whereNotType<Undefined> makes the method more generic and type-safe whilst maintaining the same functionality.
18-18: LGTM - Consistent with IterableExtension changes
The addition of .toList() properly maintains the return type whilst adopting the more generic whereNotType<Undefined> approach.
lib/src/models/decode_options.dart (1)
29-29: LGTM! Constructor parameter addition is well-implemented.
The new parameter follows the existing pattern and maintains backward compatibility with its default value of false.
lib/src/utils.dart (3)
45-52: LGTM: Robust implementation of undefined value handling
The implementation correctly maintains collection type (Set/List) whilst filtering undefined values, with proper key ordering via SplayTreeMap.
76-80: LGTM: Improved collection filtering
Good use of whereNotType<Undefined>() for a more type-safe and cleaner implementation.
104-104: Add test coverage for list construction logic
These lines handling list construction with undefined filtering lack test coverage.
Let's verify the current test coverage:
Would you like me to help generate comprehensive test cases for these scenarios?
Also applies to: 123-127
lib/src/extensions/decode.dart (1)
20-26: List limit enforcement in _parseListValue is correctly implemented
The condition currentListLength >= options.listLimit appropriately throws a RangeError when the list limit is exceeded, ensuring that lists do not grow beyond the specified limit.
test/unit/decode_test.dart (9)
415-415: No issues found in expected result
The test case correctly specifies the expected result when decoding mixed simple and explicit lists with a specified listLimit.
821-821: Verification of sparse list compacting
The test accurately checks the decoder's behaviour when dealing with sparse lists and a specified listLimit.
1773-1779: Good coverage of parameter limit within bounds
The test correctly confirms that no error is thrown when the number of parameters is within the specified limit and throwOnLimitExceeded is true.
1781-1789: Proper error handling when parameter limit is exceeded
The test appropriately asserts that a RangeError is thrown when the parameter limit is exceeded and throwOnLimitExceeded is true. This ensures robust error handling in the decoder.
1801-1809: Explicit test for silent truncation without error
The test verifies that when throwOnLimitExceeded is false, the decoder silently truncates parameters exceeding the limit. This behaviour should align with user expectations and be well-documented.
1811-1819: Confirmation of unlimited parameters with parameterLimit set to Infinity
The test correctly confirms that setting parameterLimit to double.infinity allows the decoder to handle an unlimited number of parameters as expected.
1823-1833: Testing list decoding within limits
The test appropriately checks that the decoder functions correctly when the list size is within the specified listLimit and throwOnLimitExceeded is true.
1835-1843: Correct error handling when list limit is exceeded
The test effectively asserts that a RangeError is thrown when the list limit is exceeded with throwOnLimitExceeded set to true, ensuring proper error handling in the decoder.
1845-1855: Verify the conversion of lists to maps when limits are exceeded
When the list exceeds listLimit and throwOnLimitExceeded is not set or false, the decoder converts the list into a map. Ensure that this behaviour is compatible with the rest of the codebase and does not introduce issues where lists are expected.
Run the following script to check for code that may be affected by this change:
✅ Verification successful
Let me gather more context about these usages and the options being passed.
Let me check one more thing to understand how this behaviour is documented and if there are any related tests.
List-to-map conversion behaviour is well-documented and tested
The conversion of lists to maps when exceeding listLimit is an intentional and well-tested behaviour in the codebase. The evidence shows:
- The default
listLimitof 20 is consistently used across the codebase - There are comprehensive test cases covering this behaviour, including:
- Conversion of lists to maps when indices exceed the limit
- Proper handling of sparse lists
- Consistent behaviour across both direct
QS.decodeandqueryParametersQsusage
The two main usages in the library code (in uri.dart and methods.dart) both use the default DecodeOptions, making this behaviour consistent throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of QS.decode without throwOnLimitExceeded to identify potential impacts.
# Find all QS.decode calls without throwOnLimitExceeded parameter
rg 'QS\.decode\([^\)]*\)' --glob 'lib/**/*.dart' --vimgrep --no-filename | grep -v 'throwOnLimitExceeded'
Length of output: 226
Script:
#!/bin/bash
# Get more context around the QS.decode usages
rg 'QS\.decode' -B 3 -A 3 --glob 'lib/**/*.dart'
# Also check for any documentation about listLimit
rg 'listLimit' --glob 'lib/**/*.dart'
Length of output: 2797
Script:
#!/bin/bash
# Check for documentation and tests related to listLimit behaviour
rg -B 3 -A 3 'listLimit.*20' --glob '**/*.{dart,md}'
# Also check for any tests that verify list to map conversion
rg -B 3 -A 3 'converts.*list.*to.*map' --glob 'test/**/*.dart'
Length of output: 11843
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
lib/src/extensions/extensions.dart (3)
4-5: Documentation could be enhanced with examplesThe generic implementation is elegant and more flexible than the previous type-specific approach. Consider enhancing the documentation with usage examples to make it more developer-friendly.
Add examples to the documentation:
/// Returns a new [Iterable] without elements of type [Q]. + /// + /// Example: + /// ```dart + /// final items = ['a', 1, 'b', 2]; + /// final onlyStrings = items.whereNotType<int>(); // ['a', 'b'] + /// ``` Iterable<T> whereNotType<Q>() => where((T el) => el is! Q);
13-17: Consider caching the length calculationThe implementation correctly handles negative indices and bounds checking. However, we could optimise it by caching the length calculation.
Consider this more efficient implementation:
- List<T> slice([int start = 0, int? end]) => sublist( - (start < 0 ? length + start : start).clamp(0, length), - (end == null ? length : (end < 0 ? length + end : end)) - .clamp(0, length), - ); + List<T> slice([int start = 0, int? end]) { + final len = length; + final actualStart = (start < 0 ? len + start : start).clamp(0, len); + final actualEnd = (end == null ? len : (end < 0 ? len + end : end)) + .clamp(0, len); + return sublist(actualStart, actualEnd); + }
9-12: Documentation could benefit from examplesThe documentation clearly states the purpose and includes a helpful MDN reference. However, it would be more helpful with examples showing both positive and negative indices.
Add examples to the documentation:
/// Extracts a section of a list and returns a new list. /// /// Modeled after JavaScript's `Array.prototype.slice()` method. /// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice + /// + /// Example: + /// ```dart + /// final list = [1, 2, 3, 4, 5]; + /// list.slice(1, 3); // [2, 3] + /// list.slice(-2); // [4, 5] + /// list.slice(2, -1); // [3, 4] + /// ```test/unit/extensions/extensions_test.dart (1)
23-37: Comprehensive slice test coverage with room for enhancement!The test cases effectively cover various slicing scenarios including positive indices, negative indices, and different range combinations. The use of a clear, meaningful example with animal names makes the tests easy to understand.
Consider adding these additional test cases to improve coverage:
- Slicing an empty list
- Verifying the original list remains unchanged after slicing
test('slice', () { const List<String> animals = [ 'ant', 'bison', 'camel', 'duck', 'elephant', ]; expect(animals.slice(2), ['camel', 'duck', 'elephant']); expect(animals.slice(2, 4), ['camel', 'duck']); expect(animals.slice(1, 5), ['bison', 'camel', 'duck', 'elephant']); expect(animals.slice(-2), ['duck', 'elephant']); expect(animals.slice(2, -1), ['camel', 'duck']); expect(animals.slice(), ['ant', 'bison', 'camel', 'duck', 'elephant']); + // Test empty list + expect(<String>[].slice(0), isEmpty); + // Test immutability + final result = animals.slice(1, 3); + expect(animals, ['ant', 'bison', 'camel', 'duck', 'elephant']); });lib/src/models/decode_options.dart (4)
114-116: Enhance documentation clarityThe documentation should specify which limits this option affects (listLimit and parameterLimit) to provide better context for developers.
- /// Set to `true` to throw an error when the limit is exceeded. + /// Set to `true` to throw an error when either [listLimit] or [parameterLimit] is exceeded. + /// This provides a way to handle limit violations more strictly instead of silently + /// truncating the data. final bool throwOnLimitExceeded;
Line range hint
127-166: Add missing throwOnLimitExceeded to copyWith methodThe
throwOnLimitExceededparameter is missing from thecopyWithmethod, which means this property cannot be modified when creating a copy of the options.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, Decoder? decoder, + bool? throwOnLimitExceeded, }) => 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, decoder: decoder ?? _decoder, + throwOnLimitExceeded: throwOnLimitExceeded ?? this.throwOnLimitExceeded, );
Line range hint
168-189: Add missing throwOnLimitExceeded to toString methodThe
throwOnLimitExceededproperty is missing from the string representation, which could affect debugging and logging.@override 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' ')';
Line range hint
191-209: Add missing throwOnLimitExceeded to props listThe
throwOnLimitExceededproperty is missing from thepropslist, which means it won't be considered during equality comparisons. This could lead to bugs where two instances with differentthrowOnLimitExceededvalues are incorrectly considered equal.@override List<Object?> get props => [ allowDots, allowEmptyLists, listLimit, charset, charsetSentinel, comma, decodeDotInKeys, delimiter, depth, duplicates, ignoreQueryPrefix, interpretNumericEntities, parameterLimit, parseLists, strictDepth, strictNullHandling, + throwOnLimitExceeded, _decoder, ];lib/src/utils.dart (1)
45-52: Consider simplifying the nested conditional structureThe current implementation, whilst correct, could benefit from being restructured for improved readability.
Consider this more straightforward approach:
- target = target_.values.any((el) => el is Undefined) - ? SplayTreeMap.from({ - for (final MapEntry<int, dynamic> entry in target_.entries) - if (entry.value is! Undefined) entry.key: entry.value, - }) - : target is Set - ? target_.values.toSet() - : target_.values.toList(); + final values = target_.values.whereNotType<Undefined>(); + target = target is Set ? values.toSet() : values.toList();test/unit/decode_test.dart (2)
1787-1787: Use specific exception matcher for clarityFor better readability, consider using the
throwsRangeErrormatcher instead ofthrowsA(isA<RangeError>()). This simplifies the test and makes its intent clearer.Apply this diff to update the test:
- throwsA(isA<RangeError>()), + throwsRangeError,
1841-1841: Use specific exception matcher for clarityFor better readability, consider using the
throwsRangeErrormatcher instead ofthrowsA(isA<RangeError>()). This simplifies the test and makes its intent clearer.Apply this diff to update the test:
- throwsA(isA<RangeError>()), + throwsRangeError,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
lib/src/extensions/decode.dart(4 hunks)lib/src/extensions/extensions.dart(1 hunks)lib/src/models/decode_options.dart(2 hunks)lib/src/qs.dart(1 hunks)lib/src/utils.dart(5 hunks)test/unit/decode_test.dart(3 hunks)test/unit/extensions/extensions_test.dart(2 hunks)test/unit/uri_extension_test.dart(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/src/qs.dart
🧰 Additional context used
🪛 GitHub Check: codecov/patch
lib/src/extensions/decode.dart
[warning] 143-145: lib/src/extensions/decode.dart#L143-L145
Added lines #L143 - L145 were not covered by tests
lib/src/utils.dart
[warning] 123-123: lib/src/utils.dart#L123
Added line #L123 was not covered by tests
[warning] 127-127: lib/src/utils.dart#L127
Added line #L127 was not covered by tests
[warning] 389-389: lib/src/utils.dart#L389
Added line #L389 was not covered by tests
[warning] 391-391: lib/src/utils.dart#L391
Added line #L391 was not covered by tests
🔇 Additional comments (7)
test/unit/extensions/extensions_test.dart (2)
9-9: Well-structured generic type filtering implementation!
The change to whereNotType<Undefined> makes the filtering more generic and type-safe whilst maintaining the same functionality. This approach aligns well with Dart's type system and promotes code reuse.
18-18: Consistent implementation with IterableExtension!
The change maintains consistency with the IterableExtension implementation whilst properly preserving the List type through the toList() call.
lib/src/utils.dart (2)
76-77: LGTM! Clean implementation of generic type filtering
The change to use whereNotType<Undefined>() is a good improvement, making the code more consistent with Dart's generic type system.
Also applies to: 79-80
104-104: Add test coverage for list combination logic
These lines handle important edge cases in list combination, but they currently lack test coverage. Please add tests to verify the behaviour when combining lists with Undefined values.
Let's verify the existing test coverage:
Would you like me to help generate comprehensive test cases for these scenarios?
Also applies to: 123-123, 127-127
test/unit/uri_extension_test.dart (2)
442-445: Verify the expected output for list parsing with listLimit
In this test case, given the input 'a[1]=b&a=c' and listLimit: 20, the expected output is {'a': {1: 'b', 2: 'c'}}. Please confirm if this is correct, considering that a=c might be intended to increment the list, resulting in {'a': {1: 'b', 1: 'c'}}, or if a should be parsed as a list rather than a map.
867-870: Clarify handling of sparse arrays in test case
The test expects {'a': {2: '2', 10: '1'}} for the input 'a[10]=1&a[2]=2' with listLimit: 20. Please verify if the parsing behaviour should result in a being a sparse array or an object. If listLimit allows lists up to 20 elements, should a be an array with indices at positions 2 and 10, or remain as an object with those keys?
lib/src/extensions/decode.dart (1)
143-145:
Undefined method 'firstWhereIndexedOrNull'
The method firstWhereIndexedOrNull used here may not be defined or imported in the current context. Please ensure that this method is implemented or properly imported, as it is essential for the list parsing logic.
Run the following script to verify if firstWhereIndexedOrNull is defined in the codebase:
✅ Verification successful
The method firstWhereIndexedOrNull is provided by the collection package
The code is actually correct. The firstWhereIndexedOrNull method is provided by the collection package, which is listed as a dependency in the project's pubspec.yaml. This is a standard Dart package that provides additional collection methods, including firstWhereIndexedOrNull. The method is available for use in the codebase through this dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the definition of 'firstWhereIndexedOrNull' in the codebase.
# Expectation: Find the method definition or an import statement.
rg 'firstWhereIndexedOrNull[ \t]*\('
Length of output: 149
Script:
#!/bin/bash
# Let's check if there's an extension or import that provides firstWhereIndexedOrNull
rg -l "firstWhereIndexedOrNull" lib/src/extensions/decode.dart -C 10
# Also search for any potential extension methods or imports
rg "extension.*on List"
Length of output: 200
Script:
#!/bin/bash
# Let's check the ListExtension implementation to see if it contains firstWhereIndexedOrNull
cat lib/src/extensions/extensions.dart
# Also check the imports in decode.dart
rg "^import" lib/src/extensions/decode.dart
Length of output: 1282
Script:
#!/bin/bash
# Let's check if firstWhereIndexedOrNull is provided by a package
# First, let's locate the pubspec.yaml
fd "pubspec.yaml"
# Then read its contents to check dependencies
cat pubspec.yaml
Length of output: 677
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 143-145: lib/src/extensions/decode.dart#L143-L145
Added lines #L143 - L145 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
lib/src/extensions/decode.dart (2)
11-36: Consider improving error message consistency and reusabilityThe implementation looks solid, but the error messages are duplicated. Consider extracting the message construction to a separate method for better maintainability.
Here's a suggested improvement:
+ static String _buildListLimitErrorMessage(int limit) => + 'List limit exceeded. Only $limit element${limit == 1 ? '' : 's'} allowed in a list.'; static dynamic _parseListValue( dynamic val, DecodeOptions options, int currentListLength, ) { if (val is String && val.isNotEmpty && options.comma && val.contains(',')) { final List<String> splitVal = val.split(','); if (options.throwOnLimitExceeded && splitVal.length > options.listLimit) { - throw RangeError( - 'List limit exceeded. ' - 'Only ${options.listLimit} element${options.listLimit == 1 ? '' : 's'} allowed in a list.', - ); + throw RangeError(_buildListLimitErrorMessage(options.listLimit)); } return splitVal; } if (options.throwOnLimitExceeded && currentListLength >= options.listLimit) { - throw RangeError( - 'List limit exceeded. ' - 'Only ${options.listLimit} element${options.listLimit == 1 ? '' : 's'} allowed in a list.', - ); + throw RangeError(_buildListLimitErrorMessage(options.listLimit)); } return val; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-19: lib/src/extensions/decode.dart#L19
Added line #L19 was not covered by tests
[warning] 21-21: lib/src/extensions/decode.dart#L21
Added line #L21 was not covered by tests
49-67: Consider consolidating limit validation logicThe implementation correctly handles parameter limits, but the validation and error handling logic could be more centralised.
Consider extracting the limit validation into a separate method:
+ static void _validateLimit(int? limit, String type) { + if (limit != null && limit <= 0) { + throw ArgumentError('$type limit must be a positive integer.'); + } + } static Map<String, dynamic> _parseQueryStringValues( String str, [ DecodeOptions options = const DecodeOptions(), ]) { // ... final int? limit = options.parameterLimit == double.infinity ? null : options.parameterLimit.toInt(); - if (limit != null && limit <= 0) { - throw ArgumentError('Parameter limit must be a positive integer.'); - } + _validateLimit(limit, 'Parameter');🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-54: lib/src/extensions/decode.dart#L54
Added line #L54 was not covered by teststest/unit/decode_test.dart (1)
1783-1831: Consider adding test coverage for negative parameter limitsThe parameter limit test group provides good coverage of standard cases and edge cases. However, it would be beneficial to add test cases for negative parameter limits to ensure robust error handling.
Consider adding these test cases:
+ test('handles negative parameter limit correctly', () { + expect( + () => QS.decode( + 'a=1&b=2', + const DecodeOptions(parameterLimit: -1, throwOnLimitExceeded: true), + ), + throwsA(isA<RangeError>()), + ); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/src/extensions/decode.dart(4 hunks)test/unit/decode_test.dart(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
lib/src/extensions/decode.dart
[warning] 19-19: lib/src/extensions/decode.dart#L19
Added line #L19 was not covered by tests
[warning] 21-21: lib/src/extensions/decode.dart#L21
Added line #L21 was not covered by tests
[warning] 54-54: lib/src/extensions/decode.dart#L54
Added line #L54 was not covered by tests
[warning] 154-156: lib/src/extensions/decode.dart#L154-L156
Added lines #L154 - L156 were not covered by tests
🔇 Additional comments (2)
lib/src/extensions/decode.dart (1)
148-160: 🛠️ Refactor suggestion
Add documentation and improve test coverage
The currentListLength calculation logic is complex and lacks proper documentation. Additionally, the static analysis indicates missing test coverage for this section.
Let's verify the test coverage:
Consider adding documentation and tests:
+ /// Calculates the current length of a list at a specific position in the chain.
+ ///
+ /// If the chain ends with '[]' and contains a valid parent key index,
+ /// returns the length of the list at that index. Otherwise, returns 0.
+ ///
+ /// @param chain The chain of keys being processed
+ /// @param val The current value being processed
+ /// @returns The calculated list length
late final int currentListLength;
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;
}✅ Verification successful
Test coverage exists through comprehensive query string parsing tests
The currentListLength calculation is indirectly tested through numerous test cases that exercise array/list handling in query string parsing. The test files show extensive coverage of:
- Empty list handling:
foo[]&bar=baz - Nested list operations:
a[b][]=c&a[b][]=d - List index handling:
a[0][]=b - Mixed list and object operations:
foo[bad]=baz&foo[]=bar
The implementation is well-tested through these integration tests, though they could be made more explicit by adding specific test cases for the currentListLength calculation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the currentListLength calculation logic
rg -l "currentListLength.*=.*parentKey" test/
Length of output: 47
Script:
#!/bin/bash
# Let's check for any test files related to decode.dart
fd "decode.*test\.dart$" test/
# Then search for any test cases involving array/list operations
rg "\[\]|List<|elementAt|firstWhereIndexedOrNull" test/ -A 5 -B 5
Length of output: 100512
Script:
#!/bin/bash
# Let's check for test coverage of the currentListLength calculation logic
rg -A 5 -B 5 "currentListLength.*=.*parentKey" lib/
Length of output: 784
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 154-156: lib/src/extensions/decode.dart#L154-L156
Added lines #L154 - L156 were not covered by tests
test/unit/decode_test.dart (1)
415-415: LGTM: Test cases correctly validate list limit behaviour
The modifications to the expected outputs properly verify that sparse lists exceeding the limit are converted to maps, which is the expected behaviour.
Also applies to: 832-832
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lib/src/utils.dart (1)
45-52: Consider refactoring for better readabilityThe nested ternary operators make the code harder to follow. Consider extracting the logic into a separate method or using if-else blocks for better clarity.
- target = target_.values.any((el) => el is Undefined) - ? SplayTreeMap.from({ - for (final MapEntry<int, dynamic> entry in target_.entries) - if (entry.value is! Undefined) entry.key: entry.value, - }) - : target is Set - ? target_.values.toSet() - : target_.values.toList(); + final values = target_.values; + if (values.any((el) => el is Undefined)) { + target = SplayTreeMap.from({ + for (final entry in target_.entries) + if (entry.value is! Undefined) entry.key: entry.value, + }); + } else { + target = target is Set ? values.toSet() : values.toList(); + }test/unit/decode_test.dart (1)
1782-1899: LGTM! Comprehensive test coverage for parameter and list limits.The new test groups thoroughly cover the
parameterLimit,listLimit, andthrowOnLimitExceededoptions. The tests effectively verify:
- Success and error cases
- Silent truncation behaviour
- Edge cases (zero, negative, and infinite limits)
- Nested list scenarios
Consider adding a test case to verify the interaction between
parameterLimitandlistLimitwhen both are exceeded simultaneously:test('handles both parameter and list limits being exceeded', () { expect( () => QS.decode( 'a[]=1&a[]=2&a[]=3&b=4&c=5&d=6', const DecodeOptions( parameterLimit: 2, listLimit: 2, throwOnLimitExceeded: true, ), ), throwsA(isA<RangeError>()), ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/src/utils.dart(4 hunks)test/unit/decode_test.dart(4 hunks)
🔇 Additional comments (4)
lib/src/utils.dart (2)
76-80: LGTM! Clean implementation of generic type filtering
The replacement of whereNotUndefined() with whereNotType<Undefined>() is well-implemented and maintains proper collection type handling.
104-104: LGTM! Consistent implementation of type filtering
The changes consistently replace whereNotUndefined() with whereNotType<Undefined>() across all instances, improving code consistency.
Also applies to: 123-123, 127-127
test/unit/decode_test.dart (2)
415-415: LGTM! Fixed assertion to use integer keys.
The change correctly updates the expected output to use integer keys instead of string keys when transforming lists to maps.
468-478: LGTM! Good test coverage for nested lists.
The test case effectively verifies that nested lists maintain their structure when created with a parent key. This is a valuable addition to the test suite.
Description
This pull request introduces significant enhancements to the
qs_dartpackage, focusing on improving the handling of list and parameter limits, refining type handling, and adding new utility methods. The most important changes include the introduction of limit checks and error handling inDecodeOptions, updates to list and parameter parsing, and new utility methods in extensions.Handling of list and parameter limits:
lib/src/extensions/decode.dart: Added_parseListValuemethod to handle list parsing with limit checks and error handling. Updated_parseQueryStringValuesand related methods to include limit checks for parameters and lists. [1] [2] [3] [4]Refining type handling:
lib/src/extensions/extensions.dart: ReplacedwhereNotUndefinedwith a more genericwhereNotType<Q>method.lib/src/utils.dart: Updated methods to usewhereNotType<Undefined>instead ofwhereNotUndefined. [1] [2] [3] [4] [5]New utility methods:
lib/src/extensions/extensions.dart: Addedslicemethod toListExtensionfor extracting sections of a list, modeled after JavaScript'sArray.prototype.slice().Enhancements to
DecodeOptions:lib/src/models/decode_options.dart: IntroducedthrowOnLimitExceededoption toDecodeOptionsto control error throwing when limits are exceeded. [1] [2]Updates to tests:
test/unit/decode_test.dart: Added tests for new limit handling features and updated existing tests to reflect changes in list and parameter parsing behavior. [1] [2] [3] [4]test/unit/extensions/extensions_test.dart: Added tests for the newslicemethod and updated tests forwhereNotType. [1] [2]test/unit/uri_extension_test.dart: Updated tests to reflect changes in list parsing behavior. [1] [2]Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
--
Related to ljharb/qs#517