Skip to content
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

♻️ Refactor logic to handle root_path to keep compatibility with ASGI and compatibility with other non-Starlette-specific libraries like a2wsgi #2400

Merged
merged 14 commits into from
Jan 9, 2024

Conversation

tiangolo
Copy link
Member

@tiangolo tiangolo commented Jan 7, 2024

Summary

♻️ Refactor logic to handle root_path to keep compatibility with ASGI and compatibility with other non-Starlette-specific libraries like a2wsgi (in the latest version).

Description

The ASGI spec specifies that path should include the full root_path, which is different from the equivalent in WSGI, that strips the root path equivalent. This was not the case in Starlette up to #2352 that fixed it and was released in Starlette 0.33.0.

Nevertheless, by that point, Starlette stopped modifying the root_path for mounted children ASGI apps. This means that children ASGI apps would share the same root_path as the top level Starlette app, when they were actually sub-mounted inside of Starlette, in a path prefix lower than whatever is the path prefix that Starlette was mounted on (the top level root_path). Because Starlette stopped modifying and increasing the root_path for child scopes, this means that ASGI apps mounted in Starlette with Mount would not be able to know what their actual path prefix was, to be able to handle their own routing after that prefix.

The way that other internal parts of Starlette dealt with it in that PR was by adding a couple of ASGI extension scope keys specific only to Starlette route_root_path and route_path. But that only worked for Starlette-specific components that knew about those (non-standard) scope keys.

This PR stops relying on those new scope keys (and removes them) and restores modifying the child root_scope. But keeps being ASGI compliant with the fix from #2352, keeping the full path including the root_path.

It also fixes a couple of internals that used to depend on the old behavior.

TestClient

The TestClient was also updated, when creating a TestClient, it received a root_path parameter without much information of what it would mean and how it would be used internally. In fact, it was not really used by the TestClient internally, it was just added to the ASGI scope as is.

This PR refactors that to make the client take the used root_path into account, if it is set, it will be passed to the ASGI apps, that will internally extract/remove it from the paths. But clients communicating with those ASGI apps, if they are indeed mounted at the defined root_path, would have to communicate with it using the root_path prefix. So this PR updates the client to actually require/use that, clients created with root_path would need to be used the same way that clients communicating with those ASGI apps mounted at some prefix path.

root_path

