Starlette version 0.33.0

[tarsil]

Hello,

After upgrading to Starlette 0.33.0 I came across an issue using the WSGIMiddleware recommended by the documentation. Before it was finding properly the URL but now, it returns a 404.
My understanding of this is from the latest changes done with the matches to be ASGI compatible (100%) but I'm not sure if this covers all the scenarios.

The example you can run is this:

from flask import Flask, request
from starlette.applications import Starlette
from starlette.routing import Mount, Route
from a2wsgi.wsgi import WSGIMiddleware


@flask_app.route("/")
def flask_main():
    name = request.args.get("name", "Starlette")
    return f"Hello, {escape(name)} from Flask!"

echo_paths_routes = [
    Mount(
        "/flask",
        name="mount",
        app=WSGIMiddleware(flask_app),
    ),
]

def test_paths_with_root_path(test_client_factory):
    app = Starlette(routes=echo_paths_routes)
    client = test_client_factory(app, base_url="https://www.example.org/")
    response = client.get("/flask")
    assert response.status_code == 200

This before the release was returning 200 but now its a simple 404. What Am I missing? For instance, I understand that FastAPI overrides the __call__ in the application to generate the root_path and I think something needs to be updated somehow?

This also happens if you do a Starlette application and add it into the Mount inside another Starlette application.

Example:

app = Starlette(
    routes=[Mount('/another', app=Starlette(...))]
)

[tarsil]

@Kludex not sure if you could help?

[tarsil]

Apparently, this is also being reflected here.

widgetti/solara#410

As far as I could also internally debug and based on the ASGI assumption that originated the new route_path , the final doesn't contain the root_path + path as per ASGI specification (as far as I could read) but this last one I can't be 100% sure as I haven't run a lot of tests on it but what it was reported by solara reflects exactly the errors I was also presenting here.

[Kludex]

I need to investigate a bit.

About solara, @maartenbreddels told me it was on them. 👀

[tarsil]

Thank you @Kludex . Solara I found yesterday and thankfully it is not Starlette then.

The rest I literally had some tests using exactly the example I put here and since the last version, not passing anymore giving a 404.

I will investigate more as well on my side and if I find any solution beforehand, I will share.

[maartenbreddels]

Still looking into it, what I currently see is that with

starlette_routes = [
    Route("/", endpoint=myroot),
    Mount("/solara_mount/", routes=solara.server.starlette.routes),
]

starlette_app = Starlette(routes=starlette_routes)

I never see scope['root_path'] being set to '/solara_mount', although scope['route_root_path'] is.
This does not seem to match what I see in the unit tests added in #2352 so I'm not sure what is going on.

[tarsil]

@maartenbreddels yes because before 0.33.0 the “path” and “root_path” were being set in the child_scope and now its “route_root_path” and “route_path” and actually that is what is blowing up a few of our cases too.

When i changed (for testing) those new values to the previous version (path and root_path) in the child_scope, it started to work again.

[maartenbreddels]

Ok, the test also sets the root_path argument here: https://github.com/encode/starlette/pull/2352/files#diff-d161c06bc3b5d600a6118ab40b4194a81d226f5f3fdbe17022dc501d823a4f95R1250 which confused me.

But now, when my solara routes are mounted under /solara_mount, I get passed root_path='' and path=/solara_mount/.
How should my endpoint know that it should handle /? Should I look into the non-standard route_root_path and remove that prefix?

[tarsil]

@maartenbreddels this for testing to prove the new way works but in theory we should not do it, I think. @Kludex is investigating on his side as well and he is pretty good in these type of things 👍🏼

Another finding that I found is that the WSGIMiddleware sets the PATH from the scope['path'] whereas the new version uses the route_path but it still remains the root_path issue when mounting.

From the WSGIMiddleware:

ef build_environ(scope: Scope, body: Body) -> Environ:
    """
    Builds a scope and request body into a WSGI environ object.
    """
    allow_rewrite_environ = {
        "SCRIPT_NAME": scope.get("root_path", "").encode("utf8").decode("latin1"),
    }
    for key in allow_rewrite_environ.keys():
        environ_var = os.environ.get(key, "")
        if environ_var:
            allow_rewrite_environ[key] = unicode_to_wsgi(environ_var)

    environ = {
        **allow_rewrite_environ,
        "asgi.scope": scope,
        "REQUEST_METHOD": scope["method"],
        "PATH_INFO": scope["path"].encode("utf8").decode("latin1"),
        "QUERY_STRING": scope["query_string"].decode("ascii"),
        "SERVER_PROTOCOL": f"HTTP/{scope['http_version']}",
        "wsgi.version": (1, 0),
        "wsgi.url_scheme": scope.get("scheme", "http"),
        "wsgi.input": body,
        "wsgi.errors": sys.stdout,
        "wsgi.multithread": True,
        "wsgi.multiprocess": True,
        "wsgi.run_once": False,
    }

This leads me to believe the path is the correct way as per documentation of the asgiref where the scope contains root_path and path being set.

[maartenbreddels]

widgetti/solara#413 seems to work with starlette 0.32 and 0.33. Would love @Kludex 's opinion on this.
I think Starlette's old behavior is better. I'm not sure if it makes sense to not change root_path, since when you mount a subroute, you want that endpoint to behave as if SCRIPT_NAME is set, similar to how you'd have it running under a reverse proxy.

[tarsil]

