requests: Allow files={"example": ("filename", bytes)}

[prauscher]

trafficstars

According to https://github.com/psf/requests/blob/2d5517682b3b38547634d153cea43d48fbc8cdb5/requests/models.py#L170 It should be possible to provide files={"fieldname": ("filename", [bytes-object])}, but since the last update of mypy this fails with

packet_analyser/daemon/uploader.py:78: error: Argument "files" to "post" has incompatible type "Dict[str, Tuple[str, bytes]]"; expected "Optional[Union[MutableMapping[str, IO[Any]], MutableMapping[str, Tuple[Optional[str], IO[Any]]], MutableMapping[str, Tuple[Optional[str], IO[Any], str]], MutableMapping[str, Tuple[Optional[str], IO[Any], str, MutableMapping[str, str]]]]]"  [arg-type]

The issue seems to be in https://github.com/python/typeshed/blob/63fb9af7438613dd2bfebe07911e49022ce78f57/stubs/requests/requests/sessions.pyi#L51-L56

Can this be fixed?

[bersbersbers]

I see a similar issue for the example from https://docs.python-requests.org/en/latest/user/quickstart/#post-a-multipart-encoded-file:

import requests

url = "https://httpbin.org/post"
files = {"file": open("report.xls", "rb")}

r = requests.post(url, files=files)

bug.py:6: error: Argument "files" to "post" has incompatible type "Dict[str, BufferedReader]"; expected "Optional[Union[MutableMapping[str, IO[Any]], MutableMapping[str, Tuple[str, IO[Any]]], MutableMapping[str, Tuple[str, IO[Any], str]], MutableMapping[str, Tuple[str, IO[Any], str, MutableMapping[str, str]]]]]" [arg-type]

This issue reproduces with mypy==0.942 and types-requests==2.27.21, but not with types-requests==2.27.20.

[bersbersbers]

I'd say the issue is in https://github.com/python/typeshed/pull/7696, specifically, https://github.com/python/typeshed/pull/7696/files#diff-c7fb924dc4d234c3cc1a615ee40522b3a0bb8641f0662ecb1a89762a090b8db9R32 and https://github.com/python/typeshed/pull/7696/files#diff-bfcfcf0d08b5e5c2965394517079c5cfb2b2a11b2e27cf9d08fb07090cd9a4ceR51.

[shawnwall]

I'm also seeing this issue when utilizing StringIO in a test:

response = client.put(
    f"{BASE_PATH}/name/file-import",
    files={"file": StringIO("mockfile")},
)

error: Argument "files" to "post" has incompatible type "Dict[str, Tuple[str, bytes]]"; expected "Optional[Union[MutableMapping[str, IO[Any]], MutableMapping[str, Tuple[Optional[str], IO[Any]]], MutableMapping[str, Tuple[Optional[str], IO[Any], str]], MutableMapping[str, Tuple[Optional[str], IO[Any], str, MutableMapping[str, str]]]]]"  [arg-type]

[AlexWaygood]

Thanks all — a new version of types-requests should be uploaded to PyPI shortly. Feel free to leave a comment here or open a new issue if there are still problems with the new release :)

[bersbersbers]

I have tried 2.27.23 and while the error message has changed, it is still there:

bug.py:6: error: Argument "files" to "post" has incompatible type "Dict[str, BufferedReader]"; expected "Optional[Union[MutableMapping[str, Union[SupportsRead[Union[str, bytes]], str, bytes]], MutableMapping[str, Tuple[Optional[str], Union[SupportsRead[Union[str, bytes]], str, bytes]]], MutableMapping[str, Tuple[Optional[str], Union[SupportsRead[Union[str, bytes]], str, bytes], str]], MutableMapping[str, Tuple[Optional[str], Union[SupportsRead[Union[str, bytes]], str, bytes], str, MutableMapping[str, str]]]]]" [arg-type]

This is with the code example from https://github.com/python/typeshed/issues/7724#issuecomment-1111186084

[etiennecrb]

I think this issue should be reopened. The problem here I believe is the usage of MutableMapping which is invariant. Mypy recommends using immutable collections as annotations when possible so I believe Mapping should be used here (if it's true that it is not mutated), otherwise we might be stuck.

Edit: sorry, didn't see the new issue opened, which properly describes the problem.

[AlexWaygood]

Okay, #7732 has just been merged, and a new release will be uploaded shortly — please let us know if there are still problems with the new release!

[bersbersbers]

2.27.24 works fine for me, thanks for the quick fix!

[svenpanne]

I am not really sure if this bug should be closed: Using MutableMapping instead of Mapping seems to be wrong at several other places, too: _TextMapping, which is mutable, is e.g. used for headers or cookies kwargs, but I somehow doubt that those arguments are actually mutated.

This incorrect(?) variance leads to quite a few false positives on our SW, and I guess we're not alone with this...

[milanboers]

@svenpanne _TextMapping is now the only MutableMapping left that's used in a function argument. I also doubt that it's mutated, I'll do some analysis later to see if it can be changed. Out of curiosity, are you getting these false positives because of the invariance of MutableMapping (i.e. you're passing a MutableMapping with a subtype of str as the value), or because you're passing a non-mutable Mapping?

