Conversation
WalkthroughThe changes in this pull request enhance the functionality and error handling of 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 (
|
techouse
changed the title
raise_on_limit_exceeded option
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 #11 +/- ##
==========================================
- Coverage 96.77% 96.55% -0.22%
==========================================
Files 16 16
Lines 713 726 +13
==========================================
+ Hits 690 701 +11
- Misses 23 25 +2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![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
🧹 Outside diff range and nitpick comments (9)
src/qs_codec/models/decode_options.py (1)
86-87: Enhance the docstring to clarify limit relationships.While the docstring explains the basic functionality, it could be more specific about which limits trigger the error and how it interacts with both
list_limitandparameter_limit.Consider updating the docstring to:
- """Set to ``True`` to raise an error when the input contains more parameters than the ``list_limit``.""" + """Set to ``True`` to raise an error when parsing limits are exceeded. This affects: + + - The ``list_limit`` option: Raises when array indices exceed the limit + - The ``parameter_limit`` option: Raises when the number of parameters exceeds the limit"""src/qs_codec/utils/utils.py (1)
Line range hint
17-107: Consider improving maintainability of the merge functionThe merge function has grown quite complex with many nested conditions and type checks. Consider:
- Breaking down the logic into smaller, well-named helper methods
- Adding detailed documentation about the transformation rules
- Including examples in the docstring to illustrate different merge scenarios
Example structure:
@staticmethod def merge( target: t.Optional[t.Union[t.Mapping[str, t.Any], t.List[t.Any], t.Tuple]], source: t.Optional[t.Union[t.Mapping[str, t.Any], t.List[t.Any], t.Tuple, t.Any]], options: DecodeOptions = DecodeOptions(), ) -> t.Union[t.Dict[str, t.Any], t.List, t.Tuple, t.Any]: """Merge two objects together. Examples: >>> Utils.merge([1, Undefined, 3], [4, 5]) [1, 5, 3, 4] >>> Utils.merge({"a": 1}, [1, 2]) {"a": 1, "0": 1, "1": 2} """ if source is None: return target if not isinstance(source, t.Mapping): return Utils._merge_non_mapping_source(target, source, options) return Utils._merge_mapping_source(target, source, options) @staticmethod def _merge_non_mapping_source(target, source, options): # Extract the list/tuple handling logic here ... @staticmethod def _merge_mapping_source(target, source, options): # Extract the mapping handling logic here ...🧰 Tools
🪛 Ruff (0.7.0)
44-44: Multiple
isinstancecalls forel, merge into a single callMerge
isinstancecalls forel(SIM101)
src/qs_codec/decode.py (5)
54-57: Include actual list length in exception message for clarityIncluding the actual number of elements in the exception message provides better context for the error and aids in debugging.
Apply this diff:
- raise ValueError( - f"List limit exceeded: Only {options.list_limit} element{'' if options.list_limit == 1 else 's'} allowed in a list." - ) + raise ValueError( + f"List limit exceeded: Attempted to process {len(split_val)} elements, but only {options.list_limit} allowed." + )
60-63: Enhance exception message with current list lengthIncluding
current_list_lengthin the exception message helps users understand at which point the limit was exceeded.Apply this diff:
- if options.raise_on_limit_exceeded and current_list_length >= options.list_limit: - raise ValueError( - f"List limit exceeded: Only {options.list_limit} element{'' if options.list_limit == 1 else 's'} allowed in a list." - ) + if options.raise_on_limit_exceeded and current_list_length >= options.list_limit: + raise ValueError( + f"List limit exceeded: Current list length is {current_list_length}, but limit is {options.list_limit}." + )
82-82: Simplify condition by removing redundant checkSince
limitcannot be zero (validated earlier), theor not limitpart is unnecessary.Apply this diff:
- if (limit is None) or not limit + if limit is None
88-88: Simplify condition by removing redundant checkSimilarly, simplify the condition for clarity and consistency.
Apply this diff:
- if (limit is None) or not limit + if limit is None
155-168: Refactor index checking for efficiencyReplace the dictionary conversion with direct index checking to improve performance and readability.
Apply this diff:
- if parent_key is not None and isinstance(val, (list, tuple)) and parent_key in dict(enumerate(val)): + if parent_key is not None and isinstance(val, (list, tuple)) and 0 <= parent_key < len(val):tests/unit/decode_test.py (2)
703-706: Inconsistent Use of 'throwOnLimitExceeded' vs 'raise_on_limit_exceeded'The test method names refer to
throwOnLimitExceeded, but the actual option inDecodeOptionsisraise_on_limit_exceeded. For consistency and clarity, consider updating the method names to match the option name.Apply this diff to rename the methods:
-def test_silently_truncates_when_throwOnLimitExceeded_is_not_given(self) -> None: +def test_silently_truncates_when_raise_on_limit_exceeded_is_not_given(self) -> None: -def test_silently_truncates_when_parameter_limit_exceeded_without_error(self) -> None: +def test_silently_truncates_when_parameter_limit_exceeded_without_raising(self) -> None:Also applies to: 707-712
743-745: Ambiguous Handling of Negative 'list_limit' ValuesIn
test_handles_negative_list_limit_correctly, settinglist_limit=-1and expecting aValueErrormay not be the desired behavior. Negative list limits could lead to unexpected results or might need explicit handling in thedecodefunction. Please ensure that negativelist_limitvalues are appropriately handled, and update the test case if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/qs_codec/decode.py(4 hunks)src/qs_codec/models/decode_options.py(1 hunks)src/qs_codec/utils/utils.py(1 hunks)tests/unit/decode_test.py(3 hunks)tests/unit/example_test.py(1 hunks)
🔇 Additional comments (13)
src/qs_codec/models/decode_options.py (1)
81-81: LGTM! Improved terminology consistency.
The docstring updates improve consistency with Python conventions by using "raise an error" instead of "throw an error" and using "True" instead of "true".
Also applies to: 84-84
src/qs_codec/utils/utils.py (2)
38-41: Consider optimizing the undefined check and preserving indices.
The code has a few potential improvements:
-
The condition
any(isinstance(value, Undefined) for value in target_.values())repeats the check that was already done in the outer if condition, which could impact performance with large arrays. -
When converting to a list in the else branch, the original indices are lost which might be important for preserving array positions.
Consider this optimization:
- if any(isinstance(value, Undefined) for value in target_.values()):
- target = {str(i): target_[i] for i in target_ if not isinstance(target_[i], Undefined)}
- else:
- target = list(filter(lambda el: not isinstance(el, Undefined), target_.values()))
+ # Reuse the result of the first undefined check
+ filtered_items = [(i, v) for i, v in target_.items() if not isinstance(v, Undefined)]
+ if any(isinstance(el, Undefined) for el in target):
+ target = {str(i): v for i, v in filtered_items}
+ else:
+ # Preserve original indices by sorting
+ target = [v for _, v in sorted(filtered_items)]Additionally, let's verify the impact of converting integer keys to strings:
Line range hint 17-24: Verify integration with throwOnLimitExceeded option
The merge function accepts a DecodeOptions parameter but doesn't seem to utilize the new raise_on_limit_exceeded option. Consider whether the merge operation should respect list limits and potentially throw exceptions when enabled.
Let's check how the limit exceeded feature is implemented:
🧰 Tools
🪛 Ruff (0.7.0)
44-44: Multiple isinstance calls for el, merge into a single call
Merge isinstance calls for el
(SIM101)
tests/unit/example_test.py (1)
123-123: LGTM! Consider documenting this behavior change.
The updated test case correctly reflects the new behavior where sparse array indices are converted to dictionary keys instead of creating a sparse array. This change aligns with the PR objectives for better list limit handling.
Let's check if this behavior change is documented:
✅ Verification successful
The behavior change is already well documented
The documentation in both README.rst and docs/README.rst clearly explains that:
- When indices are too large, they become dictionary keys instead of list indices
- This behavior can be controlled via the
list_limitoption - The documentation includes examples that match the updated test case behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation about list limit handling in README or docs
fd -e md -e rst . | xargs rg -l "sparse.*list|list.*limit|array.*index"
Length of output: 100
Script:
#!/bin/bash
# Check content of README.rst and docs/README.rst for documentation about this behavior
rg -A 5 -B 5 "sparse.*list|list.*limit|array.*index" README.rst docs/README.rst
Length of output: 2949
src/qs_codec/decode.py (5)
51-51: Function signature updated appropriately
The addition of current_list_length to _parse_array_value enhances the function's ability to enforce list limits effectively.
75-77: Parameter limit validation is correctly implemented
The check ensures that the parameter_limit is a positive integer, preventing invalid configurations.
92-93: Parameter limit exceeded error handling is appropriate
The code correctly raises a ValueError when the number of parameters exceeds the set limit.
126-130: Update to _parse_array_value call ensures proper limit tracking
Passing current_list_length maintains accurate tracking of the list length during parsing.
168-168: Properly updating leaf with current_list_length
The modification ensures that the list limits are correctly enforced in recursive parsing.
tests/unit/decode_test.py (4)
181-181: Possible Logical Error in Test Assertion
In the test test_parses_a_mix_of_simple_and_explicit_lists, the assertion:
assert decode("a[1]=b&a=c", DecodeOptions(list_limit=20)) == {"a": {"1": "b", "2": "c"}}may not produce the expected result. Given the input "a[1]=b&a=c" and a list_limit of 20, the decoded output should likely be {"a": ["b", "c"]} to maintain consistency with list parsing behavior. Please verify if the expected output is correct.
739-741: Unexpected Behavior with 'list_limit=0'
In the test test_handles_list_limit_of_zero_correctly, with list_limit=0, the decoded output is {"a": ["1", "2"]}. This seems counterintuitive because a list_limit of zero might suggest that lists should not be parsed or should be treated differently. Please verify whether the decode function behaves as intended when list_limit=0.
691-722: Comprehensive Test Coverage for Parameter Limits
The new tests in TestParameterList effectively cover various scenarios for the parameter_limit option, including cases when the limit is exceeded and how the raise_on_limit_exceeded flag affects behavior. Excellent work ensuring robust test coverage.
724-748: Thorough Testing of List Limits and Nested Structures
The tests in TestListLimit provide detailed coverage of the list_limit functionality, including edge cases like zero and negative limits, and the application of limits to nested lists. This enhances the reliability of list parsing in the decode function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/unit/decode_test.py (1)
739-741: Consider enhancing zero limit test coverage.The test for zero limit could be more comprehensive by also verifying behavior when
raise_on_limit_exceeded=True.Consider adding:
def test_handles_list_limit_of_zero_correctly(self) -> None: assert decode("a[]=1&a[]=2", DecodeOptions(list_limit=0)) == {"a": ["1", "2"]} + # Verify behavior with raise_on_limit_exceeded=True + assert decode("a[]=1&a[]=2", DecodeOptions(list_limit=0, raise_on_limit_exceeded=True)) == {"a": ["1", "2"]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/unit/decode_test.py(3 hunks)
🔇 Additional comments (3)
tests/unit/decode_test.py (3)
181-183: LGTM: List limit assertions are well-structured.
The assertions correctly verify the behavior of the list_limit parameter with different values and scenarios.
691-722: LGTM: Comprehensive test coverage for parameter limits.
The test class provides excellent coverage of parameter limit functionality:
- Success and error cases
- Silent truncation behavior
- Edge cases like infinity limit
- Clear and descriptive test names
724-748: LGTM: Comprehensive test coverage for list limits.
The test class provides excellent coverage of list limit functionality:
- Success and error cases
- List to map conversion
- Edge cases (zero, negative)
- Nested list handling
1 participant
Description
This pull request introduces several changes to the
qs_codecpackage, focusing on enhancing the handling of list and parameter limits, improving error handling, and updating test cases accordingly.Enhancements to list and parameter limits:
src/qs_codec/decode.py: Modified_parse_array_valueand_parse_query_string_valuesto include checks for list and parameter limits, raising errors when limits are exceeded. [1] [2]src/qs_codec/models/decode_options.py: Addedraise_on_limit_exceededoption toDecodeOptionsto control error raising when limits are exceeded.Error handling improvements:
src/qs_codec/decode.py: Updated_parse_objectto handle current list length when parsing values, ensuring limits are respected.src/qs_codec/utils/utils.py: Enhancedmergeandcompactfunctions to handleUndefinedvalues more robustly. [1] [2]Test case updates:
tests/unit/decode_test.py: Added new test cases inTestParameterListandTestListLimitto validate the new limit checks and error handling.tests/unit/example_test.py: Adjusted existing tests to reflect changes in list handling, ensuring accurate results.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:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Undefinedvalues during merging and compacting operations.Tests
These updates aim to provide a more robust and user-friendly experience when working with data decoding.