Issues

Issue #2983 wontfix

Insert deleted object problem with after flush listeners

Konsta Vesterinen
created an issue

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.

Comments (7)

  1. Konsta Vesterinen reporter

    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!'
    
  2. Mike Bayer repo owner

    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.

  3. Mike Bayer repo owner

    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 :)

  4. Mike Bayer repo owner

    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!

  5. Mike Bayer repo owner

    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...

  6. Mike Bayer repo owner

    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.

  7. Konsta Vesterinen reporter

    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.

  8. Log in to comment