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

Pass StacAPI reference to ApiExtension.register instead of FastAPI reference #374

Open
captaincoordinates opened this issue Mar 15, 2022 · 2 comments
Labels
api layer enhancement New feature or request

Comments

@captaincoordinates
Copy link
Contributor

When registering extensions app.py passes a reference to the FastAPI object with self.app and as a result any extensions have access to the FastAPI object during registration.

The FastAPI type does not provide access to useful functionality in the StacApi type that may be required by an extension, such as add_route_dependencies. Authentication / authorisation checks could form part of a useful extension and would be cleaner and simpler to implement with access to the add_route_dependencies function.

Perhaps the StacApi type itself should not be passed to ApiExtension.register as this would give the extension access to the extensions list and other properties outside the extension's concern. A stripped-down type, providing access only to the app property, add_route_dependencies, and other safe properties could be used. For example:

# define type
class StacApiExtension(BaseModel):
    app: FastAPI
    add_route_dependencies: Callable[[List[Scope], List[Depends]], None]
    [...]

# register extensions in app.py
for ext in self.extensions:
    ext.register(StacApiExtension(**self))
@geospatial-jeff
Copy link
Collaborator

geospatial-jeff commented Mar 15, 2022

Accessing the FastAPI object from within an extension (outside of register method) makes sense to me. I can see how this makes it easier to implement certain custom extensions. I think in this case we'd also want to invert control, for example: StacApi.add_extension(extension) instead of StacApiExtension.register(app).

I'm less certain about parameterizing add_route_dependencies, since that is just a wrapper over things you can normally do with FastAPI object to begin with.

@captaincoordinates
Copy link
Contributor Author

@geospatial-jeff thanks for the feedback. I don't have a strong opinion on your suggestion around inversion of control.

I see your point about add_route_dependencies. All I will add is that add_route_dependencies - although only a wrapper - does abstract some complexity and is covered by stac-fastapi's test suite, so its presence could be beneficial to extension developers that don't want the maintenance overhead and risk of working with the FastAPI API, especially when that API is not yet stable at a 1.0 release. Better if that overhead can be borne by the library.

@geospatial-jeff geospatial-jeff added api layer enhancement New feature or request labels Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api layer enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants