Merge lp:~jtv/launchpad/branchrevision into lp:launchpad/db-devel

[Jeroen T. Vermeulen]

Stormify BranchRevision.

= 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

[Edwin Grubbs (edwin-grubbs)]

Hi Jeroen,

This is a nice improvement. Comments below.

-Edwin

>=== 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:32:27 +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.

I assume you are planning on adding a bug for this XXX.

> 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:32:27 +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))

You can prejoin the revision like this. The Revision will be added
to the cache, and since BranchRevision has the foreign key, it will
retrieve the Revision from the cache by id, even though the list
comprehension appears to throw it away.

> result = Store.of(self).find(
> (BranchRevision, Revision),
> BranchRevision.revisionID == Revision.id,
> BranchRevision.branch_id == self.id,
> BranchRevision.sequence != None)
> result = result.order_by(Desc(BranchRevision.sequence))
> return [branch_revision for branch_revision, branch in result]
>
>
> 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)

Of course, if you use the prejoin above, you will need access to
the storm query before stripping out the Revision.

> 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,
>...

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Thanks for a particularly thoughtful and helpful review.

> >=== 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:32:27 +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.
>
>
> I assume you are planning on adding a bug for this XXX.

This was really just a transient development note; I've dropped it so that we don't leave unnecessary cruft lying around.

> >=== 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:32:27 +0000

> >+ result = Store.of(self).find(
> >+ BranchRevision,
> >+ BranchRevision.branch_id == self.id,
> >+ BranchRevision.sequence != None)
> >+ return result.order_by(Desc(BranchRevision.sequence))
>
>
> You can prejoin the revision like this. The Revision will be added
> to the cache, and since BranchRevision has the foreign key, it will
> retrieve the Revision from the cache by id, even though the list
> comprehension appears to throw it away.

I used Stuart's prejoin helper from lp.services.database. I cleaned that up a bit as well, and corrected the documentation.

> >@@ -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)
>
>
> Of course, if you use the prejoin above, you will need access to
> the storm query before stripping out the Revision.

Thankfully the prejoin helper takes care of that!

> >@@ -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))
>
> A prejoin would be good here, too.

Done.

> >@@ -860,8 +860,8 @@
> >
> > def getScannerData(self):
> > """See `IBranch`."""
> >- cur = cursor()
> >- cur.execute("""
> >+# XXX JeroenVermeulen 2010-07-12: We're dropping BranchRevision.id.
>
>
> This XXX needs that bug number and some indentation.

Removed as above.

> >+ rows = Store.of(self).execute("""
>
>
> Storm queries can retriev...

=== modified file 'lib/lp/code/doc/branch.txt'
--- lib/lp/code/doc/branch.txt	2010-05-27 02:09:13 +0000
+++ lib/lp/code/doc/branch.txt	2010-07-14 16:19:43 +0000
@@ -158,10 +158,10 @@
 
 == Determining the recently changed, registered and imported branches ==
 
-The IBranchSet methods getRecentlyChangedBranches, getRecentlyImportedBranches,
-and getRecentlyRegisteredBranches are used to give summary information that
-is to be displayed on the code.launchpad.net page to entice the
-user to click through.
+The IBranchSet methods getRecentlyChangedBranches,
+getRecentlyImportedBranches, and getRecentlyRegisteredBranches are used to
+give summary information that is to be displayed on the code.launchpad.net
+page to entice the user to click through.
 
 Changed branches are branches that are owned by real people or groups (as
 opposed to vcs-imports), and have recently had new revisions detected by
@@ -315,8 +315,8 @@
     True
     >>> subscription.branch == branch and subscription.person == subscriber
     True
-    >>> subscription.notification_level == BranchSubscriptionNotificationLevel.FULL
-    True
+    >>> print subscription.notification_level.name
+    FULL
     >>> subscription.max_diff_lines == BranchSubscriptionDiffSize.FIVEKLINES
     True
     >>> subscription.review_level == CodeReviewNotificationLevel.FULL
@@ -327,7 +327,7 @@
     True
     >>> from canonical.launchpad.webapp import canonical_url
     >>> print canonical_url(subscription)
-    http://code.launchpad.dev/~user/product/subscribed/+subscription/subscriber
+    http://code...dev/~user/product/subscribed/+subscription/subscriber
 
 The settings for a subscription can be changed by re-subscribing.
 
@@ -399,8 +399,8 @@
 
 Branch.revision_history gives the sequence of revisions in this branch's
 history, latest revisions first. All revision history items must implement the
-IBranchRevision interface. The Branch.revision_count attribute gives the length
-of the revision_history attribute but without building the list.
+IBranchRevision interface. The Branch.revision_count attribute gives the
+length of the revision_history attribute but without building the list.
 
     >>> from lp.code.interfaces.branchrevision import IBranchRevision
     >>> junk.revision_count
@@ -427,11 +427,11 @@
     True
 
 Branch.getRevisionsSince gives all the BranchRevisions for revisions committed
-since a given timestamp. It may give surprising results if some committers had a
-skewed clock.
+since a given timestamp. It may give surprising results if some committers had
+a skewed clock.
 
     >>> from datetime import datetime
-    >>> timestamp = datetime(2005, 10, 31, 12, 00, 00)
+    >>> timestamp = datetime(2005, 10, 31, 12, 00, 00, 0, pytz.UTC)
     >>> two_latest = list(junk.revision_history)[:2]
     >>> list(junk.getRevisionsSince(timestamp)) == two_latest
     True
@@ -440,10 +440,11 @@
 == Ancestry of Revision ==
 
 The revision-history of a given branch, is only one possible ancestry path in
-the ancestry graph. It is also possible to examine the ancestry graph directly.
+the ancestry graph. It is also possible to examine the ancestry graph
+directly.
 
-A Bazaar branch may contains references (by revision_id) to revisions for which
-no data is available. Such revisions are called "ghosts".
+A Bazaar branch may contains references (by revision_id) to revisions for
+which no data is available. Such revisions are called "ghosts".
 
 Initial commits (after a "bzr init") revisions have no parent.
 
@@ -452,8 +453,8 @@
     >>> initial.parent_ids
     []
 
-Normal commits (as opposed to merges) have exactly one parent. The first parent
-of a revision is always the revision that was current when committing.
+Normal commits (as opposed to merges) have exactly one parent. The first
+parent of a revision is always the revision that was current when committing.
 
     >>> commit = history[-2].revision
     >>> [type(a) for a in commit.parent_ids] == [unicode]
@@ -492,8 +493,8 @@
 is identified by the branch attribute "last_scanned_id".  This is the textual
 revision_id for the bzr revision.  The reason that it is a text id rather than
 an integer foreign key is so it can easily be compared to the
-"last_mirrored_id".  The "last_mirrored_id" is set by the branch puller, and is
-used to identify when a scan is needed for a branch.
+"last_mirrored_id".  The "last_mirrored_id" is set by the branch puller, and
+is used to identify when a scan is needed for a branch.
 
     >>> branch = branch_lookup.get(1)
     >>> branch.last_scanned_id
@@ -504,7 +505,8 @@
     >>> branch.getTipRevision() is None
     True
 
-    >>> branch.last_scanned_id = '[e-mail]-20051031165248-6f1bb97973c2b4f4'
+    >>> branch.last_scanned_id = (
+    ...     '[e-mail]-20051031165248-6f1bb97973c2b4f4')
     >>> rev = branch.getTipRevision()
     >>> print rev.date_created
     2005-10-31 17:21:47.381770+00:00
@@ -560,7 +562,8 @@
 
 == Deleting branches ==
 
-If a user creates a branch in error, they should be able to remove that branch.
+If a user creates a branch in error, they should be able to remove that
+branch.
 
 A branch can be deleted trivially if it is not associated with any bugs or
 blueprints, has no subscribers, and hasn't been associated with any product
@@ -585,9 +588,12 @@
     >>> cur = cursor()
     >>> references = list(postgresql.listReferences(cur, 'branch', 'id'))
 
-    >>> for name in sorted([
-    ...     '%s.%s' % (src_tab, src_col) for
-    ...     src_tab, src_col, ref_tab, ref_col, updact, delact in references]):
+    >>> listing = sorted([
+    ...     '%s.%s' % (src_tab, src_col)
+    ...     for src_tab, src_col, ref_tab, ref_col, updact, delact
+    ...     in references
+    ...     ])
+    >>> for name in listing:
     ...     print name
     branch.stacked_on
     branchjob.branch
 
=== modified file 'lib/lp/code/doc/revision.txt'
--- lib/lp/code/doc/revision.txt	2009-06-16 03:31:05 +0000
+++ lib/lp/code/doc/revision.txt	2010-07-14 16:19:43 +0000
@@ -1,11 +1,12 @@
 = Bazaar Revisions =
 
 Branches are collection of revisions, and a revision can exist independently
-from any branch. Revisions are created automatically by scanning branches, they
-have no creation interface and Launchpad cannot create or modify them.
+from any branch. Revisions are created automatically by scanning branches,
+they have no creation interface and Launchpad cannot create or modify them.
 
 == Interfaces ==
 
+    >>> from canonical.launchpad.interfaces.lpstorm import IStore
     >>> from canonical.launchpad.webapp.testing import verifyObject
     >>> from lp.code.interfaces.revision import (
     ...     IRevision, IRevisionAuthor, IRevisionParent, IRevisionSet)
@@ -21,14 +22,16 @@
     True
     >>> verifyObject(IRevisionSet, RevisionSet())
     True
-    >>> verifyObject(IBranchRevision, BranchRevision.get(1))
+    >>> verifyObject(
+    ...     IBranchRevision,
+    ...     IStore(BranchRevision).find(BranchRevision).any())
     True
 
 == Creating revisions ==
 
 The creator of a revision is identified by a RevisionAuthor. A RevisionAuthor
-is not a person because that is only an informational attribute, and even if we
-trust it, there's really no simple way to map that reliably to persons.
+is not a person because that is only an informational attribute, and even if
+we trust it, there's really no simple way to map that reliably to persons.
 
     >>> from lp.code.model.revision import RevisionAuthor
     >>> author = RevisionAuthor(name='ddaa@localhost')
@@ -55,7 +58,8 @@
     >>> date = datetime(2005, 3, 8, 12, 0, tzinfo=UTC)
     >>> from lp.code.model.revision import Revision
     >>> revision_1 = Revision(log_body=log_body_1,
-    ...     revision_author=author, revision_id=revision_id_1, revision_date=date)
+    ...     revision_author=author, revision_id=revision_id_1,
+    ...     revision_date=date)
 
 == Parents ==
 
