launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/dsd-packageset-filter into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/launchpad/dsd-packageset-filter/+merge/66757

= Summary =

Plans are underway to add various filtering options to the +localpackagediffs page.  One of those would be on packagesets.  It would be helpful for the model code to start supporting these filters.


== Proposed fix ==

Here I implement a filter on package sets.  Having it in the code will make it easier to add the UI knobs, experiment with performance etc.


== Pre-implementation notes ==

Based on discussion with Julian at the Thunderdome.  He actually wants more filters, but those will need more work because they're likely to be more complex and more performance-sensitive.

The prospective feature would ideally allow for a multi-packageset filter.  I was told I could start off with a single-packageset filter, but with the query structure I chose there was really no difference.


== Implementation details ==

I match on packagesets using an "IN" subquery.  That seems more appropriate than flattening into a join, since doing so would likely multiply the number of output rows for the database to consider by large numbers.  A subquery is of course also simpler because it does not affect sorting or uniqueness clauses.

An easy change would be to add a "UNIQUE" to the subquery, but I wouldn't expect that to pay except with significant packageset overlap.


== Tests ==

{{{
./bin/test -vvc lp.registry.tests.test_distroseriesdifference
}}}


== Demo and Q/A ==

The code isn't hooked up anywhere yet.  We'll have to experiment in "make harness" on dogfood.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/tests/test_distroseriesdifference.py
  lib/lp/soyuz/model/packagesetsources.py
  lib/lp/registry/interfaces/distroseriesdifference.py
  lib/lp/registry/model/distroseriesdifference.py
-- 
https://code.launchpad.net/~jtv/launchpad/dsd-packageset-filter/+merge/66757
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/dsd-packageset-filter into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py	2011-06-14 13:47:51 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py	2011-07-04 08:47:29 +0000
@@ -297,7 +297,8 @@
         source_package_name_filter=None,
         status=None,
         child_version_higher=False,
-        parent_series=None):
+        parent_series=None,
+        packagesets=None):
         """Return differences for the derived distro series sorted by
         package name.
 
@@ -319,6 +320,7 @@
         :param parent_series: The parent series to consider. Consider all
             parent series if this parameter is None.
         :type distro_series: `IDistroSeries`.
+        :param packagesets: Optional iterable of `Packageset` to filter by.
         :return: A result set of `IDistroSeriesDifference`.
         """
 

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-06-23 10:52:32 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-07-04 08:47:29 +0000
@@ -25,6 +25,7 @@
     And,
     Column,
     Desc,
+    Select,
     Table,
     )
 from storm.locals import (
@@ -65,7 +66,6 @@
     )
 from lp.registry.interfaces.distroseriesparent import IDistroSeriesParentSet
 from lp.registry.interfaces.person import IPersonSet
-from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.distroseriesdifferencecomment import (
     DistroSeriesDifferenceComment,
     )
@@ -203,7 +203,7 @@
     return DecoratedResultSet(comments, itemgetter(0))
 
 
