Resource extras can not be updated #2158

wardi · 2014-12-22T16:38:51Z

ckanapi action package_create name=testres
ckanapi action resource_create package_id=testres url=example newfield=first

^ this creates a new package with one resource including one extra "newfield"

In the database tables resource and resource_revision both contain:

   url   |        extras         
---------+-----------------------
 example | {"newfield": "first"}

ckanapi action resource_update id=<resource id> url=changed-url newfield=changed-newfield

^ this successfully updates the url, but the newfield extra remains unchanged.

In the database we see in resource (newfield was not updated):

     url     |        extras         
-------------+-----------------------
 changed-url | {"newfield": "first"}

But in resource_revision the correct data is stored (new revision on top is correct):

     url     |              extras              
-------------+----------------------------------
 changed-url | {"newfield": "changed-newfield"}
 example     | {"newfield": "first"}

Maybe vdm-related? There is also a code in package_update that avoids creating revisions that could be the cause.

The text was updated successfully, but these errors were encountered:

wardi · 2014-12-22T18:19:42Z

@rossjones git bisect fingers f79a20a, but 699d226 (its parent) seems to be broken too in a slightly different way: I need to update a non-extra field along with the extra for the resource extras to be updated

rossjones · 2014-12-22T20:14:17Z

Our tests suck. We need to fix this. I'll take a look when I am back off holiday if you want to assign to me.

I sort of hoped @davidread's recent fix for resource updates might be responsible, but if bisect is suggesting this commit ....

wardi · 2014-12-22T21:15:08Z

that second problem seems to start at either fea679e or 41f80ac, maybe as a result of the merge. The merge commit seems to be broken so I can't test it.

davidread · 2014-12-23T11:53:29Z

I've started this ^^ PR with some tests. The extra is written to the session, but it is prevented from being flushed due to the CkanSessionExtension thinking there is nothing changed, as session.is_modified is coming back as False for some reason.

davidread · 2014-12-23T12:23:22Z

I've been looking at the second problem - "I need to update a non-extra field along with the extra for the resource extras to be updated" - and it seems to be a problem that we've perhaps always had. The model test fails on CKAN 2.1 (August 2013).

davidread · 2014-12-23T12:50:42Z

I've added a test for the first issue - changing url and the extra only saves the new url. Here's the tests, hacked to work on old versions of ckan too:

https://gist.github.com/davidread/5a229795dada6681ed0c

benjaminlaot · 2015-01-12T14:17:28Z

Hi @wardi !
Can you give me a lead, so I can work on the problem on my side?

clementmouchet · 2015-01-15T13:34:21Z

Hi @wardi, @rossjones & and @davidread could you give us a lead to this fix?

clementmouchet · 2015-02-03T17:12:53Z

Sorry to chase you again on that @wardi, I could spend some time trying to fix this but so far I've not even been able to put my finger on the actual cause of the problem! Could you give us a lead?

wardi · 2015-02-03T17:24:11Z

@clementmouchet sorry, I don't understand. It looks like this problem or one like it has existed for a very long time so bisecting isn't helping. @davidread has created a test that reproduces the problem so the next thing I would do is step in to the failing test with a debugger.

amercader · 2015-02-16T17:46:32Z

Trying to wrap my head around this, one thing that seems suspicious is that we are using the JsonDictType for the extras field, which seems to be a variation of this example on the SQLAlchemy docs. In there it says:

Note that the ORM by default will not detect “mutability” on such a type - meaning, in-place changes to values will not be detected and will not be flushed. Without further steps, you instead would need to replace the existing value with a new one on each parent object to detect changes. Note that there’s nothing wrong with this, as many applications may not require that the values are ever mutated once created. For those which do have this requirement, support for mutability is best applied using the sqlalchemy.ext.mutable extension - see the example in Mutation Tracking.

wardi · 2015-02-16T17:51:47Z

@amercader ok, try res.extras = res.extras just before saving

amercader · 2015-02-16T19:45:13Z

@wardi you mean here?

diff --git a/ckan/lib/dictization/model_save.py b/ckan/lib/dictization/model_save.py
index 4ba4788..878389e 100644
--- a/ckan/lib/dictization/model_save.py
+++ b/ckan/lib/dictization/model_save.py
@@ -50,7 +50,7 @@ def resource_dict_save(res_dict, context):
         del obj.extras[delete_me]

     obj.state = u'active'
-
+    obj.extras = obj.extras
     session.add(obj)
     return obj

I doesn't seem to make much difference but I see where you are coming from

wardi · 2015-02-16T19:55:43Z

Yeah, that what I was thinking -- maybe there's some sneaky code that checking if the thing you're assigning has changed. Could always go deeper: Assign some other object first, and/or pass to dict() before assigning.

Or better yet let's just not treat .extras as mutable in the code that uses it, because that doesn't work.

Resource `extras` were being ignored on `resource_udpate`. The rest of the resource field got updated but no the extras. The resource `extras` field is using a custom `JsonDictType`, and according to SQLAlchemy documentation: > Note that the ORM by default will not detect “mutability” on such a > type - meaning, in-place changes to values will not be detected and > will not be flushed. Without further steps, you instead would need to > replace the existing value with a new one on each parent object to > detect changes. Replacing `obj.extras` with a new dict solves the issue http://docs.sqlalchemy.org/en/rel_0_9/core/custom_types.html#marshal-json-strings

amercader · 2015-02-17T09:48:49Z

Replacing the extras with a brand new dict did the trick and solved the two issues. Fix is on @davidread's branch: #2159

We need to assign a new extras dict so the change is detected

benjaminlaot · 2015-02-17T16:37:51Z

Nice job !!

