launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/dsd-invalid-versions into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/dsd-invalid-versions/+merge/55461

Filter out invalid versions from parsing the changelog, so that they don't try and get set as the base version.
-- 
https://code.launchpad.net/~stevenk/launchpad/dsd-invalid-versions/+merge/55461
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/dsd-invalid-versions into lp:launchpad.

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-03-29 16:23:49 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-03-30 03:17:30 +0000
@@ -9,7 +9,10 @@
     'DistroSeriesDifference',
     ]
 
-from debian.changelog import Changelog
+from debian.changelog import (
+    Changelog,
+    Version,
+    )
 from lazr.enum import DBItem
 from storm.expr import Desc
 from storm.locals import (
@@ -201,7 +204,17 @@
         """Return the version ancestry for the given SPR, or None."""
         if spr.changelog is None:
             return None
-        return set(Changelog(spr.changelog.read()).versions)
+        versions = set()
+        # It would be nicer to use .versions() here, but it won't catch the
+        # ValueError from malformed versions, and we don't want them leaking
+        # into the ancestry.
+        for raw_version in Changelog(spr.changelog.read())._raw_versions():
+            try:
+                version = Version(raw_version)
+            except ValueError:
+                continue
+            versions.add(version)
+        return versions
 
     def _getPackageDiffURL(self, package_diff):
         """Check status and return URL if appropriate."""

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2011-03-14 09:36:59 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2011-03-30 03:17:30 +0000
@@ -466,6 +466,31 @@
 
         self.assertEqual('1.1', ds_diff.base_version)
 
+    def test_base_version_invalid(self):
+        # If the maximum base version is invalid, it is discarded and not
+        # set as the base version.
+        derived_series = self.factory.makeDistroSeries(
+            parent_series=self.factory.makeDistroSeries())
+        source_package_name = self.factory.getOrMakeSourcePackageName('foo')
+        # Create changelogs for both.
+        changelog_lfa = self.factory.makeChangelog(
+            'foo', ['1:2.0-1', 'a1:1.8.8-070403-1~priv1', '1:1.7-1'])
+        parent_changelog_lfa = self.factory.makeChangelog(
+            'foo', ['1:2.0-2', 'a1:1.8.8-070403-1~priv1', '1:1.7-1'])
+        transaction.commit() # Yay, librarian.
+
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            derived_series=derived_series, source_package_name_str='foo',
+            versions={
+                'derived': '1:2.0-1',
+                'parent': '1:2.0-2',
+                },
+            changelogs={
+                'derived': changelog_lfa,
+                'parent': parent_changelog_lfa})
+
+        self.assertEqual('1:1.7-1', ds_diff.base_version)
+
     def test_base_source_pub_none(self):
         # None is simply returned if there is no base version.
         ds_diff = self.factory.makeDistroSeriesDifference()