[ckan-changes] [ckan/ckan] acf395: Revert "Upgrade to SQLAlchemy 1.1, no code changes...

Mon Jun 26 14:15:11 UTC 2017

  Branch: refs/heads/upgrade-sqlalchemy-to-1.1
  Home:   https://github.com/ckan/ckan
  Commit: acf395dc6774136b886e103d621ffb0ab2ddf9d5
      https://github.com/ckan/ckan/commit/acf395dc6774136b886e103d621ffb0ab2ddf9d5
  Author: amercader <amercadero at gmail.com>
  Date:   2017-06-26 (Mon, 26 Jun 2017)

  Changed paths:
    M ckan/tests/lib/test_jobs.py
    M ckanext/datastore/plugin.py

  Log Message:
  -----------
  Revert "Upgrade to SQLAlchemy 1.1, no code changes"

This reverts commit fef2a26902a707604eacf85aadc2c654d2ec4b7a.

  Commit: 26dd7ada5c964842cb39cad2e3d326ef5949c887
      https://github.com/ckan/ckan/commit/26dd7ada5c964842cb39cad2e3d326ef5949c887
  Author: amercader <amercadero at gmail.com>
  Date:   2017-06-26 (Mon, 26 Jun 2017)

  Changed paths:
    M ckan/lib/activity_streams_session_extension.py

  Log Message:
  -----------
  Don't emit two activity details for changes in the same object

This one took a while to debug so bear with me...

First symptom that something was not right was this test failure:

```
nosetests --with-pylons=test-core-custom.ini ckan/tests/legacy/functional/api/test_activity.py:TestActivity.test_create_package

======================================================================
FAIL: Test new package activity stream.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/adria/dev/pyenvs/ckan/local/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/adria/dev/pyenvs/ckan/src/ckan/ckan/tests/legacy/functional/api/test_activity.py", line 1382, in test_create_package
    self._create_package(user=self.normal_user)
  File "/home/adria/dev/pyenvs/ckan/src/ckan/ckan/tests/legacy/functional/api/test_activity.py", line 365, in _create_package
    assert len(details) == 5, "There should be five activity details."
AssertionError: There should be five activity details.

----------------------------------------------------------------------
Ran 1 test in 11.488s

FAILED (failures=1)
```

There was an extra activity detail created. More specifically there was a
new activity detail of type "changed" created for the Package creation, in
addition to the "new" one.

Activity details for packages are created using an SQLAlchemy session
extension in `ckan/lib/activity_streams_session_extension.py` [1] (btw these
extensions have been deprecated for a while and need to be replaced with
[events](http://docs.sqlalchemy.org/en/latest/orm/events.html#sqlalchemy.orm.events.SessionEvents)).

This extension implements the `before_commit` hook and uses `Session._object_cache`
to decide which activities and activity details are created. After upgrading to
SQLAlchemy 1.1 there is an extra item in `Session._object_cache['changed']`.

`Session._object_cache` is created in another SQLAlchemy `SessionExtension`
in `ckan/model/meta.py` [2], this time listening to `before_flush` events.
The actual code that decides whether an object has changed is this one [3]:

```
changed = [obj for obj in session.dirty if
     session.is_modified(obj, include_collections=False, passive=True)]
```

After the upgrade to SA 1.1 `session.is_modified()` returns True for the package
object on the flush triggered by the final commit in `package_create`. This
indicates that between the moment the package object was created [4] and the
final commit [5] the package object was modified in the SA Session. This sounds
perfectly plausible as this period includes setting resource ids, owner org,
calling extensions etc.

Rather than going even deeper down the rabbit hole, I'm proposing an easy fix on the
session extension that creates the activities. If there is an item recorded
on the object cache for the creation of an object *and also* for the update of
the same object in the same transaction just create an activity for the creation.

This seems like the intended behaviour. There seemed to be some checks in place to
account for this but obviously they need to be more thorough.

[1] https://github.com/ckan/ckan/blob/a77ff9c021c80eee949d2ed11cbafd743ee5c04a/ckan/lib/activity_streams_session_extension.py#L44
[2] https://github.com/ckan/ckan/blob/a77ff9c021c80eee949d2ed11cbafd743ee5c04a/ckan/model/meta.py#L56
[3] https://github.com/ckan/ckan/blob/a77ff9c021c80eee949d2ed11cbafd743ee5c04a/ckan/model/meta.py#L64:L65
[4] https://github.com/ckan/ckan/blob/a77ff9c021c80eee949d2ed11cbafd743ee5c04a/ckan/logic/action/create.py#L187
[5] https://github.com/ckan/ckan/blob/a77ff9c021c80eee949d2ed11cbafd743ee5c04a/ckan/logic/action/create.py#L221

Compare: https://github.com/ckan/ckan/compare/684a7c67fb52...26dd7ada5c96