@@ -67,7 +71,8 @@
 we can represent revisions whose at least one parent is a ghost revision.
 
     >>> revision_2 = Revision(log_body=log_body_2,
-    ...     revision_author=author, revision_id=revision_id_2, revision_date=date)
+    ...     revision_author=author, revision_id=revision_id_2,
+    ...     revision_date=date)
 
     >>> from lp.code.model.revision import RevisionParent
     >>> rev2_parent = RevisionParent(sequence=0, revision=revision_2,
@@ -86,8 +91,10 @@
 
 BranchRevision rows are created using `Branch.createBranchRevision`.
 
-    >>> rev_no_1 = branch.createBranchRevision(sequence=1, revision=revision_1)
-    >>> rev_no_2 = branch.createBranchRevision(sequence=2, revision=revision_2)
+    >>> rev_no_1 = branch.createBranchRevision(
+    ...     sequence=1, revision=revision_1)
+    >>> rev_no_2 = branch.createBranchRevision(
+    ...     sequence=2, revision=revision_2)
     >>> rev_no_1.branch == rev_no_2.branch == branch
     True
 
@@ -96,13 +103,14 @@
     >>> branch = getUtility(IBranchLookup).getByUniqueName(
     ...     '~name12/+junk/junk.contrib')
 
-The full ancestry of a branch is recorded. That includes the history commits on
-this branch, but also revisions that were merged into this branch. Such merged
-revisions are associated to the branch using BranchRevision whose sequence
-attribute is None.
+The full ancestry of a branch is recorded. That includes the history commits
+on this branch, but also revisions that were merged into this branch. Such
+merged revisions are associated to the branch using BranchRevision whose
+sequence attribute is None.
 
     >>> from lp.code.model.branchrevision import BranchRevision
-    >>> ancestry = BranchRevision.selectBy(branch=branch)
+    >>> ancestry = IStore(BranchRevision).find(
+    ...     BranchRevision, BranchRevision.branch == branch)
     >>> for branch_revision in sorted(ancestry,
     ...         key=lambda r:(r.sequence, r.revision.id), reverse=True):
     ...     print branch_revision.sequence, branch_revision.revision.id
@@ -116,16 +124,16 @@
     None 6
 
 If you need to operate on the ancestry of a branch, you should write a focused
-query to avoid creating the tens of thousands of objects necessary to represent
-the ancestry of a large branch.
+query to avoid creating the tens of thousands of objects necessary to
+represent the ancestry of a large branch.
 
 In particular, IBranch.getScannerData efficiently retrieves the BranchRevision
 data needed by the branch-scanner script.
 
     >>> ancestry, history, mapping = branch.getScannerData()
 
-The first return value is a set of revision_id strings for the full ancestry of
-the branch.
+The first return value is a set of revision_id strings for the full ancestry
+of the branch.
 
     >>> for revision_id in sorted(ancestry):
     ...     print revision_id
 
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2010-07-09 10:22:32 +0000
+++ lib/lp/code/interfaces/branch.py	2010-07-14 16:19:43 +0000
@@ -463,9 +463,9 @@
         is set, the branch gets moved into the junk namespace of the branch
         owner.
 
-        :raise: `BranchTargetError` if both project and source_package are set,
-          or if either the project or source_package fail to be adapted to an
-          IBranchTarget.
+        :raise: `BranchTargetError` if both project and source_package are
+            set, or if either the project or source_package fail to be adapted
+            to an `IBranchTarget`.
         """
 
     reviewer = exported(
@@ -668,12 +668,14 @@
 
     # Joins
     revision_history = Attribute(
-        """The sequence of BranchRevision for the mainline of that branch.
+        """The sequence of revisions for the mainline of this branch.
 
         They are ordered with the most recent revision first, and the list
         only contains those in the "leftmost tree", or in other words
         the revisions that match the revision history from bzrlib for this
         branch.
+
+        The revisions are listed as tuples of (`BranchRevision`, `Revision`).
         """)
     subscriptions = exported(
         CollectionField(
@@ -703,7 +705,7 @@
     def destroySelfBreakReferences():
         """Delete the specified branch.
 
-        BranchRevisions associated with this branch will also be deleted as 
+        BranchRevisions associated with this branch will also be deleted as
         well as any items with mandatory references.
         """
 
@@ -766,8 +768,7 @@
             title=_('Commit message'),
             description=_('Message to use when committing this merge.')),
         reviewers=List(value_type=Reference(schema=IPerson)),
-        review_types=List(value_type=TextLine())
-        )
+        review_types=List(value_type=TextLine()))
     # target_branch and prerequisite_branch are actually IBranch, patched in
     # _schema_circular_imports.
     @call_with(registrant=REQUEST_USER)
