pallets / flask

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

Handling HTTP 405 on the blueprint level #1494

Closed jacobsvante closed 7 years ago

jacobsvante commented 9 years ago

I really like being able to override the error templates for each blueprint using their errorhandler methods. All error codes work great for me. Except one. 405's are AFAIK not catchable by the blueprint. This is a bit unfortunate when you have a JSON REST API where you want your consumers to be able to parse the data that they get back.

This line in werkzeug.routing is where the exception is raised. I feel a bit lost about the whole flow of a request in werkzeug/flask and don't really have any idea what I could do to be able to catch it in the Blueprint. Is this a bug? A known "feature"? Or am I doing something wrong?

Here's a test case which shows this behavior:

import unittest
from flask import Flask, Blueprint, request

app = Flask(__name__)
myblueprint = Blueprint('myblueprint', __name__)

@myblueprint.route('/', methods=['GET'])
def hello():
    return 'hello world!'

myblueprint.errorhandler(405)(lambda e: ('myblueprint 405', 405))
app.register_blueprint(myblueprint)
app.errorhandler(405)(lambda e: ('app 405', 405))

class BlueprintOrAppTestCase(unittest.TestCase):

    def setUp(self):
        self.client = app.test_client()

    def test_200(self):
        resp = self.client.get('/')
        self.assertEqual(resp.status_code, 200)

    def test_405(self):
        with app.test_client() as client:
            resp = client.post('/?http405')
            self.assertEqual(resp.status_code, 405)
            self.assertEqual(resp.get_data(True), 'myblueprint 405')
            self.assertEqual(request.blueprint, 'myblueprint')

if __name__ == '__main__':
    # app.run(use_reloader=True)
    unittest.main()

Running this gives:

.F
======================================================================
FAIL: test_405 (__main__.BlueprintOrAppTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test.py", line 30, in test_405
    self.assertEqual(resp.get_data(True), 'myblueprint 405')
AssertionError: 'app 405' != 'myblueprint 405'
- app 405
+ myblueprint 405

----------------------------------------------------------------------
Ran 2 tests in 0.013s

FAILED (failures=1)

A version which tests more error codes can be found in this gist.

untitaker commented 9 years ago

It certainly seems like a rough edge, similar to #1498

jacobsvante commented 9 years ago

Kind of, but at least there we know of a workaround.

svieira commented 9 years ago

@jmagnusson - the way to work around this (just like in #1498) is to compose distinct Flask applications, rather than trying to make a blueprint handle 405s.

@untitaker - I think one way to help people for whom this is an issue (for 1.x or 1.0 if that ship hasn't sailed yet) would be to add a new registration method to flask.Flask that does app composition from another app or a blueprint. Something like app.mount_app(bp_or_app, url_prefix=None, subdomain=None, **config_args) Doing something like this could also help with issues like #1507 as well.

jacobsvante commented 9 years ago

@svieira Interesting idea about using app composition. However, don't we miss out on features like linking between them with url_for?

svieira commented 9 years ago

You would need to add to the set of Flask.url_build_error_handlers so that the parent app (at least) could use url_for to route to the children. You could also update the children so the children could route to the parents, but that's probably not a good idea in the general case (too much magic to avoid the infinite recursion problem + the children shouldn't be aware of the parents if they really are stand-alone applications that just happen to be mounted together for convenience)

jacobsvante commented 9 years ago

Sounds somewhat messy. I kind of like the simplicity we have today where we just add Blueprints to our app to add more HTTP endpoints.

Couldn't there be some other way to make 405 catchable? An idea I have is that we make Flask create "fake routes" for each method in the url_map. This would make it work a bit like your workaround for 404s not being catched by the blueprints.

svieira commented 9 years ago

You're absolutely right, you could iterate over all the defined routes in a blueprint and add a route handler for the url + (all_possible_http_methods - route_accepted_methods). However, as it currently stands Flask does not make this easy. The routes in a Blueprint are stored as a closure that will just call BlueprintSetupState.add_url_rule when the Blueprint is registered on the application ... and the add_url_rule method of BlueprintSetupState basically just forwards on its arguments to Flask.add_url_rule.

So you are left with either: