Skip to content

/cpython

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

[2.7] bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior (GH-16448) #16476

Merged
merged 5 commits into from Oct 8, 2019
Merged

[2.7] bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior (GH-16448) #16476

merged 5 commits into from Oct 8, 2019

Conversation

@jaraco
Copy link
Member

jaraco commented Sep 29, 2019 •
edited by bedevere-bot

  • bpo-38216: Allow bypassing input validation

  • bpo-36274: Also allow the URL encoding to be overridden.

  • bpo-38216, bpo-36274: Add tests demonstrating a hook for overriding validation, test demonstrating override encoding, and a test to capture expectation of the interface for the URL.

  • Call with skip_host to avoid tripping on the host checking in the URL.

  • Remove obsolete comment.

  • Make _prepare_path_encoding its own attr.

This makes overriding just that simpler.

Also, don't use the := operator to make backporting easier.

  • Add a news entry.

  • _prepare_path_encoding -> _encode_prepared_path()

  • Once again separate the path validation and request encoding, drastically simplifying the behavior. Drop the guarantee that all processing happens in _prepare_path..
    (cherry picked from commit 7774d78)

Co-authored-by: Jason R. Coombs jaraco@jaraco.com

https://bugs.python.org/issue38216
@jaraco

This comment has been minimized.

Sign in to view
Copy link
Member Author

jaraco commented Sep 29, 2019 •
edited by bedevere-bot

Backporting this change, I observe a couple of things:

  1. The _encode_request call is no longer meaningful because the request construction will implicitly encode the request using the default encoding when the format string is used (request = '%s %s %s'...). In order to keep the code as consistent as possible, I decided to include the call as a pass-through. I'd be just as happy to remove it entirely, but I'll leave that up to the reviewer to decide. It's okay that this functionality is disabled on Python 2 because this functionality was mainly around bpo-36274, which was mainly a concern with the transition to Python 3.
  2. Because _encode_request is no longer meaningful, neither is the test for it, so I've removed that test. Therefore, the meaningful part of this test is that for bpo-38216, adding a (underscore-protected) hook to customize/disable validation.
@jaraco
[2.7] bpo-38216, bpo-36274: Allow subclasses to separately override v…
Loading status checks…
c5bf22c 
…alidation and encoding behavior (GH-16448)

* bpo-38216: Allow bypassing input validation

* bpo-36274: Also allow the URL encoding to be overridden.

* bpo-38216, bpo-36274: Add tests demonstrating a hook for overriding validation, test demonstrating override encoding, and a test to capture expectation of the interface for the URL.

* Call with skip_host to avoid tripping on the host checking in the URL.

* Remove obsolete comment.

* Make _prepare_path_encoding its own attr.

This makes overriding just that simpler.

Also, don't use the := operator to make backporting easier.

* Add a news entry.

* _prepare_path_encoding -> _encode_prepared_path()

* Once again separate the path validation and request encoding, drastically simplifying the behavior. Drop the guarantee that all processing happens in _prepare_path..
(cherry picked from commit 7774d78)

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
@jaraco jaraco force-pushed the jaraco:backport-7774d78-2.7 branch from 4fe4818 to c5bf22c Sep 29, 2019
jaraco added 3 commits Sep 29, 2019
@jaraco
Replace f-strings with .format call.
Loading status checks…
b15fe9e
@jaraco
Correct indentation
Loading status checks…
e083d02
@jaraco
httplib is the module name
Loading status checks…
6efef34
@jaraco jaraco requested a review from gpshead Sep 30, 2019
@jaraco

This comment has been minimized.

Sign in to view
Copy link
Member Author

jaraco commented Sep 30, 2019 •
edited by bedevere-bot

As a reminder, this PR is for your consideration. The reported failure cases for bpo-38216 already have workarounds implemented, so there are no known pressing needs for this corrective bugfix. Accepting this change will, however, provide a consistent workaround that applies to other (not-yet-identified) use-cases across all (affected) Python versions, so it seems worthwhile to me.
@benjaminp
remove usage of locals()
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
55cefe3
@benjaminp

This comment has been minimized.

Sign in to view
Copy link
Contributor

benjaminp commented Oct 8, 2019 •
edited by bedevere-bot

Since 2.7 wasn't ever released with the fix for bpo-38216, I'm going to say we should take this just in case.

I'm going to edit your PR to remove the usage of locals(). Otherwise, it looks good. Thanks for doing this.
@benjaminp benjaminp merged commit f5b1abb into python:2.7 Oct 8, 2019
4 checks passed
4 checks passed
bedevere/issue-number Issue number 38216 found
Details
bedevere/maintenance-branch-pr Valid maintenance branch PR title.
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jaraco jaraco deleted the jaraco:backport-7774d78-2.7 branch Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead

Assignees

@benjaminp benjaminp

Labels
CLA signed
Projects
None yet
Milestone
No milestone
4 participants
@jaraco @benjaminp @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.