I agree @maartenbreddels mostly because the ASGI documentation also states the set of a path and root_path being set in the scope.

I'm sure @Kludex has some insights on this and how we can fix without breaking an conformity with the ASGI references.

Starlette prior to 0.33.0 has those and this is why it works for you and basically for all of us but there is more into it and lets allow @Kludex run his own investigation in the matter.

[maartenbreddels]

I wonder if root_path not being set is simply a bug in 0.33? Meaning that without root_path being set here in the test at https://github.com/encode/starlette/pull/2352/files#diff-d161c06bc3b5d600a6118ab40b4194a81d226f5f3fdbe17022dc501d823a4f95R1250
the tests should also run.

[tarsil]

Hm, not sure if it was a bug or simply removed because I can see an route_root_map that previously was the root_map but again, the team should know better about this

[tarsil]

@Kludex let me know if you need a hand on anything 👍🏼

[tarsil]

@maartenbreddels @Kludex after a lot of tests I still have the same errors. Everything that I posted here, the examples, the ASGI docs it's where is failing. 0.33.0 change is causing this.

Not to put pressure, just to confirm that this causes an issue

[Kludex]

@abersheeran do you know something obvious that I did wrong here?

[abersheeran]

starlette/starlette/routing.py

Lines 433 to 439 in d29d993

	root_path = scope.get("root_path", "")
	child_scope = {
	"path_params": path_params,
	"route_root_path": root_path + matched_path,
	"route_path": remaining_path,
	"endpoint": self.app,
	}

I thought about it carefully, and if we want to maintain the old usage of Mount, we should additionally modify the root_path here.

Then, I will then push a new version of a2wsgi to accommodate the new root_path and path. abersheeran/a2wsgi#40

---

But honestly, I think the definition of root_path and path in the ASGI standard is a failure, and it breaks the consistent usage of WSGI SCRIPT_NAME and PATH_INFO. This may cause many problems for users... maybe we will receive a lot of issues about this

[tarsil]

I know where the 404 is coming and that is why I tried to provide as much info of my tests as possible and how I got also to the code of the Middleware. @abersheeran thank for your feedback as well. Apologies @Kludex

[Kludex]

@abersheeran If I understand correctly what you said, I didn't do anything wrong. It's just that we now comply with the spec completely, and it breaks some cases. Is that correct?

But honestly, I think the definition of root_path and path in the ASGI standard is a failure, and it breaks the consistent usage of WSGI SCRIPT_NAME and PATH_INFO. This may cause many problems for users... maybe we will receive a lot of issues about this

We can talk about it on the asgiref repository. How do you think it should be?

[abersheeran]

@abersheeran If I understand correctly what you said, I didn't do anything wrong. It's just that we now comply with the spec completely, and it breaks some cases. Is that correct?

Yes, now the code should conform to the ASGI standard.

[abersheeran]

We can talk about it on the asgiref repository. How do you think it should be?

Maybe we should investigate how various ASGI frameworks and servers implement this.

[maartenbreddels]

I do not think starlette 0.33 conforms to the ASGI standard now.

The tests pass because of the root_path argument passed to test_client_factory at:

• https://github.com/encode/starlette/pull/2352/files#diff-d161c06bc3b5d600a6118ab40b4194a81d226f5f3fdbe17022dc501d823a4f95R1250

It should give the same results without that argument (I think).

[tarsil]

@abersheeran I'm a little bit confused so apologies if I will say something wrong if I don't have the whole scope of the problem but according to the asgi read the docs

The scope should contain "path" and "root_path" (let's ignore the right or wrong approach for this purpose).

As far as I could see in the latest release of Starlette, that was renamed to "route_path" and "route_root_path" and passed into the scope.

You mentioned above this now conforms with the ASGI spec. What am I missing here? Maybe I'm not understanding the whole problem but based on the image, it looks like it is not but again, I might be totally wrong (which is probably the case).

Would you mind helping me understand what I'm not seeing, please?

EDIT: Unless what I'm referring as child scope (in case of the mount) its just internal and then somewhere it builds the "path" and "root_path" in the final scope from those fields.

[Kludex]

The path and root_path are being passed... I just removed the redundancy.

You can print the scope on the WSGIMiddleware and confirm.

[tarsil]

I was just running the examples and I understand the explanation. but even after the the upgrade of a2wsgi to 1.9.0 (thank you @abersheeran), I still get 404.

@abersheeran it is weird that only returns the expected 200 if the path inside the path_info = scope["path"] uses the route_path instead and here it is where all my confusion started.

@Kludex got it now what you meant by not "overriding the path and root_path".

@abersheeran using the example above

from flask import Flask, request
from starlette.applications import Starlette
from starlette.routing import Mount, Route
from a2wsgi.wsgi import WSGIMiddleware


@flask_app.route("/")
def flask_main():
    name = request.args.get("name", "Starlette")
    return f"Hello, {escape(name)} from Flask!"

echo_paths_routes = [
    Mount(
        "/flask",
        name="mount",
        app=WSGIMiddleware(flask_app),
    ),
]

In theory should return a 200 when calling the /flask but it still does not even after you latest released (which I believe happened during this conversation), so my real question is, what should we actually do and how many cases this will break for all the existing applications already using Starlette with this middleware, basically.

[abersheeran]

Please try #2364

[tarsil]

@abersheeran it still does not work. What is works is if you do

"root_path": root_path + matched_path,
"path": remaining_path,