Skip to content

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

Closed
Rosuav wants to merge 1 commit into
python:masterfrom
Rosuav:xmlrpc-regression
Closed

bpo-43433: Reattach query parameters after parsing URI#25045
Rosuav wants to merge 1 commit into
python:masterfrom
Rosuav:xmlrpc-regression

Conversation

@Rosuav

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

Copy link
Copy Markdown
Contributor

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

https://bugs.python.org/issue43433

@terryjreedy

terryjreedy commented Mar 27, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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

bedevere-bot commented Mar 28, 2021

Copy link
Copy Markdown

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

Rosuav commented Mar 28, 2021

Copy link
Copy Markdown
Contributor Author

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

kynan commented Mar 28, 2021

Copy link
Copy Markdown

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

@kynan kynan left a comment

Copy link
Copy Markdown

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.

Comment thread 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"

@kynan kynan Mar 28, 2021

Copy link
Copy Markdown

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

Comment thread Lib/xmlrpc/client.py
self.__handler = p.path or "/RPC2"
if p.query:
self.__handler += "?" + p.query

@kynan kynan Mar 28, 2021

Copy link
Copy Markdown

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

@corona10 corona10 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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
vstinner reviewed Mar 29, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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

Rosuav commented Mar 29, 2021

Copy link
Copy Markdown
Contributor Author

Yep, I also like that PR.

@vstinner

vstinner commented Mar 29, 2021

Copy link
Copy Markdown
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
@serhiy-storchaka serhiy-storchaka serhiy-storchaka requested changes
@corona10 corona10 corona10 requested changes
+1 more reviewer
@kynan kynan kynan requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting changes

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