launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/i-hate-storm-base-storm-705197 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #705197 using storm.base.Storm leads to incorrect caches
  https://bugs.launchpad.net/bugs/705197

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/i-hate-storm-base-storm-705197/+merge/47269

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
====
-- 
https://code.launchpad.net/~jcsackett/launchpad/i-hate-storm-base-storm-705197/+merge/47269
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/i-hate-storm-base-storm-705197 into lp:launchpad.

=== 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 19:50:49 +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 19:50:49 +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,
@@ -54,9 +53,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)

=== 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 19:50:49 +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,
@@ -41,9 +40,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)

=== 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 19:50:49 +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 19:50:49 +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 19:50:49 +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 19:50:49 +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 19:50:49 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 __all__ = ['BugSubscriptionFilterTag']
 
-from storm.base import Storm
 from storm.locals import (
     Bool,
     Int,
@@ -14,8 +13,9 @@
     Unicode,
     )
 
+from lp.services.database.stormbase import StormBase
 
-class BugSubscriptionFilterTag(Storm):
+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 19:50:49 +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 19:50:49 +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 19:50:49 +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

=== 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 19:50:49 +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 19:50:49 +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,
@@ -54,9 +53,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 19:50:49 +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 19:50:49 +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)

=== 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 19:50:49 +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)