Skip to content

Fix prepare_body stream detection for __getattr__-based file wrappers#7433

Merged
nateprewitt merged 3 commits into
psf:mainfrom
k223kim:k223kim/fix-prepare-body-stream
May 12, 2026
Merged

Fix prepare_body stream detection for __getattr__-based file wrappers#7433
nateprewitt merged 3 commits into
psf:mainfrom
k223kim:k223kim/fix-prepare-body-stream

Conversation

@k223kim
Copy link
Copy Markdown
Contributor

@k223kim k223kim commented May 12, 2026

Resolves #7432 .

Add hasattr(data, "__iter__") as a fallback stream detector:

if (
    isinstance(data, Iterable) or hasattr(data, "__iter__")
) and not isinstance(data, (str, bytes, list, tuple, Mapping)):

This catches file-like objects that proxy their interface through __getattr__, restoring the 2.33.1 behavior without reverting the isinstance(Iterable) modernization for standard iterables.

@nateprewitt
Copy link
Copy Markdown
Member

Hi @k223kim,

This PR has quite a few oddities in it, did you happen to use an LLM to generate it? There are unrelated tests and the change itself is wrong (at least for the issue you reported).

@k223kim
Copy link
Copy Markdown
Contributor Author

k223kim commented May 12, 2026

Hey @nateprewitt , thanks for the review!
I did use llm to get help with mimicking the tqdm behavior on the test - but not for the fix itself 😄 The test ensures that correct objects are detected as a stream. + I believe this did fix the issue that I was having with tqdm wrapper + redirect with the latest version of requests. Would you explain how this fix is wrong? Happy to adjust as needed.

@k223kim
Copy link
Copy Markdown
Contributor Author

k223kim commented May 12, 2026

Perhaps - I can be more precise with the if-statement and just restoring it to hasattr(data, "__iter__").
i.e.

if (
    isinstance(data, Iterable) or hasattr(data, "__iter__")
) and not isinstance(data, (str, bytes, list, tuple, Mapping)):

If this sounds better, happy to adjust test accordingly.

@nateprewitt
Copy link
Copy Markdown
Member

Yep, it should be __iter__ instead of read, otherwise we're adding new semantics to the check that didn't exist in 2.33.1 either.

The tests are engraining a lot of tqdm/use case specific semantics into the test suite that I'm not sure we want. While this may work, I don't really want to call it a "supported pattern" because that tends to lead to other areas that didn't support this before getting PRs to "fix" support.

I think it's unlikely this changes again in the future, the __iter__ check was there for ~12 years, but if we want to be really safe, a single test like this is probably sufficient:

 def test_getattr_proxy_stream_follows_redirect(self, httpbin):
     """Ensure stream wrappers that don't implement __iter__ directly are still detected."""
 
     class AttrProxy:
         def __init__(self):
             self._file = io.BytesIO(b"data")
 
         def __getattr__(self, name):
             return getattr(self._file, name)
 
     r = requests.post(httpbin("redirect-to?url=/post&status_code=307"), data=AttrProxy())
     assert r.json()["data"] == "data"

@k223kim
Copy link
Copy Markdown
Contributor Author

k223kim commented May 12, 2026

@nateprewitt Makes sense. Adjusted the PR 😄 Would appreciate it if you can take a look!

Copy link
Copy Markdown
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor tweak for readability, but otherwise this looks good. Thanks @k223kim!

Comment thread src/requests/models.py Outdated
@nateprewitt nateprewitt merged commit 6404f34 into psf:main May 12, 2026
33 checks passed
@nateprewitt nateprewitt mentioned this pull request May 13, 2026
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

Successfully merging this pull request may close these issues.

prepare_body stream detection regression

2 participants