-
Notifications
You must be signed in to change notification settings - Fork 105
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
Fix content-type of /api response #287
Fix content-type of /api response #287
Conversation
class VndFastAPI(FastAPI): | ||
"""Custom FastAPI to support /api response with vendor content-type.""" | ||
|
||
def setup(self) -> None: |
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.
Perhaps unnecessary code duplication can be avoided by calling super
and then removing/re-adding the route itself.
self
is a Starlette
object, which has a routes property that directly returns the list of the router's routes. AFAIK adding a route appends it to the list and does no other special registration. While there's no "remove_route" method, if you find the route with the self.openapi_url
and replace it with the one you construct with the proper response, that might save some code dupe. The worry with the dupe is that if a later version of FastAPI changes say the docs_url add logic, then this override logic won't pick up on those changes unrelated to the VndOaiResponse.
Another thing to note is that Route exposes the endpoint Callable directly, so you could even wrap the endpoint in one that translates the return value into a VndOaiResponse.
Maybe not worth spending too much time trying to hack the internals of FastAPI and Starlette (though everything I mentioned is public API), but I think it would be preferable to reduce the code copy in the setup override to only as much as is needed for this small change.
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.
It appears as though attrs
has a converter
argument, which allows you to make modifications to supplied arguments. I've implemented a function which can be used as a converter
to alter the routes such that the appropriate vendor content-type is advertised without any inheritance shenanigans
@@ -75,7 +146,7 @@ class StacApi: | |||
search_request_model: Type[Search] = attr.ib(default=STACSearch) | |||
search_get_request: Type[SearchGetRequest] = attr.ib(default=SearchGetRequest) | |||
item_collection_uri: Type[ItemCollectionUri] = attr.ib(default=ItemCollectionUri) | |||
response_class: Type[Response] = attr.ib(default=JSONResponse) | |||
response_class: Type[Response] = attr.ib(default=ORJSONResponse) |
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 changes the default JSONResponse value to ORJSONResponse, which seems like an orthogonal change to this PR. And, orjson isn't an explicit dependency to anything but pgstac, so either orjson should be added as a dependency to the api
project or it shouldn't be used in this code file (applies also to the base class of `VndOaiResponse), as it would break installations that only relied on the sqlalchemy backend.
Related Issue(s): #281
Description:
/api provides open api JSON which is used for doc generation. This endpoint currently returns
application/json
as the content type, whereas OGC API Features - part 2 spec explicitly requiresapplication/vnd.oai.openapi+json;version=3.0
to be the content-type. This PR addresses that issue by extending theFastAPI
class to update thesetup
method, which is hides the creation of the /api endpoint.PR Checklist:
pre-commit run --all-files
)make test
)make docs
)