Merge lp:~stevenk/launchpad/db-spph-ancestry into lp:launchpad/db-devel

[Steve Kowalik]

[r=jml,jtv,stub][ui=none][bug=684101] Nail an ancestor attribute onto SourcePackagePublishingHistory.

Add an ancestor attribute onto SourcePackagePublishingHistory. This means we can explicitly set what the previous release of a publishing record is, which lays a foundation for traversing the database to find common versions for the derivation work, and as a win, allows us to massively refactor the domination routine to be a shadow of its former self.

At the moment, nothing makes use of this field, but I wanted to get it into this months rollout.

[Jeroen T. Vermeulen (jtv)]

Nicely put!

The code looks good to me. Just a few points:

Looks like it was impossible to set the right schema for ancestor in lib/lp/soyuz/interfaces/publishing.py (diff line 49), so you had to set it to mere Interface instead. When you do that, fix things up in lib/canonical/launchpad/interfaces/_schema_circular_imports.py.

Also in lib/lp/soyuz/interfaces/publishing.py, diff line 61/62, the new parameter to newSourcePublication defaults to None but the method definition in the interface does not reflect that.

In lib/lp/soyuz/tests/test_publishing.py, I don't suppose TestSPPHModel could get away with a lighter test layer such as ZopelessDatabaseLayer? Probably not, but always nice to save a dozen seconds or so on test setup when you can.

Jeroen

review: Approve (code)

[Jonathan Lange (jml)]

Two questions:
  * "parent" is a more common term for "immediate ancestor". Why not use that?
  * In previous discussions, I'd heard mention of a DAG model for ancestry, rather than a simple tree model. Why did you decide to go with the tree?

Thanks,
jml

review: Needs Information

[Steve Kowalik (stevenk)]

The original implementation for this idea was going to be a new table called 'SourcePackageAncestry', which would have links to the distroseries, archive and pocket, and the spr, allowing us to traverse the list to find the base versions. While having this discussion with Julian and William, Julian had the idea that SourcePackagePublishingHistory has all this information, and we could add an ancestor column to SPPH to serve the same purpose. So, 'hysterical raisins', I guess.

The purpose of this field is not model the ways that packages relate to another one in a definitive way, its original intent was to allow us to traverse back up the tree to see the highest common version for the derivation work.

[Jonathan Lange (jml)]

OK. I strongly recommend renaming to 'parent' then, in order to be clearer.

I don't really get your second answer. You are using a tree rather than a DAG because you want to traverse back up the tree to see the highest common version. Why do you want to do that? It also doesn't answer the question about what happened to the discussion about using the DAG model.

[Steve Kowalik (stevenk)]

Because using a DAG for this purpose is, I believe, over-engineering the problem. We aren't using this to keep track of how publishing records relate to one another (for example, tracking copies between pockets in the Ubuntu archive, or between PPAs) -- for that we certainly would require a DAG. We are using a tree to model versioning information between publishing records.

For example:

foo (1.4-1ubuntu1) [lucid] --- foo (1.4-1ubuntu1) [karmic] --- foo (1.3-2) [jaunty] --- foo (1.2-1ubuntu1) [intrepid]

In this case, both of the parents for the publishing records for foo in both lucid and karmic would point to the jaunty version, and the parent of the jaunty version would be the version in intrepid.

As for your question for why we want to do this, I shall extend my example with this tree from the versions of foo in Debian:

foo (1.4-1) [unstable] --- foo (1.4-1) [testing] --- foo (1.3-2) [stable]

One of the issues we need to solve for derived distributions is to find the base version -- that is, the point where the packages diverged. Using the two examples together, we can see that the version of package in jaunty and stable are equal, and use that to draw out the changes made in Debian, and the changes made in Ubuntu and the changes made upstream, and are able to present these to the user.

[Steve Kowalik (stevenk)]

And a further comment, ancestor and ancestry is already in widespread use throughout the Soyuz codebase, so I'm keeping that up.

