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

Fix compatibility with httpx 0.28.0 #278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ndhansen
Copy link

@ndhansen ndhansen commented Nov 29, 2024

Prior to httpx version 0.28.0, request methods were implicitly converted from bytes to strings. In version 0.28.0 that is no longer the case.

Since httpx request objects expect strings for the method field, and httpcore request objects enforce bytes, I've copied the conversion from bytes to strings from the previous httpx version in to respx.

I didn't add tests because there are no explicit tests on the to_httpx_request method, but I can if you'd like.

Fixes #277.

Comment on lines +300 to +304
method = (
request.method.decode("ascii").upper()
if isinstance(request.method, bytes)
else request.method.upper()
)

Choose a reason for hiding this comment

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

I note that the new httpx code still uppercases the method, so that's redundant here. I'd just decode from bytes:

Suggested change
method = (
request.method.decode("ascii").upper()
if isinstance(request.method, bytes)
else request.method.upper()
)
method = (
request.method.decode("ascii")
if isinstance(request.method, bytes)
else request.method
)

Comment on lines +301 to +302
request.method.decode("ascii").upper()
if isinstance(request.method, bytes)

Choose a reason for hiding this comment

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

Should a DeprecationWarning be raised for bytes so that codebases can be updated and this support can eventually be dropped?

Choose a reason for hiding this comment

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

Yes, I'd expect respx to follow the same argument conventions as httpx, and the project shouldn't try and maintain accepting bytes here if httpx has dropped support.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the minimum HTTPX version also be bumped to match?

respx/setup.py

Line 44 in 366dd0b

install_requires=["httpx>=0.21.0"],

Choose a reason for hiding this comment

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

Suggested as ndhansen#1.

Choose a reason for hiding this comment

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

Should the minimum HTTPX version also be bumped to match?

I don’t think so, the library should still be compatible with older versions of httpx (they used to accept bytes for the method kwarg).

Choose a reason for hiding this comment

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

The bytes come from httpcore, and we’re only getting bytes because we’re hooking the mock after httpcore issued the request. So there’s nothing users can do about this warning.

@francoisfreitag
Copy link

Commit that changes httpx._model.Request constructor: encode/httpx@6622553

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.

Incompatibility with httpx 0.28.0
5 participants