launchpad-reviewers team mailing list archive

[Colin Watson <mp+438685@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/lp-archive:legacy-ppa into lp-archive:main.

Commit message:
Accept legacy PPA URL syntax

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/lp-archive/+git/lp-archive/+merge/438685

PPAs are currently published on (private-)ppa.launchpadcontent.net using a somewhat odd URL scheme that lacks the usual leading tilde from the owner name and places the archive name before the distribution name.  This results from the historical evolution of PPAs, and is difficult to change with disk-based publishing.

However, thanks to the lack of a leading tilde, we can safely accept both this and the modern archive-reference-based syntax, making it easier to migrate from existing PPAs to this service.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/lp-archive:legacy-ppa into lp-archive:main.

diff --git a/lp_archive/routing.py b/lp_archive/routing.py
index c82619d..98b4d50 100644
--- a/lp_archive/routing.py
+++ b/lp_archive/routing.py
@@ -21,12 +21,27 @@ class PPAConverter(BaseConverter):
     """Match a PPA reference.
 
     See `lp.soyuz.model.archive.Archive.reference` in Launchpad.
+
+    We also accept a legacy syntax.  For historical reasons, PPAs have been
+    published for a long time using an owner/archive/distribution URL
+    scheme.  This is odd for two reasons (no leading '~' on the owner, and
+    archives should be subordinate to distributions rather than vice versa),
+    but it's entrenched and difficult to change.  Fortunately, the lack of a
+    leading '~' on the owner means that we can unambiguously accept both
+    this and the modern syntax.
     """
 
     # ~owner/distribution/archive
-    regex = r"~[^/]+/[^/]+/[^/]+"
+    regex = r"~?[^/]+/[^/]+/[^/]+"
     part_isolating = False
 
+    def to_python(self, value: str) -> str:
+        if value.startswith("~"):
+            return value
+        else:
+            owner, archive, distribution = value.split("/")
+            return f"~{owner}/{distribution}/{archive}"
+
 
 class TimestampConverter(BaseConverter):
     """Match a timestamp.
diff --git a/tests/test_routing.py b/tests/test_routing.py
index f9c3bb7..87210bc 100644
--- a/tests/test_routing.py
+++ b/tests/test_routing.py
@@ -62,6 +62,24 @@ def test_ppa(app, client):
         )
 
 
+def test_ppa_legacy(app, client):
+    @app.route("/+test/<ppa:archive>", host="ppa.ubuntu.test")
+    def index(archive):
+        return archive
+
+    response = client.get(
+        "/+test/owner/ppa/ubuntu", headers=[("Host", "ppa.ubuntu.test")]
+    )
+    assert response.status_code == 200
+    assert response.data == b"~owner/ubuntu/ppa"
+
+    with app.test_request_context():
+        assert (
+            url_for("index", archive="~owner/ubuntu/ppa")
+            == "http://ppa.ubuntu.test/+test/~owner/ubuntu/ppa";
+        )
+
+
 def test_ppa_invalid(app, client):
     @app.route("/+test/<ppa:archive>", host="ppa.ubuntu.test")
     def index(archive):  # pragma: no cover