launchpad-reviewers team mailing list archive

[Colin Watson <mp+434495@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:artifactory-strip-trailing-backslashes into launchpad:master.

Commit message:
Strip trailing backslashes from Artifactory property values

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/434495

I've previously established empirically that backslashes must not be quoted when setting Artifactory properties (contrary to the public documentation), and the `encode_properties` function used internally when setting properties behaves as follows:

    >>> from collections import OrderedDict
    >>> from artifactory import encode_properties
    >>> properties = OrderedDict([("key1", "\\;=|,\\"), ("key2", "value")])
    >>> print(encode_properties(properties))
    key1=\;\=\|\,\;key2=value

This encoding clearly leaves no way to separate the fields unambiguously, and the result is that ";key2=value" ends up being appended to the value of the "key1" property, which is no good.

Stripping trailing backslashes is a horrible workaround, but I don't see a way around it given how the Artifactory API works.  I expect trailing backslashes in property values to be extremely rare in practice, and they'll only show up in informational properties auto-populated by Artifactory itself, not by anything functional.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:artifactory-strip-trailing-backslashes into launchpad:master.

diff --git a/lib/lp/archivepublisher/artifactory.py b/lib/lp/archivepublisher/artifactory.py
index 9af731b..9fc4fc7 100644
--- a/lib/lp/archivepublisher/artifactory.py
+++ b/lib/lp/archivepublisher/artifactory.py
@@ -360,11 +360,14 @@ class ArtifactoryPoolEntry:
         # "|", and "=" (and handles URL-encoding at a lower layer).  In
         # practice, backslash must apparently not be quoted, and ";" must be
         # quoted as otherwise ";" within items will be confused with a
-        # property separator.  This is not very satisfactory as we're
-        # essentially playing core wars with the artifactory module, but
-        # this solves the problem for now.
+        # property separator; we must also remove any trailing backslashes as
+        # they'll cause any following property to be interpreted as part of
+        # this property value.  This is not very satisfactory as we're
+        # essentially playing core wars with the artifactory module, and
+        # removing trailing backslashes unavoidably loses some information,
+        # but this solves the problem for now.
         new_properties = {
-            key: [v.replace(";", r"\;") for v in value]
+            key: [v.rstrip("\\").replace(";", r"\;") for v in value]
             for key, value in new_properties.items()
         }
         if old_properties != new_properties:
diff --git a/lib/lp/archivepublisher/tests/test_artifactory.py b/lib/lp/archivepublisher/tests/test_artifactory.py
index 730e8a9..4179c2e 100644
--- a/lib/lp/archivepublisher/tests/test_artifactory.py
+++ b/lib/lp/archivepublisher/tests/test_artifactory.py
@@ -1446,7 +1446,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
         )
 
         self.assertEqual(
-            ["text with special characters: ;=|,\\"],
+            ["text with special characters: ;=|,"],
             path.properties["pypi.summary"],
         )