Repository: devopshq/artifactory

Issue:
`deploy_file` fails with "Unsupported archive extension" when specifying parameters and `explode_archive=True`
When running the below reproducer, I see the following output:

```pytb
Traceback (most recent call last):
  File "reproducer.py", line 13, in <module>
    artifactory_path.deploy_file(
  File "/home/dwatkins/foss/artifactory/artifactory.py", line 1723, in deploy_file
    target.deploy(
  File "/home/dwatkins/foss/artifactory/artifactory.py", line 1689, in deploy
    return self._accessor.deploy(
  File "/home/dwatkins/foss/artifactory/artifactory.py", line 1037, in deploy
    raise RuntimeError(text)
RuntimeError: {
  "errors" : [ {
    "status" : 500,
    "message" : "Unsupported archive extension: 'tar.architecture=amd64'; nested exception is java.lang.IllegalArgumentException: Unsupported archive extension: 'tar.architecture=amd64'"
  } ]
}
```

```py
import os
import subprocess

from artifactory import ArtifactoryPath

# Create a tarball to explode
subprocess.check_call(["touch", "test-file"])
subprocess.check_call(["tar", "cf", "test.tar", "test-file"])

artifactory_path = ArtifactoryPath(
    os.environ["ARTIFACTORY_URL"], apikey=os.environ["ARTIFACTORY_API_KEY"]
)
artifactory_path.deploy_file(
    "test.tar", explode_archive=True, parameters={"deb.architecture": "amd64"}
)
```

Discussion:
beliaev-maksim: @allburov 
we need to put more `responses` mock up, otherwise bugs such this will leak. I will try to replace it in some other places in future commits

also, need to increase tests coverage
![image](https://user-images.githubusercontent.com/51964909/130301866-94e293eb-535f-491a-8b3a-fde871d0462b.png)


can you setup CI/CD to include coverage report as well?
`coverage run --omit **/venv/* -m unittest discover`

at later stage we can add a required check that code coverage should not fall below some value, eg 80-90%
what do you think?
allburov: The coverage report doesn't show real life coverage, for instance it doesn't cover all available names for files :) 

I can add coverage to CI\CD, but I'm against any threshold on it
OddBloke: My reproducer script indicates that this PR _does_ fix the problem. Thanks!
OddBloke: Hey folks, any progress on getting this landed?
beliaev-maksim: > 
> 
> Hey folks, any progress on getting this landed?

@OddBloke 
we need to merge other PRs first, since change for this PR will be based on another PRs.

Then, I will do a test case for your scenario but will reimplement source code, so it should pass the test

as soon as we do all this fixes we will try to push all changes into production and release a new version
beliaev-maksim: @allburov 
this PR is ready for review

Patch:
diff --git a/README.md b/README.md
index b55fb1d..7bda380 100644
--- a/README.md
+++ b/README.md
@@ -253,7 +253,8 @@ path.archive().writeto(out="my.zip", chunk_size=100 * 1024)
 
 ## Uploading Artifacts ##
 
-Deploy a regular file ```myapp-1.0.tar.gz```
+Deploy a regular file ```myapp-1.0.tar.gz```. This method by default will calculate all available checksums and attach
+them to the file
 
 ```python
 from artifactory import ArtifactoryPath
@@ -266,7 +267,8 @@ path.mkdir()
 path.deploy_file("./myapp-1.0.tar.gz")
 ```
 
-Deploy artifacts from archive: this will automatically extract the contents of the archive on the server preserving the archive's paths
+Deploy artifacts from archive: this will automatically extract the contents of the archive on the server preserving 
+the archive's paths
 
 ```python
 from artifactory import ArtifactoryPath
@@ -279,7 +281,9 @@ path.mkdir()
 path.deploy_file("./myapp-1.0.tar.gz", explode_archive=True)
 ```
 
-Atomically deploy artifacts from archive: this will automatically extract the contents of the archive on the server preserving the archive's paths. This is primarily useful when you want Artifactory to see all the artifacts at once, e.g., for indexing purposes.
+Atomically deploy artifacts from archive: this will automatically extract the contents of the archive on the server 
+preserving the archive's paths. This is primarily useful when you want Artifactory to see all the artifacts at once, 
+e.g., for indexing purposes.
 
 ```python
 from artifactory import ArtifactoryPath
@@ -316,6 +320,14 @@ path = ArtifactoryPath(
 path.deploy_deb(
     "./myapp-1.0.deb", distribution="trusty", component="main", architecture="amd64"
 )
+
+# if you want to set multiple values you can use list to set them
+path.deploy_deb(
+    "./myapp-1.0.deb",
+    distribution=["dist1", "dist2"],
+    component="main",
+    architecture=["amd64", "i386"],
+)
 ```
 
 ## Copy Artifacts