@@ -782,7 +783,8 @@
         Both the target_branch and the prerequisite_branch, if it is there,
         must be branches with the same target as the source branch.
 
-        Personal branches (a.k.a. junk branches) cannot specify landing targets.
+        Personal branches (a.k.a. junk branches) cannot specify landing
+        targets.
         """
 
     def addLandingTarget(registrant, target_branch, prerequisite_branch=None,
@@ -794,7 +796,8 @@
         Both the target_branch and the prerequisite_branch, if it is there,
         must be branches with the same target as the source branch.
 
-        Personal branches (a.k.a. junk branches) cannot specify landing targets.
+        Personal branches (a.k.a. junk branches) cannot specify landing
+        targets.
 
         :param registrant: The person who is adding the landing target.
         :param target_branch: Must be another branch, and different to self.
@@ -949,8 +952,8 @@
         project is accessible using:
           lp:fooix - the linked object is the product fooix
           lp:fooix/trunk - the linked object is the trunk series of fooix
-          lp:~owner/fooix/name - the unique name of the branch where the linked
-            object is the branch itself.
+          lp:~owner/fooix/name - the unique name of the branch where the
+              linked object is the branch itself.
         """
 
     # subscription-related methods
 
=== 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-14 16:19:43 +0000
@@ -21,7 +21,6 @@
     ancestry of a branch. History revisions have an integer sequence, merged
     revisions have sequence set to None.
     """
-
     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-14 16:19:43 +0000
@@ -34,7 +34,7 @@
 from canonical.config import config
 from canonical.database.constants import DEFAULT, UTC_NOW
 from canonical.database.sqlbase import (
-    cursor, quote, SQLBase, sqlvalues)
+    SQLBase, sqlvalues)
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.enumcol import EnumCol
 
@@ -79,6 +79,7 @@
 from lp.codehosting.bzrutils import safe_open
 from lp.registry.interfaces.person import (
     validate_person_not_private_membership, validate_public_person)
+from lp.services.database.prejoin import prejoin
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.mail.notificationrecipientset import (
     NotificationRecipientSet)
@@ -226,11 +227,13 @@
 
     @property
     def revision_history(self):
-        return BranchRevision.select('''
-            BranchRevision.branch = %s AND
-            BranchRevision.sequence IS NOT NULL
-            ''' % sqlvalues(self),
-            prejoins=['revision'], orderBy='-sequence')
+        result = Store.of(self).find(
+            (BranchRevision, Revision),
+            BranchRevision.branch_id == self.id,
+            Revision.id == BranchRevision.revision_id,
+            BranchRevision.sequence != None)
+        result = result.order_by(Desc(BranchRevision.sequence))
+        return prejoin(result, return_slice=slice(0, 1))
 
     subscriptions = SQLMultipleJoin(
         'BranchSubscription', joinColumn='branch', orderBy='id')
@@ -450,7 +453,7 @@
     @property
     def code_is_browseable(self):
         """See `IBranch`."""
-        return (self.revision_count > 0  or self.last_mirrored != None)
+        return (self.revision_count > 0 or self.last_mirrored != None)
 
     def codebrowse_url(self, *extras):
         """See `IBranch`."""
@@ -509,7 +512,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 +535,15 @@
 
     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).find(
+            (BranchRevision, Revision),
+            Revision.id == BranchRevision.revision_id,
+            BranchRevision.branch == self,
+            BranchRevision.sequence != None,
+            Revision.revision_date > timestamp)
+        result = result.order_by(Desc(BranchRevision.sequence))
+        # Return BranchRevision but prejoin Revision as well.
+        return prejoin(result, slice(0, 1))
 
     def canBeDeleted(self):
         """See `IBranch`."""
@@ -860,19 +864,17 @@
 
     def getScannerData(self):
         """See `IBranch`."""
-        cur = cursor()
-        cur.execute("""
-            SELECT BranchRevision.id, BranchRevision.sequence,
-                Revision.revision_id
-            FROM Revision, BranchRevision
-            WHERE Revision.id = BranchRevision.revision
-                AND BranchRevision.branch = %s
-            ORDER BY BranchRevision.sequence
-            """ % sqlvalues(self))
+        columns = (
+            BranchRevision.id, BranchRevision.sequence, Revision.revision_id)
+        rows = Store.of(self).using(Revision, BranchRevision).find(
+            columns,
+            Revision.id == BranchRevision.revision_id,
+            BranchRevision.branch_id == self.id)
+        rows = rows.order_by(BranchRevision.sequence)
         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:
@@ -890,19 +892,19 @@
             prefix = config.launchpad.bzr_imports_root_url
             return urlappend(prefix, '%08x' % self.id)
         else:
-            raise AssertionError("No pull URL for %r" % (self,))
+            raise AssertionError("No pull URL for %r" % (self, ))
 
     def requestMirror(self):
         """See `IBranch`."""
         if self.branch_type == BranchType.REMOTE:
             raise BranchTypeError(self.unique_name)
         from canonical.launchpad.interfaces import IStore
-        IStore(self).find(
+        branch = IStore(self).find(
             Branch,
             Branch.id == self.id,
             Or(Branch.next_mirror_time > UTC_NOW,
-               Branch.next_mirror_time == None)
-            ).set(next_mirror_time=UTC_NOW)
+               Branch.next_mirror_time == None))
+        branch.set(next_mirror_time=UTC_NOW)
         self.next_mirror_time = AutoReload
         return self.next_mirror_time
 
@@ -997,11 +999,12 @@
 
     def commitsForDays(self, since):
         """See `IBranch`."""
+
         class DateTrunc(NamedFunc):
             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(
@@ -1080,6 +1083,7 @@
     def __init__(self, affected_object, rationale):
         self.affected_object = ProxyFactory(affected_object)
         self.rationale = rationale
+
     def __call__(self):
         """Perform the deletion operation."""
         raise NotImplementedError(DeletionOperation.__call__)
@@ -1161,7 +1165,7 @@
 
     def __init__(self, code_import):
         DeletionOperation.__init__(
-            self, code_import, _( 'This is the import data for this branch.'))
+            self, code_import, _('This is the import data for this branch.'))
 
     def __call__(self):
         from lp.code.model.codeimport import CodeImportSet
 
=== 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-14 16:19:43 +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,F0401
@@ -13,13 +13,13 @@
     ]
 
 from email.Utils import make_msgid
-from storm.expr import And, Or, Select
+from storm.expr import And, Desc, Join, LeftJoin, Or, Select
+from storm.info import ClassAlias
 from storm.store import Store
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import implements
 
-from storm.expr import Join, LeftJoin
 from storm.locals import Int, Reference
 from sqlobject import ForeignKey, IntCol, StringCol, SQLMultipleJoin
 
@@ -74,9 +74,17 @@
         if dupes.count() > 0:
             return False
 
-    [wip, needs_review, code_approved, rejected,
-     merged, merge_failed, queued, superseded
-     ] = BranchMergeProposalStatus.items
+    [
+        wip,
+        needs_review,
+        code_approved,
+        rejected,
+        merged,
+        merge_failed,
+        queued,
+        superseded,
+    ] = BranchMergeProposalStatus.items
+
     # Transitioning to code approved, rejected, failed or queued from
     # work in progress, needs review or merge failed needs the
     # user to be a valid reviewer, other states are fine.
@@ -88,13 +96,13 @@
             return False
         # Non-reviewers can toggle within the reviewed ok states
         # (approved/queued/failed): they can dequeue something they spot an
-        # environmental issue with (queued or failed to approved). Retry things
-        # that had an environmental issue (failed or approved to queued) and note
-        # things as failing (approved and queued to failed).
+        # environmental issue with (queued or failed to approved). Retry
+        # things that had an environmental issue (failed or approved to
+        # queued) and note things as failing (approved and queued to failed).
         # This is perhaps more generous than needed, but its not clearly wrong
-        # - a key concern is to prevent non reviewers putting things in the 
-        # queue that haven't been oked (and thus moved to approved or one of the
-        # workflow states that approved leads to).
+        # - a key concern is to prevent non reviewers putting things in the
+        # queue that haven't been approved (and thus moved to approved or one
+        # of the workflow states that approved leads to).
         elif (next_state in reviewed_ok_states and
               from_state not in reviewed_ok_states):
             return False
@@ -154,14 +162,14 @@
         from lp.code.model.branchmergeproposaljob import (
             BranchMergeProposalJob, BranchMergeProposalJobFactory,
             BranchMergeProposalJobType)
-        job = Store.of(self).find(
+        jobs = Store.of(self).find(
             BranchMergeProposalJob,
             BranchMergeProposalJob.branch_merge_proposal == self,
             BranchMergeProposalJob.job_type ==
             BranchMergeProposalJobType.UPDATE_PREVIEW_DIFF,
             BranchMergeProposalJob.job == Job.id,
-            Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING])
-            ).order_by(Job.scheduled_start, Job.date_created).first()
+            Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
+        job = jobs.order_by(Job.scheduled_start, Job.date_created).first()
         if job is not None:
             return BranchMergeProposalJobFactory.create(job)
         else:
@@ -486,8 +494,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).one()
             if branch_revision is not None:
                 date_merged = branch_revision.revision.revision_date
 
@@ -579,14 +589,20 @@
 
     def getUnlandedSourceBranchRevisions(self):
         """See `IBranchMergeProposal`."""
-        return BranchRevision.select('''
-            BranchRevision.branch = %s AND
-            BranchRevision.sequence IS NOT NULL AND
-            BranchRevision.revision NOT IN (
-              SELECT revision FROM BranchRevision
-              WHERE branch = %s)
-            ''' % sqlvalues(self.source_branch, self.target_branch),
-            prejoins=['revision'], orderBy='-sequence', limit=10)
+        store = Store.of(self)
+        SourceRevision = ClassAlias(BranchRevision)
+        TargetRevision = ClassAlias(BranchRevision)
+        target_join = LeftJoin(
+            TargetRevision, And(
+                TargetRevision.revision_id == SourceRevision.revision_id,
+                TargetRevision.branch_id == self.target_branch.id))
+        origin = [SourceRevision, target_join]
+        result = store.using(*origin).find(
+            SourceRevision,
+            SourceRevision.branch_id == self.source_branch.id,
+            SourceRevision.sequence != None,
+            TargetRevision.id == None)
+        return result.order_by(Desc(SourceRevision.sequence)).config(limit=10)
 
     def createComment(self, owner, subject, content=None, vote=None,
                       review_type=None, parent=None, _date_created=DEFAULT,
@@ -649,9 +665,8 @@
             CodeReviewVoteReference,
             CodeReviewVoteReference.branch_merge_proposal == self,
             CodeReviewVoteReference.review_type == review_type,
-            CodeReviewVoteReference.comment == None
-            ).order_by(CodeReviewVoteReference.date_created)
-        for ref in refs:
+            CodeReviewVoteReference.comment == None)
+        for ref in refs.order_by(CodeReviewVoteReference.date_created):
             if user.inTeam(ref.reviewer):
                 return ref
         return None
@@ -849,5 +864,3 @@
             BranchMergeProposal.target_branch = %s AND
             BranchMergeProposal.queue_status NOT IN %s
                 """ % sqlvalues(source_branch, target_branch, FINAL_STATES))
