Skip to content
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

Allow to override the default responders via on_request() #2071

Open
vytas7 opened this issue May 22, 2022 · 3 comments · May be fixed by #2446
Open

Allow to override the default responders via on_request() #2071

vytas7 opened this issue May 22, 2022 · 3 comments · May be fixed by #2446

Comments

@vytas7
Copy link
Member

vytas7 commented May 22, 2022

In addition to on_get(), on_post(), etc, could we allow redefining the default responders via a generic on_request()?
This idea is borrowed from the Responder framework (which is in turned inspired by Falcon and Flask), see: Class-Based Views.

Responder itself is sort of abandoned, so it was challenging to even get a test program running, but once I got it up, it seemed that providing both on_request() AND on_get() in Responder makes the said framework call both upon GET. Which IMHO doesn't fit well into Falcon's design, so maybe we could instead map on_request (if provided) only instead of the default responders. See also: #735.

Opinions welcome on what to do with on_options(), if not provided. Maybe we could still provide a default on_options(), and not use on_request() for that, in order to minimize the breaking impact of this change, and for convenience.

This change could potentially pave the way to further unifying resources and sinks, see also

@gespyrop
Copy link

Hi! If I understand correctly, this will allow users to provide a default implementation for unimplemented methods by defining the on_request() method in the resource. If implemented, this will be used instead of the default "405 Method Not Allowed" responder.

Regarding on_options() I agree that it makes sense to keep providing the default responder. If needed, on_request() can be called explicitly in on_options().

I believe I can work on this.

@vytas7
Copy link
Member Author

vytas7 commented Apr 1, 2025

Hi @gespyrop! Yes, I was thinking along these lines.

And yes, regarding on_options, it is even easier to do something like

class MyResource:
    def on_request(req, resp, ...):
        ...

    on_options = on_request

However, this issue had no "needs contributor" label, which doesn't mean you aren't welcome, but be prepared for a bit more involved feedback loop wrt the architectural part. 😈
Mostly because we cannot roll this out indiscriminately in a SemVer minor version, since if anyone already had on_request(...) for any other purpose in their resources, that would break the default responders. So we need to gate this new functionality behind an option, probably App.router_options.
Then, if we get positive feedback from the community wrt this feature, we can enable it by default in Falcon 5.0.

gespyrop added a commit to gespyrop/falcon that referenced this issue Apr 6, 2025
Add an option to CompiledRouterOptions that allows for overriding the default responders by implementing on_request() in the resource class

Closes falconry#2071
@gespyrop gespyrop linked a pull request Apr 6, 2025 that will close this issue
13 tasks
@gespyrop
Copy link

gespyrop commented Apr 6, 2025

Oh, my bad. 😅
A more involved feedback loop is not a problem.

I added an option so that it can be enabled by

app.router_options.allow_on_request = True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants