-
Notifications
You must be signed in to change notification settings - Fork 8
refactor method url_for in ProductRouter and RootRouter #120
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
Conversation
|
I don’t think this change should be necessary. That’s not to say I’m opposed to it if you need it, but
See the docs on this: https://fastapi.tiangolo.com/advanced/behind-a-proxy/. |
|
@jkeifer In my particular case, this won't work because the root_path can't be dynamically set -- it's always a fixed value set at startup. I tried a number of things with fastapi and wasn't able to get it to work, so dropping back to this option, and basically handling it the same way stac-server does with the X-STAC-Endpoint header. I think there's also an issue with setting the X-Forwarded-* headers when there's an API Gateway in the mix - I don't think the proxy can correctly rewrite them in that case. |
|
@jkeifer thoughts on this? I'd like to move this forward. Though, I'm also considering forking this, both because there isn't much active development or ownership of it, and so we can more easily add various behaviors we need that don't currently have integration points in the library. I don't necessarily like that, but I think we've always recognized that this is a much different type of server than a STAC API, and there are going to be more provider-specific behaviors. |
|
Yeah I'm curious about this as well. I almost feel like the model needs to be flipped on its head, and there can just be "STAPI response enrichers" that add links, etc. to a fastAPI app. In my implementation, I'm already doing quite a few tweaks or workarounds for our specific implementation. |
|
Hey, sorry, this last week I've been pretty thrown off by illness. Anyway, I've approved these changes, there's nothing problematic about them that I can see, and implementations that don't need to care about changing the default URL resolution behavior won't have to care about this, so 👍. It's a bummer that the proxy features don't just work for you out of the box, but oh well I guess. As to the larger questions and the idea of forking...I'd really hate to see you have to fork. I think we all acknowledge though that the current structure has significant limitations for more complex implementations. The current state is admittedly a pragmatic middle ground between where it was and where I think it needs to be that was predicated on just getting this thing functional enough to stand up an implementation. I have too thought that we need to explode the methods on the router classes such that they can be used independently and composed into custom routers as needed for more complex implementations. The thing is, I was expecting to be working on two major STAPI implementations, such that I could see how this all worked and fold back learnings into the codebase. But now I'm working on 0 of them. So I'm dependent on any of you all using this to contribute back if possible. |
What I'm changing
How I did it
I created a new static method that all of the generated links use.
The intention of this is to allow child class implementations for projects that use this library to override this if they want to change the urls. A key use case for this is when the API is being proxied from another API, and there is the desire for the links to be correct wrt the URLs provided by the reverse proxy.
Checklist
uv run pytestuv run pre-commit run --all-files