stac-fastapi icon indicating copy to clipboard operation
stac-fastapi copied to clipboard

Pass StacAPI reference to ApiExtension.register instead of FastAPI reference

Open captaincoordinates opened this issue 3 years ago • 2 comments

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))

captaincoordinates avatar Mar 15 '22 04:03 captaincoordinates

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.

geospatial-jeff avatar Mar 15 '22 04:03 geospatial-jeff

@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.

captaincoordinates avatar Mar 15 '22 19:03 captaincoordinates