Correlation header not present in response for 500 errors

[brki]

Responses that are returned without error, or responses that are returned when an HTTPException is raised, include the correlation header name and the correlation id.

However, when an unhandled exception is raised, for example with this code:

@app.get("/")
async def root():
    a = 1/0
    return {"message": "Hello!"}

then a 500 error is returned to the client, and the correlation header is not included.

It seems like it would be especially nice to have the correlation id included in the response headers for such cases.

[brki]

I notice that it is possible to add the header by using an exception handler for generic exceptions, like this:

from asgi_correlation_id.context import correlation_id
from fastapi import HTTPException

@app.exception_handler(Exception)
async def unhandled_exception_handler(request, exc):
    return await http_exception_handler(request, HTTPException(500, 'Internal server error', headers={'x-correlation-id': correlation_id.get()}))

I'm not sure if it's the best way, but it seems to work.

If it was possible to not do that, it would seem better to me.

[sondrelg]

From what I can tell it seems like unhandled errors ultimately get caught by the ServerErrorMiddleware here, and that when no handlers are set, the middleware will always return a PlainTextResponse here. Unless I'm misunderstanding something I think this means any surfaced error will always result in the headers being stripped.

It looks like we might technically be able to catch and suppress exceptions in this middleware, to prevent that from happening, but that seems like overstepping for a request-id-middleware.

I think the way to fix this for Starlette and FastAPI projects might be to provide an exception handler, like the one you've posted above, in the package and document how to include it.

Would you be interested in creating a PR for this? I think the only change I would make is to export the callable without the app decorator:

# asgi_correlation_id/exception_handlers/fastapi.py

from asgi_correlation_id.context import correlation_id

from fastapi import HTTPException


async def unhandled_exception_handler(request, exc):  # add type hints
    return await http_exception_handler(
        request, HTTPException(500, 'Internal server error', headers={'x-correlation-id': correlation_id.get()})
    )  # add 'Access-Control-Expose-Headers' header too - see middleware.py

Then a user could just import it in their main.py like this:

# app/main.py
from asgi_correlation_id.exception_handlers.fastapi import unhandled_exception_handler
from fastapi import FastAPI

app = FastAPI(
    exception_handlers=[unhandled_exception_handler]
)

[sondrelg]

On further inspection, catching and re-raising exceptions in the middleware might accomplish the same thing. I've asked for input in the encode gitter chat - perhaps that would be the way to go.

[sondrelg]

@brki I've opened #21. Would you mind taking a look at the changes to see if they make sense to you?

I think I've landed on a preference of just documenting exception handlers and explaining the problem, rather than providing finished exception handlers for each framework. See the PR description and let me know if you agree with this or not 🙂

[brki]

Yes, makes sense! Thanks for adding the documentation.

…

On Sat, 12 Feb 2022, 10:00 Sondre Lillebø Gundersen < ***@***.***> wrote: @brki <https://github.com/brki> I've opened #21 <#21>. Would you mind taking a look at the changes to see if they make sense to you? I think I've landed on a preference of just documenting exception handlers and explaining the problem, rather than providing finished exception handlers for each framework. See the PR description and let me know if you agree with this or not 🙂 — Reply to this email directly, view it on GitHub <#19 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFKHGEUBIWQY5NAZPY7GFLU2YOR5ANCNFSM5ODQ3LUA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[sondrelg]

Thanks for opening the issue! 👏

[JonasKs]

Hi!

This can be fixed without adding error handlers, if the FastAPI app is wrapped in the middleware, not the middleware passed to FastAPI/Starlette.

In other words:

app = FastAPI()
app.include_router(...)
app = CorrelationIdMiddleware(app=app)

This way, any error thrown by the inner app, will still be propagated upwards in the chain. This is how e.g. Sentry works.

This approach is however a little annoying when e.g. writing tests. You end up doing something like this:

fastapi = FastAPI()
fastapi.include_router(...)
app = CorrelationIdMiddleware(app=fastapi)

and then in your tests you do:

from main import fastapi
fastapi.override_dependencies = ...

since app no longer is a FastAPI instance.

So, I've decided to override build_middleware_stack from FastAPI, and pass in middlewares (kinda the same Sentry does under the hood, just this is a bit more generic).
Now, we can do this:

patch_fastapi_middlewares(  # note - this must be done _before_  FastAPI is initialized
    middlewares=[
        Middleware(LoggingMiddleware),
        Middleware(
            CORSMiddleware,
            allow_credentials=True,
            allow_headers=['*'],
            allow_methods=['*'],
            allow_origins=['*'],
        ),
        Middleware(CorrelationIdMiddleware, header_name='Correlation-ID'),
    ]
)

app = FastAPI()  # when this is initiliazed the `build_middleware_stack` function will be called - which we've replaced

This way app is still a pure FastAPI class, and we don't have to have different imports for uvicorn / tests etc. Now CORS, correlationID, logging etc. will also still work even on 500 server errors. (Example)

Together with Sentry it would look something like this

patch_fastapi_middlewares(  # this must be done _before_  FastAPI is initialized
    middlewares=[
        Middleware(LoggingMiddleware),
        Middleware(
            CORSMiddleware,
            allow_credentials=True,
            allow_headers=['*'],
            allow_methods=['*'],
            allow_origins=['*'],
        ),
        Middleware(CorrelationIdMiddleware, header_name='Correlation-ID'),
    ]
)

sentry_init(
    dsn='...',
    environment=settings.ENVIRONMENT,
    integrations=[StarletteIntegration(), FastApiIntegration()],
)

app = FastAPI()