[master] Fix Python3.13 compatibility regarding urllib.parse module #66899

marmarek · 2024-09-18T03:04:58Z

What does this PR do?

Python 3.13 fixed handling relative paths in urllib.parse module. Specifically, relative file URL is now constructed as file:path instead of converting it to absolute file:///path. This breaks salt.utils.url.create which expects file:/// specifically. The mismatch results in for example changing salt://top.sls into salt://.sls and thus not finding the top file.

Fix this by handling both prefixes.

Relevant python change: python/cpython#85110

What issues does this PR fix or reference?

Fixes: #66898

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
Tests written/updated (current tests in tests/unit/utils/test_url.py are enough when will start running on Python 3.13)

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices, including the
PR Guidelines.

See GitHub's page on GPG signing for more information about signing commits with GPG.

twangboy

Could you please write a test for this

dmurphy18

Needs tests written in pytest

marmarek · 2024-09-18T20:44:49Z

Could you please write a test for this

The existing tests/unit/utils/test_url.py already covers this. And indeed, without my change, it fails already on Python 3.13rc2 (Fedora 41 beta) or Python 3.12.6 (Fedora 40). The change that broke did got backported to Python 3.12.6 (released about 2 weeks ago). I see you have Fedora 40 in CI already, but it seems to run use onedir (and Python 3.10.x inside) instead of Python from Fedora. Is there any CI job that tests on Python 3.12.x (or 3.13)?

twangboy · 2024-09-18T20:49:18Z

We currently only support up to Python 3.10. There are dependency issues we need to get figured out across all the Operating systems we support before we can officially support anything newer.

marmarek · 2024-09-18T20:59:30Z

In absence of newer Python to actually test the behavior there, do you want me to mimic its new behavior by patching urllib.parse.urlunparse then (I find it weird, but if you insist I can add it)? The code is already tested that it still works on Python 3.10. And once you will be adding support for Python 3.12, you will have one thing less to fix :)

BTW, does the CI report somewhere test coverage? From what I can see, coverage-related jobs got skipped...

slimlv

how many "If/return"s you going to insert ?
note that changes in python code was introduced in minor release 3.12.6

try to make smart bulletproof improvements , for example

url = salt.utils.data.decode(urlunparse(("file", "", "/" + path, "", query, "")))

twangboy · 2024-09-20T18:24:02Z

I think, just a unit test to make sure that if the URL starts with file:/// it gets converted to 'salt://`

bdrx312 · 2024-09-29T17:32:06Z

salt/utils/url.py


    query = f"saltenv={saltenv}" if saltenv else ""
    url = salt.utils.data.decode(urlunparse(("file", "", path, "", query, "")))
-    return "salt://{}".format(url[len("file:///") :])
+    # urlunparse changed behavior in Python 3.13
+    if url.startswith("file:///"):
+        return "salt://{}".format(url[len("file:///") :])
+    return "salt://{}".format(url[len("file:") :])


This create function should not be doing path = path.replace("\\", /) and probably not

if salt.utils.platform.is_windows(): path = salt.utils.path.sanitize_win_path(path)

Instead it should use urllib.request.pathname2url or some other python builtin functionality.

Especially since the sanitize_win_path function is clearly miswritten:

def sanitize_win_path(winpath): """ Remove illegal path characters for windows """ intab = "<>:|?*" if isinstance(winpath, str): winpath = winpath.translate({ord(c): "_" for c in intab}) elif isinstance(winpath, str):

Notice the duplicate isinstance(winpath, str) in the elif that will never occur.

I would prefer instead of assuming that the string starts with file: or file:/// we do a regex search replace like re.sub(r"^file:(?:///)?", "", url) Also use f string instead of format.

return f'salt://{re.sub(r"^file:(?:///)?", "", url)}'

return f'salt://{re.sub(r"^file:(?:/+)?", "", url)}'

return f'salt://{re.sub(r"^file:(?:/+)?", "", url)}'

If you are going to do zero or more you don't + and ? nor parenthesis you just use *

return f'salt://{re.sub(r"^file:/*", "", url)}'

and if you want to do 0 to 3 / then it would be

return f'salt://{re.sub(r"^file:/{0,3}", "", url)}'

or

return f'salt://{re.sub(r"^file:/{,3}", "", url)}'

but I think the explicit 0 is more readable.

