launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28665
[Merge] ~cjwatson/launchpad:artifactory-quoting-on-deploy into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:artifactory-quoting-on-deploy into launchpad:master with ~cjwatson/launchpad:artifactory-quoting as a prerequisite.
Commit message:
URL-quote property values for ArtifactoryPath.deploy_file
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/425385
`ArtifactoryPath.deploy_file` and `ArtifactoryPath.set_properties` have two entirely distinct quoting conventions. Astonishingly, the Python bindings for the former leave this entirely up to the caller without saying so. Fortunately, all that `deploy_file` needs is URL-quoting according to `urllib.parse.quote_plus`. Do this.
In the process of fixing this, I noticed that the Artifactory fixture's endpoints don't correctly unquote the URL path, so I fixed that.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:artifactory-quoting-on-deploy into launchpad:master.
diff --git a/lib/lp/archivepublisher/artifactory.py b/lib/lp/archivepublisher/artifactory.py
index 611100c..2268d71 100644
--- a/lib/lp/archivepublisher/artifactory.py
+++ b/lib/lp/archivepublisher/artifactory.py
@@ -13,6 +13,7 @@ import tempfile
from collections import defaultdict
from pathlib import Path, PurePath
from typing import List, Optional
+from urllib.parse import quote_plus
import requests
from artifactory import ArtifactoryPath
@@ -271,6 +272,12 @@ class ArtifactoryPoolEntry:
properties = self.calculateProperties(
self.makeReleaseID(self.pub_file), []
)
+ # Property values must be URL-quoted for
+ # `ArtifactoryPath.deploy_file`; it does not handle that itself.
+ properties = {
+ key: [quote_plus(v) for v in value]
+ for key, value in properties.items()
+ }
fd, name = tempfile.mkstemp(prefix="temp-download.")
f = os.fdopen(fd, "wb")
try:
diff --git a/lib/lp/archivepublisher/tests/artifactory_fixture.py b/lib/lp/archivepublisher/tests/artifactory_fixture.py
index 93bcf41..13a9ab8 100644
--- a/lib/lp/archivepublisher/tests/artifactory_fixture.py
+++ b/lib/lp/archivepublisher/tests/artifactory_fixture.py
@@ -11,9 +11,10 @@ import fnmatch
import hashlib
import json
import re
+from collections import defaultdict
from datetime import datetime, timezone
from pathlib import Path
-from urllib.parse import parse_qs, unquote, urlparse
+from urllib.parse import parse_qs, unquote, unquote_plus, urlparse
import responses
from fixtures import Fixture
@@ -88,7 +89,7 @@ class FakeArtifactoryFixture(Fixture):
def _handle_download(self, request):
"""Handle a request to download an existing file."""
- path = urlparse(request.url[len(self.repo_url) :]).path
+ path = unquote(urlparse(request.url[len(self.repo_url) :]).path)
if path in self._fs and "size" in self._fs[path]:
return (
200,
@@ -101,7 +102,7 @@ class FakeArtifactoryFixture(Fixture):
def _handle_stat(self, request):
"""Handle a request to stat an existing file."""
parsed_url = urlparse(request.url[len(self.api_url) :])
- path = parsed_url.path
+ path = unquote(parsed_url.path)
if path in self._fs:
stat = {"repo": self.repository_name, "path": path}
stat.update(self._fs[path])
@@ -134,8 +135,22 @@ class FakeArtifactoryFixture(Fixture):
"""
return re.sub(r"\\([,|=;])", r"\1", unquote(text))
+ def _decode_matrix_parameters(self, encoded):
+ """Decode matrix parameters that were encoded as part of a request.
+
+ `ArtifactoryPath.deploy` encodes properties like this.
+ """
+ properties = defaultdict(list)
+ for param in encoded.split(";"):
+ key, value = param.split("=", 1)
+ properties[unquote_plus(key)].append(unquote_plus(value))
+ return properties
+
def _decode_properties(self, encoded):
- """Decode properties that were encoded as part of a request."""
+ """Decode properties that were encoded as part of a request.
+
+ `ArtifactoryPath.set_properties` encodes properties like this.
+ """
properties = {}
for param in self._split(";", encoded):
key, value = re.match(r"((?:\\[,|=;]|[^=])+)=(.*)", param).groups()
@@ -155,11 +170,11 @@ class FakeArtifactoryFixture(Fixture):
else:
params = ""
parsed_url = urlparse(url)
- path = parsed_url.path
+ path = unquote(parsed_url.path)
if path.endswith("/"):
self.add_dir(path.rstrip("/"))
elif path.rsplit("/", 1)[0] in self._fs:
- properties = self._decode_properties(params)
+ properties = self._decode_matrix_parameters(params)
self.add_file(
path,
request.body,
@@ -171,7 +186,7 @@ class FakeArtifactoryFixture(Fixture):
def _handle_set_properties(self, request):
"""Handle a request to set properties on an existing file."""
parsed_url = urlparse(request.url[len(self.api_url) :])
- path = parsed_url.path
+ path = unquote(parsed_url.path)
if path in self._fs:
query = parse_qs(parsed_url.query)
properties = self._decode_properties(query["properties"][0])
@@ -183,7 +198,7 @@ class FakeArtifactoryFixture(Fixture):
def _handle_delete_properties(self, request):
"""Handle a request to delete properties from an existing file."""
parsed_url = urlparse(request.url[len(self.api_url) :])
- path = parsed_url.path
+ path = unquote(parsed_url.path)
if path in self._fs:
query = parse_qs(parsed_url.query)
for key in query["properties"][0].split(","):
@@ -265,7 +280,7 @@ class FakeArtifactoryFixture(Fixture):
def _handle_delete(self, request):
"""Handle a request to delete an existing file."""
- path = urlparse(request.url[len(self.repo_url) :]).path
+ path = unquote(urlparse(request.url[len(self.repo_url) :]).path)
if not path.endswith("/") and path in self._fs:
self.remove_file(path)
return 200, {}, ""
diff --git a/lib/lp/archivepublisher/tests/test_artifactory.py b/lib/lp/archivepublisher/tests/test_artifactory.py
index 0e013e5..196571d 100644
--- a/lib/lp/archivepublisher/tests/test_artifactory.py
+++ b/lib/lp/archivepublisher/tests/test_artifactory.py
@@ -131,8 +131,8 @@ class TestArtifactoryPool(TestCase):
foo = ArtifactoryPoolTestingFile(
pool=pool,
source_name="foo",
- source_version="1.0",
- filename="foo-1.0.deb",
+ source_version="1.0+1",
+ filename="foo-1.0+1.deb",
release_type=FakeReleaseType.BINARY,
release_id=1,
)
@@ -144,7 +144,7 @@ class TestArtifactoryPool(TestCase):
{
"launchpad.release-id": ["binary:1"],
"launchpad.source-name": ["foo"],
- "launchpad.source-version": ["1.0"],
+ "launchpad.source-version": ["1.0+1"],
},
foo.getProperties(),
)
Follow ups