bpo-38216, bpo-36274: Allow subclasses to override validation and encoding behavior by jaraco · Pull Request #16321 · python/cpython (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
…alidation, test demonstrating override encoding, and a test to capture expectation of the interface for the URL.
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.
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.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
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.
I'm looking into addressing my suggested changes now so that this can go in.
I'll look at this later this evening.
This makes overriding just that simpler.
Also, don't use the := operator to make backporting easier.
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.
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.
i'm inclined to go forward with your new PR #16448
i'm inclined to go forward with your new PR #16448
Me, too.
