Insert deleted object problem with after flush listeners

[Konsta Vesterinen]

My guess is this problem is related to: https://bitbucket.org/zzzeek/sqlalchemy/issue/2501/the-delete-before-insert-problem

Failing test case:

import sqlalchemy as sa


dns = 'sqlite:///:memory:'

engine = sa.create_engine(dns)
engine.echo = True
connection = engine.connect()
Base = sa.ext.declarative.declarative_base()


class ModelA(Base):
    __tablename__ = 'a'

    id = sa.Column(sa.Integer, autoincrement=True, primary_key=True)
    name = sa.Column(sa.Unicode(255), nullable=False)


class ModelB(Base):
    __tablename__ = 'b'

    id = sa.Column(sa.Integer, autoincrement=True, primary_key=True)
    name = sa.Column(sa.Unicode(255), nullable=False)

Base.metadata.create_all(connection)

Session = sa.orm.sessionmaker(bind=connection)
session = Session()


@sa.event.listens_for(sa.orm.session.Session, 'after_flush')
def after_flush(session, flush_context):
    for obj in session:
        if not isinstance(obj, ModelA):
            continue
        b = session.query(ModelB).get(obj.id)
        if not b:
            b = ModelB(id=obj.id, name=u'b')
            session.add(b)
        else:
            b.name = u'updated b!'

a = ModelA(name=u'A')
session.add(a)
session.flush()
session.delete(a)
session.flush()
session.add(ModelA(id=a.id, name=u'A'))
session.commit()
b = session.query(ModelB).first()
assert b.name == u'updated b!'

This also throws a warning (which I think it should not throw): SAWarning: Attribute history events accumulated on 1 previously clean instances within inner-flush event handlers have been reset, and will not result in database updates. Consider using set_committed_value() within inner-flush event handlers to avoid this warning.

[Konsta Vesterinen]

Actually the problem isn't with insert deleted object at all. It seems the problem is more simpler and within after_flush listener handling. Updated test case:

import sqlalchemy as sa


dns = 'sqlite:///:memory:'

engine = sa.create_engine(dns)
engine.echo = True
connection = engine.connect()
Base = sa.ext.declarative.declarative_base()


class ModelA(Base):
    __tablename__ = 'a'

    id = sa.Column(sa.Integer, autoincrement=True, primary_key=True)
    name = sa.Column(sa.Unicode(255), nullable=False)


class ModelB(Base):
    __tablename__ = 'b'

    id = sa.Column(sa.Integer, autoincrement=True, primary_key=True)
    name = sa.Column(sa.Unicode(255), nullable=False)

Base.metadata.create_all(connection)

Session = sa.orm.sessionmaker(bind=connection)
session = Session()


@sa.event.listens_for(sa.orm.session.Session, 'after_flush')
def after_flush(session, flush_context):
    for obj in session:
        if not isinstance(obj, ModelA):
            continue
        b = session.query(ModelB).get(obj.id)
        if not b:
            b = ModelB(id=obj.id, name=u'b')
            session.add(b)
        else:
            b.name = u'updated b!'

a = ModelA(name=u'A')
session.add(a)
session.flush()
a.name = u'Updated A'
session.flush()
session.commit()
b = session.query(ModelB).first()
assert b.name == u'updated b!'

[Mike Bayer]

the issue is that in after_flush() there, the flush is done, and now the Session is about to sweep through all of its state and reset everything to "pristine", meaning, it's going to blow away your change there. after_flush() can't be used to establish changes like that, they get erased.

[Mike Bayer]

so the warning there is exactly how we responded to the last time someone reported this as an issue :). basically you're reporting the fix for the bug, as the bug :)

[Mike Bayer]

There's an easy way to do this kind of thing that avoids any kind of problem and i dont otherwise see any change to be made, so just going to close this with the recipe below:

@sa.event.listens_for(sa.orm.session.Session, 'after_flush')
def after_flush(session, flush_context):
    s2 = Session(bind=session.connection())
    for obj in session:
        if not isinstance(obj, ModelA):
            continue

        b = s2.query(ModelB).get(obj.id)
        if not b:
            b = ModelB(id=obj.id, name=u'b')
            s2.add(b)
        else:
            b.name = u'updated b!'
    s2.flush()
    session.query(ModelB).merge_result(s2.identity_map.values(), load=False)

if you still want to suggest a change to the behavior here then just reopen. thanks!

[Mike Bayer]

yeah in that second case, the B is part of the flush context already, so when you say "b.name = 'x'", that change just gets blown away, but we don't have any way to know that the particular change was after the flush, as opposed to beforehand. I guess a more bulletproof way to catch it would be to place some kind of "guard" on identity_map._modified within the post-flush period. Seems a little overkill to me though anytime I work with other libraries' extension points, things just either work or they don't...

[Mike Bayer]

yah there's not a good way to get a "guard" in there without adding a whole lot more overhead to the ORM, when a state that already has changes is modified again, it doesn't check into anything in order to save on overhead. just accessing the state's owner session involves a weakref.

[Konsta Vesterinen]

Ok, thanks a lot Mike! This clarifies the issue for me. I've been re-implementing SQLAlchemy-Continuum's transaction handling. I have a nice plugin architecture coming up for it and banged my head on the wall with this one.