marmarek · 2024-10-04T01:57:55Z

I think, just a unit test to make sure that if the URL starts with file:/// it gets converted to 'salt://`

Most tests in tests/unit/utils/test_url.py do that already, for example test_create_url. Those tests do fail when running on Python 3.13 (or Python 3.12.6), so this regression is covered already. Your CI will notice it as soon as you start testing on such Python version.
And BTW, the file:// prefix is added only in salt.utils.url.create(), just to be replaced with salt:// - it doesn't exist in the function call argument.

yangskyboxlabs · 2024-10-08T21:55:41Z

salt/utils/url.py

@@ -47,7 +47,7 @@ def create(path, saltenv=None):

    query = f"saltenv={saltenv}" if saltenv else ""
    url = salt.utils.data.decode(urlunparse(("file", "", path, "", query, "")))


I recommend just not using schema when calling unparse/unsplit:

Suggested change

url = salt.utils.data.decode(urlunparse(("file", "", path, "", query, "")))

return f"salt://{salt.utils.data.decode(urlunsplit(("", "", path, query, "")))}"

I just read through the source code of urlunparse/urlunsplit and it looks like there is not currently any special handling for the file scheme so I agree that this is probably a better solution.

Ok, lets see how this works. Tests on Python 3.12.6 are green with this version, I'll let CI check older ones.

Using urlunparse is okay, but I think using urlunsplit as @yangskyboxlabs is more ideal since it passes one less unused argument.

jfindlay · 2024-10-10T13:13:15Z

In my opinion, having written both the original salt.utils.url and its accompanying tests, I agree the existing tests already cover the code well enough because obviously I write the most amazing test cases. I don't think it is necessary to cover the case that upstream will again change the behavior of code that hasn't been modified in ~25 years.

It is a subtle breaking change for a use case they very likely didn't expect. Regexing file:/// for salt:// is a yolo move, whereas subclassing the urlparse code to add a salt proto would have been more proper. (Maybe the upstream code doesn't care because you already can sub your own proto?) If anything a rare bug in old code is an indictment of the poor usecase. Not that urlparse.urlunparse's api is user friendly (pass the url components in a fixed length anonymous list; give me a break). There are still many occurrences in the codebase that handcraft salt:// URLs anyway.

Ironically, many years ago when I worked on this, Tom said "I wish we had gone with salt:///".

Python 3.13 fixed handling relative paths in urllib.parse module. Specifically, relative file URL is now constructed as file:path instead of converting it to absolute file:///path. This breaks salt.utils.url.create which expects file:/// specifically. The mismatch results in for example changing salt://top.sls into salt://.sls and thus not finding the top file. Fix this by handling both prefixes. Relevant python change: python/cpython#85110 Fixes: saltstack#66898

dead10ck · 2024-10-25T22:24:29Z

I'd like to remind the maintainers that this bug completely and utterly breaks salt on many systems, especially because this "fix" has been backported to Python 3.12.

I understand that 3.10 is currently the officially supported version, but the reality is that a lot of folks are not, and never will, use the onedir package, so this is high impact regardless of official compatibility guarantees.

I understand the desire for tests with changes in the general case, but I think folks have to keep in mind that that should be a rule of thumb, not a hard requirement in every case no matter what the cost. Sometimes fixing high impact bugs, level of effort to benefit ratio, and potential risk for introducing new regressions in more strongly supported environments should weigh into the decision to block merging unless there are tests. It's been established that there is significant preexisting coverage on this code path, so blocking on adding more tests is not doing anyone any good.

marmarek requested a review from a team as a code owner September 18, 2024 03:04

salt-project-bot-prod-environment bot changed the title ~~Fix Python3.13 compatibility regarding urllib.parse module~~ [master] Fix Python3.13 compatibility regarding urllib.parse module Sep 18, 2024

marmarek temporarily deployed to ci September 18, 2024 03:34 — with GitHub Actions Inactive

twangboy requested changes Sep 18, 2024

View reviewed changes

dmurphy18 requested changes Sep 18, 2024

View reviewed changes

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Sep 19, 2024

community/salt: saltstack/salt#66899

c5ed1f1

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Sep 19, 2024

community/salt-lts: saltstack/salt#66899

5491641

slimlv suggested changes Sep 20, 2024

View reviewed changes

