soft_delete is wrong

Bug #1814199 reported by Mike Bayer on 2019-02-01
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
oslo.db
Medium
Matt Riedemann

Bug Description

as seen in https://bugs.launchpad.net/oslo.db/+bug/1814182, oslo_db Query.soft_delete will by design generate lots of warnings like:

    sqlalchemy.exc.SAWarning: Evaluating non-mapped column expression 'updated_at' onto ORM instances; this is a deprecated use case. Please make use of the actual mapped columns in ORM-evaluated UPDATE / DELETE expressions.

this is because the "evaluate" synchronization strategy would like to search for objects and update them based on the UPDATE criteria passed, however the columns given, literal_column('id'), literal_column('updated_at'), are not mapped to anything. the evaluator has to make a guess that the string contained in these expressions should be matched to a mapped attribute on the given entity and this guess was first removed in https://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-b1e620dece39006ab44c47044e9a6fee, then added back in https://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-dff3a469788c81a46440584406cb22be with a warning (likely since oslo.db is invoking it)

the correct implementation should be:

 class Query(sqlalchemy.orm.query.Query):
     """Subclass of sqlalchemy.query with soft_delete() method."""
     def soft_delete(self, synchronize_session='evaluate'):
      entity = self.column_descriptions[0]['entity']
         return self.update({'deleted': entity.id,
                             'updated_at': entity.updated_at,
                             'deleted_at': timeutils.utcnow()},
                            synchronize_session=synchronize_session)

Matt Riedemann (mriedem) wrote :

Thanks Mike, I can confirm that your patch fixes the warning when I patched it into my nova py35 tox environment and re-ran tests where I have the SAWarning setup with a warnings filter to be treated as an error and the test, which was failing before, is passing now.

Matt Riedemann (mriedem) wrote :

My only question is why not just fix this under bug 1814182? This is essentially a duplicate of that bug, right?

Matt Riedemann (mriedem) on 2019-02-01
Changed in oslo.db:
status: New → Confirmed
importance: Undecided → Medium

Fix proposed to branch: master
Review: https://review.openstack.org/634455

Changed in oslo.db:
assignee: nobody → Matt Riedemann (mriedem)
status: Confirmed → In Progress
Mike Bayer (zzzeek) wrote :

ugh, sorry, when I looked at https://bugs.launchpad.net/oslo.db/+bug/1814182 I thought i was looking at a nova bug. maybe I was on my phone or something, not sure but yes this should have been over there.

Reviewed: https://review.openstack.org/634455
Committed: https://git.openstack.org/cgit/openstack/oslo.db/commit/?id=af4b2263e4852de2bd4e66848a18be4d2282bacc
Submitter: Zuul
Branch: master

commit af4b2263e4852de2bd4e66848a18be4d2282bacc
Author: Mike Bayer <email address hidden>
Date: Fri Feb 1 11:53:26 2019 -0500

    Resolve SAWarning in Query.soft_delete()

    We currently see a lot of warnings like this from
    the soft_delete() method:

      sqlalchemy.exc.SAWarning: Evaluating non-mapped column expression
      'updated_at' onto ORM instances; this is a deprecated use case.
      Please make use of the actual mapped columns in ORM-evaluated
      UPDATE / DELETE expressions.

    This is because the "evaluate" synchronization strategy would like
    to search for objects and update them based on the UPDATE criteria
    passed, however the columns given, literal_column('id'),
    literal_column('updated_at'), are not mapped to anything. The
    evaluator has to make a guess that the string contained in these
    expressions should be matched to a mapped attribute on the given
    entity and this guess was first removed in [1], then added back in
    [2] with a warning (likely since oslo.db is invoking it).

    This uses the actual entity-mapped column for the query rather
    than the literal string column.

    [1] https://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-b1e620dece39006ab44c47044e9a6fee
    [2] https://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-dff3a469788c81a46440584406cb22be

    Change-Id: I192e84ce757d12d33085a209dd58d8ea46fb90fb
    Closes-Bug: #1814199

Changed in oslo.db:
status: In Progress → Fix Released

This issue was fixed in the openstack/oslo.db 4.44.0 release.

Reviewed: https://review.openstack.org/641650
Committed: https://git.openstack.org/cgit/openstack/oslo.db/commit/?id=ddc4129a6dfdc16296d16e052c882717a4125611
Submitter: Zuul
Branch: stable/rocky

commit ddc4129a6dfdc16296d16e052c882717a4125611
Author: Mike Bayer <email address hidden>
Date: Fri Feb 1 11:53:26 2019 -0500

    Resolve SAWarning in Query.soft_delete()

    We currently see a lot of warnings like this from
    the soft_delete() method:

      sqlalchemy.exc.SAWarning: Evaluating non-mapped column expression
      'updated_at' onto ORM instances; this is a deprecated use case.
      Please make use of the actual mapped columns in ORM-evaluated
      UPDATE / DELETE expressions.

    This is because the "evaluate" synchronization strategy would like
    to search for objects and update them based on the UPDATE criteria
    passed, however the columns given, literal_column('id'),
    literal_column('updated_at'), are not mapped to anything. The
    evaluator has to make a guess that the string contained in these
    expressions should be matched to a mapped attribute on the given
    entity and this guess was first removed in [1], then added back in
    [2] with a warning (likely since oslo.db is invoking it).

    This uses the actual entity-mapped column for the query rather
    than the literal string column.

    [1] https://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-b1e620dece39006ab44c47044e9a6fee
    [2] https://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-dff3a469788c81a46440584406cb22be

    Change-Id: I192e84ce757d12d33085a209dd58d8ea46fb90fb
    Closes-Bug: #1814199
    (cherry picked from commit af4b2263e4852de2bd4e66848a18be4d2282bacc)

tags: added: in-stable-rocky

This issue was fixed in the openstack/oslo.db 4.40.1 release.

Reviewed: https://review.openstack.org/641724
Committed: https://git.openstack.org/cgit/openstack/oslo.db/commit/?id=c015a89f34f6f3e339439424e0d96002c60ced49
Submitter: Zuul
Branch: stable/queens

commit c015a89f34f6f3e339439424e0d96002c60ced49
Author: Mike Bayer <email address hidden>
Date: Fri Feb 1 11:53:26 2019 -0500

    Resolve SAWarning in Query.soft_delete()

    We currently see a lot of warnings like this from
    the soft_delete() method:

      sqlalchemy.exc.SAWarning: Evaluating non-mapped column expression
      'updated_at' onto ORM instances; this is a deprecated use case.
      Please make use of the actual mapped columns in ORM-evaluated
      UPDATE / DELETE expressions.

    This is because the "evaluate" synchronization strategy would like
    to search for objects and update them based on the UPDATE criteria
    passed, however the columns given, literal_column('id'),
    literal_column('updated_at'), are not mapped to anything. The
    evaluator has to make a guess that the string contained in these
    expressions should be matched to a mapped attribute on the given
    entity and this guess was first removed in [1], then added back in
    [2] with a warning (likely since oslo.db is invoking it).

    This uses the actual entity-mapped column for the query rather
    than the literal string column.

    [1] https://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-b1e620dece39006ab44c47044e9a6fee
    [2] https://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-dff3a469788c81a46440584406cb22be

    Change-Id: I192e84ce757d12d33085a209dd58d8ea46fb90fb
    Closes-Bug: #1814199
    (cherry picked from commit af4b2263e4852de2bd4e66848a18be4d2282bacc)
    (cherry picked from commit ddc4129a6dfdc16296d16e052c882717a4125611)

tags: added: in-stable-queens

This issue was fixed in the openstack/oslo.db 4.33.3 release.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers