Merge lp:~jcsackett/launchpad/i-hate-storm-base-storm-705197 into lp:launchpad

[j.c.sackett]

[r=abentley][ui=none][bug=705197] Replaces use of storm.base.Storm with a new class, Stormbase, which descends from base.Storm but adds SQLObject's __storm methods.

Summary
=======

Several models uses storm.base.Storm as their parent class. These classes end up introducing cache errors as they don't have the necessary storm hooks to clear/invalidate caches (as other classes e.g. SQLBase do).

This branch introduces a new class, descended from storm.base.Storm, that adds the same storm hook functionality as in SQLBase.

Preimplementation
=================

Spoke with Robert Collins, Curtis Hovey, and William Grant about what exactly was needed.

Proposed Fix
============

Create a new class, StormBase, that descends from storm.base.Storm and adds the hooks needed (borrowed from SQLBase).

Implementation
==============

lib/lp/service/database/stormbase.py
------------------------------------

Adds the new StormBase class.

All other files
---------------

Updates uses of storm.base.Storm to lp.services.database.StormBase

Tests
=====

Basically you have to run ec2. You can try:

bin/test -m lp.bugs
bin/test -m lp.codehosting
bin/test -m lp.registry

But that's not guaranteed to test all areas changed. I've had to ec2 test to track down any breaks.

Demo & QA
=========

Not terribly qa-able. Make sure bugtracking and bugtasks work--they were heaviest effected. But honestly, if this weren't working, lp.dev will just crash all over the place.

Lint
====

= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:

lib/canonical/launchpad/database/librarian.py
lib/lp/bugs/model/apportjob.py
lib/lp/bugs/model/bugjob.py
lib/lp/bugs/model/bugsubscription.py
lib/lp/bugs/model/bugsubscriptionfilter.py lib/lp/bugs/model/bugsubscriptionfilterimportance.py lib/lp/bugs/model/bugsubscriptionfilterstatus.py lib/lp/bugs/model/bugsubscriptionfiltertag.py
lib/lp/bugs/model/bugtracker.py
lib/lp/bugs/model/bugwatch.py
lib/lp/code/model/branchmergeproposaljob.py
lib/lp/registry/model/nameblacklist.py
lib/lp/registry/model/persontransferjob.py
lib/lp/services/database/stormbase.py
lib/lp/soyuz/model/archivejob.py
lib/lp/soyuz/model/distributionjob.py

./lib/lp/bugs/model/apportjob.py
      35: 'ITemporaryStorageManager' imported but unused
     163: E222 multiple spaces after operator
     210: E202 whitespace before ')'
./lib/lp/bugs/model/bugjob.py
      26: 'removeSecurityProxy' imported but unused
     142: E222 multiple spaces after operator./lib/lp/bugs/model/bugsubscriptionfiltertag.py
      18: E302 expected 2 blank lines, found 1
./lib/lp/code/model/branchmergeproposaljob.py
       5: E303 too many blank lines (3)
     289: E202 whitespace before ')'
./lib/lp/registry/model/persontransferjob.py
      14: 'SQLObjectNotFound' imported but unused
./lib/lp/soyuz/model/archivejob.py
     136: E222 multiple spaces after operator

I'm going to fix lint after review; the diff has a lot of little one-off lines as it is, and I didn't want to clutter it up.

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'lib/canonical/launchpad/database/librarian.py'
--- lib/canonical/launchpad/database/librarian.py	2010-12-20 17:42:47 +0000
+++ lib/canonical/launchpad/database/librarian.py	2011-01-24 20:15:33 +0000
@@ -30,7 +30,6 @@
     SQLRelatedJoin,
     StringCol,
     )
-import storm.base
 from storm.locals import (
     Date,
     Desc,
@@ -71,6 +70,7 @@
     IRestrictedLibrarianClient,
     LIBRARIAN_SERVER_DEFAULT_TIMEOUT,
     )
+from lp.services.database.stormbase import StormBase
 
 
 class LibraryFileContent(SQLBase):
@@ -305,7 +305,7 @@
     country = Reference(country_id, 'Country.id')
 
 
-class TimeLimitedToken(storm.base.Storm):
+class TimeLimitedToken(StormBase):
     """A time limited access token for accessing a private file."""
 
     __storm_table__ = 'TimeLimitedToken'

=== modified file 'lib/lp/bugs/model/apportjob.py'
--- lib/lp/bugs/model/apportjob.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/apportjob.py	2011-01-24 20:15:33 +0000
@@ -14,7 +14,6 @@
 from lazr.delegates import delegates
 import simplejson
 from sqlobject import SQLObjectNotFound
