Remove statefulness from package extras

* This is part of getting rid of vdm.
* We don't need to store state in PackageExtra any more because you can view the history via activity stream, since it now stores the full package_dict #3972. (The package_extra_revision table will be used to migrate old versions of the package to activity_streams migrate_package_activity.py. But from this version of CKAN, we do not need to record PackageExtra.stateit any more.)
* The mapping between PackageExtra and Package is now a simply dict i.e. attribute_mapped_collection - not a vdm StatefulDict any more.
* vdm's OurAssociationProxy is replaced by the vanilla sqlalchemy association_proxy - I'm not clear why we had to prevent scalars being assigned in the first place since it is a dict mapping, not a scalar mapping. Seems ok.
* package_extras_save() now deletes PackageExtra objects, instead of changing state to 'deleted'.
* package_no_state/extras_list is no longer needed because it was only used by package_extras_save()
* migration deletes extras of state='deleted', otherwise they'd suddenly appear

ckan/lib/dictization/model_save.py

@@ -96,16 +96,14 @@ def package_resource_list_save(res_dicts, package, context):
         resource_list.append(resource)
 
 
-def package_extras_save(extra_dicts, obj, context):
+def package_extras_save(extra_dicts, pkg, context):
     allow_partial_update = context.get("allow_partial_update", False)
     if extra_dicts is None and allow_partial_update:
         return
 
-    model = context["model"]
     session = context["session"]
 
-    extras_list = obj.extras_list
-    old_extras = dict((extra.key, extra) for extra in extras_list)
+    old_extras = pkg._extras
 
     new_extras = {}
     for extra_dict in extra_dicts or []:
@@ -116,28 +114,22 @@ def package_extras_save(extra_dicts, obj, context):
             pass
         else:
             new_extras[extra_dict["key"]] = extra_dict["value"]
+
     #new
     for key in set(new_extras.keys()) - set(old_extras.keys()):
-        state = 'active'
-        extra = model.PackageExtra(state=state, key=key, value=new_extras[key])
-        session.add(extra)
-        extras_list.append(extra)
+        pkg.extras[key] = new_extras[key]
     #changed
     for key in set(new_extras.keys()) & set(old_extras.keys()):
         extra = old_extras[key]
-        if new_extras[key] == extra.value and extra.state != 'deleted':
+        if new_extras[key] == extra.value:
             continue
-        state = 'active'
         extra.value = new_extras[key]
-        extra.state = state
         session.add(extra)
     #deleted
     for key in set(old_extras.keys()) - set(new_extras.keys()):
         extra = old_extras[key]
-        if extra.state == 'deleted':
-            continue
-        state = 'deleted'
-        extra.state = state
+        extra.delete()
+
 
 def package_tag_list_save(tag_dicts, package, context):
     allow_partial_update = context.get("allow_partial_update", False)
@@ -305,7 +297,7 @@ def package_dict_save(pkg_dict, context):
         objects = pkg_dict.get('relationships_as_object')
         relationship_list_save(objects, pkg, 'relationships_as_object', context)
 
-    extras = package_extras_save(pkg_dict.get("extras"), pkg, context)
+    package_extras_save(pkg_dict.get("extras"), pkg, context)
 
     return pkg
 

ckan/migration/versions/088_delete_extras_which_are_deleted_state.py

@@ -0,0 +1,11 @@
+# encoding: utf-8
+
+
+def upgrade(migrate_engine):
+    migrate_engine.execute(
+        '''
+            ALTER TABLE "package_extra_revision"
+              DROP CONSTRAINT package_extra_revision_continuity_id_fkey;
+            DELETE FROM "package_extra" WHERE state='deleted';
+        '''
+    )

ckan/model/package_extra.py

@@ -4,6 +4,7 @@
 import vdm.sqlalchemy
 import vdm.sqlalchemy.stateful
 from sqlalchemy import orm, types, Column, Table, ForeignKey
+from sqlalchemy.ext.associationproxy import association_proxy
 
 import meta
 import core