[svenpanne]

It's mainly because we pass Mappings as parameters for cookies/headers/proxies IIRC, I would need to take a closer look if we make use of covariance in the values.

As a temporary measure, we just copy the Mappings until this bug here is fixed: https://github.com/tribe29/checkmk/commit/0447c080dfb4f4642bfe0369e26f374c3d49b984

[milanboers]

proxies seems to be mutated:

>>> proxies = {'dummy': 'dummy'}
>>> os.environ['http_proxy'] = 'http://dummy'
>>> requests.get('https://python.org',proxies=proxies)
<Response [200]>
>>> proxies
{'dummy': 'dummy', 'http': 'http://dummy'}

This would also make me a bit hesitant to change headers and cookies to Mapping. In the end requests documentation asks for a dictionary for all three.

Another thing worth considering is that to keep things internally consistent, you'd have to make Request.headers etc. also a Mapping. Request.headers is mutated (internally), and it would also break type checking on code like:

r = Request()
r.headers['..'] = '..'

I can imagine that more people use Mapping for these parameters though, so could be worth it, but it's a balancing act.

[svenpanne]

I don't think that this is a balancing act, regarding typing things are always crystal clear: If a parameter gets mutated by a function/method it must be a MutableMapping, using just a Mapping would not be sound (in other words: totally wrong). If it does not get mutated, using a Mapping would be the much better choice: Although a MutableMapping would be sound, too (every MutableMapping is a Mapping after all), this would unnecessarily restrict the function/method (it would require more than is needed). In other words: Parameters should be typed as unspecific as possible (while still being sound) and return values as specific as possible. This will ensure maximum flexibilty.

request's documentation is a bit crappy here IMHO, and the API is more than surprising: Even though it talks about dictionaries, I would have never expected that those input parameters are mutated! Looking at your proxies example, it's clear that this kwarg is mutated, but this doesn't necessarily mean that headers and cookies are mutated, too. One has to figure that out...

Regarding;

r = Request()
r.headers['..'] = '..'

It is far from clear that the headers property returns the same object which has been passed via the headers kwarg. Quite the opposite: It's an anti-pattern to return a reference to a mutable part of an object, so it might be a copy, I don't know. And if you get a mutable argument, it's more often than not a good idea to keep a copy of it, protecting you against mutation behind your back. But I don't know the internals of requests enough to say what is actually being done there...

In any case: It's already very helpful to know that proxies are mutated, I'll definitely need to have a closer look at our SW, I am not sure if it's really prepared for that.

[milanboers]

It is far from clear that the headers property returns the same object which has been passed via the headers kwarg.