-
-
 
=== 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-14 16:19:43 +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-14 16:19:43 +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-14 16:19:43 +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 (
@@ -113,7 +114,7 @@
         self.useBzrBranches(direct_database=True)
 
         tree_location = tempfile.mkdtemp()
-        self.addCleanup(lambda: shutil.rmtree(tree_location)) 
+        self.addCleanup(lambda: shutil.rmtree(tree_location))
 
         branch, tree = self.create_branch_and_tree(
             tree_location=tree_location)
@@ -331,7 +332,7 @@
         job = RevisionMailJob.create(
             branch, 0, '[e-mail]', 'hello', False, 'subject')
         job.run()
-        (mail,) = pop_notifications()
+        (mail, ) = pop_notifications()
         self.assertEqual('0', mail['X-Launchpad-Branch-Revision-Number'])
         self.assertEqual('[e-mail]', mail['from'])
         self.assertEqual('subject', mail['subject'])
@@ -344,7 +345,7 @@
             'To unsubscribe from this branch go to'
             ' %(url)s/+edit-subscription\n' % {
                 'url': canonical_url(branch),
-                'identity': branch.bzr_identity
+                'identity': branch.bzr_identity,
                 },
             mail.get_payload(decode=True))
 
@@ -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):
@@ -913,7 +916,7 @@
         self.series = None
 
     def _makeBranchWithTreeAndFile(self, file_name, file_content = None):
