Repository navigation
Recommend wrapping app directly for 500 error handling - #114
Open
sebastian-correa wants to merge 2 commits into
Open
sebastian-correa wants to merge 2 commits into
sebastian-correa wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 94.44% 94.69% +0.25%
==========================================
Files 9 9
Lines 270 283 +13
==========================================
+ Hits 255 268 +13
Misses 15 15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
To avoid breaking static typing of app when using app = CorrelationIdMiddleware(app), CorrelationIdMiddleware is now exposed to type checkers as a class whose __new__ returns the same type it was given, instead of Self. At runtime this class is unused so behavior and add_middleware() compatibility are unchanged. Add a regression test that parses the module source with ast and fails if the TYPE_CHECKING-only __new__ stub's parameters drift out of sync with the dataclass's fields.
sebastian-correa
force-pushed
the
feature/better-default-500-handling
branch
from
September 28, 2026 13:23
1ac2477 to
68440f0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type-check
CorrelationIdMiddlewareas returning the wrapped app's own type via aTYPE_CHECKING-only__new__override, so wrapping e.g. aFastAPIinstance no longer widens its type. Update README to recommendapp = CorrelationIdMiddleware(app)overadd_middleware, since it also includes theX-Request-IDheader on unhandled500responses without a custom exception handler.I added a test to ensure the signatures ofthe
TYPE_CHECKING-only declaration and the real runtime declaration stay in sync.Fixes the issues posed in #109 and #104.
Problem
PR #109 recommends wrapping the app directly instead of using
add_middleware, since that's the only way to getX-Request-IDon unhandled500responses:However,
CorrelationIdMiddlewareis a@dataclasswhose__init__takes and returnsASGIApp. Static type checkers therefore widenapp's type fromFastAPIto the middleware's own type, breaking@app.get(...), dependency injection, and any other tooling that relies onappstayingFastAPI.We need a way to satisfy both:
app's static type must remain unchanged after wrapping.500responses without requiring extra user code.Solution
Expose
CorrelationIdMiddlewareto type checkers as aTYPE_CHECKING-only class whose__new__is annotated to return the same type it was given (via aTypeVarbound toASGIApp), instead of the usualSelf.At runtime,
CorrelationIdMiddlewareis just an alias for the real dataclass (renamed_CorrelationIdMiddleware). Therefore, its behavior,add_middleware()compatibility, and the public API are all unchanged. Only static analysis sees the shim.The README now recommends
app = CorrelationIdMiddleware(app)as the primary pattern (it also solves the500-response header problem for free), whileadd_middleware(...)remains documented as a supported alternative for users who don't need500coverage.Testing performed
All checks below were run against the actual installed package (not just isolated snippets), using
ty,pyright, andmypyas three independent type checkers, plus the existing test suite.Type preservation
Confirmed no
reportAttributeAccessIssueonapp.get(...)afterward.Existing registration path unaffected
Middleware(CorrelationIdMiddleware, header_name="X-Foo")(used byadd_middleware()and the test suite) still type-checks.isinstance/ variable annotationsWe also want
isinstancechecks, and inline annotations to work, so I tested thatworks with
pyright,ty, andmypy.Runtime behavior / full test suite
Known issues
mypy(only) reports an error on the stub's own definition:I verified this does not leak to downstream consumers: I installed the package into an isolated venv and ran
mypy(including--strict) against a script that I wrote to mimic a user's usage (had the lineapp = CorrelationIdMiddleware(app)) and I got zero errors, andreveal_typecorrectly showedFastAPI.mypydoesn't re-check/re-report errors inside already-installed dependencies unless you point it directly at their source, which means users won't get bother with this. Furthermore, since this repo's CI only runsty check, this doesn't affect the project's own CI either.In all, contributors who run
mypydirectly against middleware.py would see one[misc]warning on the stub definition. No consumer of the published package and no CI check in this repo is affected.Discarded alternatives
from_appclassmethod typed-> FastAPI.Adds a second, redundant construction path (
CorrelationIdMiddleware(app)vs.from_app(app)).Auto-registering a
500exception handler in__post_init__.Starlette's
Starlette.build_middleware_stack()usesadd_middleware(...), whereself.appis not theFastAPI/Starletteinstance; it's the next middleware in the stack or the router.Plain
TYPE_CHECKINGfunction stub (def CorrelationIdMiddleware(app: _AppType, ...) -> _AppType).Worked for the main use case, but broke
isinstance()and type annotations against the middleware.Subclassing the dataclass and overriding only
__new__(cls, app, *args: Any, **kwargs: Any) -> _AppType, to avoid manually mirroring every dataclass field in the stub signature.Verified with
ty,pyright, andmypythat*args: Any, **kwargs: Anysilently disables type-checking for every other keyword argument.