That is what is happening internally. So making headers/cookies a Mapping in Session.request but MutableMapping in the attributes of Request would internally be inconsistent. From an API perspective it would still be correct (you can't access the Request made by Session.request).

The problem is these are stubs for a third party. Typeshed can decide to make the parameters as permissive as the current implementation allows, but we don't know what the authors actually intended. Following documentation would be safer, but also leads to false positives. So that's where the balacing lies.

However it's also possible that requests' authors didn't intend the input argument to proxies to be mutated, since it's kind of dangerous, so it's also an option to get this changed in requests.

[Akuli]

When "balancing", we usually consider false negatives (no errors for wrong code) less bad than false positives (errors for correct code).

So making headers/cookies a Mapping in Session.request but MutableMapping in the attributes of Request

In general, Mapping in an argument and MutableMapping in the corresponding attribute is fine. But I don't think it makes sense here:

• If I understood correctly, you will get a runtime error if you pass in an immutable mapping. Type checking is supposed to prevent things like this.
• MutableMapping[str, str] doesn't have variance issues (unless you have a custom str subclass which is rare), so we don't need Mapping just to work around that. This is different from files, which uses a union type inside the MutableMapping.

[AlexWaygood]

If people are still getting what they believe to be false-positive errors following #7696, it would be helpful if they could post a minimal repro and the exact error message they're getting 🙂 otherwise this is all purely theoretical

[svenpanne]

When "balancing", we usually consider false negatives (no errors for wrong code) less bad than false positives (errors for correct code).

I am not exactly sure who "we" is, but type checkers for other programming languages actually do the exact opposite thing and issue an error if code can not be proven correct. In other words, they err on the conservative side, because having the guarantee that things can't go wrong at runtime if the type checker is silent is far more important than any "inconvenience" caused by this conservatism. Due to Rice's theorem (all non-trivial, semantic properties of programs are undecidable) there will always be programs which don't go wrong, but can't be proven safe by any type checker. This is not as bad as it actually sounds, there are basically always ways to make your intentions clear to a type checker by rewriting things slightly.

If you do not err on the conservative side, type checkers are relatively useless.

[...] In general, Mapping in an argument and MutableMapping in the corresponding attribute is fine. [...]

Huh? Perhaps I'm misunderstanding things, but this doesn't sound fine at all: If you pass an immutable data structure into some class, which remembers the same data structure internally and makes the very same data structure available as a property, and then claim that the data structure returned is mutable, something has gone seriously wrong.

In other words: In such a scenario as above (argument remembered internally and exposed as a property, no copying/cloning involved), then the argument and the property must have the same type. From a variance POV: The class is a consumer and a producer, so we have invariance.

Coming back to our proxies kwarg and its related property, things are quite clear now: Both have to have the same type, so both have to be either Mapping or MutableMapping. But the former would be a lie, because requests seems to mutate the argument, so this leaves us with: Both kwarg and property have to be a MutableMapping.

This will cause some type errors, e.g. in our own project, but this is a good thing: We weren't aware of the fact that the argument can be mutated, so we have to fix things on our side! This is what type checking is all about...

Regarding cookies and headers: Are these handled the same way in requests, i.e. no copying + exposed as a property + mutated internally? If yes, the same reasoning applies.

[svenpanne]

If people are still getting what they believe to be false-positive errors following #7696, it would be helpful if they could post a minimal repro and the exact error message they're getting 🙂 otherwise this is all purely theoretical

As mentioned above, the situation regarding proxies is clear: Having a mutable mapping for kwarg & property (as we currently have with https://github.com/python/typeshed/pull/7696) is correct.

Regading cookies and headers: With https://github.com/python/typeshed/pull/7696 you now suddenly get errors if you pass a Mapping (trivial repro), and it remains to be clarified if this a false positive or not. This depends on the fact what requests is doing with those kwargs internally.

[Akuli]

I am not exactly sure who "we" is

Typeshed maintainers. This is documented in our CONTRIBUTING.md.

the guarantee that things can't go wrong at runtime if the type checker is silent

This isn't how type checking works in Python. It wouldn't even be possible, because you can always do something very dynamic, e.g. getattr(foo, arbitrary_string), or use argparse without specifying a custom class for the type of the object that parse_args() returns. The goal of type checking is to detect common mistakes while not getting in the way too much, and in my opinion it works very well.

This will cause some type errors, e.g. in our own project, but this is a good thing:

Indeed. If a mapping is mutated, it should be a MutableMapping :)

We all want errors for code that is most likely wrong, such as passing in an immutable mapping for code that will mutate it. The question is whether we want errors for code that might be correct or wrong. I'm saying that it's sometimes fine to not error in such cases, but I don't mean that we should just apply that blindly. It really has to be considered case by case.

Huh? Perhaps I'm misunderstanding things, but this doesn't sound fine at all:

This was my first reaction too. But some classes are used in two kinds of ways:

• Construct with immutable mapping, never mutate afterwards through the attribute
• Construct with mutable mapping, mutate afterwards through the attribute

If you want both of these to "just work", without forcing users to assert isinstance(foo.bar, MutableMapping) or similar, you have two options: either make the class generic over the mapping type (which may or may not be a good idea), or use Mapping for the argument and MutableMapping for the attribute. There was a similar situation with logging, but I couldn't find the PR.

[milanboers]

For the record, headers and cookies are not mutated as far as I can see. The reasons I was hesitant to change these were:

• It would cause issues with Request's mutable attribute later on. But I understand from Akuli's comment that this is acceptable, so we can dismiss this :)
• headers, cookies and proxies are all used for the same purpose (passing some config) and are documented in the same way ("dictionary" of strings). If proxies is mutated, then it may indicate that a mutable mapping is also expected for the other two.

I opened https://github.com/psf/requests/issues/6118 for requests. I think there is a good chance that they never intended this.

[milanboers]

Looks like it won't be changed any time soon, see https://github.com/psf/requests/issues/6118 . From the response I also gather that they really do expect a mutable argument here.

[prauscher]

PR was merged, so this can be closed now, thanks @milanboers :)