[Stuart Bishop (stub)]

I suspect we will need an index too or we will be sorry. It won't be used for the primary use case, but deleting spph records or traversing to child records will be painful without it.

CREATE INDEX sourcepackagepublishinghistory__ancestor__idx ON sourcepackagepublishinghistory(ancestor);

patch-2208-34-0.sql

review: Approve (db)

[Julian Edwards (julian-edwards)]

> OK. I strongly recommend renaming to 'parent' then, in order to be clearer.
>
> I don't really get your second answer. You are using a tree rather than a DAG
> because you want to traverse back up the tree to see the highest common
> version. Why do you want to do that? It also doesn't answer the question
> about what happened to the discussion about using the DAG model.

Source ancestry is a strong meme in the Soyuz code. I'd much rather the name was consistent with that than introducing a new name for the same concept.

[Jonathan Lange (jml)]

Thanks for the reply Steven.

I agree it's better to keep consistency with the existing bad name than introduce a new concept.

review: Approve

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2010-12-02 06:23:50 +0000
+++ database/schema/comments.sql	2010-12-02 06:53:07 +0000
@@ -1788,7 +1788,8 @@
 COMMENT ON COLUMN SourcePackagePublishingHistory.pocket IS 'The pocket into which this record is published. The RELEASE pocket (zero) provides behaviour as normal. Other pockets may append things to the distroseries name such as the UPDATES pocket (-updates), the SECURITY pocket (-security) and the PROPOSED pocket (-proposed)';
 COMMENT ON COLUMN SourcePackagePublishingHistory.removed_by IS 'Person responsible for the removal.';
 COMMENT ON COLUMN SourcePackagePublishingHistory.removal_comment IS 'Reason why the publication was removed.';
-COMMENT ON COLUMN SourcePackagePublishingHistory.archive IS 'The target archive for thi publishing record.';
+COMMENT ON COLUMN SourcePackagePublishingHistory.archive IS 'The target archive for this publishing record.';
+COMMENT ON COLUMN SourcePackagePublishingHistory.ancestor IS 'The source package record published immediately before this one.';
 
 -- Packaging
 COMMENT ON TABLE Packaging IS 'DO NOT JOIN THROUGH THIS TABLE. This is a set

=== added file 'database/schema/patch-2208-34-0.sql'
--- database/schema/patch-2208-34-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-34-0.sql	2010-12-02 06:53:07 +0000
@@ -0,0 +1,8 @@
+SET client_min_messages=ERROR;
+
+ALTER TABLE SourcePackagePublishingHistory
+    ADD COLUMN ancestor INTEGER REFERENCES SourcePackagePublishingHistory;
+CREATE INDEX sourcepackagepublishinghistory__ancestor__idx ON sourcepackagepublishinghistory(ancestor);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 34, 0);
+

=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-12-01 19:12:00 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-12-02 06:53:07 +0000
@@ -277,6 +277,9 @@
     IBinaryPackagePublishingHistory, 'archive', IArchive)
 patch_reference_property(
     ISourcePackagePublishingHistory, 'archive', IArchive)
+patch_reference_property(
+    ISourcePackagePublishingHistory, 'ancestor',
+    ISourcePackagePublishingHistory)
 
 # IArchive apocalypse.
 patch_reference_property(IArchive, 'distribution', IDistribution)

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2010-11-10 04:25:52 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2010-12-02 06:53:07 +0000
@@ -458,6 +458,12 @@
         "package that has been published in the main distribution series, "
         "if one exists, or None.")
 
+    ancestor = Reference(
+        Interface, # Really ISourcePackagePublishingHistory
+        title=_('Ancestor'),
+        description=_('The previous release of this source package.'),
+        required=False, readonly=True)
+
     # Really IBinaryPackagePublishingHistory, see below.
     @operation_returns_collection_of(Interface)
     @export_read_operation()
