launchpad-reviewers team mailing list archive

[Colin Watson <mp+425180@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:bpr-format-specific-name-constraint into launchpad:master.

Commit message:
Apply format-specific checks to BPR.binarypackagename

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This allows us to apply different rules to different packaging formats.  It won't be effective until the database constraint on `BinaryPackageName.name` is dropped, but applying this first allows us to do so without weakening the rules for Debian-format packages.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:bpr-format-specific-name-constraint into launchpad:master.

diff --git a/lib/lp/soyuz/interfaces/binarypackagerelease.py b/lib/lp/soyuz/interfaces/binarypackagerelease.py
index 2b243cb..bffca64 100644
--- a/lib/lp/soyuz/interfaces/binarypackagerelease.py
+++ b/lib/lp/soyuz/interfaces/binarypackagerelease.py
@@ -4,11 +4,15 @@
 """Binary package release interfaces."""
 
 __all__ = [
+    'BinaryPackageReleaseNameLinkageError',
     'IBinaryPackageRelease',
     'IBinaryPackageReleaseDownloadCount',
     ]
 
+import http.client
+
 from lazr.restful.declarations import (
+    error_status,
     exported,
     exported_as_webservice_entry,
     )
@@ -37,6 +41,11 @@ from lp.services.worlddata.interfaces.country import ICountry
 from lp.soyuz.interfaces.archive import IArchive
 
 
+@error_status(http.client.BAD_REQUEST)
+class BinaryPackageReleaseNameLinkageError(ValueError):
+    """A binary package name is inappropriate for this release's format."""
+
+
 class IBinaryPackageRelease(Interface):
     id = Int(title=_('ID'), required=True)
     binarypackagename = Int(required=True)
diff --git a/lib/lp/soyuz/model/binarypackagerelease.py b/lib/lp/soyuz/model/binarypackagerelease.py
index 15192c2..fd47a09 100644
--- a/lib/lp/soyuz/model/binarypackagerelease.py
+++ b/lib/lp/soyuz/model/binarypackagerelease.py
@@ -7,6 +7,7 @@ __all__ = [
     ]
 
 from operator import attrgetter
+import re
 
 import simplejson
 from storm.locals import (
@@ -19,9 +20,11 @@ from storm.locals import (
 from zope.component import getUtility
 from zope.interface import implementer
 
+from lp.app.validators.name import valid_name_pattern
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.enumcol import DBEnum
+from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import SQLBase
 from lp.services.database.sqlobject import (
     BoolCol,
@@ -40,15 +43,63 @@ from lp.soyuz.enums import (
     PackagePublishingPriority,
     )
 from lp.soyuz.interfaces.binarypackagerelease import (
+    BinaryPackageReleaseNameLinkageError,
     IBinaryPackageRelease,
     IBinaryPackageReleaseDownloadCount,
     )
 from lp.soyuz.interfaces.binarysourcereference import (
     IBinarySourceReferenceSet,
     )
+from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.files import BinaryPackageFile
 
 
+# https://packaging.python.org/en/latest/specifications/core-metadata/#id6
+wheel_name_pattern = re.compile(
+    r"^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$", re.IGNORECASE)
+
+# There doesn't seem to be a very useful specification for Conda's package
+# name syntax:
+# https://docs.conda.io/projects/conda-build/en/latest/resources/package-spec.html
+# just says 'The lowercase name of the package. May contain the "-"
+# character'.  conda_build.metadata.MetaData.name implements a few specific
+# checks, but apparently in terms of which characters are forbidden rather
+# than which characters are allowed.  For now, constrain this to something
+# reasonably conservative and hope that this is OK.
+conda_name_pattern = re.compile(r"^[a-z0-9][a-z0-9.+_-]*$")
+
+
+def _validate_bpr_name(obj, attr, value):
+    # Validate that a BinaryPackageRelease's BinaryPackageName is
+    # appropriate for its format.
+    if not isinstance(value, int):
+        raise AssertionError(
+            "Expected int for BinaryPackageName foreign key reference, got %r"
+            % type(value))
+
+    name = IStore(BinaryPackageName).get(BinaryPackageName, value).name
+    if obj.binpackageformat == BinaryPackageFormat.WHL:
+        if not wheel_name_pattern.match(name):
+            raise BinaryPackageReleaseNameLinkageError(
+                "Invalid Python wheel name '%s'; must match /%s/i"
+                % (name, wheel_name_pattern.pattern))
+    elif obj.binpackageformat in (
+        BinaryPackageFormat.CONDA_V1,
+        BinaryPackageFormat.CONDA_V2,
+    ):
+        if not conda_name_pattern.match(name):
+            raise BinaryPackageReleaseNameLinkageError(
+                "Invalid Conda name '%s'; must match /%s/"
+                % (name, conda_name_pattern.pattern))
+    else:
+        # Fall back to Launchpad's traditional name validation, which
+        # coincides with the rules for Debian-format package names.
+        if not valid_name_pattern.match(name):
+            raise BinaryPackageReleaseNameLinkageError(
+                "Invalid package name '%s'; must match /%s/"
+                % (name, valid_name_pattern.pattern))
+
+
 @implementer(IBinaryPackageRelease)
 class BinaryPackageRelease(SQLBase):
     _table = 'BinaryPackageRelease'
@@ -99,6 +150,13 @@ class BinaryPackageRelease(SQLBase):
                 kwargs['user_defined_fields'])
             del kwargs['user_defined_fields']
         super().__init__(*args, **kwargs)
+        # XXX cjwatson 2022-06-21: Ideally we'd set this up as a Storm
+        # validator, but that's difficult to arrange with SQLBase since we
+        # can't guarantee that self.binpackageformat will be set before
+        # self.binarypackagename, so just call it by hand here using the
+        # calling convention for validators.
+        _validate_bpr_name(
+            self, "binarypackagename", self.binarypackagename.id)
 
     @cachedproperty
     def built_using_references(self):
diff --git a/lib/lp/soyuz/tests/test_binarypackagerelease.py b/lib/lp/soyuz/tests/test_binarypackagerelease.py
index 56bc852..9826000 100644
--- a/lib/lp/soyuz/tests/test_binarypackagerelease.py
+++ b/lib/lp/soyuz/tests/test_binarypackagerelease.py
@@ -3,8 +3,13 @@
 
 """Test BinaryPackageRelease."""
 
+from psycopg2.errors import CheckViolation
+
 from lp.soyuz.enums import BinaryPackageFormat
-from lp.soyuz.interfaces.binarypackagerelease import IBinaryPackageRelease
+from lp.soyuz.interfaces.binarypackagerelease import (
+    BinaryPackageReleaseNameLinkageError,
+    IBinaryPackageRelease,
+    )
 from lp.soyuz.interfaces.publishing import PackagePublishingPriority
 from lp.testing import TestCaseWithFactory
 from lp.testing.layers import LaunchpadFunctionalLayer
@@ -61,3 +66,78 @@ class TestBinaryPackageRelease(TestCaseWithFactory):
         # does not have to be valid.
         bpr = self.factory.makeBinaryPackageRelease(homepage="<invalid<url")
         self.assertEqual("<invalid<url", bpr.homepage)
+
+    def test_deb_name(self):
+        self.factory.makeBinaryPackageRelease(binarypackagename="foo")
+        try:
+            self.assertRaises(
+                BinaryPackageReleaseNameLinkageError,
+                self.factory.makeBinaryPackageRelease,
+                binarypackagename="foo_bar")
+        except CheckViolation:
+            # Temporarily allow this until the BinaryPackageName.name DB
+            # constraint is relaxed.
+            pass
+
+    def test_wheel_name(self):
+        self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo", binpackageformat=BinaryPackageFormat.WHL)
+        try:
+            self.factory.makeBinaryPackageRelease(
+                binarypackagename="Foo_bar",
+                binpackageformat=BinaryPackageFormat.WHL)
+            self.assertRaises(
+                BinaryPackageReleaseNameLinkageError,
+                self.factory.makeBinaryPackageRelease,
+                binarypackagename="foo_bar+",
+                binpackageformat=BinaryPackageFormat.WHL)
+        except CheckViolation:
+            # Temporarily allow this until the BinaryPackageName.name DB
+            # constraint is relaxed.
+            pass
+
+    def test_conda_v1_name(self):
+        self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo",
+            binpackageformat=BinaryPackageFormat.CONDA_V1)
+        try:
+            self.factory.makeBinaryPackageRelease(
+                binarypackagename="foo-bar_baz",
+                binpackageformat=BinaryPackageFormat.CONDA_V1)
+            self.assertRaises(
+                BinaryPackageReleaseNameLinkageError,
+                self.factory.makeBinaryPackageRelease,
+                binarypackagename="Foo",
+                binpackageformat=BinaryPackageFormat.CONDA_V1)
+            self.assertRaises(
+                BinaryPackageReleaseNameLinkageError,
+                self.factory.makeBinaryPackageRelease,
+                binarypackagename="foo_bar#",
+                binpackageformat=BinaryPackageFormat.CONDA_V1)
+        except CheckViolation:
+            # Temporarily allow this until the BinaryPackageName.name DB
+            # constraint is relaxed.
+            pass
+
+    def test_conda_v2_name(self):
+        self.factory.makeBinaryPackageRelease(
+            binarypackagename="foo",
+            binpackageformat=BinaryPackageFormat.CONDA_V2)
+        try:
+            self.factory.makeBinaryPackageRelease(
+                binarypackagename="foo-bar_baz",
+                binpackageformat=BinaryPackageFormat.CONDA_V2)
+            self.assertRaises(
+                BinaryPackageReleaseNameLinkageError,
+                self.factory.makeBinaryPackageRelease,
+                binarypackagename="Foo",
+                binpackageformat=BinaryPackageFormat.CONDA_V2)
+            self.assertRaises(
+                BinaryPackageReleaseNameLinkageError,
+                self.factory.makeBinaryPackageRelease,
+                binarypackagename="foo_bar#",
+                binpackageformat=BinaryPackageFormat.CONDA_V2)
+        except CheckViolation:
+            # Temporarily allow this until the BinaryPackageName.name DB
+            # constraint is relaxed.
+            pass