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

WSGIMiddleware behavior after Starlette update #41

Closed
tarsil opened this issue Dec 7, 2023 · 2 comments
Closed

WSGIMiddleware behavior after Starlette update #41

tarsil opened this issue Dec 7, 2023 · 2 comments

Comments

@tarsil
Copy link

tarsil commented Dec 7, 2023

Hey @abersheeran ,

I think here is more adequate since its related with this middleware. This is a continuation of the conversation we were having.

After the update to 1.9.0, this example is still not working. The 404 received is from flask which is great meaning its trying to understand it.

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

After some testing and trying one of your suggestions, the only way we could make it work (which I assume its not the right way at all), was to change (to test it) the variable being read and received, in this case by Starlette

def build_environ(scope: Scope, body: Body) -> Environ:
    """
    Builds a scope and request body into a WSGI environ object.
    """
    script_name = scope.get("root_path", "").encode("utf8").decode("latin1")
    path_info = scope["path"].encode("utf8").decode("latin1")
    if path_info.startswith(script_name):
        path_info = path_info[len(script_name):]

    script_name_environ_var = os.environ.get("SCRIPT_NAME", "")
    if script_name_environ_var:
        script_name = unicode_to_wsgi(script_name_environ_var)

    environ = {
        "asgi.scope": scope,
        "REQUEST_METHOD": scope["method"],
        "SCRIPT_NAME": script_name,
        "PATH_INFO": path_info,
        "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,
    }

What made it work, was changing the path_info = scope["path"].encode("utf8").decode("latin1") to path_info = scope["route_path"].encode("utf8").decode("latin1") which it doesn't seem right to me since the path should have been the one being read, correct?

This is what I was trying to explain before. Somehow, I do not think the path is being updated properly or there is something else we might be missing.

So, on your middleware, that is what I found, or if you say it is correct (which I do believe it is), then to make it work, only changing the Starlette side to come back to

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

Which for my understanding, that won't be the case.

Do you have any thought on this? Because a lot of applications rely on this amazing work you have done.

@abersheeran
Copy link
Owner

abersheeran commented Dec 8, 2023

You can use starlette 0.32 and a2wsgi 1.8 first. Issues related to this are being discussed at django/asgiref#424. I want to wait for the results of the discussion before modifying starlette's PR.

@tarsilon
Copy link

tarsilon commented Dec 8, 2023

@abersheeran yap, I can confirm indeed that with Starlette <0.33.0 and the middleware with that version, works as intended (as we knew).

Thank you for your contribution and help on this.

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

No branches or pull requests

3 participants