Turn scope["client"] to None on TestClient

[aminalaee]

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]

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

[aminalaee]

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]

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

[Kludex]

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

[bordax]

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]

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]

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

[aholmes]

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]

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]

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.

[aminalaee]

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]

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]

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]

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]

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]

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

[aholmes]

@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.