#18 fix: Address issue with decorator #19
Conversation
There was a problem hiding this comment.
Hey @glinmac thank you so much for the PR! I'm sorry the CI broke. I've corrected the problem, so if you rebase your branch on top of the latest master, it should now complete the checks.
I've added a few comments as well, let me know if you disagree with anything.
| @@ -354,6 +354,10 @@ def _inject_headers( | |||
| self, response: Response, current_limit: Tuple[RateLimitItem, List[str]] | |||
| ) -> Response: | |||
| if self.enabled and self._headers_enabled and current_limit is not None: | |||
| if not isinstance(response, Response): | |||
There was a problem hiding this comment.
What do you think of using assert isinstance(response, Response), "your error message here" instead? It allows a tiny optimisation when running python in non-debug mode (and then I'd argue that the error is on the user if they still have invalid code).
Edit: I just noticed that the original code uses the pattern you used as well. Maybe I should update that part as well. What do you think?
There was a problem hiding this comment.
Yes, I re-used the pattern you had for the Request. I don't have a strong opinion for each. I had to migrate a few endpoints and add the request/response, during this process I forgot a few so having the explicit exception helped a bit.
For FastAPI, I am actually thinking it would be better to wrap the functionality as a dependency rather than a decorator and this would drop the need to have these checks and the requirement to add request and response to the endpoint.
I don't have enough knowledge with Starlette to see if something similar could help.
There was a problem hiding this comment.
That's a neat idea! I haven't seen anything similar for Starlette (Dependency injection), so I'd want to check a bit more to make sure this does not require adding lots more code just to do the same thing in a different way. If it's just a different entry point into the same functions, it would be pretty cool! I've opened #23 to track this specifically outside of the current PR.
This reverts commit 92c9963.
The value returned by an endpoint may not be an instance or subclass of `Response`, in this case FastAPI builds the actual Response at an upper level in the middleware stack. This patch allows the decorator to inspect the endpoint to retrieve the associated `Response` to allow headers to be injected.
glinmac
force-pushed
the
fix/fastapi-limit-decorator
branch
from
f3224ff
to
755bf44
Compare
|
@laurentS thanks for fixing the build. The build should be green now and taking into account your comments. |
The value returned by an endpoint may not be an instance or subclass
of
Response, in this case FastAPI builds the actual Responseat an upper level in the middleware stack.
This patch allows the decorator to inspect the endpoint to retrieve the
associated
Responseto allow headers to be injected.This should resolve #18