-
-
Notifications
You must be signed in to change notification settings - Fork 954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue with middleware args passing #2752
Conversation
starlette/middleware/__init__.py
Outdated
@@ -38,5 +36,6 @@ def __repr__(self) -> str: | |||
class_name = self.__class__.__name__ | |||
args_strings = [f"{value!r}" for value in self.args] | |||
option_strings = [f"{key}={value!r}" for key, value in self.kwargs.items()] | |||
args_repr = ", ".join([self.cls.__name__] + args_strings + option_strings) | |||
cls_name = self.cls.__name__ # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe to do? I guess the point is that the self.cls
isn't necessarily a class, it may be a function, but even functions have __name__
defined. Maybe we should do getattr(self.cls, "__name__", "")
or something to make sure we don't error in any case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(alternatively we could add __name__
to the protocol above 😆 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adriangb Thanks for suggestion, changed it to be getattr
with default value call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Are there any actual changes to the reprs? I'm not seeing why that code had to change (aside from what I pointed out in this comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were no actual changes to the repr. I changed it only because of mypy error.
Thanks! |
Upgrading from 0.41.2 to 0.41.3 gives me the following type error
for the following line of code app.add_middleware(ExceptionHandlerMiddleware) where The middleware is like this from starlette.types import ASGIApp, Receive, Scope, Send
class ExceptionHandlerMiddleware:
def __init__(self, app: ASGIApp):
self.app = app
async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
# Implementation here
... I'm guessing it's a regression resulting from this PR. |
@arni-xalgo-io There is discussion about this issue - #2757 |
Summary
A feature introduced in #2381 doesn't allow to pass args inside middleware, here is an example:
Also, make type annotation for middleware less strict, remove restriction for middleware to be class, and make it callable that returns ASGI application, it's a fully backward-compatible change.
Checklist