Skip to content

feat(routing): override default responders via on_request() #2446

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/_newsfragments/2071.newandimproved.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Added the :attr:`~.CompiledRouterOptions.allow_on_request` router option that
allows for providing a default responder by defining `on_request()` on the
resource. This option is disabled by default. If enabled, `on_request()` is
set as the responder for every unimplemented method except for `on_options()`.
If the option is disabled or `on_request()` is not provided in the resource,
the default responder for "405 Method Not Allowed" is used.
54 changes: 51 additions & 3 deletions falcon/routing/compiled.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,11 @@ class can use suffixed responders to distinguish requests
resource.
"""

return map_http_methods(resource, suffix=kwargs.get('suffix', None))
return map_http_methods(
resource,
suffix=kwargs.get('suffix', None),
allow_on_request=self._options.allow_on_request,
)

def add_route( # noqa: C901
self, uri_template: str, resource: object, **kwargs: Any
Expand Down Expand Up @@ -209,7 +213,24 @@ class can use suffixed responders to distinguish requests

method_map = self.map_http_methods(resource, **kwargs)

set_default_responders(method_map, asgi=asgi)
default_responder = None

if self._options.allow_on_request:
responder_name = 'on_request'
suffix = kwargs.get('suffix', None)

if suffix:
responder_name += '_' + suffix

default_responder = getattr(resource, responder_name, None)

# NOTE(gespyrop): We do not verify whether the default responder is
# a regular synchronous method or a coroutine since it falls under the
# general case that will be handled by _require_coroutine_responders()
# and _require_non_coroutine_responders().
set_default_responders(
method_map, asgi=asgi, default_responder=default_responder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the below _require_coroutine_responders(...) would catch the case where you provide a wrong "colour" of the default responder, but maybe it would be useful to add a # NOTE(gespyrop): ... explaining this.

Let's also add unit tests where the definition on_request() does not match the "colour" of other responder methods.

)

if asgi:
self._require_coroutine_responders(method_map)
Expand Down Expand Up @@ -941,7 +962,32 @@ class CompiledRouterOptions:
(See also: :ref:`Field Converters <routing_field_converters>`)
"""

__slots__ = ('converters',)
allow_on_request: bool
"""Allows for providing a default responder by defining `on_request()` on
the resource.

This feature is disabled by default and can be enabled by::

app.router_options.allow_on_request = True

This option does not override `on_options()`. In case `on_options()` needs
to be overriden, this can be done explicitly by aliasing::

on_options = on_request

or by explicitly calling `on_request()` in `on_options()`::

def on_options(self, req, resp):
self.on_request(req, resp)

Note:
In order for this option to take effect, it must be enabled before
calling :meth:`add_route()`.

.. versionadded:: 4.1
"""

__slots__ = ('converters', 'allow_on_request')

def __init__(self) -> None:
object.__setattr__(
Expand All @@ -950,6 +996,8 @@ def __init__(self) -> None:
ConverterDict((name, converter) for name, converter in converters.BUILTIN),
)

self.allow_on_request = False

def __setattr__(self, name: str, value: Any) -> None:
if name == 'converters':
raise AttributeError('Cannot set "converters", please update it in place.')
Expand Down
35 changes: 29 additions & 6 deletions falcon/routing/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@

from __future__ import annotations

from typing import Optional, TYPE_CHECKING
from typing import Optional, TYPE_CHECKING, Union

from falcon import constants
from falcon import responders

if TYPE_CHECKING:
from falcon._typing import AsgiResponderCallable
from falcon._typing import MethodDict
from falcon._typing import ResponderCallable


class SuffixedMethodNotFoundError(Exception):
Expand All @@ -31,7 +33,9 @@ def __init__(self, message: str) -> None:
self.message = message


def map_http_methods(resource: object, suffix: Optional[str] = None) -> MethodDict:
def map_http_methods(
resource: object, suffix: Optional[str] = None, allow_on_request: bool = False
) -> MethodDict:
"""Map HTTP methods (e.g., GET, POST) to methods of a resource object.

Args:
Expand All @@ -46,6 +50,11 @@ def map_http_methods(resource: object, suffix: Optional[str] = None) -> MethodDi
a suffix is provided, Falcon will map GET requests to
``on_get_{suffix}()``, POST requests to ``on_post_{suffix}()``,
etc.
allow_on_request (bool): If True, it prevents a
``SuffixedMethodNotFoundError`` from being raised on resources
defining ``on_request_{suffix}()``.
(See also: :ref:`CompiledRouterOptions <compiled_router_options>`.)


Returns:
dict: A mapping of HTTP methods to explicitly defined resource responders.
Expand All @@ -69,23 +78,34 @@ def map_http_methods(resource: object, suffix: Optional[str] = None) -> MethodDi
if callable(responder):
method_map[method] = responder

has_default_responder = allow_on_request and hasattr(
resource, f'on_request_{suffix}'
)

# If suffix is specified and doesn't map to any methods, raise an error
if suffix and not method_map:
if suffix and not method_map and not has_default_responder:
raise SuffixedMethodNotFoundError(
'No responders found for the specified suffix'
)

return method_map


def set_default_responders(method_map: MethodDict, asgi: bool = False) -> None:
def set_default_responders(
method_map: MethodDict,
asgi: bool = False,
default_responder: Optional[Union[ResponderCallable, AsgiResponderCallable]] = None,
) -> None:
"""Map HTTP methods not explicitly defined on a resource to default responders.

