[ckan-changes] [ckan/ckan] fef2a2: Upgrade to SQLAlchemy 1.1, no code changes

• Previous message: [ckan-changes] [ckan/ckan] b0a1ed: [#3660] Validate image URL in resource form
• Next message: [ckan-changes] [ckan/ckan] 8e194f: fix radio button PR
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

  Branch: refs/heads/master
  Home:   https://github.com/ckan/ckan
  Commit: fef2a26902a707604eacf85aadc2c654d2ec4b7a
      https://github.com/ckan/ckan/commit/fef2a26902a707604eacf85aadc2c654d2ec4b7a
  Author: amercader <amercadero at gmail.com>
  Date:   2017-06-15 (Thu, 15 Jun 2017)

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

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


  Commit: 3a6015a60644fb7ae18644b0537604f4552fae4d
      https://github.com/ckan/ckan/commit/3a6015a60644fb7ae18644b0537604f4552fae4d
  Author: amercader <amercadero at gmail.com>
  Date:   2017-06-15 (Thu, 15 Jun 2017)

  Changed paths:
    M ckan/migration/versions/008_update_vdm_ids.py
    M ckan/migration/versions/016_uuids_everywhere.py

  Log Message:
  -----------
  Update migration scripts to SA 1.1

Basically take into account that ForeignKeyConstraint.columns is now a
ColumnCollection (ie a dict) and not a list.

http://docs.sqlalchemy.org/en/latest/changelog/migration_10.html#foreignkeyconstraint-columns-is-now-a-columncollection


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

  Changed paths:
    M ckan/logic/action/create.py

  Log Message:
  -----------
  Explicitly assign group to new member object


  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


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

  Changed paths:
    M ckan/model/activity.py
    M ckan/model/group.py
    M ckan/model/tracking.py

  Log Message:
  -----------
  Supress SQLA warnings by wrapping textual statements with text()

See
http://docs.sqlalchemy.org/en/latest/changelog/migration_10.html#warnings-emitted-when-coercing-full-sql-fragments-into-text


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

  Changed paths:
    M ckanext/datastore/backend/postgres.py

  Log Message:
  -----------
  Improve already existing trigger function check

The exception raised when a trigger function already exists by
SQLAlchemy has changed from:

    (ProgrammingError) function "test_redefined" already exists
    with same argument types

to

    (psycopg2.ProgrammingError) function "test_redefined" already exists
    with same argument types

Updated the check to not rely on the exception type but rather on the
actual message.


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

  Changed paths:
    M requirements.in
    M requirements.txt

  Log Message:
  -----------
  Link to new version of VDM


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

  Changed paths:
    M requirements.in
    M requirements.txt

  Log Message:
  -----------
  Bump to new SA version


  Commit: dafa45b3ada963463e50760ef86d3f57624fabb0
      https://github.com/ckan/ckan/commit/dafa45b3ada963463e50760ef86d3f57624fabb0
  Author: amercader <amercadero at gmail.com>
  Date:   2017-07-05 (Wed, 05 Jul 2017)

  Changed paths:
    M ckan/migration/versions/016_uuids_everywhere.py

  Log Message:
  -----------
  Try to remove all sequences on migration script

They might have been created on earlir versions of the migration. Raise
any other exception.


  Commit: de4e56284f4f241f65ae15a58c259baa7d40bea4
      https://github.com/ckan/ckan/commit/de4e56284f4f241f65ae15a58c259baa7d40bea4
  Author: Ian Ward <ian at excess.org>
  Date:   2017-07-06 (Thu, 06 Jul 2017)

  Changed paths:
    M ckan/lib/activity_streams_session_extension.py
    M ckan/logic/action/create.py
    M ckan/migration/versions/008_update_vdm_ids.py
    M ckan/migration/versions/016_uuids_everywhere.py
    M ckan/model/activity.py
    M ckan/model/group.py
    M ckan/model/tracking.py
    M ckanext/datastore/backend/postgres.py
    M requirements.in
    M requirements.txt

  Log Message:
  -----------
  Merge pull request #3646 from ckan/upgrade-sqlalchemy-to-1.1

Upgrade SQLAlchemy to 1.1


Compare: https://github.com/ckan/ckan/compare/853f40de67bc...de4e56284f4f

• Previous message: [ckan-changes] [ckan/ckan] b0a1ed: [#3660] Validate image URL in resource form
• Next message: [ckan-changes] [ckan/ckan] 8e194f: fix radio button PR
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]