diff --git a/artifactory.py b/artifactory.py
index 4a03462..9fcdb5b 100755
--- a/artifactory.py
+++ b/artifactory.py
@@ -317,7 +317,7 @@ def encode_matrix_parameters(parameters):
 
     for param in iter(sorted(parameters)):
         if isinstance(parameters[param], (list, tuple)):
-            value = (";%s=" % (param)).join(parameters[param])
+            value = f";{param}=".join(parameters[param])
         else:
             value = parameters[param]
 
@@ -377,6 +377,14 @@ class nullcontext:
 
 
 def quote_url(url):
+    """
+    Quote URL to allow URL fragment identifier as artifact folder or file names.
+    See https://en.wikipedia.org/wiki/Percent-encoding#Reserved_characters
+    Function will percent-encode the URL
+
+    :param url: (str) URL that should be quoted
+    :return: (str) quoted URL
+    """
     parsed_url = urllib3.util.parse_url(url)
     if parsed_url.port:
         quoted_path = requests.utils.quote(
@@ -387,7 +395,7 @@ def quote_url(url):
         )
     else:
         quoted_path = requests.utils.quote(url.rpartition(parsed_url.host)[2])
-        quoted_url = "{}://{}{}".format(parsed_url.scheme, parsed_url.host, quoted_path)
+        quoted_url = f"{parsed_url.scheme}://{parsed_url.host}{quoted_path}"
 
     return quoted_url
 
@@ -725,12 +733,18 @@ class _ArtifactoryAccessor(pathlib._Accessor):
         verify=True,
         cert=None,
         timeout=None,
+        matrix_parameters=None,
     ):
         """
         Perform a chunked PUT request to url with requests.session
         This is specifically to upload files.
         """
         url = quote_url(url)
+
+        if matrix_parameters is not None:
+            # added later, otherwise ; and = are converted
+            url += matrix_parameters
+
         res = session.put(
             url, headers=headers, data=stream, verify=verify, cert=cert, timeout=timeout
         )
@@ -1007,9 +1021,9 @@ class _ArtifactoryAccessor(pathlib._Accessor):
 
         url = str(pathobj)
 
-        if parameters:
-            url += ";%s" % encode_matrix_parameters(parameters)
-
+        matrix_parameters = (
+            f";{encode_matrix_parameters(parameters)}" if parameters else None
+        )
         headers = {}
 
         if md5:
@@ -1031,6 +1045,7 @@ class _ArtifactoryAccessor(pathlib._Accessor):
             verify=pathobj.verify,
             cert=pathobj.cert,
             timeout=pathobj.timeout,
+            matrix_parameters=matrix_parameters,
         )
 
         if code not in (200, 201):
@@ -1716,8 +1731,8 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
 
         target = self
 