twangboy mentioned this pull request Sep 23, 2024

Python 3.12: fix for urlunparse() not returning properly-prefixed file:/// URLs #66911

Closed

bdrx312 reviewed Sep 29, 2024

View reviewed changes

marmarek force-pushed the 3.13-urllib branch from 187f1f6 to 7b28c2f Compare October 4, 2024 02:03

marmarek temporarily deployed to ci October 4, 2024 03:26 — with GitHub Actions Inactive

marmarek temporarily deployed to ci October 8, 2024 00:11 — with GitHub Actions Inactive

bdrx312 approved these changes Oct 8, 2024

View reviewed changes

yangskyboxlabs reviewed Oct 8, 2024

View reviewed changes

marmarek force-pushed the 3.13-urllib branch 3 times, most recently from 37052c1 to e7f24b7 Compare October 12, 2024 14:27

marmarek temporarily deployed to ci October 12, 2024 14:44 — with GitHub Actions Inactive

marmarek force-pushed the 3.13-urllib branch from e7f24b7 to 0f69a5a Compare October 13, 2024 01:04

marmarek temporarily deployed to ci October 13, 2024 01:20 — with GitHub Actions Inactive

bdrx312 approved these changes Oct 13, 2024

View reviewed changes

twangboy approved these changes Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[master] Fix Python3.13 compatibility regarding urllib.parse module #66899

[master] Fix Python3.13 compatibility regarding urllib.parse module #66899

marmarek commented Sep 18, 2024

twangboy left a comment

dmurphy18 left a comment

marmarek commented Sep 18, 2024

twangboy commented Sep 18, 2024

marmarek commented Sep 18, 2024

slimlv left a comment

twangboy commented Sep 20, 2024

bdrx312 Sep 29, 2024 •

edited

Loading

slimlv Oct 1, 2024 •

edited by twangboy

Loading

bdrx312 Oct 1, 2024 •

edited

Loading

bdrx312 Oct 1, 2024

marmarek commented Oct 4, 2024

yangskyboxlabs Oct 8, 2024

bdrx312 Oct 9, 2024

marmarek Oct 12, 2024

bdrx312 Oct 13, 2024

jfindlay commented Oct 10, 2024 •

edited

Loading

dead10ck commented Oct 25, 2024

		@@ -47,7 +47,7 @@ def create(path, saltenv=None):

		query = f"saltenv={saltenv}" if saltenv else ""
		url = salt.utils.data.decode(urlunparse(("file", "", path, "", query, "")))

	url = salt.utils.data.decode(urlunparse(("file", "", path, "", query, "")))
	return f"salt://{salt.utils.data.decode(urlunsplit(("", "", path, query, "")))}"

[master] Fix Python3.13 compatibility regarding urllib.parse module #66899

Are you sure you want to change the base?

[master] Fix Python3.13 compatibility regarding urllib.parse module #66899

Conversation

marmarek commented Sep 18, 2024

What does this PR do?

What issues does this PR fix or reference?

Merge requirements satisfied?

Commits signed with GPG?

twangboy left a comment

Choose a reason for hiding this comment

dmurphy18 left a comment

Choose a reason for hiding this comment

marmarek commented Sep 18, 2024

twangboy commented Sep 18, 2024

marmarek commented Sep 18, 2024

slimlv left a comment

Choose a reason for hiding this comment

twangboy commented Sep 20, 2024

bdrx312 Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

slimlv Oct 1, 2024 • edited by twangboy Loading

Choose a reason for hiding this comment

bdrx312 Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

bdrx312 Oct 1, 2024

Choose a reason for hiding this comment

marmarek commented Oct 4, 2024

yangskyboxlabs Oct 8, 2024

Choose a reason for hiding this comment

bdrx312 Oct 9, 2024

Choose a reason for hiding this comment

marmarek Oct 12, 2024

Choose a reason for hiding this comment

bdrx312 Oct 13, 2024

Choose a reason for hiding this comment

jfindlay commented Oct 10, 2024 • edited Loading

dead10ck commented Oct 25, 2024

bdrx312 Sep 29, 2024 •

edited

Loading

slimlv Oct 1, 2024 •

edited by twangboy

Loading

bdrx312 Oct 1, 2024 •

edited

Loading

jfindlay commented Oct 10, 2024 •

edited

Loading