5 files changed, 29 insertions, 33 deletions

diff --git a/src/mailman/database/alembic/versions/47294d3a604_pendable_indexes.py b/src/mailman/database/alembic/versions/47294d3a604_pendable_indexes.py
index 775294290..6b627dc77 100644
--- a/src/mailman/database/alembic/versions/47294d3a604_pendable_indexes.py
+++ b/src/mailman/database/alembic/versions/47294d3a604_pendable_indexes.py
@@ -11,10 +11,11 @@ Create Date: 2015-12-02 11:46:47.295174
 
 # revision identifiers, used by Alembic.
 revision = '47294d3a604'
-down_revision = '33bc0099223'
+down_revision = '3e09bb4a5dc'
 
 import json
 import sqlalchemy as sa
+
 from alembic import op
 
 
@@ -22,7 +23,7 @@ TYPE_CLUES = {
     'member_id': 'probe',
     'token_owner': 'subscription',
     '_mod_message_id': 'data',
-}
+    }
 
 pended_table = sa.sql.table(
     'pended',
@@ -37,6 +38,7 @@ keyvalue_table = sa.sql.table(
     sa.sql.column('pended_id', sa.Integer),
     )
 
+
 def upgrade():
     op.create_index(
         op.f('ix_pended_expiration_date'), 'pended', ['expiration_date'],
@@ -57,15 +59,17 @@ def upgrade():
         kv_type = [kv for kv in keyvalues if kv['key'] == 'type']
         if kv_type:
             # Convert existing type keys from JSON to plain text.
-            kv_type = kv_type[0] # the (pended_id, key) tuple is unique.
+            # The (pended_id, key) tuple is unique.
+            kv_type = kv_type[0]
             try:
                 new_value = json.loads(kv_type['value'])
             except ValueError:
-                pass # New-style entry (or already converted).
+                # New-style entry (or already converted).
+                pass
             else:
                 connection.execute(keyvalue_table.update().where(
                     keyvalue_table.c.id == kv_type['id']
-                ).values(value=new_value))
+                    ).values(value=new_value))
         else:
             # Detect the type and add the corresponding type key.
             keys = [kv['key'] for kv in keyvalues]
@@ -93,26 +97,7 @@ def downgrade():
         connection.execute(keyvalue_table.update().where(
             keyvalue_table.c.id == keyvalue['id']
             ).values(value=json.dumps(keyvalue['value'])))
-    # Remove indexes
-    op.drop_index(op.f('ix_pendedkeyvalue_value'), table_name='pendedkeyvalue')
-    op.drop_index(op.f('ix_pendedkeyvalue_key'), table_name='pendedkeyvalue')
-    op.drop_index(op.f('ix_pended_token'), table_name='pended')
-    op.drop_index(op.f('ix_pended_expiration_date'), table_name='pended')
-    # Data migration.
-    connection = op.get_bind()
-    # Remove the introduced type keys.
-    connection.execute(keyvalue_table.delete().where(sa.and_(
-        keyvalue_table.c.key == 'type',
-        keyvalue_table.c.value.in_(TYPE_CLUES.values())
-        )))
-    # Convert the other type keys to JSON.
-    keyvalues = connection.execute(keyvalue_table.select().where(
-        keyvalue_table.c.key == 'type')).fetchall()
-    for keyvalue in keyvalues:
-        connection.execute(keyvalue_table.update().where(
-            keyvalue_table.c.id == keyvalue['id']
-            ).values(value=json.dumps(keyvalue['value'])))
-    # Remove indexes
+    # Remove indexes.
     op.drop_index(op.f('ix_pendedkeyvalue_value'), table_name='pendedkeyvalue')
     op.drop_index(op.f('ix_pendedkeyvalue_key'), table_name='pendedkeyvalue')
     op.drop_index(op.f('ix_pended_token'), table_name='pended')
diff --git a/src/mailman/database/tests/test_migrations.py b/src/mailman/database/tests/test_migrations.py
index f446213e5..823c94652 100644
--- a/src/mailman/database/tests/test_migrations.py
+++ b/src/mailman/database/tests/test_migrations.py
@@ -109,6 +109,7 @@ class TestMigrations(unittest.TestCase):
         # - one is a moderation request
         # - one is a held message
         # - one is a registration request in the new format
