stac-utils / stac-fastapi

STAC API implementation with FastAPI.
https://stac-utils.github.io/stac-fastapi/
MIT License
226 stars 99 forks source link

Middleware Stack Rebuilt on Each StacApi Initialization #719

Closed joshimai closed 1 week ago

joshimai commented 1 week ago

The change in stac-fastapi that proxies app.add_middleware causes the middleware stack to be rebuilt or duplicated every time StacApi is initialized. It duplicates internal business logic from Starlette and now it seems incompatible with the new FastAPI/Starlette (0.110.3/0.37.2) versions. This behavior results in repeated middleware entries in the stack, leading to unintended side effects.

Steps to Reproduce:

Initialize a FastAPI app and add custom middleware. Initialize StacApi with the FastAPI app. Observe the middleware stack before and after each StacApi initialization.

Sample Code:

from fastapi import FastAPI, Request
from stac_fastapi.api.app import StacApi
from fastapi.exceptions import RequestValidationError
from fastapi.responses import JSONResponse
from stac_fastapi.types.config import ApiSettings
from stac_fastapi.pgstac.core import CoreCrudClient
from stac_fastapi.extensions.core import (
    FieldsExtension,
    ContextExtension,
    QueryExtension,
    SortExtension,
)

app = FastAPI()
app.middleware = []
extensions = [FieldsExtension(), ContextExtension(), QueryExtension(), SortExtension()]

PCClient = CoreCrudClient()
settings = ApiSettings()

# First initialization of StacApi
stac_api = StacApi(
    app=app,
    settings=settings,
    extensions=extensions,
    client=PCClient,
)

app.middleware = []
@app.exception_handler(RequestValidationError)
async def validation_exception_handler(request, exc):
    return JSONResponse(
        status_code=400,
        content={"customerrordetail": exc.errors(), "body": exc.body},
    )

# Test route to trigger RequestValidationError
@app.post("/items/")
async def create_item(item: dict):
    return item

Steps to Reproduce:

uvicorn main:app --reload
curl -X POST "http://127.0.0.1:8000/items/" -H "Content-Type: application/json" 

Expected Output: {"customerrordetail":[{"type":"missing","loc":["body"],"msg":"Field required","input":null}],"body":null} Actual Output: {"code":"RequestValidationError","description":"[{'type': 'missing', 'loc': ('body',), 'msg': 'Field required', 'input': None}]"}

Sample Code with the workaround:

.....
# no middleware
stac_api = StacApi(
    app=app,
    settings=settings,
    extensions=extensions,
    client=PCClient,
    middlewares=[],
)
..........

This does return the expected outcome which points to the middleware call causing the custom exception handlers to not be properly registered with the starlette exception handling middleware.

lossyrob commented 1 week ago

@vincentsarago this was causing issues with the custom error handling that we discussed yesterday.

I'm not sure exactly the issue, but this code: https://github.com/stac-utils/stac-fastapi/blob/main/stac_fastapi/api/stac_fastapi/api/app.py#L442-L445 proxies the starlette add_middleware call https://github.com/encode/starlette/blob/master/starlette/applications.py#L134-L142, but calls build_middleware_stack every time a middleware is added. Seems like that should be explicitly disallowed by the raise RuntimeError("Cannot add middleware after an application has started") in the starlette call, though I haven't seen this exception.

This was causing the custom exception handlers to not be properly registered with the starlette exception handling middleware.

Workaround now is to set the middleware explicitly in the FastApi constructor d to pass middlewares=[] into StacApi constructor to avoid triggering this code.

vincentsarago commented 1 week ago

🤔 so just to be clear you're using the same app to initialize two different StacApi objects. Each StacApi initialization will modify the app by adding middleware to the app middleware stack.

To me it doesn't sound like passing the same app to two different StacApi object is just not a good usage.

The StacApi init will modify the app, so by definition you shouldn't expect to be able to use the app after the StacAPI init.

We're seeing just the middleware problem but you'll also register the endpoints twice.

This will result in the first StacApi object to be useless and it will be pretty dangerous do to something with it

joshimai commented 1 week ago

To clarify, we are not actually initializing two StacApis but rather demonstrating the issue we saw with middlewares. I updated the sample code to be a better representation of our problem.

vincentsarago commented 1 week ago

alright I see what's going on!

in StacAPI we do:

https://github.com/stac-utils/stac-fastapi/blob/f311dc620d2a11df397f8d95c17e15483b9e302c/stac_fastapi/api/stac_fastapi/api/app.py#L480

https://github.com/stac-utils/stac-fastapi/blob/f311dc620d2a11df397f8d95c17e15483b9e302c/stac_fastapi/api/stac_fastapi/api/app.py#L442-L445

The issue is then we have app.middleware_stack created instead of letting starlette create it on the first app call

https://github.com/encode/starlette/blob/5a1bec33f8d6a669a3670f51034de83292d19408/starlette/applications.py#L119-L123

The fix will be to remove https://github.com/stac-utils/stac-fastapi/blob/f311dc620d2a11df397f8d95c17e15483b9e302c/stac_fastapi/api/stac_fastapi/api/app.py#L445