Refactor validate params #39
Conversation
WalkthroughThis enhancement simplifies parameter validation in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FlaskApp
participant Decorator
participant Function
Client->>FlaskApp: Send request with parameters
FlaskApp->>Decorator: Call validate_params
Decorator->>Function: Extract type hints from function signature
Function-->>Decorator: Return validation results
Decorator-->>FlaskApp: Pass results
FlaskApp-->>Client: Send response
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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 (2)
flask_utils/decorators.py (2)
Line range hint
92-94: Consider combining nestedifstatements into a single statement for clarity and efficiency.- if value in [None, "", [], {}]: - if _is_optional(type_hint) or allow_empty: - return True + if value in [None, "", [], {}] and (_is_optional(type_hint) or allow_empty): + return True
Line range hint
254-254: When re-raising exceptions, useraise ... from Noneto make clear that the re-raise is intentional and not due to a new error during handling.- raise BadRequestError("The Json Body is malformed.") + raise BadRequestError("The Json Body is malformed.") from None - raise BadRequestError( - "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." - ) + raise BadRequestError( + "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." + ) from NoneAlso applies to: 256-258
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- flask_utils/decorators.py (4 hunks)
- tests/test_validate_params.py (9 hunks)
Additional context used
Ruff
flask_utils/decorators.py
92-94: Use a single
ifstatement instead of nestedifstatements (SIM102)
248-248: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
254-254: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
256-258: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
Additional comments not posted (3)
flask_utils/decorators.py (2)
5-5: The import ofget_type_hintsis appropriate given the changes to utilize type hints directly from function signatures.
Line range hint
243-287: The refactoring of thevalidate_paramsdecorator to use type hints directly from the function signature simplifies the interface and leverages Python's built-in capabilities. Make sure the documentation is updated to reflect these changes.Tools
Ruff
248-248: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()tests/test_validate_params.py (1)
16-17: The updated usage of@validate_params()in all test methods aligns with the changes in the decorator. Ensure that all relevant test cases are updated or added to cover the new functionality comprehensively.Also applies to: 56-57, 234-240, 262-263, 285-286, 308-309, 328-329, 348-349, 376-379
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
flask_utils/decorators.py (3)
5-5: Add a brief description for theget_type_hintsimport.It's a good practice to comment on the purpose of newly added imports, especially when they play a crucial role in the functionality, as
get_type_hintsdoes here.
Line range hint
254-254: Improve exception handling by using theraise ... fromsyntax.- except BadRequest: - raise BadRequestError("The Json Body is malformed.") + except BadRequest as err: + raise BadRequestError("The Json Body is malformed.") from err - except UnsupportedMediaType: - raise BadRequestError( - "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." - ) + except UnsupportedMediaType as err: + raise BadRequestError( + "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." + ) from errExplicitly chaining exceptions with
fromprovides better context in the traceback, aiding in debugging and understanding the flow of exceptions.Also applies to: 256-258
Line range hint
18-18: Track the TODO item about error handling in the project management tools.This is an important consideration for future enhancements. Would you like me to open a GitHub issue to ensure this doesn't get overlooked?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- flask_utils/decorators.py (4 hunks)
Additional context used
Ruff
flask_utils/decorators.py
92-94: Use a single
ifstatement instead of nestedifstatements (SIM102)
254-254: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
256-258: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
Additional comments not posted (3)
flask_utils/decorators.py (3)
244-250: Efficient use of function type hints to determine parameter types.This approach leverages Python's type hints to automate parameter validation, reducing redundancy and potential errors from manually specifying types.
248-249: Remove the return type from the parameters dictionary effectively.This ensures that the return type hint does not interfere with parameter validation.
281-287: Handle optional parameters correctly by setting them toNoneif not present in the JSON data.This logic is essential for APIs that need to support optional parameters gracefully.
| @@ -173,7 +173,6 @@ def _check_type(value: Any, expected_type: Type, allow_empty: bool = False, curr | |||
|
|
|||
|
|
|||
| def validate_params( | |||
| parameters: Dict[Any, Any], | |||
| allow_empty: bool = False, | |||
There was a problem hiding this comment.
Do we still need allow_empty since we have the Optional types ? I use to use allow_empty for empty strings for example but I don't know how typing.Optional handles that
There was a problem hiding this comment.
Sorry I don't quite understand :P What does the allow_empty option do?
It means to count empty strings when validating as strings?
There was a problem hiding this comment.
Yeah, the idea was that sometimes I wanted to allow empty strings to be passed to a flask route :)
There was a problem hiding this comment.
I mean it should work the same as it did before :P
I didn't change the _check_type function, and the actual format of the data in paramaters hasn't changed because get_type_hints returns a dict in the same format as it was before.
There was a problem hiding this comment.
Okay no worries. I'm writing a bunch of tests anyway to see if what you did does what I want haha, so I'll let you know soon !
Mews
force-pushed
the
refactor-validate-params
branch
from
bb3e4a4
to
641737d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
flask_utils/decorators.py (2)
Line range hint
254-254: Useraise ... from Noneto clarify that these exceptions are not caused by previous errors.- except BadRequest: - raise BadRequestError("The Json Body is malformed.") + except BadRequest as e: + raise BadRequestError("The Json Body is malformed.") from None - except UnsupportedMediaType: - raise BadRequestError( - "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." - ) + except UnsupportedMediaType as e: + raise BadRequestError( + "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." + ) from NoneAlso applies to: 256-258
Line range hint
92-94: Consider combining nestedifstatements into a singleifstatement for clarity and efficiency.- if value in [None, "", [], {}]: - if _is_optional(type_hint) or allow_empty: - return True + if value in [None, "", [], {}] and (_is_optional(type_hint) or allow_empty): + return True
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- flask_utils/decorators.py (4 hunks)
- tests/test_validate_params.py (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_validate_params.py
Additional context used
Ruff
flask_utils/decorators.py
92-94: Use a single
ifstatement instead of nestedifstatements (SIM102)
248-248: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
254-254: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
256-258: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
Additional comments not posted (2)
flask_utils/decorators.py (2)
5-5: The addition ofget_type_hintsis consistent with the new approach of extracting type hints directly from the function. This change aligns well with the PR objectives.
248-248: Optimize the dictionary key check by removing.keys().- if "return" in parameters.keys(): + if "return" in parameters:Likely invalid or redundant comment.
Tools
Ruff
248-248: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
flask_utils/decorators.py (2)
Line range hint
254-254: Improve exception handling by using theraise ... from Nonesyntax to make clear that these are direct raises and not re-raises.- except BadRequest: - raise BadRequestError("The Json Body is malformed.") - except UnsupportedMediaType: - raise BadRequestError( - "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." - ) + except BadRequest as e: + raise BadRequestError("The Json Body is malformed.") from e + except UnsupportedMediaType as e: + raise BadRequestError( + "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." + ) from eAlso applies to: 256-258
Line range hint
92-94: Refactor nestedifstatements into a singleifstatement for clarity and efficiency.- if value in [None, "", [], {}]: - # Check if type is explicitly Optional or allow_empty is True - if _is_optional(type_hint) or allow_empty: - return True + if value in [None, "", [], {}] and (_is_optional(type_hint) or allow_empty): + return True
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- flask_utils/decorators.py (4 hunks)
Additional context used
Ruff
flask_utils/decorators.py
92-94: Use a single
ifstatement instead of nestedifstatements (SIM102)
254-254: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
256-258: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
Additional comments not posted (4)
flask_utils/decorators.py (4)
5-5: Updated import to useget_type_hintsaligns with the new approach of using type hints directly from the function.
244-250: Correctly extracts type hints from the function and handles the removal of the return type hint.
248-248: The optimization to check dictionary keys directly is already implemented as per previous review.
281-287: Handling of optional parameters and assignment of values tokwargsis clear and correctly implemented.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Seluj78
force-pushed
the
refactor-validate-params
branch
from
62bef1e
to
6a44010
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
flask_utils/decorators.py (1)
Line range hint
254-258: Improve exception handling by using thefromkeyword for clearer error context.- except BadRequest: - raise BadRequestError("The Json Body is malformed.") + except BadRequest as err: + raise BadRequestError("The Json Body is malformed.") from err - except UnsupportedMediaType: - raise BadRequestError( - "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." - ) + except UnsupportedMediaType as err: + raise BadRequestError( + "The Content-Type header is missing or is not set to application/json, or the JSON body is missing." + ) from err
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- flask_utils/decorators.py (4 hunks)
- tests/test_validate_params.py (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_validate_params.py
Additional context used
Ruff
flask_utils/decorators.py
92-94: Use a single
ifstatement instead of nestedifstatements (SIM102)
254-254: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
256-258: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
Additional comments not posted (3)
flask_utils/decorators.py (3)
5-5: LGTM! Usingget_type_hintsis a Pythonic way to leverage type hints for runtime validation.
244-244: Refactoring to use type hints from the function directly simplifies the interface and reduces redundancy.
248-249: Correctly removing the return type from parameter validation to avoid confusion.
|
Superseded by #46 |
Description
Changed the
validate_paramsdecorator to get the expected types for each argument from the decorated function's type hints rather than a parameters argument.The validated arguments are then passed to the decorated function from the json data, or passed as None if they're optional and not present in the json data.
I also changed the tests in
test_validate_paramsto use the new syntax.I'm currently skipping the tests in
TestTupleUnionuntil I understand what do to to them.Related Issue
Checklist
Type of change
Summary by CodeRabbit
New Features
Bug Fixes
validate_paramsdecorator to improve type validation in various scenarios.Tests