Make raise_400_for_marshmallow_errors raise again
#317
Conversation
Buried in plangrid#300 was a consequential change to `raise_400_for_marshmallow_errors` that changed its behavior from raising errors to returning them. This affects >= 3.2.0. This PR reverts this behavior since there seems to be no need for this change.
There was a problem hiding this comment.
Thanks for catching this!
While I strongly dislike this pattern of having the function raise an error instead of returning a BadRequest for the caller to raise, the change in #300 broke a public interface which we should definitely avoid outside a major version.
Needs one small change to return the behaviour to what it was prior to #300.
Would also be great to see a unit test validating raise_400_for_marshmallow_errors
| @@ -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) | |||
There was a problem hiding this comment.
If there's no errors this function should just return. That is what it was previously doing before this bug was introduced.
| raise errors.BadRequest(msg=msg) | |
| return |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Buried in #300 was a consequential change to
raise_400_for_marshmallow_errorsthat changed its behavior from raising errors to returning them.This affects >= 3.2.0.
This PR reverts this change as it seems unnecessary.