Merge ~cjwatson/launchpad:artifactory-strip-trailing-backslashes into launchpad:master

[Colin Watson]

Strip trailing backslashes from Artifactory property values

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.

[Jürgen Gmach (jugmac00)]

I am curious - did you get in touch with Jfrog about the incorrect documentation/API?

review: Approve

[Colin Watson (cjwatson)]

I'm afraid I considered it and decided I couldn't face figuring out how to do so. (I'd probably feel differently if it were an open source project!)

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"],
         )