Turn `scope["client"]` to `None` on `TestClient` #2377

aminalaee · 2023-12-16T14:39:05Z

Summary

Related to: #2295

I suggest to not include the client scope, instead of always setting it to None. According to the ASGI spec this has the same effect.

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.

Kludex · 2023-12-16T15:02:55Z

tests/test_testclient.py

-def test_client(test_client_factory):
-    async def app(scope, receive, send):
-        client = scope.get("client")
-        assert client is not None
-        host, port = client
-        response = JSONResponse({"host": host, "port": port})
-        await response(scope, receive, send)
-
-    client = test_client_factory(app)
-    response = client.get("/")
-    assert response.json() == {"host": "testclient", "port": 50000}


I didn't see this test before... Can we have a background of why it exists?

It was added here: #1462
I think it was not intentional, there’s another test in test_requests.py which is fine, but not needed for testclient.

Kludex · 2023-12-18T12:10:35Z

Ok. I actually don't have any opinion here. If some other member approves it, I'm fine. 👍

Kludex · 2023-12-22T15:56:16Z

Do you mind if are explicit on the "client": None? Even if the spec says "is the same".

Since PR encode/starlette#2377, we can no longer expect scope["client"] to return a tuple.

Seems that client is optional according to the ASGI spec. https://asgi.readthedocs.io/en/latest/specs/www.html With Starlette 0.35 the TestClient connection scope is None for "client". encode/starlette#2377 Signed-off-by: moson <moson@archlinux.org>

This was removed in encode/starlette#2377

The 'client' scope was removed in encode/starlette#2377, which means we can't read the client IP from it in unit tests

bordax · 2024-02-15T18:51:48Z

Can I override scope["client"] from TestClient? I have several places in a FastAPI app where I use Request.client.host to get the client's IP and this change broke my tests.

karpenkotakeoff · 2024-02-16T10:18:36Z

Can I override scope["client"] from TestClient? I have several places in a FastAPI app where I use Request.client.host to get the client's IP and this change broke my tests.

Faced the same problem

petemounce · 2024-02-16T15:22:08Z

This broke my tests, noticed when upgrading fastapi to address a security advisory :(

aholmes · 2024-02-16T19:22:22Z

I'm facing the same problem w/ broken tests. These fail because I am accessing client here. The runtime result is that applications using this middleware are unable to log requests (the whole request fails). Removing the erroring line will cause the log to not contain the host:port from the client.

It appears that, for the actual application, client is set. On testing requests I receive the log line I'm expecting, e.g., Remote address: 127.0.0.1:5346.

All that said, while initially this seems like a bug in Starlette, the ASGI spec states:

client (Iterable[Unicode string, int]) – A two-item iterable of [host, port], where host is the remote host’s IPv4 or IPv6 address, and port is the remote port as an integer. Optional; if missing defaults to None.

Although this is a frustrating breaking change, it seems the onus is on us (users of Starlette) to adhere to the spec.

Kyle-sandeman-mrdfood · 2024-02-20T16:03:57Z

As others have commented above, could we please revert this?
Just because something does not seem used doesn't mean you should just "rip it out"

Although it does not help any of us that the ASGI spec is not clear when it can/should be "missing".
As aholmes mentions above, it is used for logging the client info.
What is "improved" by changing the behaviour between TestClient and the real one?

bordax · 2024-02-20T16:37:24Z

Since client is optional per ASGI spec, I think that this change is OK. What can be done to improve it is to make scope["client"] overridable when instantiating TestClient. Doing a quick code inspection I found that this value is set at test_client.py::_TestClientTransport and it doesn't seem difficult to make it modifiable from outside of TestClient.

…2377

aminalaee · 2024-02-21T08:54:46Z

Not sure what is the point of testing Request.client.host when it should be mocked in the tests.
Other than that I think we can add it in the TestClient or revert this if it doesn’t make sense. But checking other ASGI frameworks like Daphne and Quart I see they have implemented it exactly by removing the ‘client’ in the TestClient, so this looks more like a wrong way of testing it.

Kyle-sandeman-mrdfood · 2024-02-21T09:14:13Z

Is there something fundamental I (and others) am/are missing here?
The test client is the client, why should request.client be None?
It makes more sense to behave similarly to production than "set it to None because [insert unknown reason for this change]"

aminalaee · 2024-02-22T09:25:38Z

Because it is not a normal client, it is a test client which means at this stage where scope is being set up, no connection is started yet and it's hard-coding the host and port ('testclient', 5000) and is odd because anyway it's an invalid value and the test should mock the Request.client rather than relying on a wrong value.

But anyway this is the reason behind it and based on the comments if maintainers want to change this in Starlette:

we can add "client" argument in TestClient
we can revert this to hard-code the value

Kyle-sandeman-mrdfood · 2024-02-22T09:43:57Z

Apologies if I was blunt before and thank you for explaining.

From my side, I would expect a codebase that works in production to work similarly when a TestClient is thrown at it.
None to me is saying "there is no client". I'll reiterate that if the request.client property could explain when it can be None then we'd at least know what to expect.
At this time neither starlette nor ASGI seem to explain this and "its up to you to handle None but we don't define when it happens" is not a nice answer/experience.

rather than relying on a wrong value

We didn't rely on the actual value, just that it was set. We log the host for auditing reasons.

This change has forced me to use request.client.host if request.client else "unknown" which was never needed for production.
Of course this is not the end of the world, but it shouldn't be necessary

dantownsend · 2024-02-22T11:49:11Z

we can add "client" argument in TestClient

I think this is a good idea.

Setting the client to None broke some unit tests we have for rate limiting middleware.

dantownsend · 2024-02-22T12:32:12Z

My workaround for now is using httpx.AsyncClient directly, instead of TestClient.

This reverts commit 483849a.

aholmes · 2024-02-22T17:39:30Z

@aminalaee

... the test should mock the Request.client rather than relying on a wrong value.

Can you please expand on why a test should mock these aspects of the fixture? It is my impression that that the client, the "server"/application, request, and response are all components that TestClient is responsible for.

This reverts commit 483849a.

) * Revert "Turn `scope["client"]` to `None` on `TestClient` (#2377)" This reverts commit 483849a. * format * Add type hints --------- Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

