pallets / flask

The Python micro framework for building web applications.
https://flask.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
67.59k stars 16.15k forks source link

Blueprint's errorhandler has no effect. #3572

Closed mikeckennedy closed 4 years ago

mikeckennedy commented 4 years ago

Summary

Trying to register a custom 404 (or any?) error handler using blueprints and the declaritive style seems to have no effect:

# Has no effect
@blueprint.errorhandler(404)
def ...

# Yet this works:
@app.errorhandler(404)
def ...

Of course, the blueprint wouldn't work if it wasn't registered, but I have registered it and yet, the custom error page isn't shown. Here are two files you can use to reproduce the lack of custom error handling:

# FILE: app.py
import flask
import views

app = flask.Flask(__name__)
app.register_blueprint(views.blueprint)
app.run()
# FILE: views.py
import flask
blueprint = flask.Blueprint('views', __name__, template_folder='templates')

@blueprint.route('/')
def index():
    return "Hello world, I might have errors!"

@blueprint.errorhandler(404)
def errors_404(e):
    return "I am a 404!", 404

Expected Behavior

We should see the response "I am a 404!" for http://127.0.0.1:5000/abc

Actual Behavior

We see Flask's standard error page:

Not Found

The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again.

Working if changed to app.errorhandler()

It seems like I have the setup correct (I hope I'm not wasting your time!) because with a slight move from blueprint.errorhandler() to app.errorhandler(), the expected behavior is achieved:

This one works

# FILE: app.py
import flask
import views

app = flask.Flask(__name__)
app.register_blueprint(views.blueprint)

# I just moved this from views.py and changed blueprint to app
# Of course, the error report is that this shouldn't be necessary.
@app.errorhandler(404)
def errors_404(e):
    return "I am a 404!", 404

app.run()
# FILE: views.py
import flask
blueprint = flask.Blueprint('views', __name__, template_folder='templates')

@blueprint.route('/')
def index():
    return "Hello world, I might have errors!"

Environment

ThiefMaster commented 4 years ago

I'm pretty sure there are existing issues for this. Implementing 404 (or 405 to some extent) on the blueprint level is simply impossible since a blueprint does not "own" any particular url subset, so how would you match a request not handled by any endpoint (which would belong to some blueprint) to a blueprint?

davidism commented 4 years ago

Yeah, unfortunately there's no way to handle these errors at the blueprint level, at least with the decorators. The routing has to happen first before it's possible to discover which blueprint owns the matched route, so if no route was matched there's no blueprint to handle the 404, just the app.

mikeckennedy commented 4 years ago

Hi @ThiefMaster I wouldn't be so quick to close this.

First, can you then explain to me why blueprints have an errorhandler decorator at all then?

Second, blueprints don't own URLs subsets. They probably should consistently use them. For example, probably a projects blueprint should have to do with things like /projects/* but there is nothing in the API I see that restricts the URLs specified to that URL subset.

Third, blueprints are a super way to organize all error handling into a single error_views.py file. This would handle ALL errors on the site.

The constraint: Just don't allow multiple error handlers for the same status code.

mikeckennedy commented 4 years ago

@davidism Thanks for the message too. Just saw that come in as I saved my response. See what you think about the above.

mikeckennedy commented 4 years ago

@davidism and @ThiefMaster What do you think about updating app.register_blueprints() to just see if there is a 404 or 500 blueprint.errorhandler() set and then just delegate that over to app.errorhandler().

That seems like a pretty clean way to handle it. Of course, erroring out if something has already set a 404/500 error handler previously.

Basically, stash the values of blueprint.errorhandler() and when calling register_blueprints() just call app.errorhandler() with the same values. Plus a duplicate check.

ThiefMaster commented 4 years ago

blueprints already have methods to register app-level error handlers (@bp.app_errorhandler() and bp.add_app_errorhandler() iirc.)

davidism commented 4 years ago

can you then explain to me why blueprints have an errorhandler decorator at all

Once a route is active, the blueprint is known and so raised exceptions can be routed to the most specific error handler. In all cases except 404 (and I think 500 in some cases?) this works as expected.

blueprints don't own URLs subsets. They probably should consistently use them.

We could potentially compare route prefixes, but for Flask to do it in a generic way would require special casing for the empty prefix, or for a prefix that's also a prefix of another blueprint's prefix.

The constraint: Just don't allow multiple error handlers for the same status code.

For the app and any given blueprint this already is the case, the handlers are stored in dicts.

mikeckennedy commented 4 years ago

Thanks guys. Ok, I see. The guidance is use @blueprint.app_errorhandler() Got it.

How about this? @blueprint.errorhandler(404) and @blueprint.errorhandler(500) raise some kind of unsupported exception saying, this will never work, so don't call it. Or have it just delegate over to @blueprint.app_errorhandler(404)?

ThiefMaster commented 4 years ago

it will work if you raise NotFound(...) while handling a request from your blueprint. for an API that's not too uncommon, e.g. in case the URL is valid but there's no object with the id provided in the path..

PS: 500 should work in any case, only errors that happen before routing finished don't have a blueprint that could handle them.

davidism commented 4 years ago

It would be nice to raise an error, or redirect them to app_errorhandler, but I don't think it's possible to do in a general way.

This is called out in the blueprint docs: https://flask.palletsprojects.com/en/1.1.x/blueprints/#error-handlers although it doesn't do itself any favors by using a 404 handler for the example. :man_facepalming:

davidism commented 4 years ago

Also doesn't help that there are three different pages with different information about error handlers. I'll create an issue about cleaning these docs up.

mikeckennedy commented 4 years ago

Thanks David! I agree that it was unclear. I looked up the docs, read them, and didn't see anything saying this wouldn't work. :)