Skip to content

docs: Update default middleware setup example - #109

Open
sondrelg wants to merge 1 commit into
mainfrom
docs-refactor
Open

sondrelg wants to merge 1 commit into
mainfrom
docs-refactor

Conversation

@sondrelg

Copy link
Copy Markdown
Member

It makes sense to want the default setup to cover 500-range responses.

It makes sense to want the default setup to cover 500-range responses.
@codecov

codecov Bot commented May 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.59%. Comparing base (974653d) to head (69e05d2).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #109   +/-   ##
=======================================
  Coverage   96.59%   96.59%           
=======================================
  Files          11       11           
  Lines         440      440           
=======================================
  Hits          425      425           
  Misses         15       15           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sebastian-correa sebastian-correa left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point about the type [ref] is that if we use this pattern, the app stops being a FastAPI instance.

This is ultimately user choice, since it's only a documentation change, but it may introduce lots of squigglies in code.

Image

You can cast it with typing.cast, but that's a bit hacky.

Maybe we need to move from a dataclass to a normal class and use some typing magic, or add a from_app classmethod whose typehint is still FasAPI, or look into using the post_init to auto register the handler for errors, or something.

@microwavenby

Copy link
Copy Markdown

Maybe we need to move from a dataclass to a normal class and use some typing magic, or add a from_app classmethod whose typehint is still FasAPI, or look into using the post_init to auto register the handler for errors, or something.

It definitely results in squigglies when it comes to trying to access the test_client in pytest --

AttributeError: 'CorrelationIdMiddleware' object has no attribute 'test_client'

@sondrelg

Copy link
Copy Markdown
Member Author

I both think:

  • Changing the type of app will break a lot of tooling and seems close to a hard blocker
  • Returning request IDs for 500s is pretty much always desirable

Do you reckon there's a way to satisfy both of these, or is it a hard trade-off?

@microwavenby

Copy link
Copy Markdown

FastAPI's docs include support for an exception handler:
https://fastapi.tiangolo.com/tutorial/handling-errors/#install-custom-exception-handlers

And, it's possible to override the default exception handlers
https://fastapi.tiangolo.com/tutorial/handling-errors/#override-the-default-exception-handlers

I'm guessing the right way to go about this is to also add an exception handler that generates or repeats the correlation-id from the request headers into the logs and/or response?

@sebastian-correa

Copy link
Copy Markdown

Sorry, I'm quite late on this. GitHub notifications don't work well for me.

I proposed an idea in #114. I think it ticks both boxes @sondrelg proposed (which I think should be what we aim for).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants