From eb36135cfe6a17350617e47b70b9ad383206eded Mon Sep 17 00:00:00 2001 From: Chris Hallacy Date: Mon, 2 May 2022 11:33:29 -0600 Subject: [PATCH 1/3] always warn on blueprint setupmethod after registration --- CHANGES.rst | 4 ++++ src/flask/blueprints.py | 8 ++++---- src/flask/scaffold.py | 4 +++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 91439096..d302347f 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -16,6 +16,10 @@ Unreleased - Refactor ``register_error_handler`` to consolidate error checking. Rewrite some error messages to be more consistent. :issue:`4559` +- Use Blueprint decorators and functions intended for setup after + registering the blueprint will show a warning. In the next version, + this will become an error just like the application setup methods. + :issue:`4571` Version 2.1.2 diff --git a/src/flask/blueprints.py b/src/flask/blueprints.py index c801d1b4..e4a9a5ce 100644 --- a/src/flask/blueprints.py +++ b/src/flask/blueprints.py @@ -162,7 +162,6 @@ class Blueprint(Scaffold): .. versionadded:: 0.7 """ - warn_on_modifications = False _got_registered_once = False #: Blueprint local JSON encoder class to use. Set to ``None`` to use @@ -209,7 +208,7 @@ class Blueprint(Scaffold): self._blueprints: t.List[t.Tuple["Blueprint", dict]] = [] def _is_setup_finished(self) -> bool: - return self.warn_on_modifications and self._got_registered_once + return self._got_registered_once def record(self, func: t.Callable) -> None: """Registers a function that is called when the blueprint is @@ -217,14 +216,15 @@ class Blueprint(Scaffold): state as argument as returned by the :meth:`make_setup_state` method. """ - if self._got_registered_once and self.warn_on_modifications: + if self._got_registered_once: + # TODO: Upgrade this to an error and unify it setupmethod in 2.3 from warnings import warn warn( Warning( "The blueprint was already registered once but is" " getting modified now. These changes will not show" - " up." + " up.\n This warning will be become an exception in 2.3." ) ) self.deferred_functions.append(func) diff --git a/src/flask/scaffold.py b/src/flask/scaffold.py index 10e77bbc..b2dd46f3 100644 --- a/src/flask/scaffold.py +++ b/src/flask/scaffold.py @@ -27,8 +27,8 @@ from .typing import URLDefaultCallable from .typing import URLValuePreprocessorCallable if t.TYPE_CHECKING: # pragma: no cover - from .wrappers import Response from .typing import ErrorHandlerCallable + from .wrappers import Response # a singleton sentinel value for parameter defaults _sentinel = object() @@ -411,6 +411,7 @@ class Scaffold: """ return self._method_route("PATCH", rule, options) + @setupmethod def route(self, rule: str, **options: t.Any) -> t.Callable[[F], F]: """Decorate a view function to register it with the given URL rule and options. Calls :meth:`add_url_rule`, which has more @@ -510,6 +511,7 @@ class Scaffold: """ raise NotImplementedError + @setupmethod def endpoint(self, endpoint: str) -> t.Callable: """Decorate a view function to register it for the given endpoint. Used if a rule is added without a ``view_func`` with From a406c297aafa28074d11ec6fd27c246c70418cb4 Mon Sep 17 00:00:00 2001 From: David Lord Date: Mon, 23 May 2022 08:49:30 -0700 Subject: [PATCH 2/3] apply setupmethod consistently --- src/flask/app.py | 13 ++++++++++-- src/flask/blueprints.py | 47 +++++++++++++++++++++++++++++------------ src/flask/scaffold.py | 23 +++++++------------- 3 files changed, 53 insertions(+), 30 deletions(-) diff --git a/src/flask/app.py b/src/flask/app.py index e7a07add..c29c2e87 100644 --- a/src/flask/app.py +++ b/src/flask/app.py @@ -541,8 +541,17 @@ class Flask(Scaffold): # the app's commands to another CLI tool. self.cli.name = self.name - def _is_setup_finished(self) -> bool: - return self.debug and self._got_first_request + def _check_setup_finished(self, f_name: str) -> None: + if self._got_first_request: + raise AssertionError( + f"The setup method '{f_name}' can no longer be called" + " on the application. It has already handled its first" + " request, any changes will not be applied" + " consistently.\n" + "Make sure all imports, decorators, functions, etc." + " needed to set up the application are done before" + " running it." + ) @locked_cached_property def name(self) -> str: # type: ignore diff --git a/src/flask/blueprints.py b/src/flask/blueprints.py index e4a9a5ce..a8856fe4 100644 --- a/src/flask/blueprints.py +++ b/src/flask/blueprints.py @@ -6,6 +6,7 @@ from functools import update_wrapper from .scaffold import _endpoint_from_view_func from .scaffold import _sentinel from .scaffold import Scaffold +from .scaffold import setupmethod from .typing import AfterRequestCallable from .typing import BeforeFirstRequestCallable from .typing import BeforeRequestCallable @@ -207,28 +208,33 @@ class Blueprint(Scaffold): self.cli_group = cli_group self._blueprints: t.List[t.Tuple["Blueprint", dict]] = [] - def _is_setup_finished(self) -> bool: - return self._got_registered_once + def _check_setup_finished(self, f_name: str) -> None: + if self._got_registered_once: + import warnings + warnings.warn( + f"The setup method '{f_name}' can no longer be called on" + f" the blueprint '{self.name}'. It has already been" + " registered at least once, any changes will not be" + " applied consistently.\n" + "Make sure all imports, decorators, functions, etc." + " needed to set up the blueprint are done before" + " registering it.\n" + "This warning will become an exception in Flask 2.3.", + UserWarning, + stacklevel=3, + ) + + @setupmethod def record(self, func: t.Callable) -> None: """Registers a function that is called when the blueprint is registered on the application. This function is called with the state as argument as returned by the :meth:`make_setup_state` method. """ - if self._got_registered_once: - # TODO: Upgrade this to an error and unify it setupmethod in 2.3 - from warnings import warn - - warn( - Warning( - "The blueprint was already registered once but is" - " getting modified now. These changes will not show" - " up.\n This warning will be become an exception in 2.3." - ) - ) self.deferred_functions.append(func) + @setupmethod def record_once(self, func: t.Callable) -> None: """Works like :meth:`record` but wraps the function in another function that will ensure the function is only called once. If the @@ -251,6 +257,7 @@ class Blueprint(Scaffold): """ return BlueprintSetupState(self, app, options, first_registration) + @setupmethod def register_blueprint(self, blueprint: "Blueprint", **options: t.Any) -> None: """Register a :class:`~flask.Blueprint` on this blueprint. Keyword arguments passed to this method will override the defaults set @@ -390,6 +397,7 @@ class Blueprint(Scaffold): bp_options["name_prefix"] = name blueprint.register(app, bp_options) + @setupmethod def add_url_rule( self, rule: str, @@ -417,6 +425,7 @@ class Blueprint(Scaffold): ) ) + @setupmethod def app_template_filter( self, name: t.Optional[str] = None ) -> t.Callable[[TemplateFilterCallable], TemplateFilterCallable]: @@ -433,6 +442,7 @@ class Blueprint(Scaffold): return decorator + @setupmethod def add_app_template_filter( self, f: TemplateFilterCallable, name: t.Optional[str] = None ) -> None: @@ -449,6 +459,7 @@ class Blueprint(Scaffold): self.record_once(register_template) + @setupmethod def app_template_test( self, name: t.Optional[str] = None ) -> t.Callable[[TemplateTestCallable], TemplateTestCallable]: @@ -467,6 +478,7 @@ class Blueprint(Scaffold): return decorator + @setupmethod def add_app_template_test( self, f: TemplateTestCallable, name: t.Optional[str] = None ) -> None: @@ -485,6 +497,7 @@ class Blueprint(Scaffold): self.record_once(register_template) + @setupmethod def app_template_global( self, name: t.Optional[str] = None ) -> t.Callable[[TemplateGlobalCallable], TemplateGlobalCallable]: @@ -503,6 +516,7 @@ class Blueprint(Scaffold): return decorator + @setupmethod def add_app_template_global( self, f: TemplateGlobalCallable, name: t.Optional[str] = None ) -> None: @@ -521,6 +535,7 @@ class Blueprint(Scaffold): self.record_once(register_template) + @setupmethod def before_app_request(self, f: BeforeRequestCallable) -> BeforeRequestCallable: """Like :meth:`Flask.before_request`. Such a function is executed before each request, even if outside of a blueprint. @@ -530,6 +545,7 @@ class Blueprint(Scaffold): ) return f + @setupmethod def before_app_first_request( self, f: BeforeFirstRequestCallable ) -> BeforeFirstRequestCallable: @@ -548,6 +564,7 @@ class Blueprint(Scaffold): ) return f + @setupmethod def teardown_app_request(self, f: TeardownCallable) -> TeardownCallable: """Like :meth:`Flask.teardown_request` but for a blueprint. Such a function is executed when tearing down each request, even if outside of @@ -558,6 +575,7 @@ class Blueprint(Scaffold): ) return f + @setupmethod def app_context_processor( self, f: TemplateContextProcessorCallable ) -> TemplateContextProcessorCallable: @@ -569,6 +587,7 @@ class Blueprint(Scaffold): ) return f + @setupmethod def app_errorhandler(self, code: t.Union[t.Type[Exception], int]) -> t.Callable: """Like :meth:`Flask.errorhandler` but for a blueprint. This handler is used for all requests, even if outside of the blueprint. @@ -580,6 +599,7 @@ class Blueprint(Scaffold): return decorator + @setupmethod def app_url_value_preprocessor( self, f: URLValuePreprocessorCallable ) -> URLValuePreprocessorCallable: @@ -589,6 +609,7 @@ class Blueprint(Scaffold): ) return f + @setupmethod def app_url_defaults(self, f: URLDefaultCallable) -> URLDefaultCallable: """Same as :meth:`url_defaults` but application wide.""" self.record_once( diff --git a/src/flask/scaffold.py b/src/flask/scaffold.py index b2dd46f3..154426ca 100644 --- a/src/flask/scaffold.py +++ b/src/flask/scaffold.py @@ -37,22 +37,10 @@ F = t.TypeVar("F", bound=t.Callable[..., t.Any]) def setupmethod(f: F) -> F: - """Wraps a method so that it performs a check in debug mode if the - first request was already handled. - """ + f_name = f.__name__ def wrapper_func(self, *args: t.Any, **kwargs: t.Any) -> t.Any: - if self._is_setup_finished(): - raise AssertionError( - "A setup function was called after the first request " - "was handled. This usually indicates a bug in the" - " application where a module was not imported and" - " decorators or other functionality was called too" - " late.\nTo fix this make sure to import all your view" - " modules, database models, and everything related at a" - " central place before the application starts serving" - " requests." - ) + self._check_setup_finished(f_name) return f(self, *args, **kwargs) return t.cast(F, update_wrapper(wrapper_func, f)) @@ -239,7 +227,7 @@ class Scaffold: def __repr__(self) -> str: return f"<{type(self).__name__} {self.name!r}>" - def _is_setup_finished(self) -> bool: + def _check_setup_finished(self, f_name: str) -> None: raise NotImplementedError @property @@ -376,6 +364,7 @@ class Scaffold: return self.route(rule, methods=[method], **options) + @setupmethod def get(self, rule: str, **options: t.Any) -> t.Callable[[F], F]: """Shortcut for :meth:`route` with ``methods=["GET"]``. @@ -383,6 +372,7 @@ class Scaffold: """ return self._method_route("GET", rule, options) + @setupmethod def post(self, rule: str, **options: t.Any) -> t.Callable[[F], F]: """Shortcut for :meth:`route` with ``methods=["POST"]``. @@ -390,6 +380,7 @@ class Scaffold: """ return self._method_route("POST", rule, options) + @setupmethod def put(self, rule: str, **options: t.Any) -> t.Callable[[F], F]: """Shortcut for :meth:`route` with ``methods=["PUT"]``. @@ -397,6 +388,7 @@ class Scaffold: """ return self._method_route("PUT", rule, options) + @setupmethod def delete(self, rule: str, **options: t.Any) -> t.Callable[[F], F]: """Shortcut for :meth:`route` with ``methods=["DELETE"]``. @@ -404,6 +396,7 @@ class Scaffold: """ return self._method_route("DELETE", rule, options) + @setupmethod def patch(self, rule: str, **options: t.Any) -> t.Callable[[F], F]: """Shortcut for :meth:`route` with ``methods=["PATCH"]``. From e044b00047da9bf94e7eb88049a17fe1dcf78f4e Mon Sep 17 00:00:00 2001 From: David Lord Date: Mon, 23 May 2022 08:50:13 -0700 Subject: [PATCH 3/3] avoid triggering setupmethod late in tests --- tests/test_basic.py | 94 ++++++++++++++++++++------------------------- 1 file changed, 42 insertions(+), 52 deletions(-) diff --git a/tests/test_basic.py b/tests/test_basic.py index c4be6621..fa9f902d 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -329,6 +329,11 @@ def test_session_using_session_settings(app, client): flask.session["testing"] = 42 return "Hello World" + @app.route("/clear") + def clear(): + flask.session.pop("testing", None) + return "Goodbye World" + rv = client.get("/", "http://www.example.com:8080/test/") cookie = rv.headers["set-cookie"].lower() assert "domain=.example.com" in cookie @@ -337,11 +342,6 @@ def test_session_using_session_settings(app, client): assert "httponly" not in cookie assert "samesite" in cookie - @app.route("/clear") - def clear(): - flask.session.pop("testing", None) - return "Goodbye World" - rv = client.get("/clear", "http://www.example.com:8080/test/") cookie = rv.headers["set-cookie"].lower() assert "session=;" in cookie @@ -1031,7 +1031,14 @@ def test_errorhandler_precedence(app, client): assert rv.data == b"E2" -def test_trapping_of_bad_request_key_errors(app, client): +@pytest.mark.parametrize( + ("debug", "trap", "expect_key", "expect_abort"), + [(False, None, True, True), (True, None, False, True), (False, True, False, False)], +) +def test_trap_bad_request_key_error(app, client, debug, trap, expect_key, expect_abort): + app.config["DEBUG"] = debug + app.config["TRAP_BAD_REQUEST_ERRORS"] = trap + @app.route("/key") def fail(): flask.request.form["missing_key"] @@ -1040,26 +1047,23 @@ def test_trapping_of_bad_request_key_errors(app, client): def allow_abort(): flask.abort(400) - rv = client.get("/key") - assert rv.status_code == 400 - assert b"missing_key" not in rv.data - rv = client.get("/abort") - assert rv.status_code == 400 + if expect_key: + rv = client.get("/key") + assert rv.status_code == 400 + assert b"missing_key" not in rv.data + else: + with pytest.raises(KeyError) as exc_info: + client.get("/key") - app.debug = True - with pytest.raises(KeyError) as e: - client.get("/key") - assert e.errisinstance(BadRequest) - assert "missing_key" in e.value.get_description() - rv = client.get("/abort") - assert rv.status_code == 400 + assert exc_info.errisinstance(BadRequest) + assert "missing_key" in exc_info.value.get_description() - app.debug = False - app.config["TRAP_BAD_REQUEST_ERRORS"] = True - with pytest.raises(KeyError): - client.get("/key") - with pytest.raises(BadRequest): - client.get("/abort") + if expect_abort: + rv = client.get("/abort") + assert rv.status_code == 400 + else: + with pytest.raises(BadRequest): + client.get("/abort") def test_trapping_of_all_http_exceptions(app, client): @@ -1661,7 +1665,7 @@ def test_nonascii_pathinfo(app, client): assert rv.data == b"Hello World!" -def test_debug_mode_complains_after_first_request(app, client): +def test_no_setup_after_first_request(app, client): app.debug = True @app.route("/") @@ -1671,19 +1675,10 @@ def test_debug_mode_complains_after_first_request(app, client): assert not app.got_first_request assert client.get("/").data == b"Awesome" - with pytest.raises(AssertionError) as e: + with pytest.raises(AssertionError) as exc_info: app.add_url_rule("/foo", endpoint="late") - assert "A setup function was called" in str(e.value) - - app.debug = False - - @app.route("/foo") - def working(): - return "Meh" - - assert client.get("/foo").data == b"Meh" - assert app.got_first_request + assert "setup method 'add_url_rule'" in str(exc_info.value) def test_before_first_request_functions(app, client): @@ -1720,28 +1715,23 @@ def test_before_first_request_functions_concurrent(app, client): def test_routing_redirect_debugging(monkeypatch, app, client): - @app.route("/foo/", methods=["GET", "POST"]) - def foo(): - return "success" + app.config["DEBUG"] = True - app.debug = False - rv = client.post("/foo", data={}, follow_redirects=True) + @app.route("/user/", methods=["GET", "POST"]) + def user(): + return flask.request.form["status"] + + # default redirect code preserves form data + rv = client.post("/user", data={"status": "success"}, follow_redirects=True) assert rv.data == b"success" - app.debug = True - - with client: - rv = client.post("/foo", data={}, follow_redirects=True) - assert rv.data == b"success" - rv = client.get("/foo", data={}, follow_redirects=True) - assert rv.data == b"success" - + # 301 and 302 raise error monkeypatch.setattr(RequestRedirect, "code", 301) - with client, pytest.raises(AssertionError) as e: - client.post("/foo", data={}) + with client, pytest.raises(AssertionError) as exc_info: + client.post("/user", data={"status": "error"}, follow_redirects=True) - assert "canonical URL 'http://localhost/foo/'" in str(e.value) + assert "canonical URL 'http://localhost/user/'" in str(exc_info.value) def test_route_decorator_custom_endpoint(app, client):