← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~jtv/launchpad/branchrevision into lp:launchpad

 

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/branchrevision into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code


= Stormify BranchRevision =

This branch is part of an effort to eliminate the id field from BranchRevision.  The table is too large and ids are not far from running out.  (The alternative is to expand the field and make the table even larger).  The branch is against db-devel, since this is part of work that will affect the schema.  We probably should consider landing it against devel.  But all that happens here is to stormify the class and the queries that mention it.

To test, I ran all unit and doc tests for Code (including browser & model unit tests).

There is a bit of pre-existing lint involving tuples like (foo,) not having a space after the comma.  I think the existing spelling is appropriate and I'll discuss possible changes to the checker with Curtis.


Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/branchrevision/+merge/29791
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/branchrevision into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/branchrevision.py'
--- lib/lp/code/interfaces/branchrevision.py	2009-06-25 04:06:00 +0000
+++ lib/lp/code/interfaces/branchrevision.py	2010-07-13 13:19:47 +0000
@@ -21,7 +21,7 @@
     ancestry of a branch. History revisions have an integer sequence, merged
     revisions have sequence set to None.
     """
-
+    # XXX JeroenVermeulen 2010-07-12: We're dropping this column.
     id = Int(title=_('The database revision ID'))
 
     sequence = Int(

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-07-09 10:22:32 +0000
+++ lib/lp/code/model/branch.py	2010-07-13 13:19:47 +0000
@@ -226,11 +226,12 @@
 
     @property
     def revision_history(self):
-        return BranchRevision.select('''
-            BranchRevision.branch = %s AND
-            BranchRevision.sequence IS NOT NULL
-            ''' % sqlvalues(self),
-            prejoins=['revision'], orderBy='-sequence')
+        # XXX JeroenVermeulen 2010-07-12: Prejoin revision.
+        result = Store.of(self).find(
+            BranchRevision,
+            BranchRevision.branch_id == self.id,
+            BranchRevision.sequence != None)
+        return result.order_by(Desc(BranchRevision.sequence))
 
     subscriptions = SQLMultipleJoin(
         'BranchSubscription', joinColumn='branch', orderBy='id')
@@ -509,7 +510,7 @@
 
     def latest_revisions(self, quantity=10):
         """See `IBranch`."""
-        return self.revision_history.limit(quantity)
+        return self.revision_history.config(limit=quantity)
 
     def getMainlineBranchRevisions(self, start_date, end_date=None,
                                    oldest_first=False):
@@ -532,14 +533,13 @@
 
     def getRevisionsSince(self, timestamp):
         """See `IBranch`."""
-        return BranchRevision.select(
-            'Revision.id=BranchRevision.revision AND '
-            'BranchRevision.branch = %d AND '
-            'BranchRevision.sequence IS NOT NULL AND '
-            'Revision.revision_date > %s' %
-            (self.id, quote(timestamp)),
-            orderBy='-sequence',
-            clauseTables=['Revision'])
+        result = Store.of(self).using(Revision).find(
+            BranchRevision,
+            Revision == BranchRevision.revision,
+            BranchRevision.branch == self,
+            BranchRevision.sequence != None,
+            Revision.revision_date > timestamp)
+        return result.order_by(Desc(BranchRevision.sequence))
 
     def canBeDeleted(self):
         """See `IBranch`."""
@@ -860,8 +860,8 @@
 
     def getScannerData(self):
         """See `IBranch`."""
-        cur = cursor()
-        cur.execute("""
+# XXX JeroenVermeulen 2010-07-12: We're dropping BranchRevision.id.
+        rows = Store.of(self).execute("""
             SELECT BranchRevision.id, BranchRevision.sequence,
                 Revision.revision_id
             FROM Revision, BranchRevision
@@ -872,10 +872,12 @@
         ancestry = set()
         history = []
         branch_revision_map = {}
-        for branch_revision_id, sequence, revision_id in cur.fetchall():
+        for branch_revision_id, sequence, revision_id in rows:
             ancestry.add(revision_id)
             branch_revision_map[revision_id] = branch_revision_id
             if sequence is not None:
+                assert sequence == len(history) + 1, (
+                    "Broken revision sequence number.")
                 history.append(revision_id)
         return ancestry, history, branch_revision_map
 
@@ -1001,7 +1003,7 @@
             name = "date_trunc"
         results = Store.of(self).find(
             (DateTrunc('day', Revision.revision_date), Count(Revision.id)),
-            Revision.id == BranchRevision.revisionID,
+            Revision.id == BranchRevision.revision_id,
             Revision.revision_date > since,
             BranchRevision.branch == self)
         results = results.group_by(

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2010-05-07 04:53:47 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2010-07-13 13:19:47 +0000
@@ -486,8 +486,10 @@
         self.queue_position = None
 
         if merged_revno is not None:
-            branch_revision = BranchRevision.selectOneBy(
-                branch=self.target_branch, sequence=merged_revno)
+            branch_revision = Store.of(self).find(
+                BranchRevision,
+                BranchRevision.branch == self.target_branch,
+                BranchRevision.sequence == merged_revno).get_one()
             if branch_revision is not None:
                 date_merged = branch_revision.revision.revision_date
 

=== modified file 'lib/lp/code/model/branchrevision.py'
--- lib/lp/code/model/branchrevision.py	2009-06-25 04:06:00 +0000
+++ lib/lp/code/model/branchrevision.py	2010-07-13 13:19:47 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212
@@ -8,30 +8,43 @@
 
 from zope.interface import implements
 
-from sqlobject import ForeignKey, IntCol
-
-from canonical.database.sqlbase import SQLBase
-from lp.code.interfaces.branchrevision import IBranchRevision, IBranchRevisionSet
-class BranchRevision(SQLBase):
-    """See IBranchRevision."""
+from storm.locals import (Int, Reference, Storm)
+
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
+
+from lp.code.interfaces.branchrevision import (
+    IBranchRevision, IBranchRevisionSet)
+
+
+class BranchRevision(Storm):
+    """See `IBranchRevision`."""
+    __storm_table__ = 'BranchRevision'
+
+    id = Int(primary=True)
 
     implements(IBranchRevision)
 
-    _table = 'BranchRevision'
-
-    branch = ForeignKey(
-        dbName='branch', foreignKey='Branch', notNull=True)
-
-    sequence = IntCol()
-    revision = ForeignKey(
-        dbName='revision', foreignKey='Revision', notNull=True)
+    branch_id = Int(name='branch', allow_none=False)
+    branch = Reference(branch_id, 'Branch.id')
+
+    revision_id = Int(name='revision', allow_none=False)
+    revision = Reference(revision_id, 'Revision.id')
+
+    sequence = Int(name='sequence', allow_none=True)
+
+    def __init__(self, branch, revision, sequence=None):
+        self.branch = branch
+        self.revision = revision
+        self.sequence = sequence
 
 
 class BranchRevisionSet:
-    """See IBranchRevisionSet."""
+    """See `IBranchRevisionSet`."""
 
     implements(IBranchRevisionSet)
 
     def delete(self, branch_revision_id):
         """See `IBranchRevisionSet`."""
-        BranchRevision.delete(branch_revision_id)
+        match = IMasterStore(BranchRevision).find(
+            BranchRevision, BranchRevision.id == branch_revision_id)
+        match.remove()

=== modified file 'lib/lp/code/model/revision.py'
--- lib/lp/code/model/revision.py	2010-04-16 15:06:55 +0000
+++ lib/lp/code/model/revision.py	2010-07-13 13:19:47 +0000
@@ -111,8 +111,8 @@
         store = Store.of(self)
 
         query = And(
-            self.id == BranchRevision.revisionID,
-            BranchRevision.branchID == Branch.id)
+            self.id == BranchRevision.revision_id,
+            BranchRevision.branch_id == Branch.id)
         if not allow_private:
             query = And(query, Not(Branch.private))
         if not allow_junk:
@@ -123,7 +123,7 @@
                 Or(
                     (Branch.product != None),
                     And(
-                        Branch.sourcepackagename != None, 
+                        Branch.sourcepackagename != None,
                         Branch.distroseries != None)))
         result_set = store.find(Branch, query)
         if self.revision_author.person is None:
@@ -374,7 +374,7 @@
             BranchRevision.branch == Branch.id,
             Branch.product == product,
             Branch.lifecycle_status.is_in(DEFAULT_BRANCH_STATUS_IN_LISTING),
-            BranchRevision.revisionID >= revision_subselect)
+            BranchRevision.revision_id >= revision_subselect)
         result_set.config(distinct=True)
         return result_set.order_by(Desc(Revision.revision_date))
 

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2010-06-11 03:52:02 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2010-07-13 13:19:47 +0000
@@ -26,6 +26,7 @@
 
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.testing import verifyObject
 from lp.translations.interfaces.translations import (
@@ -454,7 +455,9 @@
             except bzr_errors.NoSuchRevision:
                 revno = None
             if existing is not None:
-                BranchRevision.delete(existing.id)
+                branchrevision = IMasterStore(branch).find(
+                    BranchRevision, BranchRevision.id == existing.id)
+                branchrevision.remove()
             branch.createBranchRevision(revno, revision)
 
     def create3CommitsBranch(self):

=== modified file 'lib/lp/codehosting/scanner/tests/test_bzrsync.py'
--- lib/lp/codehosting/scanner/tests/test_bzrsync.py	2010-06-16 19:47:27 +0000
+++ lib/lp/codehosting/scanner/tests/test_bzrsync.py	2010-07-13 13:19:47 +0000
@@ -1,6 +1,6 @@
 #!/usr/bin/python
 #
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=W0141
@@ -21,6 +21,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
+from canonical.launchpad.interfaces.lpstorm import IStore
 from lp.translations.interfaces.translations import (
     TranslationsBranchImportMode)
 from lp.code.interfaces.branchjob import IRosettaUploadJobSource
@@ -94,10 +95,12 @@
         :return: (num_revisions, num_branch_revisions, num_revision_parents,
             num_revision_authors)
         """
-        return (Revision.select().count(),
-                BranchRevision.select().count(),
-                RevisionParent.select().count(),
-                RevisionAuthor.select().count())
+        store = IStore(Revision)
+        return (
+            store.find(Revision).count(),
+            store.find(BranchRevision).count(),
+            store.find(RevisionParent).count(),
+            store.find(RevisionAuthor).count())
 
     def assertCounts(self, counts, new_revisions=0, new_numbers=0,
                      new_parents=0, new_authors=0):
@@ -228,10 +231,11 @@
         :return: A set of tuples (sequence, revision-id) for all the
             BranchRevisions rows belonging to self.db_branch.
         """
+        branchrevisions = IStore(BranchRevision).find(
+            BranchRevision, BranchRevision.branch == db_branch)
         return set(
             (branch_revision.sequence, branch_revision.revision.revision_id)
-            for branch_revision
-            in BranchRevision.selectBy(branch=db_branch))
+            for branch_revision in branchrevisions)
 
     def writeToFile(self, filename="file", contents=None):
         """Set the contents of the specified file.
@@ -459,8 +463,9 @@
         # retrieveDatabaseAncestry.
         branch = getUtility(IBranchLookup).getByUniqueName(
             '~name12/+junk/junk.contrib')
-        sampledata = list(
-            BranchRevision.selectBy(branch=branch).orderBy('sequence'))
+        branchrevisions = IStore(BranchRevision).find(
+            BranchRevision, BranchRevision.branch == branch)
+        sampledata = list(branchrevisions.order_by(BranchRevision.sequence))
         expected_ancestry = set(branch_revision.revision.revision_id
             for branch_revision in sampledata)
         expected_history = [branch_revision.revision.revision_id