8000 bpo-43433: Reattach query parameters after parsing URI by Rosuav · Pull Request #25045 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-43433: Reattach query parameters after parsing URI #25045

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

bpo-43433: Reattach query parameters after parsing URI #25045

wants to merge 1 commit into from

Conversation

Rosuav
Copy link
Contributor
@Rosuav Rosuav commented Mar 27, 2021 •
edited by bedevere-bot
Loading

Reinstate pre-3.9 behaviour where query parameters would be carried through in the path component.

https://bugs.python.org/issue43433

@terryjreedy terryjreedy added the needs backport to 3.9 only security fixes label Mar 27, 2021
@terryjreedy
Copy link
Member
terryjreedy commented Mar 27, 2021 •
edited by bedevere-bot
Loading

According to Chris A., this patch fixes a regression introduced by 9c4c459, authored by Dong-he Na and merged by Serhiy for bpo-43614.

serhiy-storchaka
serhiy-storchaka requested changes Mar 28, 2021
View reviewed changes
Copy link
Member
@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Should not fragment be added too?

Also the code is not equivalent to the past code if path is empty. I do not know what is better, but all this should be tested.

Please add tests and NEWS entry.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@Rosuav
Copy link
Contributor Author
Rosuav commented Mar 28, 2021

I'm not the one to analyze this as I don't actually use xmlrpc. Will ping the person with the original concern via python-list.

@kynan
Copy link
kynan commented Mar 28, 2021

The "new" behavior breaks the DokuWiki XMLRPC interface (which is using the query string for authentication). I maintain a client package for DokuWiki which is broken by the behavior change addressed in this PR (kynan/dokuwikixmlrpc#8).

My opinion:

  • Adding fragment is not hugely important but I can see no harm in including it to maintain full backwards compatibility with previous behavior. I can't see a legitimate reason for including a fragment in an XMLRPC server URI, but there's bound to be someone who depends on it :)
  • The "new" behavior of implicitly using /RPC2 as the path if empty is confusing and I expect will break existing users. I think users generally expect a hostname with and without trailing / to behave the same way. Also note that the path /RPC2 in the spec is just an example and has no significance. Using it as a default in the first place was a misreading of the spec IMO. Restoring the previous behavior of using /RPC2 as the default only if both path and query string are empty would be preferred. Ideally there would be no default, but I expect there are users depending on this also...

cc @Aluriak who originally reported this (to me and python-list).

Anything I can help with to move this along?

kynan
kynan suggested changes Mar 28, 2021
View reviewed changes
Copy link
@kynan kynan left a comment

Choose a reason for hiding this comment

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

I suggest also restoring the previous behavior where /RPC2 is only used as the default if neither path nor query string are set. I'm indifferent on also adding fragment.

Lib/xmlrpc/client.py
@@ -1426,6 +1426,8 @@ def __init__(self, uri, transport=None, encoding=None, verbose=False,
raise OSError("unsupported XML-RPC protocol")
self.__host = p.netloc
self.__handler = p.path or "/RPC2"
Copy link

Choose a reason for hiding this comment

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

Suggested change
self.__handler = p.path or "/RPC2"
self.__handler = p.path

Lib/xmlrpc/client.py
@@ -1426,6 +1426,8 @@ def __init__(self, uri, transport=None, encoding=None, verbose=False,
raise OSError("unsupported XML-RPC protocol")
self.__host = p.netloc
self.__handler = p.path or "/RPC2"
if p.query:
self.__handler += "?" + p.query

Copy link

Choose a reason for hiding this comment

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

Suggested change
if not self.__handler:
self.__handler = "/RPC2"

@kynan kynan mentioned this pull request Mar 28, 2021
Closed
corona10
corona10 requested changes Mar 29, 2021
View reviewed changes
Copy link
Member
@corona10 corona10 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am also the same side with @serhiy-storchaka about testing and NEWS.d
we need to add the test code for checking self.__handler value.

vstinner
8000
vstinner reviewed Mar 29, 2021
View reviewed changes
Copy link
Member
@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I prefer PR #25057 which reuses urlunsplit() and adds tests.

@Rosuav
Copy link
Contributor Author
Rosuav commented Mar 29, 2021

Yep, I also like that PR.

@vstinner
Copy link
Member

PR #25057 was merged instead.

@vstinner vstinner closed this Mar 29, 2021
@TonyFlury TonyFlury mannequin mentioned this pull request Aug 1, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@kynan kynan kynan requested changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka requested changes

@corona10 corona10 corona10 requested changes

Assignees
No one assigned
Labels
awaiting changes needs backport to 3.9 only security fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Rosuav @terryjreedy @bedevere-bot @kynan @vstinner @serhiy-storchaka @corona10 @the-knights-who-say-ni
0