Subclass router and use FastAPI dependencies instead of backend protocol?

[c-wygoda]

Currently the endpoint definitions are encapsulated in the StapiRouter class, each only asking for the fastapi.Request dependency which is then forwarded to the backend method.

Probably a more flexible approach is to use subclasses of StapiRouter and allow overriding each method by the implementor, as then whatever dependencies are required can be added to the endpoint method signature - think JWT security, database client, etc -, sticking with the FastAPI approach to override them with mocks in testing. Tiny example:

from typing import Annotated

from fastapi import Depends, FastAPI, Request

from stapi_fastapi.api import StapiRouter as StapiRouterBase
from stapi_fastapi.models.root import RootResponse


def my_dependency(request: Request):
    return request.headers.get("X-Header")


class StapiRouter(StapiRouterBase):
    def root(
        self, request: Request, x_header: Annotated[str, Depends(my_dependency)]
    ) -> RootResponse:
        print(x_header)
        return super().root(request)


app = FastAPI()

stapi = StapiRouter(None)
app.include_router(stapi.router)

As we in any case expect implementors to bring their own logic, we could make the StapiRouter abstract and provide it as a barebones skeleton? Plus base schema classes for the request/response schemas based on some ProductBase and ProductParameterBase pydantic models each provider has to start with?

[c-wygoda]

Since @jkeifer is quite active the last days, let me tag you here for comment... :)

[c-wygoda]

cobbled something together showing the idea of how a subclass can nicely use FastAPIs dependency injection: https://github.com/stapi-spec/stapi-fastapi-tle-provider

[c-wygoda]

pinging for a wider audience to discuss... @mindflayer @jsignell

[jkeifer]

Thanks @c-wygoda for pulling this together, I'm happy to take a look. That said, I have concerns with this idea.

First off, depending on the implementation, requiring subclassing can be a recipe for type errors, Liskov substitution principle and all that. I get a bit concerned about this because of way FastAPI dependency injection works: it seems like it would be overly easy to run into problems because of Liskov.

Indeed, looking at https://github.com/stapi-spec/stapi-fastapi-tle-provider/blob/main/stapi_fastapi_tle/service/router.py with a type checker shows a bunch of typing problems. Of course the errors around the method signature incompatibilities, generally appear to be caused by the more-specific types in that implementation not being compatible with the generic STAPI spec models. Likely this means the base router class types are not properly genericized, so it might be possible to fix them with better typing on the base router class.

I'd also argue part of this problem is that the STAPI spec has the wrong structure to properly allow more specific typing. See my proposals stapi-spec/stapi-spec#198 and stapi-spec/stapi-spec#200; with such as structure each product would likely necessitate it's own router instance, which would then get added to a root router that has the base STAPI endpoints and implementations.

Another issue with subclassing is that it can become unclear what methods a subclass must implement while also allowing the base case to provide default behavior to simplify/deduplicate user implementations. I'd say a good way to get around this is to have the endpoint methods fully implemented on the base class, and have them call other methods that would throw a NotImplementedError. Except this pattern similarly suffers from the problem that users would be unable to override the route function call signature to facilitate dependency injection; it is effectively equivalent to the current backend protocol pattern while also not being as clean, in my opinion.

Quite honestly, I like the backend protocol pattern. I really like the separation of concerns between the "doing product things" of the backend and "handling HTTP concerns" of the router. I am also, frankly, not a big fan of FastAPI's dependency injection. I do recognize the problem with the default route implementations perhaps not meeting user requirements. Maybe this is a case where stapi-fastapi needs to provide a base implementation that has support for common needs in a configurable way such that most implementations would not need to override the base behavior. Or, the stapi-fastapi example implementations can demonstrate patterns to facilitate custom needs in a more pluggable manner.

For example, auth could be implemented as middleware on the app itself (or in a reverse proxy, even). Database connections/pools could also be passed into the protocol methods via the application/request state (I generally use the application lifespan function to setup any clients and then those become available on request objects as request.state.whatever_client, and testing can mock them by using a function to instantiate app, which each dependency can be passed into). In this case stapi-fastapi would just need to ensure request is passed into each of the backend protocol methods (which appears is already the case).

[c-wygoda]

Would be happy to see an approach using generics, though I still find their support lacking in Python and hit walls when trying. But also wasn't trying too long to rule out it can be made to work. Does Liskov really apply when the base router class isn't meant to be used as is? maybe should be abstract?

Curious to see the outcome of the mentioned stapi-spec issues in the Colorado sprint, those might help a bit with making the structure more straight-forward. In any case though, wonder how to fully OpenAPI type the endpoints without the need to have multiple routers (though, maybe that's a small price).

I'm arguing for keeping closer to "standard" FastAPI use cases; it will be more accessible for (new) devs than going further bespoke. Not so much concerned about type checkers, it's still Python after all...

[jkeifer]

I believe this is closed by #80. After a review, we can really only type the OpportunityProperties of the Opportunity model specifically for a product based on it's constraints model (which is an OpportunityProperties instance). That is, the Opportunity model is the same for all products, with only the properties being product specific. Order models could be product-specific, but as orders are retrieved via the top-level /orders endpoint (and I am pretty sure it doesn't make sense to have them be retrieved per product via product-specific endpoints) we can't have more than a generic order model that can be used for all products.

We do still have work to do on typing the order request model per product, but that is dependent on #87.

An aside:

Not so much concerned about type checkers, it's still Python after all...

I will say from my perspective, having typing problems is a complete deal breaker. If I can't depend on typing in a service implementation using stapi-fastapi then I can't use it at all. Effective typing is 100% required to ensure correctness and maintainability on the projects I work on.

I'm going to go ahead and close this out as the proposal doesn't really fit with the rework that came in on #80, but I don't want to just silence conversation so if this issue needs to be reopened for further consideration please feel free to do so.