…" (encode#2525) * Revert "Turn `scope["client"]` to `None` on `TestClient` (encode#2377)" This reverts commit 483849a. * format * Add type hints --------- Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

aminalaee force-pushed the fix-testclient-client-scope branch from 3dca547 to 55c8602 Compare December 16, 2023 14:43

aminalaee requested review from Kludex and adriangb and removed request for Kludex December 16, 2023 15:02

Kludex reviewed Dec 16, 2023

View reviewed changes

aminalaee requested a review from Kludex December 16, 2023 15:02

Kludex requested a review from tomchristie December 16, 2023 15:08

Kludex added the hold Don't merge it label Dec 16, 2023

aminalaee added 2 commits December 16, 2023 20:54

Remove client scope from testclient

f90a743

remove test

f44203c

aminalaee force-pushed the fix-testclient-client-scope branch from 5b4d284 to f44203c Compare December 16, 2023 19:54

Kludex removed the hold Don't merge it label Dec 18, 2023

Kludex and others added 2 commits December 22, 2023 16:56

Merge branch 'master' into fix-testclient-client-scope

e0010c0

Update testclient.py

c5fa90c

Kludex changed the title ~~Remove client scope from testclient~~ Turn scope["client"] to None on TestClient Dec 25, 2023

Kludex approved these changes Dec 25, 2023

View reviewed changes

Kludex merged commit 483849a into master Dec 25, 2023
5 checks passed

Kludex deleted the fix-testclient-client-scope branch December 25, 2023 09:33

torarvid added a commit to kolonialno/troncos that referenced this pull request Jan 12, 2024

fix: Logging middleware accepts TestClient without scope["client"]

8c79aff

Since PR encode/starlette#2377, we can no longer expect scope["client"] to return a tuple.

torarvid mentioned this pull request Jan 12, 2024

fix: Logging middleware accepts TestClient without scope["client"] kolonialno/troncos#283

Merged

torarvid added a commit to kolonialno/troncos that referenced this pull request Jan 12, 2024

fix: Logging middleware accepts TestClient without scope["client"]

dc11c08

Since PR encode/starlette#2377, we can no longer expect scope["client"] to return a tuple.

imjoehaines added a commit to bugsnag/bugsnag-python that referenced this pull request Jan 24, 2024

'clientIp' is no longer set when using TestClient

e02b68f

This was removed in encode/starlette#2377

imjoehaines added a commit to bugsnag/bugsnag-python that referenced this pull request Jan 24, 2024

'clientIp' can no longer be set with TestClient

c799799

The 'client' scope was removed in encode/starlette#2377, which means we can't read the client IP from it in unit tests

imjoehaines added a commit to bugsnag/bugsnag-python that referenced this pull request Jan 24, 2024

'clientIp' can no longer be set with TestClient

9dcd5a2

The 'client' scope was removed in encode/starlette#2377, which means we can't read the client IP from it in unit tests

imjoehaines mentioned this pull request Jan 24, 2024

Fix ASGI test bugsnag/bugsnag-python#370

Merged

imjoehaines added a commit to bugsnag/bugsnag-python that referenced this pull request Jan 24, 2024

'clientIp' can no longer be set with TestClient

8c650a8

The 'client' scope was removed in encode/starlette#2377, which means we can't read the client IP from it in unit tests

This was referenced Feb 16, 2024

In the scaffolder, handle application and endpoint names with Python module-unfriendly characters like hyphens uclahs-cds/BL_Python#40

Merged

Starlette 0.35.0 contains breaking change involving BL_Python.web request logging uclahs-cds/BL_Python#41

Closed

Pokegali added a commit to aeecleclair/Hyperion that referenced this pull request Feb 20, 2024

Update fastapi to 0.109.2 and fix bug introduced by encode/starlette#…

6e7bac9

…2377

sinisaos mentioned this pull request Feb 22, 2024

RateLimitingMiddleware tests failed piccolo-orm/piccolo_api#267

Closed

aminalaee added a commit that referenced this pull request Feb 22, 2024

Revert "Turn scope["client"] to None on TestClient (#2377)"

9cb2eae

This reverts commit 483849a.

aminalaee added a commit that referenced this pull request Feb 28, 2024

Revert "Turn scope["client"] to None on TestClient (#2377)"

1b65b0d

This reverts commit 483849a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn `scope["client"]` to `None` on `TestClient` #2377

Turn `scope["client"]` to `None` on `TestClient` #2377

aminalaee commented Dec 16, 2023 •

edited

Kludex Dec 16, 2023

aminalaee Dec 16, 2023 •

edited

Kludex commented Dec 18, 2023

Kludex commented Dec 22, 2023

bordax commented Feb 15, 2024

karpenkotakeoff commented Feb 16, 2024

petemounce commented Feb 16, 2024

aholmes commented Feb 16, 2024 •

edited

Kyle-sandeman-mrdfood commented Feb 20, 2024 •

edited

bordax commented Feb 20, 2024

aminalaee commented Feb 21, 2024 •

edited

Kyle-sandeman-mrdfood commented Feb 21, 2024

aminalaee commented Feb 22, 2024

Kyle-sandeman-mrdfood commented Feb 22, 2024

dantownsend commented Feb 22, 2024

dantownsend commented Feb 22, 2024

aholmes commented Feb 22, 2024 •

edited

Turn scope["client"] to None on TestClient #2377

Turn scope["client"] to None on TestClient #2377

Conversation

aminalaee commented Dec 16, 2023 • edited

Summary

Checklist

Kludex Dec 16, 2023

Choose a reason for hiding this comment

aminalaee Dec 16, 2023 • edited

Choose a reason for hiding this comment

Kludex commented Dec 18, 2023

Kludex commented Dec 22, 2023

bordax commented Feb 15, 2024

karpenkotakeoff commented Feb 16, 2024

petemounce commented Feb 16, 2024

aholmes commented Feb 16, 2024 • edited

Kyle-sandeman-mrdfood commented Feb 20, 2024 • edited

bordax commented Feb 20, 2024

aminalaee commented Feb 21, 2024 • edited

Kyle-sandeman-mrdfood commented Feb 21, 2024

aminalaee commented Feb 22, 2024

Kyle-sandeman-mrdfood commented Feb 22, 2024

dantownsend commented Feb 22, 2024

dantownsend commented Feb 22, 2024

aholmes commented Feb 22, 2024 • edited

Turn `scope["client"]` to `None` on `TestClient` #2377

Turn `scope["client"]` to `None` on `TestClient` #2377

aminalaee commented Dec 16, 2023 •

edited

aminalaee Dec 16, 2023 •

edited

aholmes commented Feb 16, 2024 •

edited

Kyle-sandeman-mrdfood commented Feb 20, 2024 •

edited

aminalaee commented Feb 21, 2024 •

edited

aholmes commented Feb 22, 2024 •

edited