Merge lp:~stevenk/launchpad/generic-overrides into lp:launchpad

[Steve Kowalik]

[r=allenap][no-qa] Add three generic override support classes.

Add a generic overrides support class.

In this context, overrides are a description of which component, section and priority a source or binary package should be stored in. This is most used in sync'ing from Debian, where the component in Debian is 'main', but Ubuntu has the source in 'universe'. It also allows further copies or uploads to the same archive/distroseries to respect the previous settings and use them for that upload.

Nothing makes use of it in this branch, but it sets up the ground work. The package copier will make use of this framework in a future branch, and the plan is to have the publisher and the upload processor also make use of this framework.

I have also done two drive-bys in this branch, I moved a test into adapters/tests, and I deleted the test harness stuff from another test.

[Gavin Panella (allenap)]

Looks good, but I have some comments about the choice of API. There
are also some problems in the way you're measuring query counts which
means that the tests have missed some problems. So, the meaty bits of
this branch are good, but it still needs some attention before
landing.

[1]

+ """Override policy that returns the SPN, component and section for
+ the latest published source publication, or the BPN, DAS, component,
+ section and priority for the latest published binary publication.
+ """

For the sake of our future selves and our children, please can you
write out SPN, BPN, etc. in full?

Also, the docstring policy is meant to be something like:

    """Short, single-line summary.

    Longer explanation.
    """

I assume it's like this so that it looks good in autogenerated
documentation, but I don't know how many people use it. I certainly
only look at the code.

[2]

+class BaseOverridePolicy:
+
+ def calculateOverrides(self, archive, distroseries, pocket, sources=None,
+ binaries=None):
+ raise NotImplementedError("Must be implemented by sub-class.")

Looking at the implementations - none of which want to be passed both
sources and binaries - I wonder why not split this into two methods,
calculateSourceOverrides() and calculateBinaryOverrides().

[3]

+ def findSourceOverrides(self, archive, distroseries, pocket, spns):
+ store = IStore(SourcePackagePublishingHistory)
+ already_published = DecoratedResultSet(
+ store.find(
+ (SQL("""DISTINCT ON (
+ SourcePackageRelease.sourcepackagename) 0 AS ignore"""),
+ SourcePackageRelease.sourcepackagenameID,
+ SourcePackagePublishingHistory.componentID,
+ SourcePackagePublishingHistory.sectionID),
+ SourcePackagePublishingHistory.pocket == pocket,
+ SourcePackagePublishingHistory.archiveID == archive.id,
+ SourcePackagePublishingHistory.distroseriesID ==
+ distroseries.id,
+ SourcePackagePublishingHistory.status.is_in(
+ active_publishing_status),
+ SourcePackageRelease.id ==
+ SourcePackagePublishingHistory.sourcepackagereleaseID,
+ SourcePackageRelease.sourcepackagenameID.is_in(
+ spn.id for spn in spns)).order_by(
+ SourcePackageRelease.sourcepackagenameID,
+ Desc(SourcePackagePublishingHistory.datecreated),
+ Desc(SourcePackagePublishingHistory.id)),
+ source_resolve_ids, pre_iter_hook=source_eager_load)
+ return list(already_published)

Why not join directly to SourcePackageName, Component and Section?
That would save the confusing dance with source_resolve_ids() and
source_eager_load() (both of which might be better off defined in
findSourceOverrides() too).

