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.

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

Attaching schema to exception data breaks out-of-the-box compatibility with flask-restful #496

Closed
reinhard-mueller opened this issue Apr 10, 2020 · 2 comments

Comments

@reinhard-mueller
Copy link

When combining webargs with flask-restful (probably a quite popular combination), it is currently not possible to use the standard error handlers on both sides, because

  • on the one hand, webargs includes the schema in the data instance variable of the raised exception, while
  • on the other hand, flask-restful tries to convert that data into JSON and fails because of exactly that schema component.
    Looking at the discussion at RFC: Remove fields attribute from ValidationError marshmallow#840, it seems like the schema was only included to make it possible for an error handler to access the location metadata of the fields - something that doesn't exist any more anyway. At the same time, the change with 6.0.0 that the location is automatically included in the "messages" structure seems to make the whole topic obsolete.

So I propose to depreciate accessing data["schema"] in error handlers and in the next major version remove it to restore out-of-the-box integration with flask-restful.

For a similar reason, I would propose to prepare removing data["headers"] and instead use a separate instance variable of the exceptions, so flask-restful doesn't by default include that in the JSON error output.

@sirosen
Copy link
Collaborator

sirosen commented Feb 2, 2021

Before we get to what changes would be necessary, I have to ask the question: is being compatible with flask-restful without any work by the user a goal for webargs?
I'm not convinced that it is a wise/good goal for us.

flask-restful defines its own request parsing framework. That doesn't rule compatibility out, but the fact that flask-restful has its own solution for the exact problem that webargs solves is important. It suggests that, especially with flask-restful at 0.x, out-of-the-box compatibility between the two is not something for which we should aim.
The existence of flask-smorest, a marshmallow-code project which tries to solve many of the same problems as flask-restful, further emphasizes the divide between these two projects.

I took a look to understand where and why our usage of flask.abort() in the default handler conflicts with whatever flask-restful is doing. It looks like they're loading any data attribute from an exception and then they pass that into response rendering. What this tells me is that flask-restful believes it knows what data is, and our error handler violates that assumption by happening to also use the name data.

So, sure, we could just switch away from using data as the attribute name. We could call it webargs_data. If we were talking about introducing a new feature, I'd be 100% onboard with that. There's no need to cause grief unnecessarily.
But at this point, the default error handler has a behavior and people may be relying on it. We'd be asking them to change their code in the next major version of webargs. Having pushed through several very major changes in the past couple of years, I'm sensitive to not causing users more pain than we absolutely have to.

For users who really do want to combine the two, we provide an easy path to customization:

import flask
from webargs.flaskparser import parser

# this is just one of many paths, but this form
# changes the behavior of the parser object provided by `webargs`
# so it's the most similar to changing the default
@parser.error_handler
def handle_error(error, req, schema, *, error_status_code, error_headers):
    status_code = error_status_code or parser.DEFAULT_VALIDATION_STATUS
    try:
        flask.abort(status_code)
    except HTTPException as e:
        e.webargs_data = {"schema": schema}  # whatever you like here
        e.data = {"messages": error.messages}  # whatever you want flask-restful to see here
        e.exc = error
        raise e

I can't get onboard with asking users who might be relying on the built-in handler to rewrite things without accepting that compatibility with flask-restful -- without asking users to write the above short handler -- is something that we want to provide.

Right now, I think that the cons outweigh the pros for trying to target that kind of compatibility.
I'm open to discussion on this, but I'd need to be convinced that it really is a good idea. The benefit of trying to be compatible would need to outweigh the cost, measured both in user-impact and in developer impact as we'd have to start testing for this.

@reinhard-mueller
Copy link
Author

I guess since Flask-RestFul is discontinued, this issue is obsolete.

@lafrech lafrech closed this as completed Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants