feat: Now using function parameters #46
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Assessment against linked issues
Possibly related PRs
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional comments not posted (4)
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
Documentation and Community
Tip Early access features: enabledWe are currently testing new code review model(s) that may lead to higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range comments (1)
flask_utils/decorators.py (1)
Line range hint
63-87: Update the docstring to reflect the updated function signature.The
_is_allow_emptyfunction no longer accepts theallow_emptyparameter, but the docstring and examples still reference it. Please update the docstring and examples to match the new function signature.Apply this diff to update the docstring:
def _is_allow_empty(value: Any, type_hint: Type) -> bool: # type: ignore """Determine if the value is considered empty and whether it's allowed. :param value: Value to check. :param type_hint: Type hint to check against. :return: True if the value is empty and allowed, False otherwise. :Example: .. code-block:: python from typing import Optional from flask_utils.decorators import _is_allow_empty - _is_allow_empty(None, str, False) # False - _is_allow_empty("", str, False) # False - _is_allow_empty(None, Optional[str], False) # True - _is_allow_empty("", Optional[str], False) # True - _is_allow_empty("", Optional[str], True) # True - _is_allow_empty("", str, True) # True - _is_allow_empty([], Optional[list], False) # True + _is_allow_empty(None, str) # False + _is_allow_empty("", str) # False + _is_allow_empty(None, Optional[str]) # True + _is_allow_empty("", Optional[str]) # True + _is_allow_empty([], Optional[list]) # True .. versionadded:: 0.2.0 """
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (5)
- docs/source/api.rst (0 hunks)
- flask_utils/init.py (1 hunks)
- flask_utils/decorators.py (10 hunks)
- tests/test_validate_params.py (12 hunks)
- tests/test_validate_params_with_error_handlers.py (0 hunks)
Files not reviewed due to no reviewable changes (2)
- docs/source/api.rst
- tests/test_validate_params_with_error_handlers.py
Additional comments not posted (11)
flask_utils/__init__.py (3)
2-2: Request for changelogFor a major version update like this, it's crucial to provide a changelog detailing the changes, especially any breaking changes or new features. If you haven't already, please add a CHANGELOG.md file or update the existing one with information about the changes in version 1.0.0.
To check for an existing changelog, run:
#!/bin/bash # Description: Check for the existence of a changelog file # Search for common changelog file names echo "Searching for changelog files:" fd -i 'change(s?|log)' -e md -e txt -e rst # If no changelog is found, remind to create one if [ $? -ne 0 ]; then echo "No changelog file found. Please create a CHANGELOG.md file." fi
2-2: Significant version bump to 1.0.0The version has been updated from 0.7.0 to 1.0.0, which is a significant change according to Semantic Versioning. This typically indicates that the library has reached a stable, production-ready state and/or includes breaking changes.
Could you please provide more details about the changes that warrant this major version increment? This information will be crucial for users of the library to understand potential impacts on their projects.
To ensure consistency across the project, please run the following script:
This script will help identify any inconsistencies in version numbers across the project.
2-2: Clarification needed on PR titleThe PR title mentions "Now using function parameters", but the changes in this file only reflect a version update. Could you please clarify where the changes related to function parameters are implemented? If they're in other files, it would be helpful to include those in the PR for a comprehensive review.
To search for changes related to function parameters, run:
This will help identify any changes to function signatures across the project.
tests/test_validate_params.py (7)
1-1: LGTM! Decorator usage updated and type annotations added.The changes to the
validate_paramsdecorator usage and the addition of type annotations in the function signature represent a shift in the validation approach. This new method seems more intuitive and aligns with Python's type hinting system.Also applies to: 18-19
61-62: LGTM! Improved type annotations in function signature.The updated
default_typesfunction now uses explicit type annotations for all parameters, which aligns with the new validation approach. This change improves code readability and provides clear expectations for the function parameters.
266-267: LGTM! Proper use of Union type.The updated
unionfunction now uses theUniontype from thetypingmodule, which is the recommended way to specify multiple allowed types. This change aligns with the new validation approach and improves code consistency.
289-290: LGTM! Proper use of Optional type. Query about commented test.The updated
optionalfunction now uses theOptionaltype from thetypingmodule, which correctly specifies that theageparameter can be of typeintorNone. This aligns with the new validation approach.There's a commented-out test case:
# response = client.post("/optional", json={"name": "John", "age": 25}) # assert response.status_code == 200Was this intentionally removed? If so, consider adding a comment explaining why, or remove it entirely if it's no longer needed.
Also applies to: 294-295
312-313: LGTM! Consistent application of type annotations.The changes in the
TestList,TestDict, andTestAnyclasses demonstrate a consistent application of the new validation approach. The use ofList[str],Dict[str, int], andAnytypes from thetypingmodule is correct and improves type checking and code readability.Also applies to: 332-333, 352-353
380-383: LGTM! Comprehensive demonstration of type annotations.The updated
mix_and_matchfunction signature showcases a comprehensive use of various type annotations, includingUnion,Optional,List,Dict, andAny. This demonstrates the flexibility and power of the new validation approach in handling complex parameter types.
Line range hint
1-567: Overall, excellent improvements to parameter validation and test coverage.The changes in this file represent a significant enhancement to the parameter validation approach:
- Consistent use of type annotations in function signatures, leveraging Python's type hinting system.
- Removal of validation logic from the decorator, simplifying its usage.
- Addition of comprehensive test cases covering various scenarios, including error handling, warnings, and edge cases.
These improvements will lead to more robust and maintainable code, with clearer expectations for function parameters and better error handling. The enhanced test suite will help catch potential issues early in the development process.
flask_utils/decorators.py (1)
1-2: Appropriate addition of required imports.The addition of
inspectandwarningsmodules is appropriate, as they are used for function signature introspection and issuing warnings within the code.
flask_utils/decorators.py
Outdated
| @@ -156,8 +134,9 @@ | |||
| """ | |||
|
|
|||
| if curr_depth >= VALIDATE_PARAMS_MAX_DEPTH: | |||
| warnings.warn(f"Maximum depth of {VALIDATE_PARAMS_MAX_DEPTH} reached.", SyntaxWarning, stacklevel=2) | |||
There was a problem hiding this comment.
Use a more suitable warning category for depth limit reached.
The warning issued when the maximum depth is reached uses SyntaxWarning. Since this is a runtime behavior concern, consider using RuntimeWarning to more accurately reflect the nature of the warning.
Apply this diff to change the warning category:
- warnings.warn(f"Maximum depth of {VALIDATE_PARAMS_MAX_DEPTH} reached.", SyntaxWarning, stacklevel=2)
+ warnings.warn(f"Maximum depth of {VALIDATE_PARAMS_MAX_DEPTH} reached.", RuntimeWarning, stacklevel=2)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| warnings.warn(f"Maximum depth of {VALIDATE_PARAMS_MAX_DEPTH} reached.", SyntaxWarning, stacklevel=2) | |
| warnings.warn(f"Maximum depth of {VALIDATE_PARAMS_MAX_DEPTH} reached.", RuntimeWarning, stacklevel=2) |
There was a problem hiding this comment.
I'm not sure about this, @JulienPalard what do you think should be raised as a warning here ? It's the first time I'm using the warnings module
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| f"Parameter {key} is defined in both the route and the JSON body. " | ||
| f"The JSON body will override the route parameter.", | ||
| SyntaxWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Use UserWarning for parameter override warnings.
When warning about parameters defined in both the route and the JSON body, SyntaxWarning is used. Since this situation pertains to user code and potential logic issues rather than syntax problems, consider using UserWarning instead.
Apply this diff:
warnings.warn(
f"Parameter {key} is defined in both the route and the JSON body. "
f"The JSON body will override the route parameter.",
- SyntaxWarning,
+ UserWarning,
stacklevel=2,
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| f"Parameter {key} is defined in both the route and the JSON body. " | |
| f"The JSON body will override the route parameter.", | |
| SyntaxWarning, | |
| stacklevel=2, | |
| ) | |
| f"Parameter {key} is defined in both the route and the JSON body. " | |
| f"The JSON body will override the route parameter.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| warnings.warn(f"Parameter {name} has no type annotation.", SyntaxWarning, stacklevel=2) | ||
| expected_params[name] = Any |
There was a problem hiding this comment.
Consider enforcing type annotations for parameters.
Parameters without type annotations are assigned Any, which may lead to unintended behavior during validation. It might be beneficial to enforce type annotations by raising an error or providing a clear message to the developer to add the missing annotations.
Alternatively, if retaining Any, consider updating the warning to a more appropriate category:
- warnings.warn(f"Parameter {name} has no type annotation.", SyntaxWarning, stacklevel=2)
+ warnings.warn(f"Parameter {name} has no type annotation.", UserWarning, stacklevel=2)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| warnings.warn(f"Parameter {name} has no type annotation.", SyntaxWarning, stacklevel=2) | |
| expected_params[name] = Any | |
| warnings.warn(f"Parameter {name} has no type annotation.", UserWarning, stacklevel=2) | |
| expected_params[name] = Any |
There was a problem hiding this comment.
Same thing here @JulienPalard, not sure how to use the warnings module 🤔
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range comments (1)
flask_utils/decorators.py (1)
Line range hint
63-88: Update the docstring of_is_allow_emptyto reflect the updated function signature.The parameter
allow_emptyhas been removed from_is_allow_empty, but the docstring still references it in the parameters and examples. Please update the docstring to prevent confusion.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (2)
- flask_utils/decorators.py (10 hunks)
- tests/test_validate_params.py (12 hunks)
Additional comments not posted (12)
tests/test_validate_params.py (12)
19-20: Use of explicit type annotations enhances readabilityThe
bad_formatfunction now uses explicit type annotations for its parameters, which improves code clarity and leverages Python's type checking capabilities.
244-245: Tuple type annotation is appropriate for testingThe use of a tuple
(str, int)in the type annotation of theunionfunction adequately tests the functionality for accepting multiple types.
267-268: Appropriate use ofUniontypeThe
unionfunction correctly uses theUniontype from thetypingmodule to accept either astror anintfor thenameparameter.
290-291: Correct implementation of optional parameterThe
optionalfunction properly usesOptional[int]to indicate that theageparameter is optional and can be of typeintorNone.
313-314: Proper usage ofListtype annotationThe
listfunction correctly usesList[str]to specify thatnameshould be a list of strings.
333-334: Correct usage ofDicttype annotationThe
dict_routefunction properly usesDict[str, int]to requirenameto be a dictionary with string keys and integer values.
353-354: Appropriate use ofAnytypeThe
any_routefunction usesAnyfor thenameparameter, allowing it to accept any type, which is suitable for testing the flexibility of the validator.
381-384: Comprehensive type annotations inmix_and_matchfunctionThe
mix_and_matchfunction effectively combines various type annotations, includingUnion,Optional,List,Dict, andAny, to test multiple scenarios.
507-540: Effective testing of annotation warningsThe
TestAnnotationWarningsclass correctly tests for warnings when parameters lack type annotations and when there are duplicate keys between route and JSON body parameters.
548-548: Deeply nested type annotations test max depth handlingThe
examplefunction inTestMaxDepthuses a deeply nestedDicttype to test the maximum depth warning functionality effectively.
574-577: Testing JSON body overriding route parametersThe
create_userfunction is designed to test the behavior when a parameter in the JSON body overrides a route parameter, which is important for understanding potential conflicts.
590-591: Correct handling of optional parameters withNonevaluesThe
empty_routefunction properly handles thenameparameter when it isNoneor an empty string, ensuring that optional parameters are managed correctly.
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range comments (1)
flask_utils/extension.py (1)
Implementation for function parameters is missing.
The introduction of
VALIDATE_PARAMS_MAX_DEPTHis properly implemented and documented. However, the primary objective outlined in the PR description—to modify thevalidate_paramsdecorator to work directly with function parameters instead of requiring a dictionary format—is not addressed in theflask_utils/decorators.pyfile.
- The
validate_paramsdecorator still validates JSON body parameters as per its current implementation.Analysis chain
Line range hint
1-95: Overall changes look good, but implementation for function parameters is missing.The introduction of
VALIDATE_PARAMS_MAX_DEPTHand its documentation are well-implemented. However, the main objective mentioned in the PR description - modifyingvalidate_paramsto work directly with function parameters instead of requiring a dictionary format - is not addressed in this file.To ensure we're not missing any relevant changes, let's check for modifications to the
validate_paramsdecorator:If the implementation is indeed missing, consider updating the PR description or adding a TODO comment to track this pending change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for changes to the validate_params decorator rg --type python -A 10 'def validate_params'Length of output: 77
Script:
#!/bin/bash # Search for changes to the validate_params decorator in Python files rg -g "*.py" 'def validate_params' -A 10Length of output: 780
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (3)
- flask_utils/decorators.py (9 hunks)
- flask_utils/extension.py (2 hunks)
- tests/test_validate_params.py (12 hunks)
Additional context used
Learnings (1)
tests/test_validate_params.py (1)
Learnt from: Seluj78 PR: Seluj78/flask-utils#46 File: tests/test_validate_params.py:244-245 Timestamp: 2024-09-22T14:16:35.584Z Learning: In tests that are specifically designed to test tuple type annotations (e.g., testing TupleUnion), use the tuple type notation (e.g., `(str, int)`) rather than `Union`.
Additional comments not posted (16)
flask_utils/decorators.py (5)
1-2: Appropriate Addition of ImportsThe inclusion of
import inspectandimport warningsis necessary for the new functionality introduced in the code, such as accessing function signatures and issuing warnings.
131-131: Dynamic Retrieval of Validation Depth ConfigurationRetrieving
max_depthfromcurrent_app.configallows the maximum validation depth to be configurable, enhancing the flexibility of the decorator for different application needs.
Line range hint
167-245: Updated Documentation Reflects Simplified Decorator UsageThe revised docstring for
validate_paramsaccurately describes the new behavior, emphasizing that the decorator now automatically reads parameter types from the function signature without requiring explicit arguments. This improves developer experience by simplifying usage.
279-289: Simplification by Extracting Parameters from Function SignatureBy extracting expected parameters directly from the function's signature, the code eliminates the need for manual parameter specification. This change reduces redundancy and potential errors, making the decorator more intuitive to use.
331-332: Ensuring Correct Argument Passing to Wrapped FunctionUpdating
kwargswithprovided_valuesensures that the validated parameters are correctly passed to the original function. This maintains the integrity of the original function's expected arguments after validation.tests/test_validate_params.py (11)
19-20: LGTM! Transition to explicit parameter annotations is correct.The function
bad_formatnow uses explicit type annotations, and the@validate_params()decorator is properly applied without parameters.
62-63: LGTM! Functiondefault_typesupdated with type annotations.The function parameters are now explicitly annotated with their respective types, enhancing clarity and validation.
244-245: Correct use of tuple type annotation for testingTupleUnion.Using the tuple
(str, int)for the parameternamein theunionfunction is appropriate for testingTupleUnionfunctionality as intended.
267-268: Functionunioncorrectly usesUniontype annotation.The parameter
nameis properly annotated withUnion[str, int], allowing it to accept either a string or an integer.
290-291: Functionoptionalcorrectly usesOptionaltype annotation.The parameter
ageis correctly annotated withOptional[int], indicating that it can be anintorNone.
333-334: Functiondict_routecorrectly usesDicttype annotation.The parameter
nameis appropriately annotated withDict[str, int], ensuring it accepts a dictionary with string keys and integer values.
353-354: Functionany_routecorrectly usesAnytype annotation.The parameter
nameis correctly annotated withAny, allowing acceptance of any data type for comprehensive testing.
381-384: Functionmix_and_matchcorrectly combines various type annotations.Parameters are properly annotated using
Union,Optional,List,Dict, andAny, effectively demonstrating mixed type validations.
512-513: Appropriate test for missing type annotations.The function
exampleintentionally leaves thenameparameter without a type annotation to test the warning mechanism, which is correctly handled in the test cases.
606-607: Functionempty_routecorrectly handles optional parameters.The parameter
nameis appropriately annotated withOptional[str], allowing it to be omitted or set toNone, which is correctly tested.
590-593: Potential issue with route parameter being overridden by JSON body.In the
create_userfunction, the route parameteruser_idcould be overridden by auser_idkey in the JSON body, which may lead to unexpected behavior or security concerns.Consider modifying the function to prevent the JSON body from overriding route parameters. One approach is to exclude
user_idfrom the JSON payload or rename the parameter in the JSON body.Apply this diff to adjust the function:
@flask_client.post("/users/<int:user_id>") @validate_params() -def create_user(user_id: int): +def create_user(user_id: int, data: dict): return f"{user_id}"And update the test to avoid passing
user_idin the JSON:def test_valid_request(self, client): - response = client.post("/users/123", json={"user_id": 456}) + response = client.post("/users/123", json={"name": "John"}) assert response.status_code == 200 - assert response.text == "456" + assert response.text == "123"Likely invalid or redundant comment.
Seluj78
force-pushed
the
feature/use-func-params
branch
from
f1049aa
to
d6f5efc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range comments (1)
flask_utils/__init__.py (1)
Documentation updates for
validate_paramsare incomplete.While
validate_paramsis referenced inREADME.md, its internal docstrings have not been updated to reflect recent changes. Please ensure that all docstrings and relevant documentation are revised to align with the major version update.Analysis chain
Line range hint
1-35: Verify documentation updates for major version change.Given the significant version increment and the changes mentioned in the PR objectives, it's crucial to ensure that the documentation has been updated to reflect the new usage of
validate_params.Please run the following script to check for documentation updates:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify documentation updates for validate_params # Test 1: Check for changes in README or documentation files echo "Checking for changes in README or documentation files:" rg --type md --type rst 'validate_params' # Test 2: Look for updates in docstrings of validate_params echo "Checking for updates in validate_params docstring:" ast-grep --lang python --pattern $'def validate_params($$$): """ $$$ """ $$$' # Test 3: Check for changes in example usage in documentation echo "Checking for changes in example usage in documentation:" rg --type md --type rst '@validate_params'Length of output: 857
Script:
#!/bin/bash # Description: Verify documentation updates for validate_params # Test 1: Check for mentions of 'validate_params' in README or documentation files echo "Checking for mentions of 'validate_params' in documentation files:" rg --type md --type rst 'validate_params' # Test 2: Look for updates in docstrings of validate_params with multiline support echo "Checking for updates in validate_params docstrings:" ast-grep --lang python --pattern $'def validate_params($$$): """ $$$ """ $$$' # Test 3: Check for example usage of validate_params in documentation with multiline mode echo "Checking for example usage of 'validate_params' in documentation:" rg --type md --type rst -U '@validate_params'Length of output: 861
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (6)
- docs/source/api.rst (0 hunks)
- flask_utils/init.py (1 hunks)
- flask_utils/decorators.py (9 hunks)
- flask_utils/extension.py (2 hunks)
- tests/test_validate_params.py (12 hunks)
- tests/test_validate_params_with_error_handlers.py (0 hunks)
Files not reviewed due to no reviewable changes (2)
- docs/source/api.rst
- tests/test_validate_params_with_error_handlers.py
Additional context used
Learnings (1)
tests/test_validate_params.py (1)
Learnt from: Seluj78 PR: Seluj78/flask-utils#46 File: tests/test_validate_params.py:244-245 Timestamp: 2024-09-22T14:16:35.584Z Learning: In tests that are specifically designed to test tuple type annotations (e.g., testing TupleUnion), use the tuple type notation (e.g., `(str, int)`) rather than `Union`.
Additional comments not posted (13)
flask_utils/__init__.py (1)
2-2: Version update approved, but verify major changes.The version update from 0.7.1 to 1.0.0 is consistent with Semantic Versioning for a major change. This aligns with the PR objectives to modify the
validate_paramsdecorator's functionality.To ensure the major version bump is justified, please run the following script to verify the implementation of the
validate_paramschanges:#!/bin/bash # Description: Verify changes to validate_params decorator # Test 1: Check for changes in the validate_params implementation echo "Checking for changes in validate_params implementation:" rg --type python -p 'def validate_params' -A 10 # Test 2: Look for usage examples of the new validate_params syntax echo "Checking for new usage examples of validate_params:" rg --type python '@validate_params\s*\n\s*def' # Test 3: Check for updates in docstrings or comments related to validate_params echo "Checking for updates in docstrings or comments:" rg --type python -i 'validate_params.*?("""|\'\'\')' -A 5flask_utils/extension.py (1)
Line range hint
1-125: LGTM! Changes align well with PR objectivesThe introduction of
VALIDATE_PARAMS_MAX_DEPTHconfiguration, its documentation, and default value setting are well-implemented and consistent with the PR objectives. These changes effectively address the goals outlined in issue #30, allowing for more flexible parameter validation depth control.The implementation looks correct, and the documentation provides clear guidance on how to use the new configuration. Good job on improving the
validate_paramsfunctionality!flask_utils/decorators.py (3)
1-2: Imports ofinspectandwarningsare appropriateThe added imports are necessary for utilizing
inspect.signatureand issuing warnings within the code.
200-201: Example correctly demonstrates the updatedvalidate_paramsusageThe example now illustrates how to use
@validate_params()without parameters, relying on function annotations for validation. This makes the decorator more intuitive and user-friendly.
225-228: Documentation effectively warns about parameter overridingIncluding a warning in the documentation about JSON body parameters overriding route parameters enhances clarity and helps prevent unexpected behavior for users.
tests/test_validate_params.py (8)
1-1: LGTM!The imports have been appropriately added to support the new tests and functionalities.
Also applies to: 9-10
19-20: LGTM!The
validate_paramsdecorator is correctly applied, and the function uses explicit type annotations.
244-245: LGTM!The function
unioncorrectly uses tuple type annotations for testing purposes.
267-268: LGTM!The use of
Uniontype annotation in theunionfunction is appropriate and enhances type flexibility.
290-291: LGTM!The
optionalfunction accurately utilizesOptionaltype annotation for theageparameter.
333-334: LGTM!The
dict_routefunction correctly employsDicttype annotation for thenameparameter.
353-354: LGTM!The
any_routefunction properly usesAnytype annotation, allowing for flexible input types.
381-384: LGTM!The
mix_and_matchfunction effectively combines multiple type annotations, showcasing comprehensive testing of type validations.
| If a parameter exists both in the route parameters and in the JSON body, | ||
| the value from the JSON body will override the route parameter. A warning |
There was a problem hiding this comment.
Ohhhh this is interesting, it is not what I would expect, and it even feels like a security issue to me.
I bet this can be exploited, at the very least least to hide malicious requests in the logs (appearing with the wrong id), like me doing:
curl -XGET 0:8000/users/12 -H "Content-Type: application/json" -d '{"user_id": 1}'
being me (user id 12) looking like I look at my profile but in fact receiving the profile of the admin. Yes I know mixing a GET and a request body is debatable (challenge: let's not debate it right now).
or
curl -XGET 0:8000/users/12 -H "Content-Type: application/json" -d '{"user_id": "' OR 1 = 1 -- "}'
being me doing SQL injections on the user id while still producing clean logs :D
I would prefer having the route segment win, for "consistency" between a request having no body and a request having one.
There's a way to have this fixed by design: have the body parameter being passed as an argument named body instead, I mean:
def create_user(user_id: int, body: TypedDict('User', {'user_id': int, "username": str}):
...this way one can do the (legitimate):
curl -XPUT 0:8000/users/12 -H "Content-Type: application/json" -d '{"user_id": 12, "username": "mdk"}'
(we would see this kind of request in the GET / modify locally / PUT way of altering documents).
and even have an assert user_id == body["user_id"] as they would not be mixed.
Ohhhh this mean that you could have:
class User(TypedDict):
user_id: int
username: str
def update_user(user_id: int, body: User):
...which is a tad longer to write, but probably way way more readable for big bodies (having a big class instead of a big list of arguments).
This would allow to reuse types too, I bet the User typeddict could be used in many places.
Another slightly different way would be to use **kwargs to provide the request body:
def update_user(user_id: int, **kwargs: Unpack[User]):
...but this have to be a design choice: using kwargs one cannot call a method with the same name in the body and the route, as it would lead to a call like update_user(12, user_id=42) (which would TypeError with got multiple values for argument 'user_id'. It would not reduce the API design choices, the developper would just have to rename the route argument to have it pass.
I have to cook now, I'm french, and I'll have to digest what I just wrote too, let's speak about it later.
There was a problem hiding this comment.
Oh wow, that's a lot of stuff to unpack (lol), thanks a lot for the suggestions, I'll think about it as well on my side and we can try and find a middleground later.
But from my first read of your answer, I do agree that it's a security risk, and that another way needs to be found ! And I love the idea of classes, it would make typing and reusing much easier ! Thought might feel like double the work when also using an ORM or PyDantic (which I've just started using) ?
There was a problem hiding this comment.
ORM
Beware, you cannot easily auto-generate those document types from an ORM, it would imply that modifying the DB modifies the API, maybe not what you want every time. You'd also need a kind of way to chose which DB fields to expose and which not to expose, which one to expose read-only, which one are "relations" that should automagically be converted to hyperlinks, and so on and so on, that's a whole other project.
There was a problem hiding this comment.
Yes of course !
My thought was rather on how similar it would look from openapi definitions ! That might be something I can do in the future 👀 when you use this extension, you can auto generate a swagger 🥰
There was a problem hiding this comment.
But anyway, I do like the idea of explicit parameters inside the function, that way you don't have to unpack a body variable, which comes back to body = request.get_json()
There was a problem hiding this comment.
I could also probably support multiple ways of passing parameters to the function? 🤔
There was a problem hiding this comment.
There was a problem hiding this comment.
Hehe, if it already exists, better not doing it again: solving a problem with 0 lines of code :D
I'm really not a flask user, and probably almost never used pydantic, so I'll refrain to compare your solution and Flask-Pydantic, I hope you have a better overview than mine here.
I do appreciate that they properly name body and query parameters though.
/me angrily looks at PHP about it calling the query string GET and the request body POST.
@Mews @JulienPalard I would love your input/tests on this !
Fixes #30
📚 Documentation preview 📚: https://flask-utils--46.org.readthedocs.build/en/46/
Summary by CodeRabbit
New Features
flask_utilsmodule to version 1.0.0, introducing enhanced parameter validation with automatic type extraction from function signatures.VALIDATE_PARAMS_MAX_DEPTH, to manage validation depth.Bug Fixes
Documentation
Tests
validate_paramsdecorator and added new tests for error handling and validation depth.