From abe062b371b971ab39c4e4e5f3f1a2f9e6d8d904 Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Sat, 18 Dec 2021 18:58:14 +0200 Subject: [PATCH] Remove app instance from Config for error handler setting (#2320) --- sanic/app.py | 8 +-- sanic/config.py | 73 ++++++++++++++++----------- sanic/errorpages.py | 1 + sanic/handlers.py | 105 ++++++++++++++++++++++++++++++++++----- tests/test_errorpages.py | 54 ++++++++++---------- 5 files changed, 164 insertions(+), 77 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 6a287471..0d1238e3 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -197,9 +197,7 @@ class Sanic(BaseSanic, metaclass=TouchUpMeta): self._state: ApplicationState = ApplicationState(app=self) self.blueprints: Dict[str, Blueprint] = {} self.config: Config = config or Config( - load_env=load_env, - env_prefix=env_prefix, - app=self, + load_env=load_env, env_prefix=env_prefix ) self.configure_logging: bool = configure_logging self.ctx: Any = ctx or SimpleNamespace() @@ -1699,9 +1697,7 @@ class Sanic(BaseSanic, metaclass=TouchUpMeta): self._future_registry.clear() self.signalize() self.finalize() - ErrorHandler.finalize( - self.error_handler, fallback=self.config.FALLBACK_ERROR_FORMAT - ) + ErrorHandler.finalize(self.error_handler, config=self.config) TouchUp.run(self) self.state.is_started = True diff --git a/sanic/config.py b/sanic/config.py index 261f608a..6994e605 100644 --- a/sanic/config.py +++ b/sanic/config.py @@ -1,28 +1,26 @@ from __future__ import annotations -from inspect import isclass +from inspect import getmembers, isclass, isdatadescriptor from os import environ from pathlib import Path -from typing import TYPE_CHECKING, Any, Dict, Optional, Union +from typing import Any, Dict, Optional, Union from warnings import warn -from sanic.errorpages import check_error_format +from sanic.errorpages import DEFAULT_FORMAT, check_error_format +from sanic.helpers import _default from sanic.http import Http +from sanic.log import error_logger from sanic.utils import load_module_from_file_location, str_to_bool -if TYPE_CHECKING: # no cov - from sanic import Sanic - - SANIC_PREFIX = "SANIC_" DEFAULT_CONFIG = { + "_FALLBACK_ERROR_FORMAT": _default, "ACCESS_LOG": True, "AUTO_RELOAD": False, "EVENT_AUTOREGISTER": False, - "FALLBACK_ERROR_FORMAT": "auto", "FORWARDED_FOR_HEADER": "X-Forwarded-For", "FORWARDED_SECRET": None, "GRACEFUL_SHUTDOWN_TIMEOUT": 15.0, # 15 sec @@ -46,11 +44,19 @@ DEFAULT_CONFIG = { } -class Config(dict): +class DescriptorMeta(type): + def __init__(cls, *_): + cls.__setters__ = {name for name, _ in getmembers(cls, cls._is_setter)} + + @staticmethod + def _is_setter(member: object): + return isdatadescriptor(member) and hasattr(member, "setter") + + +class Config(dict, metaclass=DescriptorMeta): ACCESS_LOG: bool AUTO_RELOAD: bool EVENT_AUTOREGISTER: bool - FALLBACK_ERROR_FORMAT: str FORWARDED_FOR_HEADER: str FORWARDED_SECRET: Optional[str] GRACEFUL_SHUTDOWN_TIMEOUT: float @@ -79,13 +85,10 @@ class Config(dict): load_env: Optional[Union[bool, str]] = True, env_prefix: Optional[str] = SANIC_PREFIX, keep_alive: Optional[bool] = None, - *, - app: Optional[Sanic] = None, ): defaults = defaults or {} super().__init__({**DEFAULT_CONFIG, **defaults}) - self._app = app self._LOGO = "" if keep_alive is not None: @@ -117,6 +120,13 @@ class Config(dict): raise AttributeError(f"Config has no '{ke.args[0]}'") def __setattr__(self, attr, value) -> None: + if attr in self.__class__.__setters__: + try: + super().__setattr__(attr, value) + except AttributeError: + ... + else: + return None self.update({attr: value}) def __setitem__(self, attr, value) -> None: @@ -136,16 +146,6 @@ class Config(dict): "REQUEST_MAX_SIZE", ): self._configure_header_size() - elif attr == "FALLBACK_ERROR_FORMAT": - self._check_error_format() - if self.app and value != self.app.error_handler.fallback: - if self.app.error_handler.fallback != "auto": - warn( - "Overriding non-default ErrorHandler fallback " - "value. Changing from " - f"{self.app.error_handler.fallback} to {value}." - ) - self.app.error_handler.fallback = value elif attr == "LOGO": self._LOGO = value warn( @@ -154,14 +154,29 @@ class Config(dict): DeprecationWarning, ) - @property - def app(self): - return self._app - @property def LOGO(self): return self._LOGO + @property + def FALLBACK_ERROR_FORMAT(self) -> str: + if self._FALLBACK_ERROR_FORMAT is _default: + return DEFAULT_FORMAT + return self._FALLBACK_ERROR_FORMAT + + @FALLBACK_ERROR_FORMAT.setter + def FALLBACK_ERROR_FORMAT(self, value): + self._check_error_format(value) + if ( + self._FALLBACK_ERROR_FORMAT is not _default + and value != self._FALLBACK_ERROR_FORMAT + ): + error_logger.warning( + "Setting config.FALLBACK_ERROR_FORMAT on an already " + "configured value may have unintended consequences." + ) + self._FALLBACK_ERROR_FORMAT = value + def _configure_header_size(self): Http.set_header_max_size( self.REQUEST_MAX_HEADER_SIZE, @@ -169,8 +184,8 @@ class Config(dict): self.REQUEST_MAX_SIZE, ) - def _check_error_format(self): - check_error_format(self.FALLBACK_ERROR_FORMAT) + def _check_error_format(self, format: Optional[str] = None): + check_error_format(format or self.FALLBACK_ERROR_FORMAT) def load_environment_vars(self, prefix=SANIC_PREFIX): """ diff --git a/sanic/errorpages.py b/sanic/errorpages.py index 66ff6c95..06495b0a 100644 --- a/sanic/errorpages.py +++ b/sanic/errorpages.py @@ -34,6 +34,7 @@ except ImportError: # noqa from json import dumps +DEFAULT_FORMAT = "auto" FALLBACK_TEXT = ( "The server encountered an internal error and " "cannot complete your request." diff --git a/sanic/handlers.py b/sanic/handlers.py index 8c543c6d..d564663d 100644 --- a/sanic/handlers.py +++ b/sanic/handlers.py @@ -1,13 +1,23 @@ +from __future__ import annotations + from inspect import signature -from typing import Dict, List, Optional, Tuple, Type +from typing import Dict, List, Optional, Tuple, Type, Union from warnings import warn -from sanic.errorpages import BaseRenderer, HTMLRenderer, exception_response +from sanic.config import Config +from sanic.errorpages import ( + DEFAULT_FORMAT, + BaseRenderer, + HTMLRenderer, + exception_response, +) from sanic.exceptions import ( ContentRangeError, HeaderNotFound, InvalidRangeType, + SanicException, ) +from sanic.helpers import Default, _default from sanic.log import error_logger from sanic.models.handler_types import RouteHandler from sanic.response import text @@ -28,24 +38,91 @@ class ErrorHandler: # Beginning in v22.3, the base renderer will be TextRenderer def __init__( - self, fallback: str = "auto", base: Type[BaseRenderer] = HTMLRenderer + self, + fallback: Union[str, Default] = _default, + base: Type[BaseRenderer] = HTMLRenderer, ): self.handlers: List[Tuple[Type[BaseException], RouteHandler]] = [] self.cached_handlers: Dict[ Tuple[Type[BaseException], Optional[str]], Optional[RouteHandler] ] = {} self.debug = False - self.fallback = fallback + self._fallback = fallback self.base = base + if fallback is not _default: + self._warn_fallback_deprecation() + + @property + def fallback(self): + # This is for backwards compat and can be removed in v22.6 + if self._fallback is _default: + return DEFAULT_FORMAT + return self._fallback + + @fallback.setter + def fallback(self, value: str): + self._warn_fallback_deprecation() + if not isinstance(value, str): + raise SanicException( + f"Cannot set error handler fallback to: value={value}" + ) + self._fallback = value + + @staticmethod + def _warn_fallback_deprecation(): + warn( + "Setting the ErrorHandler fallback value directly is " + "deprecated and no longer supported. This feature will " + "be removed in v22.6. Instead, use " + "app.config.FALLBACK_ERROR_FORMAT.", + DeprecationWarning, + ) + @classmethod - def finalize(cls, error_handler, fallback: Optional[str] = None): - if ( - fallback - and fallback != "auto" - and error_handler.fallback == "auto" - ): - error_handler.fallback = fallback + def _get_fallback_value(cls, error_handler: ErrorHandler, config: Config): + if error_handler._fallback is not _default: + if config._FALLBACK_ERROR_FORMAT is _default: + return error_handler.fallback + + error_logger.warning( + "Conflicting error fallback values were found in the " + "error handler and in the app.config while handling an " + "exception. Using the value from app.config." + ) + return config.FALLBACK_ERROR_FORMAT + + @classmethod + def finalize( + cls, + error_handler: ErrorHandler, + fallback: Optional[str] = None, + config: Optional[Config] = None, + ): + if fallback: + warn( + "Setting the ErrorHandler fallback value via finalize() " + "is deprecated and no longer supported. This feature will " + "be removed in v22.6. Instead, use " + "app.config.FALLBACK_ERROR_FORMAT.", + DeprecationWarning, + ) + + if config is None: + warn( + "Starting in v22.3, config will be a required argument " + "for ErrorHandler.finalize().", + DeprecationWarning, + ) + + if fallback and fallback != DEFAULT_FORMAT: + if error_handler._fallback is not _default: + error_logger.warning( + f"Setting the fallback value to {fallback}. This changes " + "the current non-default value " + f"'{error_handler._fallback}'." + ) + error_handler._fallback = fallback if not isinstance(error_handler, cls): error_logger.warning( @@ -64,7 +141,8 @@ class ErrorHandler: "work at all.", DeprecationWarning, ) - error_handler._lookup = error_handler._legacy_lookup + legacy_lookup = error_handler._legacy_lookup + error_handler._lookup = legacy_lookup # type: ignore def _full_lookup(self, exception, route_name: Optional[str] = None): return self.lookup(exception, route_name) @@ -188,12 +266,13 @@ class ErrorHandler: :return: """ self.log(request, exception) + fallback = ErrorHandler._get_fallback_value(self, request.app.config) return exception_response( request, exception, debug=self.debug, base=self.base, - fallback=self.fallback, + fallback=fallback, ) @staticmethod diff --git a/tests/test_errorpages.py b/tests/test_errorpages.py index 1843f6a7..f8e425b0 100644 --- a/tests/test_errorpages.py +++ b/tests/test_errorpages.py @@ -280,40 +280,20 @@ def test_allow_fallback_error_format_set_main_process_start(app): async def start(app, _): app.config.FALLBACK_ERROR_FORMAT = "text" - request, response = app.test_client.get("/error") - assert request.app.error_handler.fallback == "text" + _, response = app.test_client.get("/error") assert response.status == 500 assert response.content_type == "text/plain; charset=utf-8" -def test_setting_fallback_to_non_default_raise_warning(app): - app.error_handler = ErrorHandler(fallback="text") +def test_setting_fallback_on_config_changes_as_expected(app): + app.error_handler = ErrorHandler() - assert app.error_handler.fallback == "text" - - with pytest.warns( - UserWarning, - match=( - "Overriding non-default ErrorHandler fallback value. " - "Changing from text to auto." - ), - ): - app.config.FALLBACK_ERROR_FORMAT = "auto" - - assert app.error_handler.fallback == "auto" + _, response = app.test_client.get("/error") + assert response.content_type == "text/html; charset=utf-8" app.config.FALLBACK_ERROR_FORMAT = "text" - - with pytest.warns( - UserWarning, - match=( - "Overriding non-default ErrorHandler fallback value. " - "Changing from text to json." - ), - ): - app.config.FALLBACK_ERROR_FORMAT = "json" - - assert app.error_handler.fallback == "json" + _, response = app.test_client.get("/error") + assert response.content_type == "text/plain; charset=utf-8" def test_allow_fallback_error_format_in_config_injection(): @@ -327,7 +307,6 @@ def test_allow_fallback_error_format_in_config_injection(): raise Exception("something went wrong") request, response = app.test_client.get("/error") - assert request.app.error_handler.fallback == "text" assert response.status == 500 assert response.content_type == "text/plain; charset=utf-8" @@ -339,6 +318,23 @@ def test_allow_fallback_error_format_in_config_replacement(app): app.config = MyConfig() request, response = app.test_client.get("/error") - assert request.app.error_handler.fallback == "text" assert response.status == 500 assert response.content_type == "text/plain; charset=utf-8" + + +def test_config_fallback_before_and_after_startup(app): + app.config.FALLBACK_ERROR_FORMAT = "json" + + @app.main_process_start + async def start(app, _): + app.config.FALLBACK_ERROR_FORMAT = "text" + + _, response = app.test_client.get("/error") + assert response.status == 500 + assert response.content_type == "text/plain; charset=utf-8" + + +def test_config_fallback_bad_value(app): + message = "Unknown format: fake" + with pytest.raises(SanicException, match=message): + app.config.FALLBACK_ERROR_FORMAT = "fake"