I think root_path in general is sometimes confusing, as it is about path prefixes that exist in some scenarios, for some parties, e.g. the final clients communicating through a proxy with a path prefix (e.g. /api/v2), but not for the other parties, e.g. the ASGI apps that has a root_path (e.g. the Starlette app that doesn't know anything about /api/v2).

It also applies to ASGI apps mounted inside of other things, like other ASGI apps, under a path prefix. For example, a Flask app mounted through a2wsgi WSGIMiddleware inside of a Starlette app at /oldapp. In this case, Starlette would know about the path prefix /oldapp, but the Flask app (through WSGIMiddleware) would not know about /oldapp, so, WSGIMiddleware would be the thing in charge of reading that /oldapp from the root_path passed to it in its ASGI scope (a child scope) and remove that path prefix from the paths passed to Flask.

I wrote a lot more about root_path with diagrams and stuff in the FastAPI docs: https://fastapi.tiangolo.com/advanced/behind-a-proxy/

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

…ing the child scope with its own root_pat, this allows sub-apps (e.g. WSGIMiddleware) to know

where it was mounted, and from which path prefix starts the path its sub-app should handle
…w about Starlette internals (e.g. the route_root_path extension scope key that was added in
… a root_path to pass the root_path, which is what clients would have to do if the app is

mounted.
@tiangolo tiangolo changed the title ♻️ Refactor logic to handle root_path to keep compatibility with ASGI and compatibility with other non-starlette specific libraries like a2wsgi ♻️ Refactor logic to handle root_path to keep compatibility with ASGI and compatibility with other non-Starlette specific libraries like a2wsgi Jan 7, 2024
@tiangolo tiangolo changed the title ♻️ Refactor logic to handle root_path to keep compatibility with ASGI and compatibility with other non-Starlette specific libraries like a2wsgi ♻️ Refactor logic to handle root_path to keep compatibility with ASGI and compatibility with other non-Starlette-specific libraries like a2wsgi Jan 7, 2024
@tiangolo tiangolo marked this pull request as ready for review January 7, 2024 21:06
@Kludex Kludex removed their request for review January 8, 2024 10:02
@Kludex Kludex mentioned this pull request Jan 8, 2024
1 task
Copy link
Member

@abersheeran abersheeran left a comment

Choose a reason for hiding this comment

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

Look good to me.

@abersheeran abersheeran merged commit c3c6314 into encode:master Jan 9, 2024
5 checks passed
@tiangolo tiangolo deleted the asgi-root-path branch January 9, 2024 06:22
@tiangolo
Copy link
Member Author

tiangolo commented Jan 9, 2024

Thanks @abersheeran! 🚀

@Pentusha
Copy link

Pentusha commented Feb 8, 2024

@tiangolo @abersheeran @Kludex

It seems there's a regression in 0.35.0 for mounts that starts with substring, that equals to root_path. For example, if root_path is set to /auth and there are sub-routes like /auth/ and /users/, the paths behind the reverse proxy become the following:
/auth/auth # have an issue since 0.35.0.
/auth/users # have not issue

It works just fine before 0.35.0 in both cases.
Should I create a separate issue for this?

I created a repository with the bare minimum to demonstrate regression.
This is a FastAPI example, but it seems to me that the real reason is this PR.

FastAPI app: https://github.com/Pentusha/fastapi-root-path-issue/blob/main/app/main.py
test cases: https://github.com/Pentusha/fastapi-root-path-issue/blob/main/tests/test_views.py
Logs for tests:

fastapi 0.108.0 & starlette 0.32.0.post1
pentusha at arco in ~/projects/misc/fastapi-root-path-issue (main) (fastapi-root-path-issue-7usYWfl9-py3.12) 
$ poetry add 'fastapi==0.108.0'

Updating dependencies
Resolving dependencies... (0.1s)

No dependencies to install or update

pentusha at arco in ~/projects/misc/fastapi-root-path-issue (main) (fastapi-root-path-issue-7usYWfl9-py3.12) 
$ PYTHONPATH=. poetry run pytest                                           
====================================================================== test session starts =======================================================================
platform linux -- Python 3.12.1, pytest-8.0.0, pluggy-1.4.0
rootdir: /home/pentusha/projects/misc/fastapi-root-path-issue
configfile: setup.cfg
plugins: anyio-4.2.0, asyncio-0.23.5a0
asyncio: mode=Mode.AUTO
collected 2 items                                                                                                                                                

tests/test_views.py ..                                                                                                                                     [100%]

======================================================================= 2 passed in 0.01s ========================================================================
fastapi 0.109.0 & starlette 0.35.1
pentusha at arco in ~/projects/misc/fastapi-root-path-issue (main) (fastapi-root-path-issue-7usYWfl9-py3.12) 
$ poetry add 'fastapi==0.109.0' 

Updating dependencies
Resolving dependencies... (0.2s)

Package operations: 0 installs, 2 updates, 0 removals

  • Updating starlette (0.32.0.post1 -> 0.35.1)
  • Updating fastapi (0.108.0 -> 0.109.0)

Writing lock file

pentusha at arco in ~/projects/misc/fastapi-root-path-issue (main) (fastapi-root-path-issue-7usYWfl9-py3.12) 
$ PYTHONPATH=. poetry run pytest
====================================================================== test session starts =======================================================================
platform linux -- Python 3.12.1, pytest-8.0.0, pluggy-1.4.0
rootdir: /home/pentusha/projects/misc/fastapi-root-path-issue
configfile: setup.cfg
plugins: anyio-4.2.0, asyncio-0.23.5a0
asyncio: mode=Mode.AUTO
collected 2 items                                                                                                                                                

tests/test_views.py .F                                                                                                                                     [100%]

============================================================================ FAILURES ============================================================================
_________________________________________________________________________ test_get_auth __________________________________________________________________________

client = <httpx.AsyncClient object at 0x7e480fe53050>

    async def test_get_auth(client):
        resp = await client.get('/auth/')
>       assert resp.raise_for_status().json() == {
            'path': '/auth/',
            'raw_path': '/auth/',
            'root_path': '/auth',
        }

tests/test_views.py:12: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <Response [404 Not Found]>

    def raise_for_status(self) -> "Response":
        """
        Raise the `HTTPStatusError` if one occurred.
        """
        request = self._request
        if request is None:
            raise RuntimeError(
                "Cannot call `raise_for_status` as the request "
                "instance has not been set on this response."
            )
    
        if self.is_success:
            return self
    
        if self.has_redirect_location:
            message = (
                "{error_type} '{0.status_code} {0.reason_phrase}' for url '{0.url}'\n"
                "Redirect location: '{0.headers[location]}'\n"
                "For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/{0.status_code}"
            )
        else:
            message = (
                "{error_type} '{0.status_code} {0.reason_phrase}' for url '{0.url}'\n"
                "For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/{0.status_code}"
            )
    
        status_class = self.status_code // 100
        error_types = {
            1: "Informational response",
            3: "Redirect response",
            4: "Client error",
            5: "Server error",
        }
        error_type = error_types.get(status_class, "Invalid status code")
        message = message.format(self, error_type=error_type)
>       raise HTTPStatusError(message, request=request, response=self)
E       httpx.HTTPStatusError: Client error '404 Not Found' for url 'http://test/auth/'
E       For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/404

../../../.cache/pypoetry/virtualenvs/fastapi-root-path-issue-7usYWfl9-py3.12/lib/python3.12/site-packages/httpx/_models.py:759: HTTPStatusError
==================================================================== short test summary info =====================================================================
FAILED tests/test_views.py::test_get_auth - httpx.HTTPStatusError: Client error '404 Not Found' for url 'http://test/auth/'
================================================================== 1 failed, 1 passed in 0.04s ===================================================================

Related discussion: #2495

maartenbreddels added a commit to maartenbreddels/py-shiny that referenced this pull request Jun 28, 2024
When a shiny app is running under a prefix (root_path in ASGI terms),
the static files are not served correctly under pyodide.
This is because the ASGI path includes the prefix, and the root_path
should be removed.
Starlette 0.33 and 0.34 however, did not set root_path correctly,
and in those cases, we have to rely on route_path.

Related discussions:
 encode/starlette#2400
 encode/starlette#2361

A related fix we had in Solara:
 widgetti/solara#413

But this fix also did not seem to work for our situation at https://py.cafe

I logged the output of the relevant entries in the scope dict, together
with the starlette version:

```
scope 0.32.0 {'path': '/strftime-min.js', 'root_path': '/_app/lib/strftime-0.9.2'}
scope 0.33.0 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '', 'route_root_path': '/lib/strftime-0.9.2', 'route_path': '/strftime-min.js'}
scope 0.34.0 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '', 'route_root_path': '/lib/strftime-0.9.2', 'route_path': '/strftime-min.js'}
scope 0.35.0 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '/_app/lib/strftime-0.9.2'}
scope 0.36.0 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '/_app/lib/strftime-0.9.2'}
scope 0.37.2 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '/_app/lib/strftime-0.9.2'}
```

This was using a similar situation as on py.cafe:
```python
....
routes = [
    Mount('/static', app=app_static),
    Mount('/_app', app=app_shiny)
]

app = Starlette(routes=routes)
```

Which led me to the following fix, making shiny work under pyodide
in combination with a prefix with the above mentioned versions of starlette.
maartenbreddels added a commit to maartenbreddels/py-shiny that referenced this pull request Jun 28, 2024
When a shiny app is running under a prefix (root_path in ASGI terms),
the static files are not served correctly under pyodide.
This is because the ASGI path includes the root_path, and the root_path
should be removed.
Starlette 0.33 and 0.34 however, did not set root_path correctly,
and in those cases, we have to rely on route_path.

Related discussions:
 encode/starlette#2400
 encode/starlette#2361

A related fix we had in Solara:
 widgetti/solara#413

But this fix also did not seem to work for our situation at https://py.cafe

I logged the output of the relevant entries in the scope dict, together
with the starlette version:

```
starlette 0.32.0 {'path': '/strftime-min.js', 'root_path': '/_app/lib/strftime-0.9.2'}
starlette 0.33.0 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '', 'route_root_path': '/lib/strftime-0.9.2', 'route_path': '/strftime-min.js'}
starlette 0.34.0 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '', 'route_root_path': '/lib/strftime-0.9.2', 'route_path': '/strftime-min.js'}
starlette 0.35.0 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '/_app/lib/strftime-0.9.2'}
starlette 0.36.0 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '/_app/lib/strftime-0.9.2'}
starlette 0.37.2 {'path': '/_app/lib/strftime-0.9.2/strftime-min.js', 'root_path': '/_app/lib/strftime-0.9.2'}
```

This was using a similar situation as on py.cafe:
```python
....
routes = [
    Mount('/static', app=app_static),
    Mount('/_app', app=app_shiny)
]

app = Starlette(routes=routes)
```

Which led me to the following fix, making shiny work under pyodide
in combination with a prefix with the above mentioned versions of starlette.
DaGenix added a commit to DaGenix/hypercorn that referenced this pull request Sep 29, 2024
…spec

Before this change, the dispatcher middleware didn't do anything with
'root_path'. It did, however, modify 'path'.

The problem with this behavior, is that the child ASGI app has no way to
determine what its prefix it. And if it can't determine its prefix, it
doesn't know how to construct URLs.

The ASGI spec isn't super clear on the expected behavior. However, some
resources to review are:

* https://asgi.readthedocs.io/en/latest/specs/www.html#wsgi-compatibility
* encode/starlette#2400
* django/asgiref#229

Based on the above, I believe that the correct behavior is that
"root_path" should be updated by the dispatcher middleware but that
"path" should not be modified.

In addition to the above change, I also updated the tests. And I also
added a new test case where the dispatcher middleware is nested inside
of itself.
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.

4 participants