+        #
         # The first three used to have no 'type' key and must be properly
         # typed, the held message used to have a type key, but in JSON, and
         # must be converted.
@@ -144,7 +145,8 @@ class TestMigrations(unittest.TestCase):
             config.db.store.execute(keyvalue_table.insert().values([
                 {'pended_id': 1, 'key': 'member_id', 'value': 'test-value'},
                 {'pended_id': 2, 'key': 'token_owner', 'value': 'test-value'},
-                {'pended_id': 3, 'key': '_mod_message_id', 'value': 'test-value'},
+                {'pended_id': 3, 'key': '_mod_message_id',
+                                 'value': 'test-value'},
                 {'pended_id': 4, 'key': 'type', 'value': '"held message"'},
                 {'pended_id': 5, 'key': 'type', 'value': 'registration'},
                 ]))
diff --git a/src/mailman/interfaces/pending.py b/src/mailman/interfaces/pending.py
index 152946b66..59890ee03 100644
--- a/src/mailman/interfaces/pending.py
+++ b/src/mailman/interfaces/pending.py
@@ -37,7 +37,13 @@ from zope.interface import Interface, Attribute
 class IPendable(Interface):
     """A pendable object."""
 
-    PEND_TYPE = Attribute("""The type of this pendable.""")
+    PEND_TYPE = Attribute(
+        """The type of this pendable.
+
+        Subclasses must define this attribute, and it must be a unique string;
+        it's used to efficiently search for all pendables of the given type.
+        The PEND_TYPE "type" is reserved.
+        """)
 
     def keys():
         """The keys of the pending event data, all of which are strings."""
diff --git a/src/mailman/model/docs/pending.rst b/src/mailman/model/docs/pending.rst
index 9c21a09eb..a871297b6 100644
--- a/src/mailman/model/docs/pending.rst
+++ b/src/mailman/model/docs/pending.rst
@@ -45,9 +45,10 @@ There's exactly one entry in the pendings database now.
 You can *confirm* the pending, which means returning the `IPendable` structure
 (as a dictionary) from the database that matches the token.
 
-All `IPendable` classes have a `PEND_TYPE` attribute which must be a string. It
-is used to identify and query pendables in the database, and will be returned
-as the `type` key in the dictionary.
+All `IPendable` classes have a `PEND_TYPE` attribute which must be a string.
+It is used to identify and query pendables in the database, and will be
+returned as the `type` key in the dictionary.  Thus `type` is a reserved key
+and pendables may not otherwise set it.
 
 If the token isn't in the database, None is returned.
 
diff --git a/src/mailman/model/pending.py b/src/mailman/model/pending.py
index 95c88c015..1973820d0 100644
--- a/src/mailman/model/pending.py
+++ b/src/mailman/model/pending.py
@@ -35,7 +35,7 @@ from mailman.database.transaction import dbconnection
 from mailman.interfaces.pending import (
     IPendable, IPended, IPendedKeyValue, IPendings)
 from mailman.utilities.datetime import now
-from sqlalchemy import and_, Column, DateTime, ForeignKey, Integer, Unicode
+from sqlalchemy import Column, DateTime, ForeignKey, Integer, Unicode, and_
 from sqlalchemy.orm import aliased, relationship
 from zope.interface import implementer
 from zope.interface.verify import verifyObject
@@ -139,9 +139,11 @@ class Pendings:
             'Unexpected token count: {0}'.format(pendings.count()))
         pending = pendings[0]
         pendable = UnpendedPendable()
-        # Iter on PendedKeyValue entries that are associated with the pending
-        # object's ID.  Watch out for type conversions.
+        # Iterate on PendedKeyValue entries that are associated with the
+        # pending object's ID.  Watch out for type conversions.
         for keyvalue in pending.key_values:
+            # The `type` key is special and served.  It is not JSONified.  See
+            # the IPendable interface for details.
             if keyvalue.key == 'type':
                 value = keyvalue.value
             else: