Merge lp:~stevenk/launchpad/db-add-bprc into lp:launchpad/db-devel

[Steve Kowalik]

[r=stub,wgrant][bug=796997][incr] Add infrastructure to eventually support contents generation from the database.

(Updated to facilitate code review.)

Add a two new tables, BinaryPackagePath and BinaryPackageReleaseContents, and associated interfaces and models.

Background:

Currently, Contents generation (for example, the file at http://archive.ubuntu.com/ubuntu/dists/natty/Contents-amd64.gz) is generated every day, where it stops the publisher from running, and takes an hour or slightly more.

These two tables completes the first step of moving contents generation into the database, and lets it do the heavy lifting, rather than cocoplum's disk. This also gives us the opportunity to perform Contents generation for PPAs.

The next step of the plan is to have a garbo script that slowly populates these tables (but not yet, since they will be large).

The end goal is to move Contents generation into the publisher itself, where it will query the table by a list of BPRs and get back every single filename that the BPRs contain.

IBinaryPackagePath{,Source}:

The idea behind these two interfaces and database table is so that a filename that is in the contents of a Binary Package Release (such as, 'bin/true'), is only mentioned once.

IBinaryPackageReleaseContents{,Set}:

These two interfaces and database table are to link a given Binary Package Release (BPR) to its contents, where the filenames are joined from BinaryPackagePath. Note that a given BPR will have multiple rows in BPRC, one for each file, but each BPR can only link to a given file once.

Tests: All new, all shiny.

[Stuart Bishop (stub)]

BinaryPackagePath.path should be UNIQUE NOT NULL.

Shouldn't this table be called BinaryPackageReleasePath to match BinaryPackageReleaseContents?

Both columns in BinaryPackageReleaseContents should be NOT NULL.

patch-2208-76-0.sql

You likely will want this construct to ensure a path exists in the BinaryPackageReleasePaths table. This will avoid race conditions in scripts (not running in serializable isolation):

insert into binarypackagepaths (path) select 'foo' where not exists (select * from binarypackagepaths where path='foo');

review: Approve (db)

[Gavin Panella (allenap)]

review: Abstain

[William Grant (wgrant)]

82 + <class
83 + class="lp.soyuz.model.binarypackagepath.BinaryPackagePath">
84 + <allow
85 + interface="lp.soyuz.interfaces.binarypackagepath.IBinaryPackagePathSource"/>
86 + </class>

No need for this. You already grant it to the securedutility.

104 + <class
105 + class="lp.soyuz.model.binarypackagereleasecontents.BinaryPackageReleaseContents">
106 + <allow
107 + interface="lp.soyuz.interfaces.binarypackagereleasecontents.IBinaryPackageReleaseContentsSet"/>
108 + </class>

Same here.

You also have one Source and one Set. Please make them the same.

+ path = TextLine(title=_('Full path name'), required=True, readonly=True)

s/Full path name/Absolute path/, perhaps?

158 + def getOrCreate(path):
159 + """Fetch the ID of the given path name, or create it.
160 +
161 + :param: path: The full path name to query or create.
162 +
163 + :return: An ID.
164 + """

Why an ID? Why not the object itself?

191 + """The contents of a binary package release.
192 +
193 + A binary package release is a representation of a binary package in
194 + one or more releases. This class models the files contained within
195 + the binary package.
196 + """

In one or more releases? I think replacing all this with a one-liner would be good. "A file contained by an `IBinaryPackageRelease`." or so.

250 + def getOrCreate(self, path):
251 + """See `IBinaryPackagePathSource`."""
252 + store = IMasterStore(BinaryPackagePath)
253 + bpp = store.find(BinaryPackagePath, BinaryPackagePath.path == path)
254 + if bpp.count():
255 + return bpp[0]
256 + else:
257 + bpp = BinaryPackagePath()
258 + bpp.path = path
259 + return store.add(bpp)

Try something like this:

    bpp = store.find(BinaryPackagePath, BinaryPackagePath.path == path).one()
    if bpp is None:
        bpp = BinaryPackagePath(path)
        store.add(bpp)
    return bpp

314 + with TempDir() as tmp_dir:

Can you use a TemporaryFile instead? Or even a NamedTemporaryFile, if that doesn't work. Or does DebPackage really want a filename, and care about what it is? Also, might as well break out of the with block as soon as you're done with DebPackage.

324 + bpp = getUtility(IBinaryPackagePathSource).getOrCreate(
325 + unicode(filename))

Objection! unicode() on potentially non-ASCII data == wrong. Probably best to treat it as UTF-8, probably skipping if it's undecodable.

Also, this is going to be pretty slow. You may need to think about optimising it down to a single query eventually.

334 + results = store.find(
335 + BinaryPackageReleaseContents,
336 + BinaryPackageReleaseContents.binarypackagerelease == bpr.id)
337 + for bprc in results:
338 + store.remove(bprc)

You can't just call remove() on the resultset?

362 + def test_get_or_create(self):

Method name style should be preserved here. So, test_getOrCreate.

405 + deb = open(datadir('pmount_0.9.7-2ubuntu2_amd64.deb'), 'r')
406 + lfa = self.factory.makeLibraryFileAlias(...

review: Needs Fixing (code)

[William Grant (wgrant)]

IBPP's docstring is a lie (seems copied from IBPRC).

Also, try using NamedTemporaryFile as a context manager. Bit clearer and shorter.

review: Approve (code)

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2011-06-10 12:53:23 +0000
+++ database/schema/comments.sql	2011-06-21 01:46:48 +0000
@@ -1625,6 +1625,11 @@
 COMMENT ON COLUMN BinaryPackageRelease.user_defined_fields IS 'A JSON struct containing a sequence of key-value pairs with user defined fields in the control file.';
 COMMENT ON COLUMN BinaryPackageRelease.homepage IS 'Upstream project homepage URL, not checked for validity.';
 
+-- BinaryPackageReleaseContents
+
+COMMENT ON TABLE BinaryPackageReleaseContents IS 'BinaryPackageReleaseContents: Mapping table that maps from BinaryPackageReleases to path names.';
+COMMENT ON COLUMN BinaryPackageReleaseContents.binarypackagerelease IS 'The BinaryPackageRelease that contains the path name.';
+COMMENT ON COLUMN BinaryPackageReleaseContents.binarypackagepath IS 'The path name, via the BinaryPackagePath table.';
 
 -- BinaryPackageFile
 
@@ -1637,6 +1642,11 @@
 
 COMMENT ON TABLE BinaryPackageName IS 'BinaryPackageName: A soyuz binary package name.';
 
+-- BinaryPackagePath
+
+COMMENT ON TABLE BinaryPackagePath IS 'BinaryPackagePath: A table of filenames shipped in binary packages.';
+COMMENT ON COLUMN BinaryPackagePath.path IS 'The full path of the file.';
+
 -- Distribution
 
 COMMENT ON TABLE Distribution IS 'Distribution: A soyuz distribution. A distribution is a collection of DistroSeries. Distributions often group together policy and may be referred to by a name such as "Ubuntu" or "Debian"';

=== added file 'database/schema/patch-2208-76-0.sql'
--- database/schema/patch-2208-76-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-76-0.sql	2011-06-21 01:46:48 +0000
@@ -0,0 +1,17 @@
+-- Copyright 2011 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+SET client_min_messages=ERROR;
+
+CREATE TABLE BinaryPackagePath (
+    id serial PRIMARY KEY,
+    path bytea UNIQUE NOT NULL
+);
+
+CREATE TABLE BinaryPackageReleaseContents (
+    binarypackagerelease integer NOT NULL REFERENCES BinaryPackageRelease,
+    binarypackagepath integer NOT NULL REFERENCES BinaryPackagePath,
+    PRIMARY KEY (binarypackagerelease, binarypackagepath)
+);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 76, 0);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-06-17 17:32:37 +0000
+++ database/schema/security.cfg	2011-06-21 01:46:48 +0000
@@ -118,7 +118,9 @@
 public.archivesubscriber                = SELECT, INSERT, UPDATE
 public.authtoken                        = SELECT, INSERT, UPDATE, DELETE
 public.binaryandsourcepackagenameview   = SELECT
+public.binarypackagepath                = SELECT, INSERT, DELETE
 public.binarypackagepublishinghistory   = SELECT
+public.binarypackagereleasecontents     = SELECT, INSERT, DELETE
 public.binarypackagereleasedownloadcount= SELECT, INSERT, UPDATE
 public.bountysubscription               = SELECT, INSERT, UPDATE, DELETE
 public.branch                           = SELECT, INSERT, UPDATE, DELETE

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2011-06-15 02:41:34 +0000
+++ lib/lp/soyuz/configure.zcml	2011-06-21 01:46:48 +0000
@@ -982,6 +982,40 @@
         <allow interface="lp.soyuz.adapters.overrides.IOverridePolicy" />
    </class>
 
+    <!-- BinaryPackagePath -->
+
+    <class
+        class="lp.soyuz.model.binarypackagepath.BinaryPackagePath">
+        <allow
+            interface="lp.soyuz.interfaces.binarypackagepath.IBinaryPackagePath"/>
+    </class>
+
+    <!-- BinaryPackagePathSet -->
+
+    <securedutility
+        class="lp.soyuz.model.binarypackagepath.BinaryPackagePath"
+        provides="lp.soyuz.interfaces.binarypackagepath.IBinaryPackagePathSet">
+        <allow
+            interface="lp.soyuz.interfaces.binarypackagepath.IBinaryPackagePathSet"/>
+    </securedutility>
+
+    <!-- BinaryPackageReleaseContents -->
+
+    <class
+        class="lp.soyuz.model.binarypackagereleasecontents.BinaryPackageReleaseContents">
+        <allow
+            interface="lp.soyuz.interfaces.binarypackagereleasecontents.IBinaryPackageReleaseContents"/>
+    </class>
+
+    <!-- BinaryPackageReleaseContentsSet -->
+
+    <securedutility
+        class="lp.soyuz.model.binarypackagereleasecontents.BinaryPackageReleaseContents"
+        provides="lp.soyuz.interfaces.binarypackagereleasecontents.IBinaryPackageReleaseContentsSet">
+        <allow
+            interface="lp.soyuz.interfaces.binarypackagereleasecontents.IBinaryPackageReleaseContentsSet"/>
+    </securedutility>
+
    <webservice:register module="lp.soyuz.interfaces.webservice" />
 
 </configure>

=== added file 'lib/lp/soyuz/interfaces/binarypackagepath.py'
--- lib/lp/soyuz/interfaces/binarypackagepath.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/interfaces/binarypackagepath.py	2011-06-21 01:46:48 +0000
@@ -0,0 +1,36 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Binary Package Path interface."""
+
+__metaclass__ = type
+
+__all__ = [
+    'IBinaryPackagePath',
+    'IBinaryPackagePathSet',
+    ]
+
+from zope.interface import Interface
+from zope.schema import (
+    Int,
+    TextLine,
+    )
+
+from canonical.launchpad import _
+
+
+class IBinaryPackagePath(Interface):
+    """The path of a file contained in a binary package."""
+    id = Int(title=_('ID'), required=True, readonly=True)
+    path = TextLine(title=_('Full path name'), required=True, readonly=True)
+
+
+class IBinaryPackagePathSet(Interface):
+
+    def getOrCreate(path):
+        """Fetch the ID of the given path name, or create it.
+
+        :param: path: The full path name to query or create.
+
+        :return: A `IBinaryPackagePath`.
+        """

=== added file 'lib/lp/soyuz/interfaces/binarypackagereleasecontents.py'
--- lib/lp/soyuz/interfaces/binarypackagereleasecontents.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/interfaces/binarypackagereleasecontents.py	2011-06-21 01:46:48 +0000
@@ -0,0 +1,43 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Binary Package Release Contents interface."""
+
+__metaclass__ = type
+
+__all__ = [
+    'IBinaryPackageReleaseContents',
+    'IBinaryPackageReleaseContentsSet',
+    ]
+
+from lazr.restful.fields import Reference
+from zope.interface import Interface
+
+from canonical.launchpad import _
+from lp.soyuz.interfaces.binarypackagepath import IBinaryPackagePath
+from lp.soyuz.interfaces.binarypackagerelease import IBinaryPackageRelease
+
+
+class IBinaryPackageReleaseContents(Interface):
+    """A file contained by an `IBinaryPackageRelease`."""
+    binarypackagerelease = Reference(
+        IBinaryPackageRelease, title=_('Binary Package Release'),
+        required=True, readonly=True)
+    binarypackagepath = Reference(
+        IBinaryPackagePath, title=_('Binary Package Pathname'),
+        required=True, readonly=True)
+
+
+class IBinaryPackageReleaseContentsSet(Interface):
+
+    def add(bpr):
+        """Add the contents of the given binary package release.
+
+        :param: bpr: The `IBinaryPackageRelease` to add.
+        """
+
+    def remove(bpr):
+        """Remove the contents of the given binary package release.
+
+        :param: bpr: The `IBinaryPackageRelease` to remove.
+        """

=== added file 'lib/lp/soyuz/model/binarypackagepath.py'
--- lib/lp/soyuz/model/binarypackagepath.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/model/binarypackagepath.py	2011-06-21 01:46:48 +0000
@@ -0,0 +1,37 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+__all__ = [
+    'BinaryPackagePath',
+    ]
+
+from storm.locals import (
+    Int,
+    RawStr,
+    Storm,
+    )
+from zope.interface import implements
+
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from lp.soyuz.interfaces.binarypackagepath import IBinaryPackagePath
+
+
+class BinaryPackagePath(Storm):
+    """See `IBinaryPackagePath`."""
+    implements(IBinaryPackagePath)
+    __storm_table__ = 'BinaryPackagePath'
+    id = Int(primary=True)
+    path = RawStr(name='path', allow_none=False)
+
+    def getOrCreate(self, path):
+        """See `IBinaryPackagePathSet`."""
+        store = IMasterStore(BinaryPackagePath)
+        bpp = store.find(
+            BinaryPackagePath, BinaryPackagePath.path == path).one()
+        if bpp is None:
+            bpp = BinaryPackagePath()
+            bpp.path = path
+            store.add(bpp)
+        return bpp

=== added file 'lib/lp/soyuz/model/binarypackagereleasecontents.py'
--- lib/lp/soyuz/model/binarypackagereleasecontents.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/model/binarypackagereleasecontents.py	2011-06-21 01:46:48 +0000
@@ -0,0 +1,70 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+__all__ = [
+    'BinaryPackageReleaseContents',
+    ]
+
+import tempfile
+
+from apt.debfile import DebPackage
+from bzrlib.osutils import pumpfile
+from storm.locals import (
+    Int,
+    Reference,
+    Storm,
+    )
+from zope.component import getUtility
+from zope.interface import implements
+
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from lp.soyuz.interfaces.binarypackagepath import IBinaryPackagePathSet
+from lp.soyuz.interfaces.binarypackagereleasecontents import (
+    IBinaryPackageReleaseContents,
+    )
+
+
+class BinaryPackageReleaseContents(Storm):
+    """See `IBinaryPackageReleaseContents`."""
+    implements(IBinaryPackageReleaseContents)
+    __storm_table__ = 'BinaryPackageReleaseContents'
+    __storm_primary__ = ("binarypackagerelease_id", "binaypackagepath_id")
+
+    binarypackagerelease_id = Int(
+        name='binarypackagerelease', allow_none=False)
+    binarypackagerelease = Reference(
+        binarypackagerelease_id, 'BinaryPackageRelease.id')
+
+    binaypackagepath_id = Int(name='binarypackagepath', allow_none=False)
+    binarypackagepath = Reference(
+        binaypackagepath_id, 'BinaryPackagePath.id')
+
+    def add(self, bpr):
+        """See `IBinaryPackageReleaseContentsSet`."""
+        if not bpr.files:
+            return None
+        store = IMasterStore(BinaryPackageReleaseContents)
+        with tempfile.NamedTemporaryFile() as dest_file:
+            bpr.files[0].libraryfile.open()
+            pumpfile(bpr.files[0].libraryfile, dest_file)
+            dest_file.seek(0)
+            deb = DebPackage(filename=dest_file.name)
+        # Filter out directories.
+        filelist = filter(lambda x: not x.endswith('/'), deb.filelist)
+        for filename in filelist:
+            bpp = getUtility(IBinaryPackagePathSet).getOrCreate(
+                filename)
+            bprc = BinaryPackageReleaseContents()
+            bprc.binarypackagerelease = bpr
+            bprc.binarypackagepath = bpp
+            store.add(bprc)
+
+    def remove(self, bpr):
+        """See `IBinaryPackageReleaseContentsSet`."""
+        store = IMasterStore(BinaryPackageReleaseContents)
+        results = store.find(
+            BinaryPackageReleaseContents,
+            BinaryPackageReleaseContents.binarypackagerelease == bpr.id)
+        results.remove()

=== added file 'lib/lp/soyuz/tests/test_binarypackagepath.py'
--- lib/lp/soyuz/tests/test_binarypackagepath.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_binarypackagepath.py	2011-06-21 01:46:48 +0000
@@ -0,0 +1,26 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the Binary Package Path model."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.soyuz.interfaces.binarypackagepath import IBinaryPackagePathSet
+from lp.testing import TestCaseWithFactory
+
+
+class TestBinaryPackagePath(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_getOrCreate(self):
+        bpp = getUtility(IBinaryPackagePathSet).getOrCreate('bin/bash')
+        self.assertEqual('bin/bash', bpp.path)
+
+    def test_getOrCreate_existing(self):
+        orig_bpp = getUtility(IBinaryPackagePathSet).getOrCreate('bin/bash')
+        bpp = getUtility(IBinaryPackagePathSet).getOrCreate('bin/bash')
+        self.assertEqual(orig_bpp.id, bpp.id)

=== added file 'lib/lp/soyuz/tests/test_binarypackagereleasecontents.py'
--- lib/lp/soyuz/tests/test_binarypackagereleasecontents.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_binarypackagereleasecontents.py	2011-06-21 01:46:48 +0000
@@ -0,0 +1,66 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the Binary Package Release Contents model."""
+
+__metaclass__ = type
+
+import transaction
+
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.testing.layers import LaunchpadFunctionalLayer
+from lp.archiveuploader.tests import datadir
+from lp.soyuz.interfaces.binarypackagereleasecontents import (
+    IBinaryPackageReleaseContentsSet,
+    )
+from lp.soyuz.model.binarypackagereleasecontents import (
+    BinaryPackageReleaseContents,
+    )
+from lp.testing import TestCaseWithFactory
+
+
+class TestBinaryPackagePath(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def create_bpr(self):
+        bpr = self.factory.makeBinaryPackageRelease()
+        deb = open(datadir('pmount_0.9.7-2ubuntu2_amd64.deb'), 'r')
+        lfa = self.factory.makeLibraryFileAlias(
+            filename='pmount_0.9.7-2ubuntu2_amd64.deb', content=deb.read())
+        deb.close()
+        transaction.commit()
+        bpr.addFile(lfa)
+        return bpr
+
+    def test_add(self):
+        bpr = self.create_bpr()
+        getUtility(IBinaryPackageReleaseContentsSet).add(bpr)
+        store = IStore(BinaryPackageReleaseContents)
+        results = store.find(
+            BinaryPackageReleaseContents,
+            BinaryPackageReleaseContents.binarypackagerelease == bpr.id)
+        paths = map(lambda x: x.binarypackagepath.path, results)
+        expected_paths = [
+            'etc/pmount.allow', 'usr/bin/pumount', 'usr/bin/pmount-hal',
+            'usr/bin/pmount', 'usr/share/doc/pmount/TODO',
+            'usr/share/doc/pmount/README.Debian',
+            'usr/share/doc/pmount/AUTHORS', 'usr/share/doc/pmount/copyright',
+            'usr/share/doc/pmount/changelog.gz',
+            'usr/share/doc/pmount/changelog.Debian.gz',
+            'usr/share/man/man1/pmount-hal.1.gz',
+            'usr/share/man/man1/pmount.1.gz',
+            'usr/share/man/man1/pumount.1.gz']
+        self.assertContentEqual(expected_paths, paths)
+
+    def test_remove(self):
+        bpr = self.create_bpr()
+        getUtility(IBinaryPackageReleaseContentsSet).add(bpr)
+        getUtility(IBinaryPackageReleaseContentsSet).remove(bpr)
+        store = IStore(BinaryPackageReleaseContents)
+        results = store.find(
+            BinaryPackageReleaseContents,
+            BinaryPackageReleaseContents.binarypackagerelease == bpr.id)
+        self.assertEqual(0, results.count())