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

Pass urllib3.SKIP_HEADER when headers should be unset #5693

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sigmavirus24
Copy link
Contributor

urllib3 introduced some default headers and a way to skip them if
desired. Let's use that sentinel value to pass along information about
Requests' users desire to skip those headers as well.

Closes gh-5671


I'm pretty sure the tests will fail to start with, but wanted to get a sketch of something up for early review

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

One comment, otherwise looks good

requests/models.py Outdated Show resolved Hide resolved
name, value = header
if value is None:
if name.lower() in urllib3.util.SKIPPABLE_HEADERS:
value = urllib3.SKIP_HEADER
Copy link
Member

Choose a reason for hiding this comment

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

This is going to fail for older versions of urllib3, right? What do you think about doing something in compat like:

try:
    import urllib3
    SKIP_HEADER = urllib3.SKIP_HEADER
    SKIPPABLE_HEADERS = urllib3.util.SKIPPABLE_HEADERS
except AttributeError:
    SKIP_HEADER = None
    SKIPPABLE_HEADERS = []

Copy link
Member

Choose a reason for hiding this comment

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

Also this is giving an attribute error, should be urllib3.util.SKIP_HEADER

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and done. Good call @nateprewitt

Copy link
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.

I think the changes look good for what we originally discussed. There is this new oddity the failing test is pointing out though. We're now creating a PreparedRequest with these odd placeholders for "None" when inspected. That's gonna be a bit jarring from a ux standpoint, and I'm wondering if it's going to create issues for anyone relying on validating/modifying their PreparedRequest before sending.

I'm not immediately sure how we get around that. Do we need to keep a list of headers that need this value and then inject the placeholder just before calling out to urllib3?

urllib3 introduced some default headers and a way to skip them if
desired. Let's use that sentinel value to pass along information about
Requests' users desire to skip those headers as well.

Closes gh-5671
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.

Urllib3 >1.26.x | Introduces default User-Agent header
3 participants