-
Notifications
You must be signed in to change notification settings - Fork 104
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
Convenience method to allow customizing route dependencies #295
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks good to me! I've taken the liberty to add this to TiTiler as well 🙏 @alukach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool!
Can we add tests before merging 🙏 |
Sounds good. It sounds like we're fine with the idea and interface, so I'll go ahead and write some tests and maybe build out the docstring a bit more for better documentation. |
This is very handy indeed! Looking forward to having it in main! Thank you all. |
Cool yea! |
@alukach do you think you could add tests and maybe some docs? It seems that people are interested having this merged in master 😄 |
Sorry about the delay all, I had forgotten about this PR 😬 (thanks for the ping, @vincentsarago!) @vincentsarago @geospatial-jeff Added some tests in 146e28b. Can you run approve the workflow? I wanted them to be separate from the SqlAlchemy and the PgStac tests. To make them work, I had to create a dummy core client and dummy transaction client. I'm open to suggestions on how these tests can be cleaner. |
thanks @alukach 🙏 |
@AsgerPetersen @jonhealy1 If either of you have a chance to try this out, let me know how it goes! |
@alukach OK for authentication check (which is super cool) but… is it possible that this approach is not working for adding new query parameters to the defined route? I mean: like in the FastAPI hello world example about dependencies: https://fastapi.tiangolo.com/tutorial/dependencies/#create-a-dependency-or-dependable |
@keul Can you share more about what you're trying to do? There's a difference between having a dependency passed in as an argument to a FastAPI route (as you linked to) and dependencies inside a path operator (link). This PR does the later, it allows a dependency to be run before a route's logic is run, however you can't really do anything with the results. I believe that works as advertised, but I'm definitely open to the possibility that there may be a bug if you are able to provide a reproducible demonstration of it. |
@alukach my need is to inject custom query parameters for the As an example: what if I want to implement the collection pagination? https://github.com/radiantearth/stac-api-spec/tree/main/ogcapi-features#collection-pagination But this is probably a topic for a new issue, sorry for the noise |
@keul I guess you'll need to change this stac-fastapi/stac_fastapi/api/stac_fastapi/api/app.py Lines 214 to 233 in 25879af
page dependency (instead of EmptyRequest )
|
Yes, I agree with what @vincentsarago has said. @keul When you say that you want to inject a custom query parameter, I suspect that you're leaving out the detail that you probably want to do something with that custom query parameter later. Assuming that is true, then you probably don't want to be augmenting an existing endpoint with the
Yeah, it may be best to open a discussion for something like this. You're welcome to mention my username if you'd like to get my attention for further conversation. |
Related Issue(s): #
Description:
We're using stac-fastapi on a project and would like to add auth requirements to a number of endpoints. To the best of my knowledge, there's not an official way to do this in this codebase. This PR adds a convenience method (
add_route_dependencies
) to the mainStacApi
class. This allows us to do something like the following:This manner of injecting dependencies into already-existing routes doesn't seem well documented in either the FastAPI documentation or this codebase, so I figure it may be of use to someone. It seems to work well and properly updates the OpenAPI spec as well:
I'm admittedly a little concerned that this is a bit heavy-handed (i.e. jamming more logic into that class), however wanted to present this solution as a starting point. I'm all ears if there's a better way to achieve our goal.
Update
To be clear about the status of this PR, I consider it still in the "proof-of-concept" stage. The following should occur before merging:
PR Checklist:
pre-commit run --all-files
)make test
)make docs
)