Revert including ASGI root_path in url to validate #47
Conversation
This file contains 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
Starlette instantiates middleware lazily now so `app.build_middleware_stack()` have been added to the tests checking for exceptions when the `MAuthASGIMiddleware` class is instantiated.
The ASGI spec states that path should always include root_path already. Starlette did not correctly implement this part of the spec, but as of the v0.35.0 release now does.
ejinotti-mdsol
approved these changes
Jan 23, 2024
Yes, I forgot to add those. Will do so in a few. |
ykitamura-mdsol
approved these changes
Jan 23, 2024
|
I think it's going to make conflicts so I'll rebase my PR after this one |
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.
The issue I attempted to fix in #46 it turns out was due to Starlette not implementing that part of the ASGI spec correctly. path is supposed to already include root_path. It looks like this was fixed in a combination of encode/starlette#2352 and encode/starlette#2400 with the latter one being the change that made the incorrect fix I made start to break with Starlette >= 0.35.0.
I have updated FastAPI used in the tests to the latest version and fixed a few unrelated tests that broke because of that. I also reverted the code change I made in #46 but left the test.