Skip to content

Conversation

@vincentsarago
Copy link
Member

closes #719
closes #720

This PR removes the middleware stack building and the add_middleware method to avoid conflict with the error handler middleware created by starlette later.

the middleware stack will be created by starlette on the first app call

@vincentsarago vincentsarago merged commit 4f410eb into main Jun 27, 2024
@vincentsarago vincentsarago deleted the patch/do-no-build-middleware-stack branch June 27, 2024 15:59
# add middlewares
for middleware in self.middlewares:
self.add_middleware(middleware)
self.app.user_middleware.insert(0, middleware)
Copy link
Member

Choose a reason for hiding this comment

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

@vincentsarago why use the internal user_middleware list here, and not just use add_middleware? Do we need to add middleware after a call is made to the FastAPI app, so you're trying to avoid an error with registering middleware after the initialization? That seems like it might not be a good pattern, to register middleware after initialization; if we aren't supporting that, thne self.app.add_middleware(middleware) seems better here.

Copy link
Member Author

Choose a reason for hiding this comment

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

good call, let's use self.app.add_middleware

🙏 thanks @lossyrob

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhhh I see why we can't use starlette's add_middleware.

The add_middleware method expect a list of Callable + arguments (which will be turned into Middleware object) https://github.com/encode/starlette/blob/5a1bec33f8d6a669a3670f51034de83292d19408/starlette/applications.py#L134-L142

But the StacApi.middlewares is a list of Middleware objects so we need to insert them into the user_middleware list ourself.

That's said, I think it would be good to raise the same error from starlette https://github.com/encode/starlette/blob/5a1bec33f8d6a669a3670f51034de83292d19408/starlette/applications.py#L140-L141

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.

Custom RequestValidationError Handling Overwritten in stac-fastapi Middleware Stack Rebuilt on Each StacApi Initialization

4 participants