Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make raise_400_for_marshmallow_errors raise again #317

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Make raise_400_for_marshmallow_errors raise again #317

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 13 additions & 5 deletions flask_rebar/utils/request_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,15 @@
"""
import collections
import copy
from typing import Any, Dict, Iterator, List, Optional, Type, Union, overload
from typing import Any
from typing import Dict
from typing import Iterator
from typing import List
from typing import NoReturn
from typing import Optional
from typing import Type
from typing import Union
from typing import overload

import marshmallow
from marshmallow import Schema
Expand Down Expand Up @@ -148,7 +156,7 @@ def normalize_schema(

def raise_400_for_marshmallow_errors(
errs: Dict[str, Any], msg: Union[str, messages.ErrorMessage]
) -> errors.BadRequest:
) -> NoReturn:
"""
Throws a 400 error properly formatted from the given marshmallow errors.

Expand All @@ -157,15 +165,15 @@ def raise_400_for_marshmallow_errors(
:raises: errors.BadRequest
"""
if not errs:
return errors.BadRequest(msg=msg)
raise errors.BadRequest(msg=msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no errors this function should just return. That is what it was previously doing before this bug was introduced.

Suggested change
raise errors.BadRequest(msg=msg)
return

Copy link
Contributor Author

@kaiku kaiku Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think keeping this change actually makes things better.

This function is designed around marshmallow.ValidationError. I think that if we handle a ValidationError and this function is called with the props of the validation error, it should raise a BadRequest. Intuitively, by naming, and practically, thinking about a web request, this makes sense.

Instead, we have a case where if errs is empty/falsy, we don't do anything. The only caller in flask-rebar is _get_data_or_400 that passes errs=e.messages_dict where e is a ValidationError. The marshmallow src will raise a TypeError if the underlying messages prop isn't a dict. In real world use of validating request payloads, I haven't seen this error, which tells me that it is practically speaking nearly or always set as a dict when raised as the result of compat.load.

I think this is a case where introducing a bit of breakage in service of correctness is OK. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sold, as long as we document in the release notes that this is a bugfix where previously errors were not raised, it's a small enough change I'm on board with making it as a correction.


copied = copy.deepcopy(errs)

_format_marshmallow_errors_for_response_in_place(copied)

additional_data = {"errors": copied}

return errors.BadRequest(msg=msg, additional_data=additional_data)
raise errors.BadRequest(msg=msg, additional_data=additional_data)


def get_json_body_params_or_400(schema: Schema) -> Dict[str, Any]:
Expand Down Expand Up @@ -219,7 +227,7 @@ def _get_data_or_400(
try:
return compat.load(schema=schema, data=data)
except marshmallow.ValidationError as e:
raise raise_400_for_marshmallow_errors(errs=e.messages_dict, msg=message)
raise_400_for_marshmallow_errors(errs=e.messages_dict, msg=message)


def _get_json_body_or_400() -> Union[List[Any], Dict[str, Any]]:
Expand Down
Loading