Skip to content

bpo-38216, bpo-36274: Allow subclasses to override validation and encoding behavior #16321

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

Closed
wants to merge 9 commits into from
Closed

bpo-38216, bpo-36274: Allow subclasses to override validation and encoding behavior #16321

wants to merge 9 commits into from

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Sep 21, 2019

This patch introduces a new semi-private hook for subclasses to override the validation and encoding behavior in http.client.HTTPConnection.putrequest, allowing select clients to customize the behavior and implement invalid requests as was possible prior to the patch in bpo-30458 (for control characters) and prior to Python 3.0 (for encoding).

https://bugs.python.org/issue38216

@tirkarthi
Copy link
Member

So the fix would be that projects like cherrypy can override this private function to make sure crlf is not validated? I understand it's a private hook but not sure how it could be documented if some other project stumbles upon this and wants to bypass the validation. IIRC, the original discussion was about introducing a parameter to turn off validation in putrequest but was given up due to introducing a new parameter in a maintenance release.

@jaraco
Copy link
Member Author

jaraco commented Sep 21, 2019

So the fix would be that projects like cherrypy can override this private function to make sure crlf is not validated?

Essentially, yes. I specifically wanted to avoid adding a parameter due to the public exposure that brings (documentation, explanation, additional interfaces). Almost no clients are going to want to override this behavior. Those that do will very likely trace the code to this place and see that the behavior can be overridden. The tests enforce that contract so it isn't accidentally removed, but otherwise it's tucked away with little documentation. Anyone asking about it may refer to the tests for examples of how it might be used.

@ned-deily ned-deily requested a review from gpshead September 22, 2019 05:35
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@gpshead gpshead self-assigned this Sep 27, 2019
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

also, please use blurb to add a NEWS entry for this change.

@gpshead
Copy link
Member

gpshead commented Sep 27, 2019

I'm looking into addressing my suggested changes now so that this can go in.

@jaraco
Copy link
Member Author

jaraco commented Sep 27, 2019

I'll look at this later this evening.

This makes overriding just that simpler.

Also, don't use the := operator to make backporting easier.
@gpshead
Copy link
Member

gpshead commented Sep 27, 2019

Waiting on the CI results here. I don't anticipate problems. Please take a look and let me know what you think. I want to get this in over the weekend.

@gpshead gpshead requested a review from ned-deily September 27, 2019 22:38
@jaraco
Copy link
Member Author

jaraco commented Sep 28, 2019

This approach looks acceptable to me. I'm still not super-happy with it (not your changes in particular, just the problem); I hate how the _prepare_path and _encode_prepared_path are entangled. I took a stab at a slightly less aggressive approach that once again separates the concerns of _prepare_path and _encode_prepared_path in this branch (currently one commit ahead of this PR). @gpshead, if you have time or energy, would you comment on that approach? Otherwise, we can move forward with this approach.

Edit: I created #16448 for easier consideration of that branch.

@gpshead
Copy link
Member

gpshead commented Sep 28, 2019

i'm inclined to go forward with your new PR #16448

@ned-deily
Copy link
Member

i'm inclined to go forward with your new PR #16448
Me, too.

@jaraco jaraco closed this Sep 28, 2019
@jaraco jaraco deleted the feature/38216-putrequest-hooks branch September 28, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants