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

Include responder in middleware process_resource #1629

Open
timc13 opened this issue Jan 3, 2020 · 9 comments
Open

Include responder in middleware process_resource #1629

timc13 opened this issue Jan 3, 2020 · 9 comments

Comments

@timc13
Copy link
Contributor

timc13 commented Jan 3, 2020

I like the flexibility that responder name suffixes offers - it would be great if this flexibility were extended to the middleware. For example, when writing middleware, you cannot distinguish between a non-suffixed responder and a suffixed responder for any given HTTP method.

A nice API for this might be to pass down the actual responder callable to the process_resource hook

@open-collective-bot
Copy link

Hi 👋,

Thanks for using Falcon. The large amount of time and effort needed to
maintain the project and develop new features is not sustainable without
the generous financial support of community members like you.

Please consider helping us secure the future of the Falcon framework with a
one-time or recurring donation.

Thank you for your support!

@vytas7
Copy link
Member

vytas7 commented Jan 3, 2020

Hi @timc13!
And thank you for this design proposal, I have labeled the issue accordingly.

For the time being, note that the request object already exposes the req.uri_template attribute which could probably in some cases be useful for deriving the suffix.

A couple of alternative ideas off the top of my head:

  • Add a new request attribute telling which suffix was matched, if any (not sure if it is worth the overhead?).
  • Add a CompiledRouter method to get the suffix corresponding to the given uri_template, if any. One could then pass the req.uri_template to that method to obtain the suffix.

@timc13
Copy link
Contributor Author

timc13 commented Jan 3, 2020

Those seem like fine workarounds - although maybe a bit indirect. As far as i'm concerned, it's not the suffix that is important or need be exposed, it's the actual responder method itself and perhaps the method name. As a counterpoint, I think this proposal - #709 is also a good idea. If a custom get_responder is ever used, suffixes may be irrelevant.

My ultimate wish, is to be able to decorate responder methods and attach some metadata to them at declaration time. Then my middleware could process_resource with that metadata.

Is there a trade-off to exposing the responder to the middleware that I'm not seeing?

@vytas7
Copy link
Member

vytas7 commented Jan 3, 2020

Is there a trade-off to exposing the responder to the middleware that I'm not seeing?

Just adding more parameters to the middleware interface is a bit of a trade-off in itself; both from the compatibility point of view (we'll have to provide shims), and well as just keeping the arity reasonably low.
But by that I don't mean that I dislike your proposal. I just wanted to provide some alternatives so we can compare and contrast 🙂

The mentioned idea from #709 is also a possible way to solve this.

@vytas7
Copy link
Member

vytas7 commented Dec 24, 2020

FWIW, this was raised again (on Gitter), see also #1818.

There, I was thinking to myself, maybe we could define a Route class (could be a namedtuple or something similar, rigid and clean) that would contain all the relevant info: URI template, responder method, HTTP verb, suffix (if any) etc.
And we would then set req.route instead of req.uri_template (req.uri_template could be left as a compatibility wrapper property).
Not sure if this wouldn't be too much burden to implement for custom routers though? 🤔

@CaselIT
Copy link
Member

CaselIT commented Dec 25, 2020

I like the idea.

A drawback to namedtuples is that they are slow to instanciate and also to access parameters (at least until 3.8).
I'm increasingly replacing them with frozen dataclasses with slots.

@vytas7
Copy link
Member

vytas7 commented Dec 25, 2020

Yes, sorry, I merely meant that these objects must be reasonably fast to access, and ideally immutable, but their instantiation may take time since that would happen only upon adding/reshuffling routes.

Edit: good to know they are slow even to access, sounds like that defeats the purpose a bit.

@CaselIT
Copy link
Member

CaselIT commented Dec 25, 2020

Edit: good to know they are slow even to access, sounds like that defeats the purpose a bit.

py 3.8 mostly fixed them, but before it the access to named attributes of a named tuple was basically implemented as a @property, so definitely slower then a normal attribute lockup

@vytas7 vytas7 added this to the Version 5.0 milestone Jan 8, 2023
@vytas7
Copy link
Member

vytas7 commented Jan 12, 2024

It still benchmarks slightly slower than a normal attribute on a bare class object, but the difference is rather negligible. Much faster than a normal property.

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

No branches or pull requests

3 participants