PATCH updates the database multiple times, broken hooks

[cburchert]

I found a very weird behaviour when using the SQLAlchemy extension. When I register a function to the on_updated hook, it should receive two arguments, updates and original. However original has the updates applied already. The hook is not helpful for me because of this behaviour, so I traced the cause of the issue. My analysis showed the following: The on_updated event hook is called from eve/methods/patch.py. The variable original is also there in that function and has the correct original data when it is assigned in line 126:

original = get_document(resource, concurrency_check, **lookup)

In line 172 a copy of it is created:

updated = original.copy()

In line 179 it is then updated:

updated.update(updates)

In line 186 then the database is updated:

app.data.update(resource, object_id, updates)

However the copy does not seem to work. Printing dict(original) before and after line 179 shows that its values, which is the database's content, has changed. As these changes are buffered by SQLAlchemy there might not be many side effects, however it still breaks the hooks.

There is an even bigger problem. The copy does not work, because SQLAResult.copy only creates a shallow copy of the object. I patched that function to

class SQLAResult(collections.MutableMapping):
    def copy(self):             
        return dict(self).copy()

Now the database is not updated in Line 179 anymore. However 'original' still refers to what is actually in the database, which is changed by the SQL update in line 186. Therefore the hooks still don't work. Eve exspects the return value of get_document to be detached from the database, which eve_sqlalchemy does not.

I created a basic proof of concept application, which clearly shows the misbehaviour. You can download it here(Can I upload files here?): http://people.ee.ethz.ch/~bconrad/eve-sqlalchemy-bug-poc.tar.gz

[cburchert]

I am trying to figure out a solution. Why exactly do we need the SQLAResult class? Eve exspects results of app.data.find and app.data.find_one to be detached from the database and manipulatable as a dict without consequences, so why don't we just convert the extracted entries to dict and return them?

[cburchert]

I have further looked into the issue. There are many more problems:

1. As the SQLAResult objects don't know about embedding or projection both these features don't work at all.
2. Some requests fail when updating etags because the custom json serializer is not used when calculating etags and SQLAResult objects can't be serialized. (This is also why unit tests fail, an easy way to see the problem).
3. Requests to one resource sometimes return objects of a different resource, when those where loaded during hooks.

In addition to those hooks still don't work, the database is randomly updated when it shouldn't be and there may be many more bugs now and in the future due to eve's ignorance of the fact that every returned object still forwards changes to the database. The design of this extension is seriously broken here.

I wrote a patch which removes the SQLAResult class. It fixes all of the listed issues, also no jsonify function is needed anymore, as dicts are returned, which can simply be serialized.

[amleczko]

Thanks for the #21 I have tested it with one of our in-house project that uses eve-sqlalchemy. There are minor fixes but the general approach is good.