wardi · 2015-02-18T14:30:39Z

fixed in #2159

Resource `extras` were being ignored on `resource_udpate`. The rest of the resource field got updated but no the extras. The resource `extras` field is using a custom `JsonDictType`, and according to SQLAlchemy documentation: > Note that the ORM by default will not detect “mutability” on such a > type - meaning, in-place changes to values will not be detected and > will not be flushed. Without further steps, you instead would need to > replace the existing value with a new one on each parent object to > detect changes. Replacing `obj.extras` with a new dict solves the issue http://docs.sqlalchemy.org/en/rel_0_9/core/custom_types.html#marshal-json-strings Conflicts: ckan/lib/dictization/model_save.py

Resource `extras` were being ignored on `resource_udpate`. The rest of the resource field got updated but no the extras. The resource `extras` field is using a custom `JsonDictType`, and according to SQLAlchemy documentation: > Note that the ORM by default will not detect “mutability” on such a > type - meaning, in-place changes to values will not be detected and > will not be flushed. Without further steps, you instead would need to > replace the existing value with a new one on each parent object to > detect changes. Replacing `obj.extras` with a new dict solves the issue http://docs.sqlalchemy.org/en/rel_0_9/core/custom_types.html#marshal-json-strings

We need to assign a new extras dict so the change is detected

* release-v2.2.2: (37 commits) Update version number for release 2.2.2 Rebuild frontend [ckan#2107] Don't create a source link if not a valid url [ckan#2037] PEP8 [ckan#2037] Cherry-pick functional test for dataset creation [ckan#2037] Fix exception on resource creation [ckan#1909] phix phantomjs peerinvalid phailure Update changelog ahead of 2.2.2 release [ckan#2320] Don't assume resource format is there [ckan#2324] Don't "normalize" resource URL in recline view [ckan#2319] Clean up field names before rendering the table Compile updated translation files Update translations from Transifex [ckan#2283] Pack template_legacy resources on python setup.py install [ckan#2158] Replace all resource extras on update so they don't get lost [ckan#2044] Fix datastore docs link [ckan#2272] Fix docs that say Celery will be be installed [ckan#1647] Stats extension. 'Largest Groups' links lead either to organizations or to groups page [ckan#1649] Stats extension. Changed tests [ckan#1649] Stats extension. Tag counter works properly ...

…f5f161.

Fixes ckanext-dgu#275 using fix from ckan master ckan#2158

wardi mentioned this issue Dec 22, 2014

Custom fields do not get updated when editing a resource ckan/ckanext-scheming#17

Closed

davidread mentioned this issue Dec 23, 2014

#2158 Resource extras can not be updated #2159

Merged

wardi self-assigned this Jan 6, 2015

wardi mentioned this issue Feb 16, 2015

resource_update does not handle free-form resource extras #2290

Closed

amercader added a commit that referenced this issue Feb 17, 2015

[#2158] Add more tests

d1cacbb

amercader added a commit that referenced this issue Feb 17, 2015

[#2158] Remove print

3a80b47

amercader added a commit that referenced this issue Feb 17, 2015

[#2158] Fix model test

dbc2908

We need to assign a new extras dict so the change is detected

wardi closed this as completed Feb 18, 2015

amercader added a commit that referenced this issue Feb 18, 2015

[#2158] Add more tests

5bbd58c

amercader added a commit that referenced this issue Feb 18, 2015

[#2158] Remove print

a037826

amercader added a commit that referenced this issue Feb 18, 2015

[#2158] Fix model test

fd61c83

We need to assign a new extras dict so the change is detected

davidread mentioned this issue Dec 18, 2015

Time series form bug datagovuk/ckanext-dgu#275

Closed

davidread pushed a commit to datagovuk/ckan that referenced this issue Dec 18, 2015

Fixes ckanext-dgu#275 using fix from ckan master ckan#2158 - commit 8…

93ca22c

…f5f161.

rossjones added a commit to datagovuk/ckan that referenced this issue Dec 21, 2015

Merge pull request #17 from datagovuk/resource-date-change-lost

dadeea2

Fixes ckanext-dgu#275 using fix from ckan master ckan#2158

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource extras can not be updated #2158

Resource extras can not be updated #2158

wardi commented Dec 22, 2014

wardi commented Dec 22, 2014

rossjones commented Dec 22, 2014

wardi commented Dec 22, 2014

davidread commented Dec 23, 2014

davidread commented Dec 23, 2014

davidread commented Dec 23, 2014

benjaminlaot commented Jan 12, 2015

clementmouchet commented Jan 15, 2015

clementmouchet commented Feb 3, 2015

wardi commented Feb 3, 2015

amercader commented Feb 16, 2015

wardi commented Feb 16, 2015

amercader commented Feb 16, 2015

wardi commented Feb 16, 2015

amercader commented Feb 17, 2015

benjaminlaot commented Feb 17, 2015

wardi commented Feb 18, 2015

Resource extras can not be updated #2158

Resource extras can not be updated #2158

Comments

wardi commented Dec 22, 2014

wardi commented Dec 22, 2014

rossjones commented Dec 22, 2014

wardi commented Dec 22, 2014

davidread commented Dec 23, 2014

davidread commented Dec 23, 2014

davidread commented Dec 23, 2014

benjaminlaot commented Jan 12, 2015

clementmouchet commented Jan 15, 2015

clementmouchet commented Feb 3, 2015

wardi commented Feb 3, 2015

amercader commented Feb 16, 2015

wardi commented Feb 16, 2015

amercader commented Feb 16, 2015

wardi commented Feb 16, 2015

amercader commented Feb 17, 2015

benjaminlaot commented Feb 17, 2015

wardi commented Feb 18, 2015