-def packagesets(dsds, in_parent):
+def get_packagesets(dsds, in_parent):
     """Return the packagesets for the given dsds inside the parent or
     the derived `DistroSeries`.
 
@@ -336,7 +336,8 @@
         source_package_name_filter=None,
         status=None,
         child_version_higher=False,
-        parent_series=None):
+        parent_series=None,
+        packagesets=None):
         """See `IDistroSeriesDifferenceSource`."""
         if difference_type is None:
             difference_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
@@ -368,6 +369,14 @@
                 DistroSeriesDifference.source_version >
                     DistroSeriesDifference.parent_source_version])
 
+        if packagesets is not None:
+            set_ids = [packageset.id for packageset in packagesets]
+            conditions.extend([
+                DistroSeriesDifference.source_package_name_id.is_in(
+                    Select(
+                        PackagesetSources.sourcepackagename_id,
+                        PackagesetSources.packageset_id.is_in(set_ids)))])
+
         differences = IStore(DistroSeriesDifference).find(
             DistroSeriesDifference,
             And(*conditions)).order_by(SourcePackageName.name)
@@ -410,8 +419,8 @@
                 ("sourcepackagereleaseID",))
 
             # Get packagesets and parent_packagesets for each DSD.
-            dsd_packagesets = packagesets(dsds, in_parent=False)
-            dsd_parent_packagesets = packagesets(dsds, in_parent=True)
+            dsd_packagesets = get_packagesets(dsds, in_parent=False)
+            dsd_parent_packagesets = get_packagesets(dsds, in_parent=True)
 
             # Cache latest messages contents (MessageChunk).
             messages = bulk.load_related(

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2011-06-10 23:02:22 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2011-07-04 08:47:29 +0000
@@ -36,9 +36,9 @@
     most_recent_publications,
     )
 from lp.services.propertycache import get_property_cache
-from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.enums import PackageDiffStatus
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.soyuz.model.packagesetsources import PackagesetSources
 from lp.testing import (
     celebrity_logged_in,
     person_logged_in,
@@ -217,7 +217,8 @@
             source_package_name_str="foonew",
             versions=dict(parent='1.0', derived='1.0'),
             status=DistroSeriesDifferenceStatus.RESOLVED)
-        new_parent_pub = self.factory.makeSourcePackagePublishingHistory(
+        # Publish package in the parent series.
+        self.factory.makeSourcePackagePublishingHistory(
             sourcepackagename=ds_diff.source_package_name,
             distroseries=ds_diff.parent_series,
             status=PackagePublishingStatus.PENDING,
@@ -1063,6 +1064,39 @@
         self.assertContentEqual(diffs['normal'], results)
         self.assertContentEqual(diffs2['normal'], results2)
 
+    def test_getForDistroSeries_matches_packageset(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        packageset = self.factory.makePackageset(
+            distroseries=dsd.derived_series)
+        Store.of(dsd).add(PackagesetSources(
+            packageset=packageset, sourcepackagename=dsd.source_package_name))
+        dsd_source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual(
+            [dsd], dsd_source.getForDistroSeries(
+                dsd.derived_series, packagesets=(packageset, )))
+
+    def test_getForDistroSeries_matches_any_packageset_in_filter(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        packagesets = [
+            self.factory.makePackageset(distroseries=dsd.derived_series)
+            for counter in xrange(2)]
+        Store.of(dsd).add(PackagesetSources(
+            packageset=packagesets[0],
+            sourcepackagename=dsd.source_package_name))
+        dsd_source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual(
+            [dsd], dsd_source.getForDistroSeries(
+                dsd.derived_series, packagesets=packagesets))
+
+    def test_getForDistroSeries_filters_by_packageset(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        packageset = self.factory.makePackageset(
+            distroseries=dsd.derived_series)
+        dsd_source = getUtility(IDistroSeriesDifferenceSource)
+        self.assertContentEqual(
+            [], dsd_source.getForDistroSeries(
+                dsd.derived_series, packagesets=(packageset, )))
+
     def test_getByDistroSeriesNameAndParentSeries(self):
         # An individual difference is obtained using the name.
         ds_diff = self.factory.makeDistroSeriesDifference(

=== modified file 'lib/lp/soyuz/model/packagesetsources.py'
--- lib/lp/soyuz/model/packagesetsources.py	2011-06-22 13:08:05 +0000
+++ lib/lp/soyuz/model/packagesetsources.py	2011-07-04 08:47:29 +0000
@@ -39,3 +39,7 @@
     sourcepackagename_id = Int(name='sourcepackagename')
     sourcepackagename = Reference(
         sourcepackagename_id, 'SourcePackageName.id')
+
+    def __init__(self, packageset, sourcepackagename):
+        self.packageset = packageset
+        self.sourcepackagename = sourcepackagename