Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion docs/source/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ Private API
----------------------

.. autofunction:: flask_utils.decorators._is_optional
.. autofunction:: flask_utils.decorators._make_optional
.. autofunction:: flask_utils.decorators._is_allow_empty
.. autofunction:: flask_utils.decorators._check_type

Expand Down
2 changes: 1 addition & 1 deletion flask_utils/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Increment versions here according to SemVer
__version__ = "0.7.1"
__version__ = "1.0.0"

from flask_utils.utils import is_it_true
from flask_utils.errors import GoneError
Expand Down
181 changes: 99 additions & 82 deletions flask_utils/decorators.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import inspect
import warnings
from typing import Any
from typing import Dict
from typing import Type
from typing import Union
from typing import Callable
Expand All @@ -18,8 +19,6 @@

from flask_utils.errors import BadRequestError

VALIDATE_PARAMS_MAX_DEPTH = 4


def _handle_bad_request(
use_error_handlers: bool,
Expand Down Expand Up @@ -61,41 +60,13 @@ def _is_optional(type_hint: Type) -> bool: # type: ignore
return get_origin(type_hint) is Union and type(None) in get_args(type_hint)


def _make_optional(type_hint: Type) -> Type: # type: ignore
"""Wrap type hint with :data:`~typing.Optional` if it's not already.

:param type_hint: Type hint to wrap.
:type type_hint: Type

:return: Type hint wrapped with :data:`~typing.Optional`.
:rtype: Type

:Example:

.. code-block:: python

from typing import Optional
from flask_utils.decorators import _make_optional

_make_optional(str) # Optional[str]
_make_optional(Optional[str]) # Optional[str]

.. versionadded:: 0.2.0
"""
if not _is_optional(type_hint):
return Optional[type_hint] # type: ignore
return type_hint


def _is_allow_empty(value: Any, type_hint: Type, allow_empty: bool) -> bool: # type: ignore
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.
:type value: Any
:param type_hint: Type hint to check against.
:type type_hint: Type
:param allow_empty: Whether to allow empty values.
:type allow_empty: bool

:return: True if the value is empty and allowed, False otherwise.
:rtype: bool
Expand All @@ -117,22 +88,20 @@ def _is_allow_empty(value: Any, type_hint: Type, allow_empty: bool) -> bool: #

.. versionadded:: 0.2.0
"""
if value in [None, "", [], {}]:
# Check if type is explicitly Optional or allow_empty is True
if _is_optional(type_hint) or allow_empty:
if not value:
# Check if type is explicitly Optional
if _is_optional(type_hint):
return True
return False


def _check_type(value: Any, expected_type: Type, allow_empty: bool = False, curr_depth: int = 0) -> bool: # type: ignore
def _check_type(value: Any, expected_type: Type, curr_depth: int = 0) -> bool: # type: ignore
"""Check if the value matches the expected type, recursively if necessary.

:param value: Value to check.
:type value: Any
:param expected_type: Expected type.
:type expected_type: Type
:param allow_empty: Whether to allow empty values.
:type allow_empty: bool
:param curr_depth: Current depth of the recursive check.
:type curr_depth: int

Expand Down Expand Up @@ -169,10 +138,12 @@ def _check_type(value: Any, expected_type: Type, allow_empty: bool = False, curr

.. versionadded:: 0.2.0
"""
max_depth = current_app.config.get("VALIDATE_PARAMS_MAX_DEPTH", 4)

if curr_depth >= VALIDATE_PARAMS_MAX_DEPTH:
if curr_depth >= max_depth:
warnings.warn(f"Maximum depth of {max_depth} reached.", SyntaxWarning, stacklevel=2)
return True
if expected_type is Any or _is_allow_empty(value, expected_type, allow_empty): # type: ignore
if expected_type is Any or _is_allow_empty(value, expected_type): # type: ignore
return True

if isinstance(value, bool):
Expand All @@ -186,41 +157,30 @@ def _check_type(value: Any, expected_type: Type, allow_empty: bool = False, curr
args = get_args(expected_type)

if origin is Union:
return any(_check_type(value, arg, allow_empty, (curr_depth + 1)) for arg in args)
return any(_check_type(value, arg, (curr_depth + 1)) for arg in args)
elif origin is list:
return isinstance(value, list) and all(
_check_type(item, args[0], allow_empty, (curr_depth + 1)) for item in value
)
return isinstance(value, list) and all(_check_type(item, args[0], (curr_depth + 1)) for item in value)
elif origin is dict:
key_type, val_type = args
if not isinstance(value, dict):
return False
for k, v in value.items():
if not isinstance(k, key_type):
return False
if not _check_type(v, val_type, allow_empty, (curr_depth + 1)):
if not _check_type(v, val_type, (curr_depth + 1)):
return False
return True
else:
return isinstance(value, expected_type)


def validate_params(
parameters: Dict[Any, Any],
allow_empty: bool = False,
) -> Callable: # type: ignore
def validate_params() -> Callable: # type: ignore
"""
Decorator to validate request JSON body parameters.

This decorator ensures that the JSON body of a request matches the specified
parameter types and includes all required parameters.

:param parameters: Dictionary of parameters to validate. The keys are parameter names
and the values are the expected types.
:type parameters: Dict[Any, Any]
:param allow_empty: Allow empty values for parameters. Defaults to False.
:type allow_empty: bool

:raises BadRequestError: If the JSON body is malformed,
the Content-Type header is missing or incorrect, required parameters are missing,
or parameters are of the wrong type.
Expand All @@ -232,21 +192,13 @@ def validate_params(
from flask import Flask, request
from typing import List, Dict
from flask_utils.decorators import validate_params
from flask_utils.errors.badrequest import BadRequestError
from flask_utils.errors import BadRequestError

app = Flask(__name__)

@app.route("/example", methods=["POST"])
@validate_params(
{
"name": str,
"age": int,
"is_student": bool,
"courses": List[str],
"grades": Dict[str, int],
}
)
def example():
@validate_params()
def example(name: str, age: int, is_student: bool, courses: List[str], grades: Dict[str, int]):
\"""
This route expects a JSON body with the following:
- name: str
Expand All @@ -255,8 +207,8 @@ def example():
- courses: list of str
- grades: dict with str keys and int values
\"""
data = request.get_json()
return data
# Use the data in your route
...

.. tip::
You can use any of the following types:
Expand All @@ -270,6 +222,37 @@ def example():
* Optional
* Union

.. warning::
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
Comment on lines +226 to +227
Copy link

@JulienPalard JulienPalard Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

@Seluj78 Seluj78 Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🥰

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also probably support multiple ways of passing parameters to the function? 🤔

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

is issued when this occurs.

:Example:

.. code-block:: python

from flask import Flask, request
from typing import List, Dict
from flask_utils.decorators import validate_params
from flask_utils.errors import BadRequestError

app = Flask(__name__)

@app.route("/users/<int:user_id>", methods=["POST"])
@validate_params()
def create_user(user_id: int):
print(f"User ID: {user_id}")
return "User created"

...

requests.post("/users/123", json={"user_id": 456})
# Output: User ID: 456

.. versionchanged:: 1.0.0
The decorator doesn't take any parameters anymore,
it loads the types and parameters from the function signature as well as the Flask route's slug parameters.

.. versionchanged:: 0.7.0
The decorator will now use the custom error handlers if ``register_error_handlers`` has been set to ``True``
when initializing the :class:`~flask_utils.extension.FlaskUtils` extension.
Expand All @@ -296,33 +279,67 @@ def wrapper(*args, **kwargs): # type: ignore
"or the JSON body is missing.",
original_exception=e,
)

if not data:
return _handle_bad_request(use_error_handlers, "Missing json body.")

if not isinstance(data, dict):
return _handle_bad_request(use_error_handlers, "JSON body must be a dict")
return _handle_bad_request(
use_error_handlers,
"JSON body must be a dict",
original_exception=BadRequestError("JSON body must be a dict"),
)

for key, type_hint in parameters.items():
if not _is_optional(type_hint) and key not in data:
signature = inspect.signature(fn)
parameters = signature.parameters
# Extract the parameter names and annotations
expected_params = {}
for name, param in parameters.items():
if param.annotation != inspect.Parameter.empty:
expected_params[name] = param.annotation
else:
warnings.warn(f"Parameter {name} has no type annotation.", SyntaxWarning, stacklevel=2)
expected_params[name] = Any
Comment on lines +297 to +298
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here @JulienPalard, not sure how to use the warnings module 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


request_data = request.view_args # Flask route parameters
for key in data:
if key in request_data:
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,
stacklevel=2,
)
Comment on lines +304 to +308
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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,
)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

request_data.update(data or {})

for key, type_hint in expected_params.items():
# TODO: Handle deeply nested types
if key not in request_data and not _is_optional(type_hint):
return _handle_bad_request(
use_error_handlers, f"Missing key: {key}", f"Expected keys are: {list(parameters.keys())}"
use_error_handlers, f"Missing key: {key}", f"Expected keys are: {list(expected_params.keys())}"
)

for key in data:
if key not in parameters:
for key in request_data:
if key not in expected_params:
return _handle_bad_request(
use_error_handlers, f"Unexpected key: {key}.", f"Expected keys are: {list(parameters.keys())}"
use_error_handlers,
f"Unexpected key: {key}.",
f"Expected keys are: {list(expected_params.keys())}",
)

for key in data:
if key in parameters and not _check_type(data[key], parameters[key], allow_empty):
for key, value in request_data.items():
if key in expected_params and not _check_type(value, expected_params[key]):
return _handle_bad_request(
use_error_handlers,
f"Wrong type for key {key}.",
f"It should be {getattr(parameters[key], '__name__', str(parameters[key]))}",
f"It should be {getattr(expected_params[key], '__name__', str(expected_params[key]))}",
)

provided_values = {}
for key in expected_params:
if not _is_optional(expected_params[key]):
provided_values[key] = request_data[key]
else:
provided_values[key] = request_data.get(key, None)

kwargs.update(provided_values)

return fn(*args, **kwargs)

return wrapper
Expand Down
22 changes: 22 additions & 0 deletions flask_utils/extension.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,26 @@ class FlaskUtils(object):
fu = FlaskUtils()
fu.init_app(app)

.. versionchanged:: 1.0.0
The :func:`~flask_utils.decorators.validate_params` decorator will now use the ``VALIDATE_PARAMS_MAX_DEPTH``
config variable to determine the maximum depth of the validation for dictionaries.

:Example:

.. code-block:: python

from flask import Flask
from flask_utils import FlaskUtils

app = Flask(__name__)
fu = FlaskUtils(app)
app.config["VALIDATE_PARAMS_MAX_DEPTH"] = 3

The `VALIDATE_PARAMS_MAX_DEPTH` configuration determines the maximum depth of nested dictionary validation
when using the `validate_params` decorator. This allows fine-tuning of validation behavior for
complex nested structures.


.. versionadded:: 0.5.0
"""

Expand Down Expand Up @@ -107,3 +127,5 @@ def init_app(self, app: Flask, register_error_handlers: bool = True) -> None:
self.has_error_handlers_registered = True

app.extensions["flask_utils"] = self
# Default depth of 4 allows for moderately nested structures while not slowing down too much the validation
app.config.setdefault("VALIDATE_PARAMS_MAX_DEPTH", 4)
Loading