Merge branch Generic Foreign Key changes

5 files changed, 308 insertions, 176 deletions

diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py
index 74c1194f..4f2f8915 100644
--- a/mediagoblin/db/migrations.py
+++ b/mediagoblin/db/migrations.py
@@ -910,6 +910,14 @@ class ActivityIntermediator_R0(declarative_base()):
     id = Column(Integer, primary_key=True)
     type = Column(Unicode, nullable=False)
 
+    # These are needed for migration 29
+    TYPES = {
+        "user": User,
+        "media": MediaEntry,
+        "comment": MediaComment,
+        "collection": Collection,
+    }
+
 class Activity_R0(declarative_base()):
     __tablename__ = "core__activities"
     id = Column(Integer, primary_key=True)
@@ -927,6 +935,7 @@ class Activity_R0(declarative_base()):
                     ForeignKey(ActivityIntermediator_R0.id),
                     nullable=True)
 
+
 @RegisterMigration(24, MIGRATIONS)
 def activity_migration(db):
     """
@@ -1249,3 +1258,174 @@ def datetime_to_utc(db):
 
     # Commit this to the database
     db.commit()
+
+##
+# Migrations to handle migrating from activity specific foreign key to the
+# new GenericForeignKey implementations. They have been split up to improve
+# readability and minimise errors
+##
+
+class GenericModelReference_V0(declarative_base()):
+    __tablename__ = "core__generic_model_reference"
+
+    id = Column(Integer, primary_key=True)
+    obj_pk = Column(Integer, nullable=False)
+    model_type = Column(Unicode, nullable=False)
+
+@RegisterMigration(27, MIGRATIONS)
+def create_generic_model_reference(db):
+    """ Creates the Generic Model Reference table """
+    GenericModelReference_V0.__table__.create(db.bind)
+    db.commit()
+
+@RegisterMigration(28, MIGRATIONS)
+def add_foreign_key_fields(db):
+    """
+    Add the fields for GenericForeignKey to the model under temporary name,
+    this is so that later a data migration can occur. They will be renamed to
+    the origional names.
+    """
+    metadata = MetaData(bind=db.bind)
+    activity_table = inspect_table(metadata, "core__activities")
+
+    # Create column and add to model.
+    object_column = Column("temp_object", Integer, ForeignKey(GenericModelReference_V0.id))
+    object_column.create(activity_table)
+
+    target_column = Column("temp_target", Integer, ForeignKey(GenericModelReference_V0.id))
+    target_column.create(activity_table)
+
+    # Commit this to the database
+    db.commit()
+
+@RegisterMigration(29, MIGRATIONS)
+def migrate_data_foreign_keys(db):
+    """
+    This will migrate the data from the old object and target attributes which
+    use the old ActivityIntermediator to the new temparay fields which use the
+    new GenericForeignKey.
+    """
+    metadata = MetaData(bind=db.bind)
+    activity_table = inspect_table(metadata, "core__activities")
+    ai_table = inspect_table(metadata, "core__activity_intermediators")
+    gmr_table = inspect_table(metadata, "core__generic_model_reference")
+
+
+    # Iterate through all activities doing the migration per activity.
+    for activity in db.execute(activity_table.select()):
+        # First do the "Activity.object" migration to "Activity.temp_object"
+        # I need to get the object from the Activity, I can't use the old
+        # Activity.get_object as we're in a migration.
+        object_ai = db.execute(ai_table.select(
+            ai_table.c.id==activity.object
+        )).first()
+
+        object_ai_type = ActivityIntermediator_R0.TYPES[object_ai.type]
+        object_ai_table = inspect_table(metadata, object_ai_type.__tablename__)
+
+        activity_object = db.execute(object_ai_table.select(
+            object_ai_table.c.activity==object_ai.id
+        )).first()
+
+        # now we need to create the GenericModelReference
+        object_gmr = db.execute(gmr_table.insert().values(
+            obj_pk=activity_object.id,
+            model_type=object_ai_type.__tablename__
+        ))
+
+        # Now set the ID of the GenericModelReference in the GenericForignKey
+        db.execute(activity_table.update().values(
+            temp_object=object_gmr.inserted_primary_key[0]
+        ))
+
+        # Now do same process for "Activity.target" to "Activity.temp_target"
+        # not all Activities have a target so if it doesn't just skip the rest
+        # of this.
+        if activity.target is None:
+            continue
+
+        # Now get the target for the activity.
+        target_ai = db.execute(ai_table.select(
+            ai_table.c.id==activity.target
+        )).first()
+
+        target_ai_type = ActivityIntermediator_R0.TYPES[target_ai.type]
+        target_ai_table = inspect_table(metadata, target_ai_type.__tablename__)
+
+        activity_target = db.execute(target_ai_table.select(
+            target_ai_table.c.activity==target_ai.id
+        )).first()
+
+        # We now want to create the new target GenericModelReference
+        target_gmr = db.execute(gmr_table.insert().values(
+            obj_pk=activity_target.id,
+            model_type=target_ai_type.__tablename__
+        ))
+
+        # Now set the ID of the GenericModelReference in the GenericForignKey
+        db.execute(activity_table.update().values(
+            temp_object=target_gmr.inserted_primary_key[0]
+        ))
+
+    # Commit to the database.
+    db.commit()
+
+@RegisterMigration(30, MIGRATIONS)
+def rename_and_remove_object_and_target(db):
+    """
+    Renames the new Activity.object and Activity.target fields and removes the
+    old ones.
+    """
+    metadata = MetaData(bind=db.bind)
+    activity_table = inspect_table(metadata, "core__activities")
+
+    # Firstly lets remove the old fields.
+    old_object_column = activity_table.columns["object"]
+    old_target_column = activity_table.columns["target"]
+
+    # Drop the tables.
+    old_object_column.drop()
+    old_target_column.drop()
+
+    # Now get the new columns.
+    new_object_column = activity_table.columns["temp_object"]
+    new_target_column = activity_table.columns["temp_target"]
+
+    # rename them to the old names.
+    new_object_column.alter(name="object_id")
+    new_target_column.alter(name="target_id")
+
+    # Commit the changes to the database.
+    db.commit()
+
+@RegisterMigration(31, MIGRATIONS)
+def remove_activityintermediator(db):
+    """
+    This removes the old specific ActivityIntermediator model which has been
+    superseeded by the GenericForeignKey field.
+    """
+    metadata = MetaData(bind=db.bind)
+
+    # Remove the columns which reference the AI
+    collection_table = inspect_table(metadata, "core__collections")
+    collection_ai_column = collection_table.columns["activity"]
+    collection_ai_column.drop()
+
+    media_entry_table = inspect_table(metadata, "core__media_entries")
+    media_entry_ai_column = media_entry_table.columns["activity"]
+    media_entry_ai_column.drop()
+
+    comments_table = inspect_table(metadata, "core__media_comments")
+    comments_ai_column = comments_table.columns["activity"]
+    comments_ai_column.drop()
+
+    user_table = inspect_table(metadata, "core__users")
+    user_ai_column = user_table.columns["activity"]
+    user_ai_column.drop()
+
+    # Drop the table
+    ai_table = inspect_table(metadata, "core__activity_intermediators")
+    ai_table.drop()
+
+    # Commit the changes
+    db.commit()
diff --git a/mediagoblin/db/mixin.py b/mediagoblin/db/mixin.py
index 4602c709..88df1f6b 100644
--- a/mediagoblin/db/mixin.py
+++ b/mediagoblin/db/mixin.py
@@ -432,13 +432,12 @@ class ActivityMixin(object):
             "audio": _("audio"),
             "person": _("a person"),
         }
-
-        obj = self.get_object
-        target = self.get_target
+        obj = self.object_helper.get_object()
+        target = None if self.target_helper is None else self.target_helper.get_object()
         actor = self.get_actor
         content = verb_to_content.get(self.verb, None)
 
-        if content is None or obj is None:
+        if content is None or self.object is None:
             return
 
         # Decide what to fill the object with
@@ -452,7 +451,7 @@ class ActivityMixin(object):
         # Do we want to add a target (indirect object) to content?
         if target is not None and "targetted" in content:
             if hasattr(target, "title") and target.title.strip(" "):
-                target_value = target.title
+                target_value = terget.title
             elif target.object_type in object_map:
                 target_value = object_map[target.object_type]
             else:
diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py
index e8fb17a7..054e1677 100644
--- a/mediagoblin/db/models.py
+++ b/mediagoblin/db/models.py
@@ -25,8 +25,9 @@ import datetime
 
 from sqlalchemy import Column, Integer, Unicode, UnicodeText, DateTime, \
         Boolean, ForeignKey, UniqueConstraint, PrimaryKeyConstraint, \
-        SmallInteger, Date
-from sqlalchemy.orm import relationship, backref, with_polymorphic, validates
+        SmallInteger, Date, types
+from sqlalchemy.orm import relationship, backref, with_polymorphic, validates, \
+        class_mapper
 from sqlalchemy.orm.collections import attribute_mapped_collection
 from sqlalchemy.sql.expression import desc
 from sqlalchemy.ext.associationproxy import association_proxy
@@ -47,6 +48,104 @@ from pytz import UTC
 
 _log = logging.getLogger(__name__)
 
+class GenericModelReference(Base):
+    """
+    Represents a relationship to any model that is defined with a integer pk
+    """
+    __tablename__ = "core__generic_model_reference"
+
+    id = Column(Integer, primary_key=True)
+    obj_pk = Column(Integer, nullable=False)
+
+    # This will be the tablename of the model
+    model_type = Column(Unicode, nullable=False)
+
+    # Constrain it so obj_pk and model_type have to be unique
+    # They should be this order as the index is generated, "model_type" will be
+    # the major order as it's put first.
+    __table_args__ = (
+        UniqueConstraint("model_type", "obj_pk"),
+        {})
+
+    def get_object(self):
+        # This can happen if it's yet to be saved
+        if self.model_type is None or self.obj_pk is None:
+            return None
+
+        model = self._get_model_from_type(self.model_type)
+        return model.query.filter_by(id=self.obj_pk).first()
+
+    def set_object(self, obj):
+        model = obj.__class__
+
+        # Check we've been given a object
+        if not issubclass(model, Base):
+            raise ValueError("Only models can be set as using the GMR")
+
+        # Check that the model has an explicit __tablename__ declaration
+        if getattr(model, "__tablename__", None) is None:
+            raise ValueError("Models must have __tablename__ attribute")
+
+        # Check that it's not a composite primary key
+        primary_keys = [key.name for key in class_mapper(model).primary_key]
+        if len(primary_keys) > 1:
+            raise ValueError("Models can not have composite primary keys")
+
+        # Check that the field on the model is a an integer field
+        pk_column = getattr(model, primary_keys[0])
+        if not isinstance(pk_column.type, Integer):
+            raise ValueError("Only models with integer pks can be set")
+
+        if getattr(obj, pk_column.key) is None:
+            obj.save(commit=False)
+
+        self.obj_pk = getattr(obj, pk_column.key)
+        self.model_type = obj.__tablename__
+
+    def _get_model_from_type(self, model_type):
+        """ Gets a model from a tablename (model type) """
+        if getattr(type(self), "_TYPE_MAP", None) is None:
+            # We want to build on the class (not the instance) a map of all the
+            # models by the table name (type) for easy lookup, this is done on
+            # the class so it can be shared between all instances
+
+            # to prevent circular imports do import here
+            registry = dict(Base._decl_class_registry).values()
+            self._TYPE_MAP = dict(
+                ((m.__tablename__, m) for m in registry if hasattr(m, "__tablename__"))
+            )
+            setattr(type(self), "_TYPE_MAP",  self._TYPE_MAP)
+
+        return self.__class__._TYPE_MAP[model_type]
+
+    @classmethod
+    def find_for_obj(cls, obj):
+        """ Finds a GMR for an object or returns None """
+        # Is there one for this already.
+        model = type(obj)
+        pk = getattr(obj, "id")
+
+        gmr = cls.query.filter_by(
+            obj_pk=pk,
+            model_type=model.__tablename__
+        )
+
+        return gmr.first()
+
+    @classmethod
+    def find_or_new(cls, obj):
+        """ Finds an existing GMR or creates a new one for the object """
+        gmr = cls.find_for_obj(obj)
+
+        # If there isn't one already create one
+        if gmr is None:
+            gmr = cls(
+                obj_pk=obj.id,
+                model_type=type(obj).__tablename__
+            )
+
+        return gmr
+
 class Location(Base):
     """ Represents a physical location """
     __tablename__ = "core__locations"
@@ -149,8 +248,6 @@ class User(Base, UserMixin):
     location = Column(Integer, ForeignKey("core__locations.id"))
     get_location = relationship("Location", lazy="joined")
 
-    activity = Column(Integer, ForeignKey("core__activity_intermediators.id"))
-
     ## TODO
     # plugin data would be in a separate model
 
@@ -402,8 +499,6 @@ class MediaEntry(Base, MediaEntryMixin):
     media_metadata = Column(MutationDict.as_mutable(JSONEncoded),
         default=MutationDict())
 
-    activity = Column(Integer, ForeignKey("core__activity_intermediators.id"))
-
     ## TODO
     # fail_error
 
@@ -621,7 +716,7 @@ class MediaEntry(Base, MediaEntryMixin):
             self.license = data["license"]
 
         if "location" in data:
-            Licence.create(data["location"], self)
+            License.create(data["location"], self)
 
         return True
 
@@ -772,9 +867,6 @@ class MediaComment(Base, MediaCommentMixin):
                                                    lazy="dynamic",
                                                    cascade="all, delete-orphan"))
 
-
-    activity = Column(Integer, ForeignKey("core__activity_intermediators.id"))
-
     def serialize(self, request):
         """ Unserialize to python dictionary for API """
         href = request.urlgen(
@@ -855,8 +947,6 @@ class Collection(Base, CollectionMixin):
                                backref=backref("collections",
                                                cascade="all, delete-orphan"))
 
-    activity = Column(Integer, ForeignKey("core__activity_intermediators.id"))
-
     __table_args__ = (
         UniqueConstraint('creator', 'slug'),
         {})
@@ -1262,62 +1352,6 @@ class Generator(Base):
         if "displayName" in data:
             self.name = data["displayName"]
 
-
-class ActivityIntermediator(Base):
-    """
-    This is used so that objects/targets can have a foreign key back to this
-    object and activities can a foreign key to this object. This objects to be
-    used multiple times for the activity object or target and also allows for
-    different types of objects to be used as an Activity.
-    """
-    __tablename__ = "core__activity_intermediators"
-
-    id = Column(Integer, primary_key=True)
-    type = Column(Unicode, nullable=False)
-
-    TYPES = {
-        "user": User,
-        "media": MediaEntry,
-        "comment": MediaComment,
-        "collection": Collection,
-    }
-
-    def _find_model(self, obj):
-        """ Finds the model for a given object """
-        for key, model in self.TYPES.items():
-            if isinstance(obj, model):
-                return key, model
-
-        return None, None
-
-    def set(self, obj):
-        """ This sets itself as the activity """
-        key, model = self._find_model(obj)
-        if key is None:
-            raise ValueError("Invalid type of object given")
-
-        self.type = key
-
-        # We need to populate the self.id so we need to save but, we don't
-        # want to save this AI in the database (yet) so commit=False.
-        self.save(commit=False)
-        obj.activity = self.id
-        obj.save()
-
-    def get(self):
-        """ Finds the object for an activity """
-        if self.type is None:
-            return None
-
-        model = self.TYPES[self.type]
-        return model.query.filter_by(activity=self.id).first()
-
-    @validates("type")
-    def validate_type(self, key, value):
-        """ Validate that the type set is a valid type """
-        assert value in self.TYPES
-        return value
-
 class Activity(Base, ActivityMixin):
     """
     This holds all the metadata about an activity such as uploading an image,