I tested it to see if I was missing something, but it seems not. The
following definition passes the tests:

    def findSourceOverrides(self, archive, distroseries, pocket, spns):
        store = IStore(SourcePacka...

review: Needs Fixing

[Gavin Panella (allenap)]

> [4]
> ...
> Also, see [2].

Actually, see [3].

[Steve Kowalik (stevenk)]

[1]: Both done, thanks.

[2]: After thinking about this for a while, I JFDI'd, and I think you'll agree it looks cleaner.

[3]: Mainly because {Source,Binary}PackageName are large tables, and I don't want to bog down the query planner by making a complex query even more complex.

[4]: Nice catch! Missing ORDER BY added.

[5]: Shifted around, and switched to bulk.load().

[6]: There is, so I can call them directly from inside UbuntuOverridePolicy.

[7]: Indeed, it was masking the failure in the eager loading functions, thanks. It also pointed out that both source and binary query counts were wrong -- I was wondering why the source call only issued two queries when I was expecting four.

[8]: We may as well materialize them, since the caller is just going to loop over it anyway.

[Steve Kowalik (stevenk)]

[3]: Further, lots of the components and section will be the same, so we gain a large penalty by having to construct lots of similar objects inside Storm.

[Gavin Panella (allenap)]

Tip top, land it :)

review: Approve

