feat(routing): override default responders via on_request() #2446
Conversation
Add an option to CompiledRouterOptions that allows for overriding the default responders by implementing on_request() in the resource class Closes falconry#2071
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2446 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 7747 7754 +7
Branches 1072 1074 +2
=========================================
+ Hits 7747 7754 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| __slots__ = ('converters', 'allow_on_request') | ||
|
|
||
| def __init__(self, allow_on_request: bool = False) -> None: |
There was a problem hiding this comment.
I think we should just follow the existing pattern(s), and always initialize "options" objects with default settings.
Then, the user can customize this option as you have shown.
| object.__setattr__( | ||
| self, | ||
| 'converters', | ||
| ConverterDict((name, converter) for name, converter in converters.BUILTIN), | ||
| ) | ||
|
|
||
| self.allow_on_request = allow_on_request |
There was a problem hiding this comment.
I had a different name in mind, but that wasn't too great either... Let me think if I can come up with anything better 😅
There was a problem hiding this comment.
Naming things is not my forte, but I will also try to think of a better name 😅
|
|
||
| for method, responder in method_map.items(): | ||
| if method not in ('GET', 'OPTIONS'): | ||
| assert responder == resource.on_request |
There was a problem hiding this comment.
Let us use comparison is, and is not, respectively, because we are comparing function pointers (PyObject* pointers on CPython), not values.
There was a problem hiding this comment.
I was researching why using is for the comparison was failing. Turns out that the values in method_map are of two types:
- function, when created by utilities in
responders.py - bound method, when retrieved by the resource using
getattr(resource, responder_name)
Since, a new instance method object is created in the second case, comparison with is does not work. I thought of using responder.__func__ to compare with the original function object but this would not work for the first case.
Should I add different checks depending on the responder's type?
| resource, method_map, __, __ = router.find('/default') | ||
|
|
||
| for method, responder in method_map.items(): | ||
| assert responder != resource.on_request |
| default_responder = None | ||
|
|
||
| if self._options.allow_on_request: | ||
| default_responder = getattr(resource, 'on_request', None) |
There was a problem hiding this comment.
We need to pay heed to the suffix preference here too.
Could you also add a test where the default responder is picked up from on_request_suffix?
There was a problem hiding this comment.
Of course, I will implement this.
I also need to make sure that a SuffixedMethodNotFoundError is not raised in case the resource contains only on_request_suffix.
| app.router_options.allow_on_request = True | ||
|
|
||
| Note: | ||
| This option does not override `on_options()`. |
There was a problem hiding this comment.
This docstring looks great, let's just move it to a different place similar to how other router options are documented.
Also, here it would be helpful to provide suggestions what to do if one wants to actually use on_request even for on_options (you can show both variants, what you wrote on the issue, as well as aliasing that I suggested).
| default_responder = getattr(resource, 'on_request', None) | ||
|
|
||
| set_default_responders( | ||
| method_map, asgi=asgi, default_responder=default_responder |
There was a problem hiding this comment.
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.
| @@ -99,8 +108,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 not default_responder: | |||
There was a problem hiding this comment.
This may not work correctly in the edge cases where default_responder evaluates as falsy in a boolean test.
Let's refuse to guess, and write if default_responder is None: ... instead.
Add an option to CompiledRouterOptions that allows for overriding the default responders by implementing
on_request()in the resource classCloses #2071
Summary of Changes
Added a new router option (
allow_on_request) infalcon.routing.compiled.CompiledRouterOptionsthat allows for providing a default responder by definingon_request()on the resource. This option is disabled by default. If enabled,on_request()is set as the responder for every unimplemented method except foron_options(). If the option is disabled oron_request()is not provided in the resource, the default responder for "405 Method Not Allowed" is used.Related Issues
on_request()#2071Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.
docs/.docs/.versionadded,versionchanged, ordeprecateddirectives.docs/_newsfragments/, with the file name format{issue_number}.{fragment_type}.rst. (Runtowncrier --draftto ensure it renders correctly.)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
PR template inspired by the attrs project.