@@ -1337,12 +1371,18 @@ class Activity(Base, ActivityMixin):
     generator = Column(Integer,
                        ForeignKey("core__generators.id"),
                        nullable=True)
-    object = Column(Integer,
-                    ForeignKey("core__activity_intermediators.id"),
-                    nullable=False)
-    target = Column(Integer,
-                    ForeignKey("core__activity_intermediators.id"),
-                    nullable=True)
+
+    # Create the generic foreign keys for the object
+    object_id = Column(Integer, ForeignKey(GenericModelReference.id), nullable=False)
+    object_helper = relationship(GenericModelReference, foreign_keys=[object_id])
+    object = association_proxy("object_helper", "get_object",
+                                creator=GenericModelReference.find_or_new)
+
+    # Create the generic foreign Key for the target
+    target_id = Column(Integer, ForeignKey(GenericModelReference.id), nullable=True)
+    target_helper = relationship(GenericModelReference, foreign_keys=[target_id])
+    taget = association_proxy("target_helper", "get_target",
+                              creator=GenericModelReference.find_or_new)
 
     get_actor = relationship(User,
                              backref=backref("activities",
@@ -1361,44 +1401,6 @@ class Activity(Base, ActivityMixin):
                 content=self.content
             )
 
-    @property
-    def get_object(self):
-        if self.object is None:
-            return None
-
-        ai = ActivityIntermediator.query.filter_by(id=self.object).first()
-        return ai.get()
-
-    def set_object(self, obj):
-        self.object = self._set_model(obj)
-
-    @property
-    def get_target(self):
-        if self.target is None:
-            return None
-
-        ai = ActivityIntermediator.query.filter_by(id=self.target).first()
-        return ai.get()
-
-    def set_target(self, obj):
-        self.target = self._set_model(obj)
-
-    def _set_model(self, obj):
-        # Firstly can we set obj
-        if not hasattr(obj, "activity"):
-            raise ValueError(
-                "{0!r} is unable to be set on activity".format(obj))
-
-        if obj.activity is None:
-            # We need to create a new AI
-            ai = ActivityIntermediator()
-            ai.set(obj)
-            ai.save()
-            return ai.id
-
-        # Okay we should have an existing AI
-        return ActivityIntermediator.query.filter_by(id=obj.activity).first().id
-
     def save(self, set_updated=True, *args, **kwargs):
         if set_updated:
             self.updated = datetime.datetime.now()
@@ -1415,8 +1417,7 @@ MODELS = [
     CommentSubscription, ReportBase, CommentReport, MediaReport, UserBan,
 	Privilege, PrivilegeUserAssociation,
     RequestToken, AccessToken, NonceTimestamp,
-    Activity, ActivityIntermediator, Generator,
-    Location]
+    Activity, Generator, Location, GenericModelReference]
 
 """
  Foundations are the default rows that are created immediately after the tables
diff --git a/mediagoblin/tests/test_modelmethods.py b/mediagoblin/tests/test_modelmethods.py
index 1ab0c476..0706aae4 100644
--- a/mediagoblin/tests/test_modelmethods.py
+++ b/mediagoblin/tests/test_modelmethods.py
@@ -232,55 +232,3 @@ class TestUserUrlForSelf(MGClientTestCase):
             self.user(u'lindsay').url_for_self(fake_urlgen())
         assert excinfo.errisinstance(TypeError)
         assert 'object is not callable' in str(excinfo)
-
-class TestActivitySetGet(object):
-    """ Test methods on the Activity and ActivityIntermediator models """
-
-    @pytest.fixture(autouse=True)
-    def setup(self, test_app):
-        self.app = test_app
-        self.user = fixture_add_user()
-        self.obj = fixture_media_entry()
-        self.target = fixture_media_entry()
-
-    def test_set_activity_object(self):
-        """ Activity.set_object should produce ActivityIntermediator """
-        # The fixture will set self.obj as the object on the activity.
-        activity = fixture_add_activity(self.obj, actor=self.user)
-
-        # Assert the media has been associated with an AI
-        assert self.obj.activity is not None
-
-        # Assert the AI on the media and object are the same
-        assert activity.object == self.obj.activity
-
-    def test_activity_set_target(self):
-        """ Activity.set_target should produce ActivityIntermediator """
-        # This should set everything needed on the target
-        activity = fixture_add_activity(self.obj, actor=self.user)
-        activity.set_target(self.target)
-
-        # Assert the media has been associated with the AI
-        assert self.target.activity is not None
-
-        # assert the AI on the media and target are the same
-        assert activity.target == self.target.activity
-
-    def test_get_activity_object(self):
-        """ Activity.get_object should return a set object """
-        activity = fixture_add_activity(self.obj, actor=self.user)
-
-        print("self.obj.activity = {0}".format(self.obj.activity))
-
-        # check we now can get the object
-        assert activity.get_object is not None
-        assert activity.get_object.id == self.obj.id
-
-    def test_get_activity_target(self):
-        """ Activity.set_target should return a set target """
-        activity = fixture_add_activity(self.obj, actor=self.user)
-        activity.set_target(self.target)
-
-        # check we can get the target
-        assert activity.get_target is not None
-        assert activity.get_target.id == self.target.id
diff --git a/mediagoblin/tools/federation.py b/mediagoblin/tools/federation.py
index 6c2d27da..39b465bf 100644
--- a/mediagoblin/tools/federation.py
+++ b/mediagoblin/tools/federation.py
@@ -26,7 +26,7 @@ def create_generator(request):
         return None
 
     client = request.access_token.get_requesttoken.get_client
-    
+
     # Check if there is a generator already
     generator = Generator.query.filter_by(
         name=client.application_name,
@@ -40,8 +40,8 @@ def create_generator(request):
         generator.save()
 
     return generator
-    
-     
+
+
 
 def create_activity(verb, obj, actor, target=None, generator=None):
     """
@@ -71,11 +71,15 @@ def create_activity(verb, obj, actor, target=None, generator=None):
             )
             generator.save()
 
+    # Ensure the object has an ID which is needed by the activity.
+    obj.save(commit=False)
+
+    # Create the activity
     activity = Activity(verb=verb)
-    activity.set_object(obj)
+    activity.object = obj
 
     if target is not None:
-        activity.set_target(target)
+        activity.target = target
 
    # If they've set it override the actor from the obj.
     activity.actor = actor.id if isinstance(actor, User) else actor