issue modified_event after attribute extensions

[Mike Bayer]

patch attached.

[Former user]

Test file showing the problem.

[Mike Bayer]

with the patch, the test fails differently:

sqlalchemy.orm.exc.ConcurrentModificationError: Updated rowcount 0 does not match number of objects updated 1

workaround is to call session.query(...).autoflush(False) when querying inside of an AttributeExtension.

[Mike Bayer]

OK if I fix the test as below, I can get the patch to resolve this particular issue.

f = Foo()
session.add(f)
session.flush()
a = Bar(Id=f.Id)

[Mike Bayer]

here's a test case that fails regardless of the patch, when flush() is called within an extension:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()

engine = create_engine('sqlite:///:memory:', echo = False)

session = sessionmaker(engine)()

class AttrEx(AttributeExtension):
    def set(self, state, value, oldvalue, initiator):
        session.flush()
        return value

class Foo(Base):
    id = Column( Integer, primary_key=True)
    bar_id = Column(Integer, ForeignKey('bar.id'))
    bar = relation("Bar", cascade="all, delete-orphan", extension=AttrEx(), single_parent=True)

    __tablename__ = 'foo'

class Bar(Base):
    id = Column( Integer, primary_key=True)
    __tablename__ = 'bar'

Base.metadata.create_all(engine)

f1 = Foo()
b1 = Bar()
b2 = Bar()
session.add_all([b1, b2](f1,))

f1.bar = b1
f1.bar = b2

session.commit()

[Former user]

Yes.. Filip's attributeextension_flush.py have a "bug".. It should be:

class Bar(Foo):

instead of

class Bar(Base):

anyway... We finally upgraded our project to SQLA 0.6, tested (couple of hundreds SQLA specific tests) with our patch and still looks pretty solid.

I think that flushing inside AttributeExtension is another (more complex) problem and should be handled as a separate ticket.

[Mike Bayer]

from my research, the move of modified_event maintains the same state even for an attribute extension that calls get_history(), so in that regard the patch is harmless and is committed in 8a282a9b60dabd86bb16b6241055a619d61dc09b.

However, even in the rudimentary tests I added in that changeset, another case was revealed where calling commit inside the extension, which is the essential result of calling flush(), produces a different end state than if commit were not called.

The basic fact is your current strategy of allowing flush() to be called inside of an extension is never going to work completely, and will result in silent failures.

The real issue here is that AttributeExtension is yet another example of a generally useful capability that was very easy to open up to the public and soon led to API abuse - it wasn't designed from the start to suit end-user use cases.

What you're looking for is a system of instrumenting the outside of the operation.

One way to accomplish this is with a monkeypatch approach. this can be accomplished using a variant of the example in source:sqlalchemy/trunk/examples/custom_attributes/listen_for_events.py. But that's dependent on the structure of AttributeImpl and is still pretty brittle.

a feature add to SQLAlchemy would be a helper object that provides a generic AttributeProxy so that the monkeypatch step can be averted. But this is all a mess since its still tightly linked to internal API.

Alternatively, adding a whole new AttributeExtension hook to all attributes then adds latency + complexity to the internals, complexity to the API, for a feature that almost nobody would ever need.

The short answer is, don't call flush() in an attribute extension. For more involved business rules, the recommended pattern is http://www.sqlalchemy.org/docs/05/mappers.html#using-descriptors . If you have end-users using an object relational system via API hooks, its just a basic fact that they're going to require developmental guidelines.

[Mike Bayer]

Removing milestone: 0.6.0 (automated comment)