From 641ae2f1e1eaadb66446ca67fe29672c90155ca7 Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Wed, 25 Feb 2015 13:41:53 +0100 Subject: [PATCH 01/10] Add GenericForeignKey field and reference helper model --- mediagoblin/db/migrations.py | 15 +++++++ mediagoblin/db/models.py | 80 +++++++++++++++++++++++++++++++++++- 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py index 74c1194f..446f30df 100644 --- a/mediagoblin/db/migrations.py +++ b/mediagoblin/db/migrations.py @@ -1249,3 +1249,18 @@ def datetime_to_utc(db): # Commit this to the database db.commit() + +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() + + diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index e8fb17a7..97f8b398 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -26,7 +26,8 @@ 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 +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,81 @@ from pytz import UTC _log = logging.getLogger(__name__) +class GenericModelReference(Base): + """ + Represents a relationship to any model that is defined with a integer pk + + NB: This model should not be used directly but through the GenericForeignKey + field provided. + """ + __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) + + @property + def get(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) + + @property + def set(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 GenericForeignKeys") + + # 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 issubclass(Integer, pk_column): + raise ValueError("Only models with integer pks can be set") + + # Ensure that everything has it's ID set + obj.save(commit=False) + + self.obj_pk = obj.id + self.model_type = obj.__tablename__ + + def _get_model_from_type(self, model_type): + """ Gets a model from a tablename (model type) """ + if getattr(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 + self._TYPE_MAP = dict(((m.__tablename__, m) for m in MODELS)) + setattr(self.__class__._TYPE_MAP, self._TYPE_MAP) + + return self._TYPE_MAP[model_type] + + +class GenericForeignKey(ForeignKey): + + def __init__(self, *args, **kwargs): + super(GenericForeignKey, self).__init__( + "core__generic_model_reference.id", + *args, + **kwargs + ) + class Location(Base): """ Represents a physical location """ __tablename__ = "core__locations" @@ -1416,7 +1492,7 @@ MODELS = [ Privilege, PrivilegeUserAssociation, RequestToken, AccessToken, NonceTimestamp, Activity, ActivityIntermediator, Generator, - Location] + Location, GenericModelReference] """ Foundations are the default rows that are created immediately after the tables From bfe1e8ce880d3ea30c24bb1c6840126f5a50638d Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Mon, 2 Mar 2015 14:27:52 +0100 Subject: [PATCH 02/10] Migrate Activity to using the new GenericForeignKey --- mediagoblin/db/migrations.py | 141 ++++++++++++++++++++++++++++++++++- mediagoblin/db/models.py | 72 +++++++----------- 2 files changed, 166 insertions(+), 47 deletions(-) diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py index 446f30df..8661c95a 100644 --- a/mediagoblin/db/migrations.py +++ b/mediagoblin/db/migrations.py @@ -36,7 +36,7 @@ from mediagoblin.db.extratypes import JSONEncoded, MutationDict from mediagoblin.db.migration_tools import ( RegisterMigration, inspect_table, replace_table_hack) from mediagoblin.db.models import (MediaEntry, Collection, MediaComment, User, - Privilege, Generator) + Privilege, Generator, GenericForeignKey) from mediagoblin.db.extratypes import JSONEncoded, MutationDict @@ -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): """ @@ -1250,6 +1259,12 @@ 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" @@ -1263,4 +1278,128 @@ def create_generic_model_reference(db): 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, GenericForeignKey()) + object_column.create(activity_table) + + target_column = Column("temp_target", Integer, GenericForeignKey()) + 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") + new_target_column.alter(name="target") + + # Commit the changes to the database. + db.commit() + + + + + diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index 97f8b398..4b592792 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -53,7 +53,7 @@ class GenericModelReference(Base): Represents a relationship to any model that is defined with a integer pk NB: This model should not be used directly but through the GenericForeignKey - field provided. + field provided. """ __tablename__ = "core__generic_model_reference" @@ -63,8 +63,7 @@ class GenericModelReference(Base): # This will be the tablename of the model model_type = Column(Unicode, nullable=False) - @property - def get(self): + 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 @@ -72,8 +71,7 @@ class GenericModelReference(Base): model = self._get_model_from_type(self.model_type) return model.query.filter_by(id=self.obj_pk) - @property - def set(self, obj): + def set_object(self, obj): model = obj.__class__ # Check we've been given a object @@ -118,11 +116,31 @@ class GenericForeignKey(ForeignKey): def __init__(self, *args, **kwargs): super(GenericForeignKey, self).__init__( - "core__generic_model_reference.id", + GenericModelReference.id, *args, **kwargs ) + def __get__(self, *args, **kwargs): + """ Looks up GenericModelReference and model for field """ + # Find the value of the foreign key. + ref = super(self, GenericForeignKey).__get__(*args, **kwargs) + + # If this hasn't been set yet return None + if ref is None: + return None + + # Look up the GenericModelReference for this. + gmr = GenericModelReference.query.filter_by(id=ref).first() + + # If it's set to something invalid (i.e. no GMR exists return None) + if gmr is None: + return None + + # Ask the GMR for the corresponding model + return gmr.get_object() + + class Location(Base): """ Represents a physical location """ __tablename__ = "core__locations" @@ -1414,10 +1432,10 @@ class Activity(Base, ActivityMixin): ForeignKey("core__generators.id"), nullable=True) object = Column(Integer, - ForeignKey("core__activity_intermediators.id"), + GenericForeignKey(), nullable=False) target = Column(Integer, - ForeignKey("core__activity_intermediators.id"), + GenericForeignKey(), nullable=True) get_actor = relationship(User, @@ -1437,44 +1455,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() From 6185a4b9e6465ecd9c4806ffeec7688f7baa1f2f Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Tue, 28 Apr 2015 19:53:25 +0200 Subject: [PATCH 03/10] Fix the GenericForeignKey implementation --- mediagoblin/db/models.py | 58 +++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index 4b592792..b4cfe2a8 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -25,7 +25,7 @@ import datetime from sqlalchemy import Column, Integer, Unicode, UnicodeText, DateTime, \ Boolean, ForeignKey, UniqueConstraint, PrimaryKeyConstraint, \ - SmallInteger, Date + SmallInteger, Date, types from sqlalchemy.orm import relationship, backref, with_polymorphic, validates, \ class_mapper from sqlalchemy.orm.collections import attribute_mapped_collection @@ -69,7 +69,7 @@ class GenericModelReference(Base): return None model = self._get_model_from_type(self.model_type) - return model.query.filter_by(id=self.obj_pk) + return model.query.filter_by(id=self.obj_pk).first() def set_object(self, obj): model = obj.__class__ @@ -100,38 +100,30 @@ class GenericModelReference(Base): def _get_model_from_type(self, model_type): """ Gets a model from a tablename (model type) """ - if getattr(self, "_TYPE_MAP", None) is None: + if getattr(self.__class__, "_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 self._TYPE_MAP = dict(((m.__tablename__, m) for m in MODELS)) - setattr(self.__class__._TYPE_MAP, self._TYPE_MAP) + setattr(self.__class__, "_TYPE_MAP", self._TYPE_MAP) - return self._TYPE_MAP[model_type] + return self.__class__._TYPE_MAP[model_type] -class GenericForeignKey(ForeignKey): +class GenericForeignKey(types.TypeDecorator): - def __init__(self, *args, **kwargs): - super(GenericForeignKey, self).__init__( - GenericModelReference.id, - *args, - **kwargs - ) + impl = Integer - def __get__(self, *args, **kwargs): + def process_result_value(self, value, *args, **kwargs): """ Looks up GenericModelReference and model for field """ - # Find the value of the foreign key. - ref = super(self, GenericForeignKey).__get__(*args, **kwargs) - # If this hasn't been set yet return None - if ref is None: + if value is None: return None # Look up the GenericModelReference for this. - gmr = GenericModelReference.query.filter_by(id=ref).first() + gmr = GenericModelReference.query.filter_by(id=value).first() # If it's set to something invalid (i.e. no GMR exists return None) if gmr is None: @@ -140,6 +132,30 @@ class GenericForeignKey(ForeignKey): # Ask the GMR for the corresponding model return gmr.get_object() + def process_bind_param(self, value, *args, **kwargs): + """ Save the foreign key """ + if value is None: + return None + + # Is there one for this already. + model = type(value) + pk = getattr(value, "id") + + gmr = GenericModelReference.query.filter_by(id=pk).first() + if gmr is None: + # We need to create one + gmr = GenericModelReference( + obj_pk=pk, + model_type=model.__tablename__ + ) + gmr.save() + + return gmr.id + + def _set_parent_with_dispatch(self, parent): + self.parent = parent + + class Location(Base): """ Represents a physical location """ @@ -1431,11 +1447,9 @@ class Activity(Base, ActivityMixin): generator = Column(Integer, ForeignKey("core__generators.id"), nullable=True) - object = Column(Integer, - GenericForeignKey(), + object = Column(GenericForeignKey(), nullable=False) - target = Column(Integer, - GenericForeignKey(), + target = Column(GenericForeignKey(), nullable=True) get_actor = relationship(User, From e8b44d7c093ce24c2c77674a389238efc4d58f60 Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Tue, 28 Apr 2015 20:43:15 +0200 Subject: [PATCH 04/10] Add migration to remove ActivityIntermediator Migration to drop the table and removal of it from the model as it has now been superseeded by the GenericForeignKey field. --- mediagoblin/db/migrations.py | 15 ++++++++-- mediagoblin/db/models.py | 56 ------------------------------------ 2 files changed, 12 insertions(+), 59 deletions(-) diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py index 8661c95a..70bf6234 100644 --- a/mediagoblin/db/migrations.py +++ b/mediagoblin/db/migrations.py @@ -1398,8 +1398,17 @@ def rename_and_remove_object_and_target(db): # 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) + # Drop the table + ai_table = inspect_table(metadata, "core__activity_intermediators") + ai_table.drop() - - - + # Commit the changes + db.commit() diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index b4cfe2a8..a771ec2f 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -1372,62 +1372,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, From 0b405a3ee279d04ea4ee39a5644b54fa61a1836d Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Mon, 18 May 2015 11:12:47 +0200 Subject: [PATCH 05/10] Add some fixes Elrond suggested and doc strings --- mediagoblin/db/models.py | 55 ++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index a771ec2f..9829ff9d 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -95,11 +95,17 @@ class GenericModelReference(Base): # Ensure that everything has it's ID set obj.save(commit=False) - self.obj_pk = obj.id + self.obj_pk = getattr(obj, pk_column) self.model_type = obj.__tablename__ def _get_model_from_type(self, model_type): - """ Gets a model from a tablename (model type) """ + """ + Gets a model from a tablename (model type) + + This currently only works for core models, I've not been able to find + this data built by SQLAlchemy, the closes I found was + Base._metadata.tables but that only gives me a `Table` object. + """ if getattr(self.__class__, "_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 @@ -111,8 +117,28 @@ class GenericModelReference(Base): 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 = GenericModelReference.query.filter_by( + obj_pk=pk, + model_type=model.__tablename__ + ) + + return gmr.first() + class GenericForeignKey(types.TypeDecorator): + """ + GenericForeignKey type used to refer to objects with an integer foreign key. + + This will break referential integrity, only use in places where that is + not important. + """ impl = Integer @@ -133,21 +159,23 @@ class GenericForeignKey(types.TypeDecorator): return gmr.get_object() def process_bind_param(self, value, *args, **kwargs): - """ Save the foreign key """ + """ + Save the foreign key + + There is no mechanism to put a constraint to make this only save foreign + keys to GenericModelReference. The only thing you can do is have this + method which only saves GenericModelReference. + """ if value is None: return None - # Is there one for this already. - model = type(value) - pk = getattr(value, "id") + # Find the GMR if there is one. + gmr = GenericModelReference.find_for_obj(value) - gmr = GenericModelReference.query.filter_by(id=pk).first() + # Create one if there isn't if gmr is None: - # We need to create one - gmr = GenericModelReference( - obj_pk=pk, - model_type=model.__tablename__ - ) + gmr = GenericModelReference() + gmr.set_object(value) gmr.save() return gmr.id @@ -1429,8 +1457,7 @@ MODELS = [ CommentSubscription, ReportBase, CommentReport, MediaReport, UserBan, Privilege, PrivilegeUserAssociation, RequestToken, AccessToken, NonceTimestamp, - Activity, ActivityIntermediator, Generator, - Location, GenericModelReference] + Activity, Generator, Location, GenericModelReference] """ Foundations are the default rows that are created immediately after the tables From 2e4782ef6d0f35b68a3b01c98c1274f8b7fd523e Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Wed, 20 May 2015 12:41:42 +0200 Subject: [PATCH 06/10] More fixed recommended by Elrond This fixes the problem where GenericForeignKey could only be used with models that are in the core of Mediagoblin, it now can be used with any model that SQLAlchemy knows about, including plugins. This also fixes some small bugs caused by incorrect ordering of params into a function. --- mediagoblin/db/models.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index 9829ff9d..30dd80df 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -89,7 +89,7 @@ class GenericModelReference(Base): # Check that the field on the model is a an integer field pk_column = getattr(model, primary_keys[0]) - if issubclass(Integer, pk_column): + if issubclass(pk_column, Integer): raise ValueError("Only models with integer pks can be set") # Ensure that everything has it's ID set @@ -99,20 +99,17 @@ class GenericModelReference(Base): self.model_type = obj.__tablename__ def _get_model_from_type(self, model_type): - """ - Gets a model from a tablename (model type) - - This currently only works for core models, I've not been able to find - this data built by SQLAlchemy, the closes I found was - Base._metadata.tables but that only gives me a `Table` object. - """ - if getattr(self.__class__, "_TYPE_MAP", None) is None: + """ 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 - self._TYPE_MAP = dict(((m.__tablename__, m) for m in MODELS)) + registry = Base._decl_class_registry + self._TYPE_MAP = dict( + ((m.__tablename__, m) for m in six.itervalues(registry)) + ) setattr(self.__class__, "_TYPE_MAP", self._TYPE_MAP) return self.__class__._TYPE_MAP[model_type] @@ -176,7 +173,7 @@ class GenericForeignKey(types.TypeDecorator): if gmr is None: gmr = GenericModelReference() gmr.set_object(value) - gmr.save() + gmr.save(commit=False) return gmr.id From d2256d0b3b6fa03cf7b0911496f891fc4d5f56f7 Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Wed, 20 May 2015 14:45:25 +0200 Subject: [PATCH 07/10] Remove deprecated fields and fix activity creation in tools --- mediagoblin/db/models.py | 9 --------- mediagoblin/tools/federation.py | 4 ++-- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index 30dd80df..d1df1da1 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -284,8 +284,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 @@ -537,8 +535,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 @@ -907,9 +903,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( @@ -990,8 +983,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'), {}) diff --git a/mediagoblin/tools/federation.py b/mediagoblin/tools/federation.py index 6c2d27da..e8d6fc36 100644 --- a/mediagoblin/tools/federation.py +++ b/mediagoblin/tools/federation.py @@ -72,10 +72,10 @@ def create_activity(verb, obj, actor, target=None, generator=None): generator.save() 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 From c1d27aa01939b37c68a7dc44217105c91f95ffbf Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Mon, 25 May 2015 15:53:51 +0200 Subject: [PATCH 08/10] Add a more verbose GenericForeignKey implementation --- mediagoblin/db/migrations.py | 4 +- mediagoblin/db/models.py | 99 +++++++++++++----------------------- 2 files changed, 37 insertions(+), 66 deletions(-) diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py index 70bf6234..1cc4b625 100644 --- a/mediagoblin/db/migrations.py +++ b/mediagoblin/db/migrations.py @@ -1392,8 +1392,8 @@ def rename_and_remove_object_and_target(db): new_target_column = activity_table.columns["temp_target"] # rename them to the old names. - new_object_column.alter(name="object") - new_target_column.alter(name="target") + new_object_column.alter(name="object_id") + new_target_column.alter(name="target_id") # Commit the changes to the database. db.commit() diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index d1df1da1..6fdf0a0f 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -51,9 +51,6 @@ _log = logging.getLogger(__name__) class GenericModelReference(Base): """ Represents a relationship to any model that is defined with a integer pk - - NB: This model should not be used directly but through the GenericForeignKey - field provided. """ __tablename__ = "core__generic_model_reference" @@ -63,6 +60,11 @@ class GenericModelReference(Base): # 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 + __table_args__ = ( + UniqueConstraint("obj_pk", "model_type"), + {}) + 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: @@ -76,7 +78,7 @@ class GenericModelReference(Base): # Check we've been given a object if not issubclass(model, Base): - raise ValueError("Only models can be set as GenericForeignKeys") + 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: @@ -89,13 +91,13 @@ class GenericModelReference(Base): # Check that the field on the model is a an integer field pk_column = getattr(model, primary_keys[0]) - if issubclass(pk_column, Integer): + if not isinstance(pk_column.type, Integer): raise ValueError("Only models with integer pks can be set") - # Ensure that everything has it's ID set - obj.save(commit=False) + if getattr(obj, pk_column.key) is None: + obj.save(commit=False) - self.obj_pk = getattr(obj, pk_column) + self.obj_pk = getattr(obj, pk_column.key) self.model_type = obj.__tablename__ def _get_model_from_type(self, model_type): @@ -121,66 +123,26 @@ class GenericModelReference(Base): model = type(obj) pk = getattr(obj, "id") - gmr = GenericModelReference.query.filter_by( + 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 """ + obj = cls.find_for_obj(obj) -class GenericForeignKey(types.TypeDecorator): - """ - GenericForeignKey type used to refer to objects with an integer foreign key. - - This will break referential integrity, only use in places where that is - not important. - """ - - impl = Integer - - def process_result_value(self, value, *args, **kwargs): - """ Looks up GenericModelReference and model for field """ - # If this hasn't been set yet return None - if value is None: - return None - - # Look up the GenericModelReference for this. - gmr = GenericModelReference.query.filter_by(id=value).first() - - # If it's set to something invalid (i.e. no GMR exists return None) - if gmr is None: - return None - - # Ask the GMR for the corresponding model - return gmr.get_object() - - def process_bind_param(self, value, *args, **kwargs): - """ - Save the foreign key - - There is no mechanism to put a constraint to make this only save foreign - keys to GenericModelReference. The only thing you can do is have this - method which only saves GenericModelReference. - """ - if value is None: - return None - - # Find the GMR if there is one. - gmr = GenericModelReference.find_for_obj(value) - - # Create one if there isn't - if gmr is None: - gmr = GenericModelReference() - gmr.set_object(value) - gmr.save(commit=False) - - return gmr.id - - def _set_parent_with_dispatch(self, parent): - self.parent = parent - + # If there isn't one already create one + if obj is None: + obj = cls( + obj_pk=obj.id, + model_type=type(obj).__tablename__ + ) + return obj class Location(Base): """ Represents a physical location """ @@ -620,6 +582,7 @@ class MediaEntry(Base, MediaEntryMixin): return import_component(self.media_type + '.models:BACKREF_NAME') def __repr__(self): + return "" if six.PY2: # obj.__repr__() should return a str on Python 2 safe_title = self.title.encode('utf-8', 'replace') @@ -1407,10 +1370,18 @@ class Activity(Base, ActivityMixin): generator = Column(Integer, ForeignKey("core__generators.id"), nullable=True) - object = Column(GenericForeignKey(), - nullable=False) - target = Column(GenericForeignKey(), - nullable=True) + + # Create the generic foreign keys for the object + object_id = Column(Integer, ForeignKey(GenericModelReference.id), nullable=False) + object_helper = relationship(GenericModelReference) + 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), nullable=True) + target_helper = relationship(GenericModelReference) + taget = association_proxy("target_helper", "get_target", + creator=GenericModelReference.find_or_new) get_actor = relationship(User, backref=backref("activities", From 2d73983e8ca353f47ad25642e1e0c292d94b20d5 Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Mon, 25 May 2015 17:22:32 +0200 Subject: [PATCH 09/10] Fix some problems with activity mixins and migrations --- mediagoblin/db/migrations.py | 6 +-- mediagoblin/db/mixin.py | 9 ++--- mediagoblin/db/models.py | 27 ++++++------- mediagoblin/tests/test_modelmethods.py | 52 -------------------------- mediagoblin/tools/federation.py | 6 +-- 5 files changed, 24 insertions(+), 76 deletions(-) diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py index 1cc4b625..9ecd6e62 100644 --- a/mediagoblin/db/migrations.py +++ b/mediagoblin/db/migrations.py @@ -36,7 +36,7 @@ from mediagoblin.db.extratypes import JSONEncoded, MutationDict from mediagoblin.db.migration_tools import ( RegisterMigration, inspect_table, replace_table_hack) from mediagoblin.db.models import (MediaEntry, Collection, MediaComment, User, - Privilege, Generator, GenericForeignKey) + Privilege, Generator) from mediagoblin.db.extratypes import JSONEncoded, MutationDict @@ -1289,10 +1289,10 @@ def add_foreign_key_fields(db): activity_table = inspect_table(metadata, "core__activities") # Create column and add to model. - object_column = Column("temp_object", Integer, GenericForeignKey()) + object_column = Column("temp_object", Integer, ForeignKey(GenericModelReference_V0)) object_column.create(activity_table) - target_column = Column("temp_target", Integer, GenericForeignKey()) + target_column = Column("temp_target", Integer, ForeignKey(GenericModelReference_V0)) target_column.create(activity_table) # Commit this to the database 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 6fdf0a0f..054e1677 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -61,8 +61,10 @@ class GenericModelReference(Base): 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("obj_pk", "model_type"), + UniqueConstraint("model_type", "obj_pk"), {}) def get_object(self): @@ -108,11 +110,11 @@ class GenericModelReference(Base): # the class so it can be shared between all instances # to prevent circular imports do import here - registry = Base._decl_class_registry + registry = dict(Base._decl_class_registry).values() self._TYPE_MAP = dict( - ((m.__tablename__, m) for m in six.itervalues(registry)) + ((m.__tablename__, m) for m in registry if hasattr(m, "__tablename__")) ) - setattr(self.__class__, "_TYPE_MAP", self._TYPE_MAP) + setattr(type(self), "_TYPE_MAP", self._TYPE_MAP) return self.__class__._TYPE_MAP[model_type] @@ -133,16 +135,16 @@ class GenericModelReference(Base): @classmethod def find_or_new(cls, obj): """ Finds an existing GMR or creates a new one for the object """ - obj = cls.find_for_obj(obj) + gmr = cls.find_for_obj(obj) # If there isn't one already create one - if obj is None: - obj = cls( + if gmr is None: + gmr = cls( obj_pk=obj.id, model_type=type(obj).__tablename__ ) - return obj + return gmr class Location(Base): """ Represents a physical location """ @@ -582,7 +584,6 @@ class MediaEntry(Base, MediaEntryMixin): return import_component(self.media_type + '.models:BACKREF_NAME') def __repr__(self): - return "" if six.PY2: # obj.__repr__() should return a str on Python 2 safe_title = self.title.encode('utf-8', 'replace') @@ -715,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 @@ -1373,13 +1374,13 @@ class Activity(Base, ActivityMixin): # Create the generic foreign keys for the object object_id = Column(Integer, ForeignKey(GenericModelReference.id), nullable=False) - object_helper = relationship(GenericModelReference) + 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), nullable=True) - target_helper = relationship(GenericModelReference) + 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) 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 e8d6fc36..e7593a92 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): """ From ddc2db746f3291dbe11393a814eed9a0fd651365 Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Wed, 24 Jun 2015 21:22:03 +0200 Subject: [PATCH 10/10] Fix removal of ActivityIntermediatory migration The migration had a problem where other tables still referenced the migration as well as a typo in an earlier migration. They have both been fixed and tested on PostgreSQL and SQLite3. This also fixes a bug where sometimes when creating an activity it'd raise an Exception as the object hadn't got an ID. This has been fixed globally with a fix to the create_activity federation tool. --- mediagoblin/db/migrations.py | 21 +++++++++++++++++++-- mediagoblin/tools/federation.py | 4 ++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py index 9ecd6e62..4f2f8915 100644 --- a/mediagoblin/db/migrations.py +++ b/mediagoblin/db/migrations.py @@ -1289,10 +1289,10 @@ def add_foreign_key_fields(db): activity_table = inspect_table(metadata, "core__activities") # Create column and add to model. - object_column = Column("temp_object", Integer, ForeignKey(GenericModelReference_V0)) + object_column = Column("temp_object", Integer, ForeignKey(GenericModelReference_V0.id)) object_column.create(activity_table) - target_column = Column("temp_target", Integer, ForeignKey(GenericModelReference_V0)) + target_column = Column("temp_target", Integer, ForeignKey(GenericModelReference_V0.id)) target_column.create(activity_table) # Commit this to the database @@ -1406,6 +1406,23 @@ def remove_activityintermediator(db): """ 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() diff --git a/mediagoblin/tools/federation.py b/mediagoblin/tools/federation.py index e7593a92..39b465bf 100644 --- a/mediagoblin/tools/federation.py +++ b/mediagoblin/tools/federation.py @@ -71,6 +71,10 @@ 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.object = obj