@@ -874,7 +880,7 @@
         """
 
     def newSourcePublication(archive, sourcepackagerelease, distroseries,
-                             component, section, pocket):
+                             component, section, pocket, ancestor):
         """Create a new `SourcePackagePublishingHistory`.
 
         :param archive: An `IArchive`
@@ -883,6 +889,8 @@
         :param component: An `IComponent`
         :param section: An `ISection`
         :param pocket: A `PackagePublishingPocket`
+        :param ancestor: A `ISourcePackagePublishingHistory` for the previous
+            version of this publishing record
 
         datecreated will be UTC_NOW.
         status will be PackagePublishingStatus.PENDING

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-12-01 11:26:57 +0000
+++ lib/lp/soyuz/model/publishing.py	2010-12-02 06:53:07 +0000
@@ -432,6 +432,9 @@
         dbName="removed_by", foreignKey="Person",
         storm_validator=validate_public_person, default=None)
     removal_comment = StringCol(dbName="removal_comment", default=None)
+    ancestor = ForeignKey(
+        dbName="ancestor", foreignKey="SourcePackagePublishingHistory",
+        default=None)
 
     @property
     def package_creator(self):
@@ -1309,7 +1312,8 @@
         return pub
 
     def newSourcePublication(self, archive, sourcepackagerelease,
-                             distroseries, component, section, pocket):
+                             distroseries, component, section, pocket,
+                             ancestor=None):
         """See `IPublishingSet`."""
         if archive.is_ppa:
             # PPA component must always be 'main', so we override it
@@ -1323,7 +1327,8 @@
             component=component,
             section=section,
             status=PackagePublishingStatus.PENDING,
-            datecreated=UTC_NOW)
+            datecreated=UTC_NOW,
+            ancestor=ancestor)
         # Import here to prevent import loop.
         from lp.registry.model.distributionsourcepackage import (
             DistributionSourcePackage)

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2010-10-06 11:46:51 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2010-12-02 06:53:07 +0000
@@ -23,8 +23,8 @@
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadZopelessLayer,
+    reconnect_stores,
     )
-from canonical.testing.layers import reconnect_stores
 from lp.app.errors import NotFoundError
 from lp.archivepublisher.config import Config
 from lp.archivepublisher.diskpool import DiskPool
@@ -57,7 +57,10 @@
     BinaryPackagePublishingHistory,
     SourcePackagePublishingHistory,
     )
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    login_as,
+    TestCaseWithFactory,
+    )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.fakemethod import FakeMethod
 
@@ -1334,3 +1337,15 @@
             series, bins[0].pocket, bins[0].archive)
         self.checkOtherPublications(bins[0], bins)
         self.checkOtherPublications(foreign_bins[0], foreign_bins)
+
+class TestSPPHModel(TestCaseWithFactory):
+    """Test parts of the SourcePackagePublishingHistory model."""
+
+    layer = LaunchpadZopelessLayer
+
+    def testAncestry(self):
+        """Ancestry can be traversed."""
+        ancestor = self.factory.makeSourcePackagePublishingHistory()
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            ancestor=ancestor)
+        self.assertEquals(spph.ancestor.displayname, ancestor.displayname)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-12-01 11:26:57 +0000
+++ lib/lp/testing/factory.py	2010-12-02 06:53:07 +0000
@@ -2836,6 +2836,7 @@
                                            dsc_format='1.0',
                                            dsc_binaries='foo-bin',
                                            sourcepackagerelease=None,
+                                           ancestor=None,
                                            ):
         """Make a `SourcePackagePublishingHistory`."""
         if distroseries is None:
@@ -2879,7 +2880,7 @@
         spph = getUtility(IPublishingSet).newSourcePublication(
             archive, sourcepackagerelease, distroseries,
             sourcepackagerelease.component, sourcepackagerelease.section,
-            pocket)
+            pocket, ancestor)
 
         naked_spph = removeSecurityProxy(spph)
         naked_spph.status = status