Merge ~xypron/ppa-dev-tools:urldecode into ppa-dev-tools:main

[Heinrich Schuchardt]

Job.request_url: urlencode parameter strings

Triggering autopkgtests for a package with a version number like 8.2313.0-2ubuntu1+ppa1 fail due to missing urlencoding (LP: #2049105).

Update the corresponding test.

[Bryce Harrington (bryce)]

Hi Heinrich,

Big thanks for proposing a fix along with the bug. Yes, this looks like a very good way to solve the problem and I agree with your approach.

Can you also include a unit test case along with this fix? It should fail without your fix applied, and pass with it (usual TDD stuff).

Thanks again for implementing the fix!

review: Needs Information

[Bryce Harrington (bryce)]

Thanks for adding the thorough set of tests! I marked them as xfail in the first commit to make pytest happy, and have pushed to main:

Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 12 threads
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 1.57 KiB | 1.57 MiB/s, done.
Total 10 (delta 8), reused 0 (delta 0), pack-reused 0
To git+ssh://git.launchpad.net/ppa-dev-tools
   55e2df3..8a46dc9 main -> main

review: Approve

Unmerged commits

9f4b2da... by Heinrich Schuchardt on 2024-01-11

Job.request_url: urlencode parameter strings

Triggering autopkgtests for a package with a version number like
8.2313.0-2ubuntu1+ppa1 fail due to missing urlencoding.
(LP: #2049105)

Signed-off-by: Heinrich Schuchardt <email address hidden>

6da4220... by Heinrich Schuchardt on 2024-01-12

test_request_url: provide more test cases

Parameterize test_request_url() and provide more test cases.
Expect parameters in a URL to be URL encoded.

Signed-off-by: Heinrich Schuchardt <email address hidden>

diff --git a/ppa/job.py b/ppa/job.py
index 669fe48..ec675e0 100755
--- a/ppa/job.py
+++ b/ppa/job.py
@@ -14,6 +14,7 @@
 import json
 from functools import lru_cache
 from typing import Iterator
+import urllib
 
 from .constants import URL_AUTOPKGTEST
 
@@ -82,13 +83,13 @@ class Job:
         :rtype: str
         :returns: Full URL for invoking the test.
         """
-        rel_str = f"release={self.series}"
-        arch_str = f"&arch={self.arch}"
-        pkg_str = f"&package={self.source_package}"
-        trigger_str = ''
+        parameter_str = urllib.parse.urlencode({
+            'release' : self.series,
+            'arch' : self.arch,
+            'package' : self.source_package})
         for trigger in self.triggers:
-            trigger_str += f"&trigger={trigger}"
-        return f"{URL_AUTOPKGTEST}/request.cgi?{rel_str}{arch_str}{pkg_str}{trigger_str}"
+            parameter_str += "&" + urllib.parse.urlencode({"trigger" : trigger})
+        return f"{URL_AUTOPKGTEST}/request.cgi?{parameter_str}"
 
 
 def get_running(response, releases=None, ppa=None) -> Iterator[Job]:
diff --git a/tests/test_job.py b/tests/test_job.py
index 852e04e..b33d299 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -14,6 +14,7 @@ import os
 import sys
 
 import json
+import pytest
 
 sys.path.insert(0, os.path.realpath(
     os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
@@ -89,16 +90,24 @@ def test_ppas():
     job = Job(0, '', '', '', '', ppas=['ppa:a/b', 'ppa:c/d'])
     assert job.ppas == ['ppa:a/b', 'ppa:c/d']
 
-
-def test_request_url():
[e-mail]('triggers, endswith', [
+        ( ['t/1'], '?release=c&arch=d&package=b&trigger=t%2F1' ),
+        ( ['t/2.3'], '?release=c&arch=d&package=b&trigger=t%2F2.3' ),
+        ( ['t/2.3-1'], '?release=c&arch=d&package=b&trigger=t%2F2.3-1' ),
+        ( ['t/2.3-1ubuntu2'], '?release=c&arch=d&package=b&trigger=t%2F2.3-1ubuntu2' ),
+        ( ['t/2.3-1ubuntu2~ppa1'], '?release=c&arch=d&package=b&trigger=t%2F2.3-1ubuntu2~ppa1' ),
+        ( ['t/2.3-1ubuntu2+ppa2'], '?release=c&arch=d&package=b&trigger=t%2F2.3-1ubuntu2%2Bppa2' ),
+        ( ['t/1', 'u/2'], '?release=c&arch=d&package=b&trigger=t%2F1&trigger=u%2F2' ),
+    ])
+def test_request_url(triggers, endswith):
     """Checks Job object textual presentation."""
     jobinfo = {
-        'triggers': ['t/1'],
+        'triggers': triggers,
         'ppas': ['ppa:a/b', 'ppa:c/d']
     }
     job = Job(0, 'a', 'b', 'c', 'd', jobinfo['triggers'], jobinfo['ppas'])
     assert job.request_url.startswith("https://autopkgtest.ubuntu.com/request.cgi")
-    assert job.request_url.endswith("?release=c&arch=d&package=b&trigger=t/1")
+    assert job.request_url.endswith(endswith)
 
 
 def test_get_running():