-        if self.is_dir():
-            target = self / pathlib.Path(file_name).name
+        if target.is_dir():
+            target = target / pathlib.Path(file_name).name
 
         with open(file_name, "rb") as fobj:
             target.deploy(


Test patch:
diff --git a/tests/unit/test_artifactory_path.py b/tests/unit/test_artifactory_path.py
index 7df8214..e5e8fad 100644
--- a/tests/unit/test_artifactory_path.py
+++ b/tests/unit/test_artifactory_path.py
@@ -1,12 +1,11 @@
 #!/usr/bin/env python
-import io
 import os
+import pathlib
 import tempfile
 import unittest
 
 import dateutil
 import responses
-from mock import MagicMock as MM
 
 import artifactory
 from artifactory import ArtifactoryPath
@@ -41,6 +40,13 @@ class UtilTest(unittest.TestCase):
         s = artifactory.encode_properties(params)
         self.assertEqual(s, r"baz=ba\r,qu\|ux;foo=a\,s\=df")
 
+    def test_checksum(self):
+        """
+        All checksum functions are validated in ArtifactoryPathTest.test_deploy_file
+        no need to validate any more
+        :return: None
+        """
+
 
 class ArtifactoryFlavorTest(unittest.TestCase):
     flavour = artifactory._artifactory_flavour
@@ -613,26 +619,6 @@ class ArtifactoryAccessorTest(ClassSetup):
     def test_mkdir(self):
         pass
 
-    def test_deploy(self):
-        a = self.cls()
-        P = ArtifactoryPath
-
-        p = P("http://b/artifactory/c/d")
-
-        params = {"foo": "bar", "baz": "quux"}
-
-        a.rest_put_stream = MM(return_value=("OK", 200))
-
-        f = io.StringIO()
-
-        a.deploy(p, f, parameters=params)
-
-        url = "http://b/artifactory/c/d;baz=quux;foo=bar"
-
-        a.rest_put_stream.assert_called_with(
-            url, f, headers={}, session=p.session, verify=True, cert=None, timeout=None
-        )
-
     @responses.activate
     def test_get_properties(self):
         properties = {
@@ -751,6 +737,125 @@ class ArtifactoryPathTest(ClassSetup):
         a = P("http://a/artifactory/", auth=("foo", "bar"))
         self.assertEqual(a.auth, ("foo", "bar"))
 
+    @responses.activate
+    def test_deploy_file(self):
+        """
+        Test that file uploads to the path
+        :return:
+        """
+        P = self.cls
+        path = P(
+            "http://artifactory.local/artifactory/libs-release-local",
+            auth=("foo", "bar"),
+        )
+
+        constructed_url = (
+            "http://artifactory.local/artifactory" "/api/storage" "/libs-release-local"
+        )
+        responses.add(
+            responses.GET,
+            constructed_url,
+            status=200,
+            json=self.dir_stat,
+        )
+
+        matrix_parameters = "deb.architecture=amd64;deb.component=contrib;deb.distribution=dist1;deb.distribution=dist2"
+
+        # file is required to calculate checksums
+        with tempfile.NamedTemporaryFile(mode="w") as file:
+            test_file = pathlib.Path(file.name)
+            file.write("I am a test file")
+
+            constructed_url = f"{path}{test_file.name};{matrix_parameters}"
+            responses.add(
+                responses.PUT, constructed_url, status=200, match_querystring=True
+            )
+
+            path.deploy_file(
+                test_file,
+                explode_archive=True,
+                explode_archive_atomic=True,
+                parameters={
+                    "deb.architecture": "amd64",
+                    "deb.component": "contrib",
+                    "deb.distribution": ["dist1", "dist2"],
+                },
+            )
+
+        request_url = responses.calls[1].request.url
+        self.assertEqual(request_url, constructed_url)
+
+        # verify that all headers are present and checksums are calculated properly
+        headers = responses.calls[1].request.headers
+        self.assertIn("X-Checksum-Md5", headers)
+        self.assertEqual(headers["X-Checksum-Md5"], "d41d8cd98f00b204e9800998ecf8427e")
+
+        self.assertIn("X-Checksum-Sha1", headers)
+        self.assertEqual(
+            headers["X-Checksum-Sha1"], "da39a3ee5e6b4b0d3255bfef95601890afd80709"
+        )
+
+        self.assertIn("X-Checksum-Sha256", headers)
+        self.assertEqual(
+            headers["X-Checksum-Sha256"],
+            "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+        )
+
+        self.assertIn("X-Explode-Archive", headers)
+        self.assertEqual(headers["X-Explode-Archive"], "true")
+
+        self.assertIn("X-Explode-Archive-Atomic", headers)
+        self.assertEqual(headers["X-Explode-Archive-Atomic"], "true")
+
+    @responses.activate
+    def test_deploy_deb(self):
+        """
+        Test that debian package is deployed
+        :return:
+        """
+        P = self.cls
+        path = P(
+            "http://artifactory.local/artifactory/libs-release-local",
+            auth=("foo", "bar"),
+        )
+
+        constructed_url = (
+            "http://artifactory.local/artifactory" "/api/storage" "/libs-release-local"
+        )
+        responses.add(
+            responses.GET,
+            constructed_url,
+            status=200,
+            json=self.dir_stat,
+        )
+
+        matrix_parameters = (
+            "deb.architecture=amd64;deb.component=contrib;"
+            "deb.distribution=dist1;deb.distribution=dist2;"
+            "z.additional=param"
+        )
+
+        # file is required to calculate checksums
+        with tempfile.NamedTemporaryFile(mode="w") as file:
+            test_file = pathlib.Path(file.name)
+            file.write("I am a test file")
+
+            constructed_url = f"{path}{test_file.name};{matrix_parameters}"
+            responses.add(
+                responses.PUT, constructed_url, status=200, match_querystring=True
+            )
+
+            path.deploy_deb(
+                test_file,
+                distribution=["dist1", "dist2"],
+                component="contrib",
+                architecture="amd64",
+                parameters={"z.additional": "param"},
+            )
+
+        request_url = responses.calls[1].request.url
+        self.assertEqual(request_url, constructed_url)
+
     def test_auth_inheritance(self):
         P = self.cls
         b = P("http://b/artifactory/c/d", auth=("foo", "bar"))