-from storm.base import Storm
 from storm.expr import And
 from storm.locals import (
     Int,
@@ -33,9 +32,6 @@
     )
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.interfaces.lpstorm import IStore
-from canonical.launchpad.interfaces.temporaryblobstorage import (
-    ITemporaryStorageManager,
-    )
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
     IStoreSelector,
@@ -54,9 +50,10 @@
     )
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
-
-
-class ApportJob(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class ApportJob(StormBase):
     """Base class for jobs related to Apport BLOBs."""
 
     implements(IApportJob)
@@ -160,7 +157,7 @@
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
-        vars =  BaseRunnableJob.getOopsVars(self)
+        vars = BaseRunnableJob.getOopsVars(self)
         vars.extend([
             ('apport_blob_uuid', self.context.blob.uuid),
             ('apport_blob_librarian_url',
@@ -207,8 +204,7 @@
             ApportJob.job == Job.id,
             ApportJob.job_type == cls.class_job_type,
             ApportJob.blob_id == TemporaryBlobStorage.id,
-            TemporaryBlobStorage.uuid == uuid
-            )
+            TemporaryBlobStorage.uuid == uuid)
 
         job_for_blob = jobs_for_blob.one()
 

=== modified file 'lib/lp/bugs/model/bugjob.py'
--- lib/lp/bugs/model/bugjob.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bugjob.py	2011-01-24 20:15:33 +0000
@@ -12,7 +12,6 @@
 from lazr.delegates import delegates
 import simplejson
 from sqlobject import SQLObjectNotFound
-from storm.base import Storm
 from storm.expr import And
 from storm.locals import (
     Int,
@@ -24,7 +23,6 @@
     classProvides,
     implements,
     )
-from zope.security.proxy import removeSecurityProxy
 
 from canonical.database.enumcol import EnumCol
 from canonical.launchpad.webapp.interfaces import (
@@ -41,9 +39,10 @@
 from lp.bugs.model.bug import Bug
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
-
-
-class BugJob(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class BugJob(StormBase):
     """Base class for jobs related to Bugs."""
 
     implements(IBugJob)
@@ -139,7 +138,7 @@
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
-        vars =  BaseRunnableJob.getOopsVars(self)
+        vars = BaseRunnableJob.getOopsVars(self)
         vars.extend([
             ('bug_id', self.context.bug.id),
             ('bug_job_id', self.context.id),

=== modified file 'lib/lp/bugs/model/bugsubscription.py'
--- lib/lp/bugs/model/bugsubscription.py	2010-10-14 20:49:34 +0000
+++ lib/lp/bugs/model/bugsubscription.py	2011-01-24 20:15:33 +0000
@@ -7,7 +7,6 @@
 __all__ = ['BugSubscription']
 
 import pytz
-from storm.base import Storm
 from storm.locals import (
     DateTime,
     Int,
@@ -20,9 +19,10 @@
 from lp.bugs.interfaces.bugsubscription import IBugSubscription
 from lp.registry.enum import BugNotificationLevel
 from lp.registry.interfaces.person import validate_person
-
-
-class BugSubscription(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class BugSubscription(StormBase):
     """A relationship between a person and a bug."""
 
     implements(IBugSubscription)

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2010-10-04 13:31:12 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2011-01-24 20:15:33 +0000
@@ -8,7 +8,6 @@
 
 from itertools import chain
 
-from storm.base import Storm
 from storm.locals import (
     Bool,
     Int,
@@ -28,9 +27,10 @@
     BugSubscriptionFilterStatus,
     )
 from lp.bugs.model.bugsubscriptionfiltertag import BugSubscriptionFilterTag
-
-
-class BugSubscriptionFilter(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class BugSubscriptionFilter(StormBase):
     """A filter to specialize a *structural* subscription."""
 
     implements(IBugSubscriptionFilter)

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilterimportance.py'
--- lib/lp/bugs/model/bugsubscriptionfilterimportance.py	2010-09-17 11:14:25 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilterimportance.py	2011-01-24 20:15:33 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 __all__ = ['BugSubscriptionFilterImportance']
 
-from storm.base import Storm
 from storm.locals import (
     Int,
     Reference,
@@ -14,9 +13,10 @@
 
 from canonical.database.enumcol import DBEnum
 from lp.bugs.interfaces.bugtask import BugTaskImportance
-
-
-class BugSubscriptionFilterImportance(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class BugSubscriptionFilterImportance(StormBase):
     """Importances to filter."""
 
     __storm_table__ = "BugSubscriptionFilterImportance"

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilterstatus.py'
--- lib/lp/bugs/model/bugsubscriptionfilterstatus.py	2010-09-17 11:14:25 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilterstatus.py	2011-01-24 20:15:33 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 __all__ = ['BugSubscriptionFilterStatus']
 
-from storm.base import Storm
 from storm.locals import (
     Int,
     Reference,
@@ -14,9 +13,10 @@
 
 from canonical.database.enumcol import DBEnum
 from lp.bugs.interfaces.bugtask import BugTaskStatus
-
-
-class BugSubscriptionFilterStatus(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class BugSubscriptionFilterStatus(StormBase):
     """Statuses to filter."""
 
     __storm_table__ = "BugSubscriptionFilterStatus"

=== modified file 'lib/lp/bugs/model/bugsubscriptionfiltertag.py'
--- lib/lp/bugs/model/bugsubscriptionfiltertag.py	2010-10-01 11:06:05 +0000
+++ lib/lp/bugs/model/bugsubscriptionfiltertag.py	2011-01-24 20:15:33 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 __all__ = ['BugSubscriptionFilterTag']
 
-from storm.base import Storm
 from storm.locals import (
     Bool,
     Int,
@@ -14,8 +13,10 @@
     Unicode,
     )
 
-
-class BugSubscriptionFilterTag(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class BugSubscriptionFilterTag(StormBase):
     """Tags to filter."""
 
     __storm_table__ = "BugSubscriptionFilterTag"

=== modified file 'lib/lp/bugs/model/bugtracker.py'
--- lib/lp/bugs/model/bugtracker.py	2010-11-04 02:32:16 +0000
+++ lib/lp/bugs/model/bugtracker.py	2011-01-24 20:15:33 +0000
@@ -24,7 +24,6 @@
     splittype,
     )
 
-from storm.base import Storm
 from storm.locals import (
         Int,
         Reference,
@@ -86,6 +85,7 @@
     IPersonSet,
     validate_public_person,
     )
+from lp.services.database.stormbase import StormBase
 
 
 def normalise_leading_slashes(rest):
@@ -175,7 +175,7 @@
         return base_uri.host + base_uri.path
 
 
-class BugTrackerComponent(Storm):
+class BugTrackerComponent(StormBase):
     """The software component in the remote bug tracker.
 
     Most bug trackers organize bug reports by the software 'component'
@@ -229,7 +229,7 @@
         """The distribution's source package for this component""")
 
 
-class BugTrackerComponentGroup(Storm):
+class BugTrackerComponentGroup(StormBase):
     """A collection of components in a remote bug tracker.
 
     Some bug trackers organize sets of components into higher level

=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py	2010-11-05 13:46:55 +0000
+++ lib/lp/bugs/model/bugwatch.py	2011-01-24 20:15:33 +0000
@@ -26,7 +26,6 @@
     SQLObjectNotFound,
     StringCol,
     )
-from storm.base import Storm
 from storm.expr import (
     Desc,
     Not,
@@ -79,6 +78,7 @@
 from lp.bugs.model.bugset import BugSetBase
 from lp.bugs.model.bugtask import BugTask
 from lp.registry.interfaces.person import validate_public_person
+from lp.services.database.stormbase import StormBase
 
 
 BUG_TRACKER_URL_FORMATS = {
@@ -751,7 +751,7 @@
                     result, message, oops_id, bug_watch_ids))
 
 
-class BugWatchActivity(Storm):
+class BugWatchActivity(StormBase):
     """See `IBugWatchActivity`."""
 
     implements(IBugWatchActivity)

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2010-10-29 14:03:31 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2011-01-24 20:15:33 +0000
@@ -42,7 +42,6 @@
 import pytz
 import simplejson
 from sqlobject import SQLObjectNotFound
-from storm.base import Storm
 from storm.expr import (
     And,
     Desc,
@@ -111,6 +110,7 @@
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
 from lp.services.mail.sendmail import format_address_for_person
+from lp.services.database.stormbase import StormBase
 
 
 class BranchMergeProposalJobType(DBEnumeratedType):
@@ -155,7 +155,7 @@
         This job generates an incremental diff for a merge proposal.""")
 
 
-class BranchMergeProposalJob(Storm):
+class BranchMergeProposalJob(StormBase):
     """Base class for jobs related to branch merge proposals."""
 
     implements(IBranchMergeProposalJob)
@@ -187,7 +187,7 @@
         :param metadata: The type-specific variables, as a JSON-compatible
             dict.
         """
-        Storm.__init__(self)
+        super(BranchMergeProposalJob, self).__init__()
         json_data = simplejson.dumps(metadata)
         self.job = Job()
         self.branch_merge_proposal = branch_merge_proposal
@@ -286,8 +286,7 @@
                 # or if it is hosted but pending a mirror.
                 Branch.revision_count > 0,
                 Or(Branch.next_mirror_time == None,
-                   Branch.branch_type != BranchType.HOSTED)
-                ))
+                   Branch.branch_type != BranchType.HOSTED)))
         return (klass(job) for job in jobs)
 
     def getOopsVars(self):

=== modified file 'lib/lp/registry/model/nameblacklist.py'
--- lib/lp/registry/model/nameblacklist.py	2010-11-11 16:06:24 +0000
+++ lib/lp/registry/model/nameblacklist.py	2011-01-24 20:15:33 +0000
@@ -10,7 +10,6 @@
     ]
 
 
-from storm.base import Storm
 from storm.locals import (
     Int,
     Unicode,
@@ -22,9 +21,10 @@
     INameBlacklist,
     INameBlacklistSet,
     )
-
-
-class NameBlacklist(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class NameBlacklist(StormBase):
     """Class for the NameBlacklist table."""
 
     implements(INameBlacklist)

=== modified file 'lib/lp/registry/model/persontransferjob.py'
--- lib/lp/registry/model/persontransferjob.py	2010-09-30 21:29:11 +0000
+++ lib/lp/registry/model/persontransferjob.py	2011-01-24 20:15:33 +0000
@@ -11,8 +11,6 @@
 
 from lazr.delegates import delegates
 import simplejson
-from sqlobject import SQLObjectNotFound
-from storm.base import Storm
 from storm.expr import And
 from storm.locals import (
     Int,
@@ -54,9 +52,10 @@
 from lp.registry.model.person import Person
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
-
-
-class PersonTransferJob(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class PersonTransferJob(StormBase):
     """Base class for team membership and person merge jobs."""
 
     implements(IPersonTransferJob)

=== added file 'lib/lp/services/database/stormbase.py'
--- lib/lp/services/database/stormbase.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/database/stormbase.py	2011-01-24 20:15:33 +0000
@@ -0,0 +1,31 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+__all__ = [
+    'StormBase',
+    ]
+
+from lazr.restful.interfaces import IRepresentationCache
+from storm.base import Storm
+from zope.component import getUtility
+
+from lp.services.propertycache import clear_property_cache
+
+
+class StormBase(Storm):
+    """A safe version of storm.base.Storm to use in launchpad.
+
+    This class adds storm cache management functions to base.Storm.
+    """
+
+    # XXX: jcsackett 2011-01-20 bug=622648: Much as with the SQLBase,
+    # this is not tested.
+    def __storm_flushed__(self):
+        """Invalidate the web service cache."""
+        cache = getUtility(IRepresentationCache)
+        cache.delete(self)
+
+    def __storm_invalidated__(self):
+        """Flush cached properties."""
+        clear_property_cache(self)

=== modified file 'lib/lp/soyuz/model/archivejob.py'
--- lib/lp/soyuz/model/archivejob.py	2010-08-23 17:01:11 +0000
+++ lib/lp/soyuz/model/archivejob.py	2011-01-24 20:15:33 +0000
@@ -6,7 +6,6 @@
 from lazr.delegates import delegates
 import simplejson
 from sqlobject import SQLObjectNotFound
-from storm.base import Storm
 from storm.expr import And
 from storm.locals import (
     Int,
@@ -28,6 +27,7 @@
     )
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
+from lp.services.database.stormbase import StormBase
 from lp.soyuz.enums import ArchiveJobType
 from lp.soyuz.interfaces.archivejob import (
     IArchiveJob,
@@ -36,7 +36,7 @@
 from lp.soyuz.model.archive import Archive
 
 
-class ArchiveJob(Storm):
+class ArchiveJob(StormBase):
     """Base class for jobs related to Archives."""
 
     implements(IArchiveJob)
@@ -133,7 +133,7 @@
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
-        vars =  BaseRunnableJob.getOopsVars(self)
+        vars = BaseRunnableJob.getOopsVars(self)
         vars.extend([
             ('archive_id', self.context.archive.id),
             ('archive_job_id', self.context.id),

=== modified file 'lib/lp/soyuz/model/distributionjob.py'
--- lib/lp/soyuz/model/distributionjob.py	2010-09-16 01:38:42 +0000
+++ lib/lp/soyuz/model/distributionjob.py	2011-01-24 20:15:33 +0000
@@ -10,7 +10,6 @@
 
 import simplejson
 
-from storm.base import Storm
 from storm.locals import And, Int, Reference, Unicode
 
 from zope.interface import implements
@@ -29,9 +28,10 @@
     )
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
-
-
-class DistributionJob(Storm):
+from lp.services.database.stormbase import StormBase
+
+
+class DistributionJob(StormBase):
     """Base class for jobs related to Distributions."""
 
     implements(IDistributionJob)