=== added file 'lib/lp/soyuz/adapters/overrides.py'
--- lib/lp/soyuz/adapters/overrides.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/adapters/overrides.py	2011-05-13 08:32:29 +0000
@@ -0,0 +1,215 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Generic Override Policy classes.
+"""
+
+__metaclass__ = type
+
+__all__ = [
+    'FromExistingOverridePolicy',
+    'UbuntuOverridePolicy',
+    'UnknownOverridePolicy',
+    ]
+
+
+from storm.expr import (
+    And,
+    Desc,
+    Or,
+    SQL,
+    )
+
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet,
+    )
+from canonical.launchpad.interfaces.lpstorm import IStore
+from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.services.database import bulk
+from lp.soyuz.interfaces.publishing import active_publishing_status
+from lp.soyuz.model.binarypackagename import BinaryPackageName
+from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
+from lp.soyuz.model.component import Component
+from lp.soyuz.model.distroarchseries import DistroArchSeries
+from lp.soyuz.model.publishing import (
+    BinaryPackagePublishingHistory,
+    SourcePackagePublishingHistory,
+    )
+from lp.soyuz.model.section import Section
+from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+
+
+class BaseOverridePolicy:
+
+    def calculateSourceOverrides(self, archive, distroseries, pocket,
+                                 sources):
+        raise NotImplementedError()
+
+    def calculateBinaryOverrides(self, archive, distroseries, pocket,
+                                 binaries):
+        raise NotImplementedError()
+
+
+class FromExistingOverridePolicy(BaseOverridePolicy):
+    """Override policy that only searches for existing publications.
+    
+    Override policy that returns the SourcePackageName, component and
+    section for the latest published source publication, or the
+    BinaryPackageName, DistroArchSeries, component, section and priority
+    for the latest published binary publication.
+    """
+
+    def calculateSourceOverrides(self, archive, distroseries, pocket, spns):
+        store = IStore(SourcePackagePublishingHistory)
+        def eager_load(rows):
+            bulk.load(Component, (row[2] for row in rows))
+            bulk.load(Section, (row[3] for row in rows))
+        already_published = DecoratedResultSet(
+            store.find(
+                (SQL("""DISTINCT ON (
+                    SourcePackageRelease.sourcepackagename) 0 AS ignore"""),
+                    SourcePackageRelease.sourcepackagenameID,
+                    SourcePackagePublishingHistory.componentID,
+                    SourcePackagePublishingHistory.sectionID),
+                SourcePackagePublishingHistory.pocket == pocket,
+                SourcePackagePublishingHistory.archiveID == archive.id,
+                SourcePackagePublishingHistory.distroseriesID ==
+                    distroseries.id,
+                SourcePackagePublishingHistory.status.is_in(
+                    active_publishing_status),
+                SourcePackageRelease.id ==
+                    SourcePackagePublishingHistory.sourcepackagereleaseID,
+                SourcePackageRelease.sourcepackagenameID.is_in(
+                    spn.id for spn in spns)).order_by(
+                        SourcePackageRelease.sourcepackagenameID,
+                        Desc(SourcePackagePublishingHistory.datecreated),
+                        Desc(SourcePackagePublishingHistory.id)),
+            id_resolver(
+                (SourcePackageName, Component, Section), slice(1, None)),
+            pre_iter_hook=eager_load)
+        return list(already_published)
+
+    def calculateBinaryOverrides(self, archive, distroseries, pocket,
+                                 binaries):
+        store = IStore(BinaryPackagePublishingHistory)
+        def eager_load(rows):
+            bulk.load(Component, (row[3] for row in rows))
+            bulk.load(Section, (row[4] for row in rows))
+        expanded = calculate_target_das(distroseries, binaries)
+        candidates = (
+            make_package_condition(archive, das, bpn)
+            for bpn, das in expanded)
+        already_published = DecoratedResultSet(
+            store.find(
+                (SQL("""DISTINCT ON (
+                    BinaryPackagePublishingHistory.distroarchseries,
+                    BinaryPackageRelease.binarypackagename) 0
+                    AS ignore"""),
+                    BinaryPackageRelease.binarypackagenameID,
+                    BinaryPackagePublishingHistory.distroarchseriesID,
+                    BinaryPackagePublishingHistory.componentID,
+                    BinaryPackagePublishingHistory.sectionID,
+                    BinaryPackagePublishingHistory.priority),
+                BinaryPackagePublishingHistory.pocket == pocket,
+                BinaryPackagePublishingHistory.status.is_in(
+                    active_publishing_status),
+                BinaryPackageRelease.id ==
+                    BinaryPackagePublishingHistory.binarypackagereleaseID,
+                Or(*candidates)).order_by(
+                    BinaryPackagePublishingHistory.distroarchseriesID,
+                    BinaryPackageRelease.binarypackagenameID,
+                    Desc(BinaryPackagePublishingHistory.datecreated),
+                    Desc(BinaryPackagePublishingHistory.id)),
+            id_resolver(
+                (BinaryPackageName, DistroArchSeries, Component, Section,
+                None),
+                slice(1, None)),
+            pre_iter_hook=eager_load)
+        return list(already_published)
+
+
+class UnknownOverridePolicy(BaseOverridePolicy):
+    """Override policy that returns defaults.
+    
+    Override policy that assumes everything passed in doesn't exist, so
+    returns the defaults.
+    """
+    
+    def calculateSourceOverrides(self, archive, distroseries, pocket,
+                                 sources):
+        default_component = archive.default_component or 'universe'
+        return [(source, default_component, None) for source in sources]
+
+    def calculateBinaryOverrides(self, archive, distroseries, pocket,
+                                 binaries):
+        default_component = archive.default_component or 'universe'
+        return [
+            (binary, das, default_component, None, None)
+            for binary, das in calculate_target_das(distroseries, binaries)]
+
+
+class UbuntuOverridePolicy(FromExistingOverridePolicy,
+                           UnknownOverridePolicy):
+    """Override policy for Ubuntu.
+    
+    An override policy that incorporates both the from existing policy 
+    and the unknown policy.
+    """
+
+    def calculateSourceOverrides(self, archive, distroseries, pocket,
+                                 sources):
+        total = set(sources)
+        overrides = FromExistingOverridePolicy.calculateSourceOverrides(
+            self, archive, distroseries, pocket, sources)
+        existing = set(override[0] for override in overrides)
+        missing = total.difference(existing)
+        if missing:
+            unknown = UnknownOverridePolicy.calculateSourceOverrides(
+                self, archive, distroseries, pocket, missing)
+            overrides.extend(unknown)
+        return overrides
+
+    def calculateBinaryOverrides(self, archive, distroseries, pocket,
+                                 binaries):
+        total = set(binaries)
+        overrides = FromExistingOverridePolicy.calculateBinaryOverrides(
+            self, archive, distroseries, pocket, binaries)
+        existing = set((
+            overide[0], overide[1].architecturetag)
+                for overide in overrides)
+        missing = total.difference(existing)
+        if missing:
+            unknown = UnknownOverridePolicy.calculateBinaryOverrides(
+                self, archive, distroseries, pocket, missing)
+            overrides.extend(unknown)
+        return overrides
+
+
+def calculate_target_das(distroseries, binaries):
+    arch_map = dict(
+        (arch.architecturetag, arch)
+        for arch in distroseries.enabled_architectures)
+
+    with_das = []
+    for bpn, archtag in binaries:
+        if archtag is not None:
+            with_das.append((bpn, arch_map.get(archtag)))
+        else:
+            with_das.append((bpn, distroseries.nominatedarchindep))
+    return with_das
+
+
+def make_package_condition(archive, das, bpn):
+    return And(
+        BinaryPackagePublishingHistory.archiveID == archive.id,
+        BinaryPackagePublishingHistory.distroarchseriesID == das.id,
+        BinaryPackageRelease.binarypackagenameID == bpn.id)
+
+
+def id_resolver(lookups, slice):
+    def _resolve(row):
+        store = IStore(SourcePackagePublishingHistory)
+        return tuple(
+            (value if cls is None else store.get(cls, value))
+            for value, cls in zip(row[slice], lookups))
+    return _resolve

=== renamed file 'lib/lp/soyuz/tests/test_copypolicy.py' => 'lib/lp/soyuz/adapters/tests/test_copypolicy.py'
=== added file 'lib/lp/soyuz/adapters/tests/test_overrides.py'
--- lib/lp/soyuz/adapters/tests/test_overrides.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/adapters/tests/test_overrides.py	2011-05-13 08:32:29 +0000
@@ -0,0 +1,223 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test generic override policy classes."""
+
+from testtools.matchers import Equals
+
+from canonical.database.sqlbase import flush_database_caches
+from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.services.database import bulk
+from lp.soyuz.adapters.overrides import (
+    FromExistingOverridePolicy,
+    UbuntuOverridePolicy,
+    UnknownOverridePolicy,
+    )
+from lp.soyuz.enums import PackagePublishingStatus
+from lp.testing import (
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import HasQueryCount
+
+
+class TestOverrides(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_no_source_overrides(self):
+        # If the spn is not published in the given archive/distroseries, an
+        # empty list is returned.
+        spn = self.factory.makeSourcePackageName()
+        distroseries = self.factory.makeDistroSeries()
+        pocket = self.factory.getAnyPocket()
+        policy = FromExistingOverridePolicy()
+        overrides = policy.calculateSourceOverrides(
+            distroseries.main_archive, distroseries, pocket, (spn,))
+        self.assertEqual([], overrides)
+
+    def test_source_overrides(self):
+        # When the spn is published in the given archive/distroseries, the
+        # overrides for that archive/distroseries are returned.
+        spph = self.factory.makeSourcePackagePublishingHistory()
+        policy = FromExistingOverridePolicy()
+        overrides = policy.calculateSourceOverrides(
+            spph.distroseries.main_archive, spph.distroseries, spph.pocket,
+            (spph.sourcepackagerelease.sourcepackagename,))
+        expected = [(
+            spph.sourcepackagerelease.sourcepackagename,
+            spph.component, spph.section)]
+        self.assertEqual(expected, overrides)
+
+    def test_source_overrides_latest_only_is_returned(self):
+        # When the spn is published multiple times in the given
+        # archive/distroseries, the latest publication's overrides are
+        # returned.
+        spn = self.factory.makeSourcePackageName()
+        distroseries = self.factory.makeDistroSeries()
+        published_spr = self.factory.makeSourcePackageRelease(
+            sourcepackagename=spn)
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagerelease=published_spr, distroseries=distroseries,
+            status=PackagePublishingStatus.PUBLISHED)
+        spr = self.factory.makeSourcePackageRelease(
+            sourcepackagename=spn)
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagerelease=spr, distroseries=distroseries)
+        policy = FromExistingOverridePolicy()
+        overrides = policy.calculateSourceOverrides(
+            distroseries.main_archive, distroseries, spph.pocket, (spn,))
+        self.assertEqual([(spn, spph.component, spph.section)], overrides)
+
+    def test_source_overrides_constant_query_count(self):
+        # The query count is constant, no matter how many sources are
+        # checked.
+        spns = []
+        distroseries = self.factory.makeDistroSeries()
+        pocket = self.factory.getAnyPocket()
+        for i in xrange(10):
+            spph = self.factory.makeSourcePackagePublishingHistory(
+                distroseries=distroseries, archive=distroseries.main_archive,
+                pocket=pocket)
+            spns.append(spph.sourcepackagerelease.sourcepackagename)
+        flush_database_caches()
+        distroseries.main_archive
+        bulk.reload(spns)
+        policy = FromExistingOverridePolicy()
+        with StormStatementRecorder() as recorder:
+            policy.calculateSourceOverrides(
+                spph.distroseries.main_archive, spph.distroseries,
+                spph.pocket, spns)
+        self.assertThat(recorder, HasQueryCount(Equals(4)))
+
+    def test_no_binary_overrides(self):
+        # if the given binary is not published in the given distroarchseries,
+        # an empty list is returned.
+        distroseries = self.factory.makeDistroSeries()
+        das = self.factory.makeDistroArchSeries(distroseries=distroseries)
+        distroseries.nominatedarchindep = das
+        bpn = self.factory.makeBinaryPackageName()
+        pocket = self.factory.getAnyPocket()
+        policy = FromExistingOverridePolicy()
+        overrides = policy.calculateBinaryOverrides(
+            distroseries.main_archive, distroseries, pocket, ((bpn, None),))
+        self.assertEqual([], overrides)
+
+    def test_binary_overrides(self):
+        # When a binary is published in the given distroarchseries, the
+        # overrides are returned.
+        bpph = self.factory.makeBinaryPackagePublishingHistory()
+        distroseries = bpph.distroarchseries.distroseries
+        distroseries.nominatedarchindep = bpph.distroarchseries
+        policy = FromExistingOverridePolicy()
+        overrides = policy.calculateBinaryOverrides(
+            distroseries.main_archive, distroseries, bpph.pocket,
+            ((bpph.binarypackagerelease.binarypackagename, None),))
+        expected = [(
+            bpph.binarypackagerelease.binarypackagename,
+            bpph.distroarchseries, bpph.component, bpph.section,
+            bpph.priority)]
+        self.assertEqual(expected, overrides)
+
+    def test_binary_overrides_constant_query_count(self):
+        # The query count is constant, no matter how many bpn-das pairs are
+        # checked.
+        bpns = []
+        distroarchseries = self.factory.makeDistroArchSeries()
+        distroseries = distroarchseries.distroseries
+        distroseries.nominatedarchindep = distroarchseries
+        pocket = self.factory.getAnyPocket()
+        for i in xrange(10):
+            bpph = self.factory.makeBinaryPackagePublishingHistory(
+                distroarchseries=distroarchseries, 
+                archive=distroseries.main_archive, pocket=pocket)
+            bpns.append((bpph.binarypackagerelease.binarypackagename, None))
+        flush_database_caches()
+        distroseries.main_archive
+        bulk.reload(bpn[0] for bpn in bpns)
+        policy = FromExistingOverridePolicy()
+        with StormStatementRecorder() as recorder:
+            policy.calculateBinaryOverrides(
+                distroseries.main_archive, distroseries, pocket, bpns)
+        self.assertThat(recorder, HasQueryCount(Equals(4)))
+
+    def test_unknown_sources(self):
+        # If the unknown policy is used, it does no checks, just returns the
+        # defaults.
+        spph = self.factory.makeSourcePackagePublishingHistory()
+        policy = UnknownOverridePolicy()
+        overrides = policy.calculateSourceOverrides(
+            spph.distroseries.main_archive, spph.distroseries, spph.pocket,
+            (spph.sourcepackagerelease.sourcepackagename,))
+        expected = [(spph.sourcepackagerelease.sourcepackagename, 'universe',
+            None)]
+        self.assertEqual(expected, overrides)
+
+    def test_unknown_binaries(self):
+        # If the unknown policy is used, it does no checks, just returns the
+        # defaults.
+        bpph = self.factory.makeBinaryPackagePublishingHistory()
+        distroseries = bpph.distroarchseries.distroseries
+        distroseries.nominatedarchindep = bpph.distroarchseries
+        policy = UnknownOverridePolicy()
+        overrides = policy.calculateBinaryOverrides(
+            distroseries.main_archive, distroseries, bpph.pocket,
+            ((bpph.binarypackagerelease.binarypackagename, None),))
+        expected = [(bpph.binarypackagerelease.binarypackagename,
+            bpph.distroarchseries, 'universe', None, None)]
+        self.assertEqual(expected, overrides)
+
+    def test_ubuntu_override_policy_sources(self):
+        # The Ubuntu policy incorporates both the existing and the unknown
+        # policy.
+        spns = [self.factory.makeSourcePackageName()]
+        expected = [(spns[0], 'universe', None)]
+        distroseries = self.factory.makeDistroSeries()
+        pocket = self.factory.getAnyPocket()
+        for i in xrange(8):
+            spph = self.factory.makeSourcePackagePublishingHistory(
+                distroseries=distroseries, archive=distroseries.main_archive,
+                pocket=pocket)
+            spns.append(spph.sourcepackagerelease.sourcepackagename)
+            expected.append((
+                spph.sourcepackagerelease.sourcepackagename, spph.component,
+                spph.section))
+        spns.append(self.factory.makeSourcePackageName())
+        expected.append((spns[-1], 'universe', None))
+        policy = UbuntuOverridePolicy()
+        overrides = policy.calculateSourceOverrides(
+            distroseries.main_archive, distroseries, pocket, spns)
+        self.assertEqual(10, len(overrides))
+        self.assertContentEqual(expected, overrides)
+
+    def test_ubuntu_override_policy_binaries(self):
+        # The Ubuntu policy incorporates both the existing and the unknown
+        # policy.
+        distroseries = self.factory.makeDistroSeries()
+        pocket = self.factory.getAnyPocket()
+        bpn = self.factory.makeBinaryPackageName()
+        bpns = []
+        expected = []
+        for i in xrange(3):
+            distroarchseries = self.factory.makeDistroArchSeries(
+                distroseries=distroseries)
+            bpr = self.factory.makeBinaryPackageRelease(
+                binarypackagename=bpn)
+            bpph = self.factory.makeBinaryPackagePublishingHistory(
+                binarypackagerelease=bpr, distroarchseries=distroarchseries,
+                archive=distroseries.main_archive, pocket=pocket)
+            bpns.append((bpn, distroarchseries.architecturetag))
+            expected.append((
+                bpn, distroarchseries, bpph.component, bpph.section,
+                bpph.priority))
+        for i in xrange(2):
+            distroarchseries = self.factory.makeDistroArchSeries(
+                distroseries=distroseries)
+            bpns.append((bpn, distroarchseries.architecturetag))
+            expected.append((bpn, distroarchseries, 'universe', None, None))
+        distroseries.nominatedarchindep = distroarchseries
+        policy = UbuntuOverridePolicy()
+        overrides = policy.calculateBinaryOverrides(
+            distroseries.main_archive, distroseries, pocket, bpns)
+        self.assertEqual(5, len(overrides))
+        self.assertContentEqual(expected, overrides)

=== modified file 'lib/lp/soyuz/adapters/tests/test_packagelocation.py'
--- lib/lp/soyuz/adapters/tests/test_packagelocation.py	2010-10-04 19:50:45 +0000
+++ lib/lp/soyuz/adapters/tests/test_packagelocation.py	2011-05-13 08:32:29 +0000
@@ -3,8 +3,6 @@
 
 """Test PackageLocation class."""
 
-import unittest
-
 from zope.component import getUtility
 
 from canonical.testing.layers import LaunchpadZopelessLayer
@@ -260,7 +258,3 @@
             str(location_ubuntu_packageset),
             'Primary Archive for Ubuntu Linux: '
             'hoary-RELEASE [foo-packageset]')
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)