-        return self._makeBranchWithTreeAndFiles(((file_name, file_content),))
+        return self._makeBranchWithTreeAndFiles(((file_name, file_content), ))
 
     def _makeBranchWithTreeAndFiles(self, files):
         """Create a branch with a tree that contains the given files.
@@ -984,7 +987,7 @@
 
     def _runJobWithFile(self, import_mode, file_name, file_content = None):
         return self._runJobWithFiles(
-            import_mode, ((file_name, file_content),))
+            import_mode, ((file_name, file_content), ))
 
     def _runJobWithFiles(self, import_mode, files,
                          do_upload_translations=False):
@@ -1018,8 +1021,7 @@
         pot_name = "foo.pot"
         entries = self._runJobWithFiles(
             TranslationsBranchImportMode.IMPORT_TEMPLATES,
-            ((pot_name,), ('eo.po',), ('README',))
-            )
+            ((pot_name,), ('eo.po',), ('README',)))
         self.assertEqual(len(entries), 1)
         entry = entries[0]
         self.assertEqual(pot_name, entry.path)
@@ -1058,8 +1060,7 @@
         pot_name = "en-US.xpi"
         entries = self._runJobWithFiles(
             TranslationsBranchImportMode.IMPORT_TEMPLATES,
-            ((pot_name,), ('eo.xpi',), ('README',))
-            )
+            ((pot_name,), ('eo.xpi',), ('README',)))
         self.assertEqual(len(entries), 1)
         entry = entries[0]
         self.assertEqual(pot_name, entry.path)
@@ -1108,7 +1109,7 @@
         pot_name = "foo.pot"
         revision_id = self._makeBranchWithTreeAndFiles(
             ((pot_name,), ('eo.po',), ('README',)))
-        self._commitFilesToTree(((pot_name,),))
+        self._commitFilesToTree(((pot_name, ), ))
         entries = self._runJob(
             TranslationsBranchImportMode.IMPORT_TEMPLATES, revision_id)
         self.assertEqual(len(entries), 1)
@@ -1120,8 +1121,7 @@
         # not configured to do so.
         entries = self._runJobWithFiles(
             TranslationsBranchImportMode.NO_IMPORT,
-            (('foo.pot',), ('eo.po',), ('README',))
-            )
+            (('foo.pot',), ('eo.po',), ('README',)))
         self.assertEqual([], entries)
 
     def test_upload_translations(self):
@@ -1183,7 +1183,7 @@
         # only POTemplate object in the database, if there is only one such
         # object for this product series.
         self._makeBranchWithTreeAndFiles(
-            [('foo.pot', None),('bar.pot', None)])
+            [('foo.pot', None), ('bar.pot', None)])
         self._makeProductSeries(TranslationsBranchImportMode.IMPORT_TEMPLATES)
         self.factory.makePOTemplate(self.series, path='foo.pot')
         self.factory.makePOTemplate(self.series, path='bar.pot')
@@ -1354,6 +1354,5 @@
         self.assertFalse(os.path.exists(branch_path))
 
 
-
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)
 
=== 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-14 16:19:43 +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
@@ -36,7 +37,9 @@
 def run_as_db_user(username):
     """Create a decorator that will run a function as the given database user.
     """
+
     def _run_with_different_user(f):
+
         def decorated(*args, **kwargs):
             current_user = LaunchpadZopelessLayer.txn._dbuser
             if current_user == username:
@@ -47,6 +50,7 @@
             finally:
                 LaunchpadZopelessLayer.switchDbUser(current_user)
         return mergeFunctionMetadata(f, decorated)
+
     return _run_with_different_user
 
 
@@ -94,10 +98,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 +234,10 @@
         :return: A set of tuples (sequence, revision-id) for all the
             BranchRevisions rows belonging to self.db_branch.
         """