Args:
method_map: A dict with HTTP methods mapped to responders explicitly
defined in a resource.
asgi (bool): ``True`` if using an ASGI app, ``False`` otherwise
(default ``False``).
default_responder: An optional default responder for unimplmented
resource methods. If not provided a responder for
"405 Method Not Allowed" is used.
"""

# Attach a resource for unsupported HTTP methods
Expand All @@ -99,8 +119,11 @@ def set_default_responders(method_map: MethodDict, asgi: bool = False) -> None:
method_map['OPTIONS'] = opt_responder # type: ignore[assignment]
allowed_methods.append('OPTIONS')

na_responder = responders.create_method_not_allowed(allowed_methods, asgi=asgi)
if default_responder is None:
default_responder = responders.create_method_not_allowed(
allowed_methods, asgi=asgi
)

for method in constants.COMBINED_METHODS:
if method not in method_map:
method_map[method] = na_responder # type: ignore[assignment]
method_map[method] = default_responder # type: ignore[assignment]
106 changes: 106 additions & 0 deletions tests/test_default_router.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import textwrap
from types import MethodType

import pytest

Expand Down Expand Up @@ -103,6 +104,28 @@ def on_get(self, req, resp):
resp.text = self.resource_id


class ResourceWithDefaultResponder:
def on_get(self, req, resp):
pass

def on_request(self, req, resp):
pass

def on_request_id(self, req, res, id):
pass


class ResourceWithDefaultResponderAsgi:
async def on_get(self, req, resp):
pass

async def on_request(self, req, resp):
pass

async def on_request_id(self, req, res, id):
pass


class SpamConverter:
def __init__(self, times, eggs=False):
self._times = times
Expand Down Expand Up @@ -695,6 +718,89 @@ def test_options_converters_invalid_name_on_update(router):
)


def test_options_allow_on_request_disabled():
router = DefaultRouter()
router.add_route('/default', ResourceWithDefaultResponder())

__, method_map, __, __ = router.find('/default')

for responder in method_map.values():
if isinstance(responder, MethodType):
responder = responder.__func__

assert responder is not ResourceWithDefaultResponder.on_request


def test_options_allow_on_request_enabled():
router = DefaultRouter()
router.options.allow_on_request = True
router.add_route('/default', ResourceWithDefaultResponder())

__, method_map, __, __ = router.find('/default')

for method, responder in method_map.items():
if isinstance(responder, MethodType):
responder = responder.__func__

if method in ('GET', 'OPTIONS'):
assert responder is not ResourceWithDefaultResponder.on_request
else:
assert responder is ResourceWithDefaultResponder.on_request


def test_on_request_suffix():
router = DefaultRouter()
router.options.allow_on_request = True
router.add_route('/default/{id}', ResourceWithDefaultResponder(), suffix='id')

__, method_map, __, __ = router.find('/default/1')

for method, responder in method_map.items():
if isinstance(responder, MethodType):
responder = responder.__func__

if method == 'OPTIONS':
assert responder is not ResourceWithDefaultResponder.on_request_id
else:
assert responder is ResourceWithDefaultResponder.on_request_id


def test_synchronous_in_asgi(util):
router = DefaultRouter()
router.options.allow_on_request = True
kwargs = {'_asgi': True}

with util.disable_asgi_non_coroutine_wrapping():
with pytest.raises(
TypeError,
match='responder must be a non-blocking '
'async coroutine \\(i.e., defined using async def\\) to '
'avoid blocking the main request thread.',
):
router.add_route('/default', ResourceWithDefaultResponder(), **kwargs)


def test_coroutine_in_wsgi():
router = DefaultRouter()
router.options.allow_on_request = True

with pytest.raises(
TypeError,
match='responder must be a regular synchronous '
'method to be used with a WSGI app.',
):
router.add_route('/default', ResourceWithDefaultResponderAsgi())

with pytest.raises(
TypeError,
match='responder must be a regular synchronous '
'method to be used with a WSGI app.',
):
router.add_route(
'/default/{id}', ResourceWithDefaultResponderAsgi(), suffix='id'
)


@pytest.fixture
def param_router():
r = DefaultRouter()
Expand Down