@@ -58,11 +59,6 @@ def activity_stream_detail(self, activity_id, activity_type):
             cascade='all, delete, delete-orphan',
             ),
         ),
-    'package_no_state': orm.relation(_package.Package,
-        backref=orm.backref('extras_list',
-            cascade='all, delete, delete-orphan',
-            ),
-        )
     },
     order_by=[package_extra_table.c.package_id, package_extra_table.c.key],
     extension=[vdm.sqlalchemy.Revisioner(extra_revision_table),
@@ -79,8 +75,5 @@ def activity_stream_detail(self, activity_id, activity_type):
 def _create_extra(key, value):
     return PackageExtra(key=text_type(key), value=value)
 
-_extras_active = vdm.sqlalchemy.stateful.DeferredProperty('_extras',
-        vdm.sqlalchemy.stateful.StatefulDict, base_modifier=lambda x: x.get_as_of())
-setattr(_package.Package, 'extras_active', _extras_active)
-_package.Package.extras = vdm.sqlalchemy.stateful.OurAssociationProxy('extras_active', 'value',
-            creator=_create_extra)
+_package.Package.extras = association_proxy(
+    '_extras', 'value', creator=_create_extra)

ckan/model/resource.py

@@ -8,7 +8,6 @@
 from sqlalchemy import orm
 from ckan.common import config
 import vdm.sqlalchemy
-import vdm.sqlalchemy.stateful
 from sqlalchemy import types, func, Column, Table, ForeignKey, and_
 
 import meta

ckan/tests/legacy/models/test_extras.py

@@ -5,11 +5,11 @@
 import ckan.model as model
 
 class TestExtras:
-    @classmethod 
+    @classmethod
     def setup_class(self):
         CreateTestData.create()
 
-    @classmethod 
+    @classmethod
     def teardown_class(self):
         model.repo.rebuild_db()
 
@@ -20,45 +20,27 @@ def test_1(self):
 
         rev = model.repo.new_revision()
         pkg._extras[u'country'] = model.PackageExtra(key=u'country', value='us')
-        pkg.extras_active[u'xxx'] = model.PackageExtra(key=u'xxx', value='yyy')
+        pkg.extras[u'xxx'] = u'yyy'
         pkg.extras[u'format'] = u'rdf'
         model.repo.commit_and_remove()
 
         # now test it is saved
         rev1 = model.repo.youngest_revision().id
         samepkg = model.Package.by_name(u'warandpeace')
         assert len(samepkg._extras) == 3, samepkg._extras
-        assert samepkg.extras_active[u'country'].value == 'us', samepkg.extras_active
         assert samepkg.extras[u'country'] == 'us'
         assert samepkg.extras[u'format'] == 'rdf'
         model.Session.remove()
 
-        # now delete and extras
+        # now delete an extra
         samepkg = model.Package.by_name(u'warandpeace')
         model.repo.new_revision()
         del samepkg.extras[u'country']
         model.repo.commit_and_remove()
 
         samepkg = model.Package.by_name(u'warandpeace')
-        assert len(samepkg._extras) == 3
+        assert len(samepkg._extras) == 2
         assert len(samepkg.extras) == 2
         extra = model.Session.query(model.PackageExtra).filter_by(key=u'country').first()
-        assert extra and extra.state == model.State.DELETED, extra
+        assert not extra, extra
         model.Session.remove()
-
-        samepkg = model.Package.by_name(u'warandpeace')
-        samepkg.get_as_of(model.Session.query(model.Revision).get(rev1))
-        assert len(samepkg.extras) == 3, len(samepkg.extras)
-        model.Session.remove()
-
-        # now restore it ...
-        model.repo.new_revision()
-        samepkg = model.Package.by_name(u'warandpeace')
-        samepkg.extras[u'country'] = 'uk'
-        model.repo.commit_and_remove()
-
-        samepkg = model.Package.by_name(u'warandpeace')
-        assert len(samepkg.extras) == 3
-        assert len(samepkg._extras) == 3
-        assert samepkg.extras[u'country'] == 'uk'
-