passive_deletes='all' is not complete

[Mike Bayer]

Probably has to wait until 1.2 because this is a behavior change, but passive_deletes is not skipping setting the FK to NULL in all cases:

from sqlalchemy import Column, Integer, String, ForeignKey, Boolean
from sqlalchemy import create_engine, and_
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import relationship
from sqlalchemy.orm import sessionmaker
from sqlalchemy import event
from sqlalchemy import inspect

Base = declarative_base()
engine = create_engine('sqlite:///:memory:', echo=True)
Session = sessionmaker(bind=engine)


class User(Base):
    __tablename__ = 'users'

    id = Column(Integer, primary_key=True)
    address = relationship(
        "Address",
        uselist=False,
        passive_deletes="all")


class Address(Base):
    __tablename__ = 'addresses'
    id = Column(Integer, primary_key=True)
    email = Column(String, nullable=False)
    deleted = Column(Boolean, nullable=False, default=False)

    user_id = Column(Integer, ForeignKey('users.id'))
    user = relationship("User")

Base.metadata.create_all(engine)
sess = Session()

a1 = Address(email='foo')
u = User(id=1, address=a1)
sess.add_all([u, a1])
sess.commit()

u.address = Address(email='bar')
sess.commit()

assert a1.user_id == 1, a1.user_id

because we have a sync to None not checking it, needs this:

diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py
index a3e5b12..f2193b6 100644
--- a/lib/sqlalchemy/orm/dependency.py
+++ b/lib/sqlalchemy/orm/dependency.py
@@ -553,7 +553,8 @@ class OneToManyDP(DependencyProcessor):

                 for child in history.deleted:
                     if not self.cascade.delete_orphan and \
-                            not self.hasparent(child):
+                            not self.hasparent(child) and \
+                            not self.passive_deletes == 'all':
                         self._synchronize(state, child, None, True,
                                           uowcommit, False)

check if this bug is a dupe b.c. this seems kind of obvious, also check for any other synchronize(.. None) that is being allowed. review the docs for passive_deletes to make sure I'm not misunderstanding the intent.

making this critical so it remains noticed.

[Mike Bayer]

so I think this bug is invalid. I can't think of a reason you'd not want to set NULL when the child object is de-associated from the parent. the rationale for "passive_deletes="all"' was so that a referential integrity error may be allowed to raise. this does not occur when the child object is being de-associated whether or not the value changes; for the case of removing an element from a collection, the patch above indicates that nothing should happen, which is definitely not what people would want.

[Mike Bayer]

see acf64c4178169b765f3f7ae492b1774955cf541f where we test that the current behavior remains

[Yegor Roganov]

@zzzeek

The obvious application of passive_deletes="all" not de-associating from the parent is soft deletes.

Here is a complete test which I believe should pass, otherwise the passive_deletes="all" option is kind of misleading.

from sqlalchemy import Column, Integer, ForeignKey, Boolean
from sqlalchemy import create_engine, and_
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import relationship
from sqlalchemy.orm import sessionmaker
from sqlalchemy import event

Base = declarative_base()
engine = create_engine('sqlite:///:memory:', echo=True)
Session = sessionmaker(bind=engine)


class Address(Base):
    __tablename__ = 'addresses'
    id = Column(Integer, primary_key=True)
    deleted = Column(Boolean, nullable=False, default=False)

    user_id = Column(Integer, ForeignKey('users.id'))
    user = relationship("User")


class User(Base):
    __tablename__ = 'users'

    id = Column(Integer, primary_key=True)
    addresses = relationship(
        "Address",
        primaryjoin=and_(id==Address.user_id, ~Address.deleted),
        passive_deletes="all",
    )


@event.listens_for(User.addresses, 'remove')
def delete_address(target, value, initiator):
    value.deleted = True


engine = create_engine('sqlite:///:memory:', echo=True)
Base.metadata.create_all(engine)
session = sessionmaker(bind=engine)()

u = User(addresses=[Address()])

session.add(u)
session.commit()
a = u.addresses[0]
assert a.user_id == u.id
u.addresses = []
session.commit()
assert a.deleted
assert a.user_id == u.id, (a.user_id, u.id)

[Mike Bayer]

unfortunately that use case is totally different than the original use case for passive_deletes="all", which was for users who wanted integrity errors to raise when the parent object is deleted. for those users, removal of an object suddenly doing nothing would be a disaster.

the case for "soft-deletes" should most likely be handled using an entirely new feature, likely an event hook (or modification to before_delete()) that intercepts all deletes in such a way that any DELETE can be turned into whatever the user wants. That way the ORM continues to see a delete as a delete and you'd put "cascade='all, delete-orphan'" here normally.

that is, a feature that silently removes a behavior when selected is less preferable than a feature that adds a behavior when selected.

[Mike Bayer]

also you don't need passive_deletes='all' for this (and really, we don't need passive_deletes='all', this should likely be deprecated b.c. it is so ambiguous):

from sqlalchemy import inspect


@event.listens_for(Address, "before_update")
def before_address_update(mapper, connection, obj):
    obj.deleted = True
    obj.user_id = inspect(obj).attrs.user_id.history.deleted[0]

the ORM's rules should be simple and event hooks should be made effective enough that its rules can be modified, so I'd still favor some kind of all-encompassing "this is the soft delete hook", so that it handles session.delete() and everything else.

[Mike Bayer]

see #4004

[Yegor Roganov]

the case for "soft-deletes" should most likely be handled using an entirely new feature, likely an event hook (or modification to before_delete()) that intercepts all deletes in such a way that any DELETE can be turned into whatever the user wants. That way the ORM continues to see a delete as a delete and you'd put "cascade='all, delete-orphan'" here normally.

If I could have soft deletes with session.delete, that would simplify things a lot.