launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-805791 into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-805791/+merge/66856

OAuthNonce.id overflowed today. This branch promotes the existing (access_token, request_timestamp, nonce) candidate key to primary, and drops OAuthNonce.id. The patch is adapted from BranchRevision.id's demise.

garbo's BulkPruner needed a bit of hackery to work with the composite key. It's a tad ugly, but I could possibly be convinced to clean it up a little.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-805791/+merge/66856
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-805791 into lp:launchpad/db-devel.

=== added file 'database/schema/patch-2208-99-0.sql'
--- database/schema/patch-2208-99-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-99-0.sql	2011-07-05 06:11:36 +0000
@@ -0,0 +1,32 @@
+-- Copyright 2011 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+-- Make the existing primary key index think it is not the primary key.
+UPDATE pg_index SET indisprimary = FALSE
+WHERE pg_index.indexrelid = 'oauthnonce_pkey'::regclass;
+
+UPDATE pg_constraint SET contype = 'u'
+WHERE
+    conrelid='oauthnonce'::regclass
+    AND conname='oauthnonce_pkey';
+
+
+-- Make an existing index think it is the primary key.
+UPDATE pg_index SET indisprimary = TRUE
+WHERE pg_index.indexrelid =
+    'oauthnonce__access_token__request_timestamp__nonce__key'::regclass;
+
+UPDATE pg_constraint SET contype='p'
+WHERE
+    conrelid='oauthnonce'::regclass
+    AND conname='oauthnonce__access_token__request_timestamp__nonce__key';
+
+
+ALTER TABLE OAuthNonce DROP COLUMN id;
+
+-- Rename our new primary key index to the old name to keep Slony-I happy.
+ALTER INDEX oauthnonce__access_token__request_timestamp__nonce__key RENAME TO oauthnonce_pkey;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 99, 0);
+

=== modified file 'lib/canonical/launchpad/database/oauth.py'
--- lib/canonical/launchpad/database/oauth.py	2010-11-08 12:52:43 +0000
+++ lib/canonical/launchpad/database/oauth.py	2011-07-05 06:11:36 +0000
@@ -23,6 +23,7 @@
     StringCol,
     )
 from storm.expr import And
+from storm.locals import Int, Reference
 from zope.component import getUtility
 from zope.interface import implements
 
@@ -58,6 +59,7 @@
     )
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
+from lp.services.database.stormbase import StormBase
 
 # How many hours should a request token be valid for?
 REQUEST_TOKEN_VALIDITY = 2
@@ -78,7 +80,8 @@
 # amount.
 TIMESTAMP_SKEW_WINDOW = 60*60 # seconds, +/-
 
-class OAuthBase(SQLBase):
+
+class OAuthBase:
     """Base class for all OAuth database classes."""
 
     @staticmethod
@@ -95,7 +98,7 @@
     getStore = _get_store
 
 
-class OAuthConsumer(OAuthBase):
+class OAuthConsumer(OAuthBase, SQLBase):
     """See `IOAuthConsumer`."""
     implements(IOAuthConsumer)
 
@@ -191,7 +194,7 @@
         return OAuthConsumer.selectOneBy(key=key)
 
 
-class OAuthAccessToken(OAuthBase):
+class OAuthAccessToken(OAuthBase, SQLBase):
     """See `IOAuthAccessToken`."""
     implements(IOAuthAccessToken)
 
@@ -268,11 +271,13 @@
             raise TimestampOrderingError(
                 'Timestamp too old compared to most recent request')
         # Looks OK.  Give a Nonce object back.
-        return OAuthNonce(
+        nonce = OAuthNonce(
             access_token=self, nonce=nonce, request_timestamp=date)
-
-
-class OAuthRequestToken(OAuthBase):
+        store.add(nonce)
+        return nonce
+
+
+class OAuthRequestToken(OAuthBase, SQLBase):
     """See `IOAuthRequestToken`."""
     implements(IOAuthRequestToken)
 
@@ -387,15 +392,24 @@
         return OAuthRequestToken.selectOneBy(key=key)
 
 
-class OAuthNonce(OAuthBase):
+class OAuthNonce(OAuthBase, StormBase):
     """See `IOAuthNonce`."""
     implements(IOAuthNonce)
 
-    access_token = ForeignKey(
-        dbName='access_token', foreignKey='OAuthAccessToken', notNull=True)
+    __storm_table__ = 'OAuthNonce'
+    __storm_primary__ = 'access_token_id', 'request_timestamp', 'nonce'
+
+    access_token_id = Int(name='access_token')
+    access_token = Reference(access_token_id, OAuthAccessToken.id)
     request_timestamp = UtcDateTimeCol(default=UTC_NOW, notNull=True)
     nonce = StringCol(notNull=True)
 
+    def __init__(self, access_token, request_timestamp, nonce):
+        super(OAuthNonce, self).__init__()
+        self.access_token = access_token
+        self.request_timestamp = request_timestamp
+        self.nonce = nonce
+
 
 def create_token_key_and_secret(table):
     """Create a key and secret for an OAuth token.

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2011-04-21 20:43:38 +0000
+++ lib/lp/scripts/garbo.py	2011-07-05 06:11:36 +0000
@@ -119,7 +119,7 @@
     target_table_key = 'id'
 
     # SQL type of the target_table_key. May be overridden.
-    target_table_key_type = 'integer'
+    target_table_key_type = 'id integer'
 
     # An SQL query returning a list of ids to remove from target_table.
     # The query must return a single column named 'id' and should not
@@ -164,9 +164,9 @@
         """See `ITunableLoop`."""
         result = self.store.execute("""
             DELETE FROM %s
-            WHERE %s IN (
-                SELECT id FROM
-                cursor_fetch('%s', %d) AS f(id %s))
+            WHERE (%s) IN (
+                SELECT * FROM
+                cursor_fetch('%s', %d) AS f(%s))
             """
             % (
                 self.target_table_name, self.target_table_key,
@@ -216,7 +216,7 @@
 
     target_table_class = SessionData
     target_table_key = 'client_id'
-    target_table_key_type = 'text'
+    target_table_key_type = 'id text'
 
 
 class AntiqueSessionPruner(SessionPruner):
@@ -278,9 +278,13 @@
 
     We remove all OAuthNonce records older than 1 day.
     """
+    target_table_key = 'access_token, request_timestamp, nonce'
+    target_table_key_type = (
+        'access_token integer, request_timestamp timestamp without time zone,'
+        ' nonce text')
     target_table_class = OAuthNonce
     ids_to_prune_query = """
-        SELECT id FROM OAuthNonce
+        SELECT access_token, request_timestamp, nonce FROM OAuthNonce
         WHERE request_timestamp
             < CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - CAST('1 day' AS interval)
         """

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2011-05-12 21:33:10 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2011-07-05 06:11:36 +0000
@@ -36,7 +36,10 @@
     )
 from canonical.launchpad.database.librarian import TimeLimitedToken
 from lp.services.messages.model.message import Message
-from canonical.launchpad.database.oauth import OAuthNonce
+from canonical.launchpad.database.oauth import (
+    OAuthAccessToken,
+    OAuthNonce,
+    )
 from canonical.launchpad.database.openidconsumer import OpenIDConsumerNonce
 from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
@@ -394,10 +397,10 @@
         self.failUnlessEqual(store.find(OAuthNonce).count(), 0)
 
         for timestamp in timestamps:
-            OAuthNonce(
-                access_tokenID=1,
+            store.add(OAuthNonce(
+                access_token=OAuthAccessToken.get(1),
                 request_timestamp = timestamp,
-                nonce = str(timestamp))
+                nonce = str(timestamp)))
         transaction.commit()
 
         # Make sure we have 4 nonces now.