-        return set(
-            (branch_revision.sequence, branch_revision.revision.revision_id)
-            for branch_revision
-            in BranchRevision.selectBy(branch=db_branch))
+        return set(IStore(BranchRevision).find(
+            (BranchRevision.sequence, Revision.revision_id),
+            Revision.id == BranchRevision.revision_id,
+            BranchRevision.branch == db_branch))
 
     def writeToFile(self, filename="file", contents=None):
         """Set the contents of the specified file.
@@ -459,8 +465,9 @@
         # retrieveDatabaseAncestry.
         branch = getUtility(IBranchLookup).getByUniqueName(
             '~name12/+junk/junk.contrib')
-        sampledata = list(
-            BranchRevision.selectBy(branch=branch).orderBy('sequence'))
+        branch_revisions = IStore(BranchRevision).find(
+            BranchRevision, BranchRevision.branch == branch)
+        sampledata = list(branch_revisions.order_by(BranchRevision.sequence))
         expected_ancestry = set(branch_revision.revision.revision_id
             for branch_revision in sampledata)
         expected_history = [branch_revision.revision.revision_id
@@ -540,7 +547,7 @@
 
     def test_upload_on_new_revision_series_not_configured(self):
         # Syncing a branch with a changed tip does not create a
-        # new RosettaUploadJob if the linked product series is not 
+        # new RosettaUploadJob if the linked product series is not
         # configured for translation uploads.
         self._makeProductSeries()
         self.commitRevision()
 
=== modified file 'lib/lp/services/database/prejoin.py'
--- lib/lp/services/database/prejoin.py	2009-10-28 12:11:35 +0000
+++ lib/lp/services/database/prejoin.py	2010-07-14 16:19:43 +0000
@@ -10,6 +10,7 @@
 
 from lazr.delegates import delegates
 
+
 class PrejoinResultSet:
     """Prejoin support.
 
@@ -21,12 +22,12 @@
     The preferred solution is support in Storm core, so we can just do
     something like:
 
-    >>> results = store.find(Product).prejoin(
-    ...     (Person, EmailAddress),
-    ...     Product._ownerID == Person.id,
-    ...     EmailAddress.personID == Person.id)
+    # Select Product, but prejoin the owner as well.
+    >>> join = store.find((Product, Person), Product.name == name)
+    >>> results = prejoin(join, slice(0, 1))
     """
     delegates(IResultSet, context='result_set')
+
     def __init__(self, result_set, return_slice=slice(0, 1)):
         self.result_set = result_set
         self.return_slice = return_slice