[Sqlalchemy-tickets] Issue #4134: `AttributeEvents.remove` has inconsistent timing (zzzeek/sqlalchemy)

[Jack Z. <iss...@bi...>]

New issue 4134: `AttributeEvents.remove` has inconsistent timing
https://bitbucket.org/zzzeek/sqlalchemy/issues/4134/attributeeventsremove-has-inconsistent

Jack Zhou:

`AttributeEvents.set` and `AttributeEvents.append` both fire before the underlying collection has been changed, but the behavior for `AttributeEvents.remove` is not consistent, both between different methods of a single collection, as well as between the same methods of different collection types.

Demo:

```
#!python
class Department(Base):
    __tablename__ = "departments"
    id = Column(Integer, primary_key=True)
    employees = relationship(lambda: Employee,
                             cascade="all, delete-orphan")
    executives = relationship(lambda: Executive,
                              collection_class=attribute_mapped_collection("type"),
                              cascade="all, delete-orphan")
    contractors = relationship(lambda: Contractor, collection_class=set,
                               cascade="all, delete-orphan")


class Executive(Base):
    __tablename__ = "executives"
    id = Column(Integer, primary_key=True)
    type = Column(String, nullable=False)
    department_id = Column(Integer, ForeignKey(Department.id), nullable=False)


class Employee(Base):
    __tablename__ = "employees"
    id = Column(Integer, primary_key=True)

    department_id = Column(Integer, ForeignKey(Department.id), nullable=False)


class Contractor(Base):
    __tablename__ = "contractors"
    id = Column(Integer, primary_key=True)

    department_id = Column(Integer, ForeignKey(Department.id), nullable=False)


def _set_up_event(attr, name):
    @event.listens_for(attr, name, raw=True)
    def _check_if_modified(target, value, initiator):
        print(attr, name, target.attrs[initiator.key].history.has_changes())


session = Session()
emp1 = Employee()
emp2 = Employee()
exec1 = Executive(type="finance")
exec2 = Executive(type="operations")
con1 = Contractor()
con2 = Contractor()
dept = Department(employees=[emp1, emp2],
                  executives={"finance": exec1, "operations": exec2},
                  contractors={con1, con2})
session.add(dept)
session.commit()

for attr in [Department.employees, Department.contractors,
             Department.executives]:
    for name in ["remove", "append"]:
        _set_up_event(attr, name)

# list
print("remove()")
dept.employees.remove(emp1)
session.rollback()

print("pop()")
dept.employees.pop()
session.rollback()

print("clear()")
dept.employees.clear()
session.rollback()

emp3 = Employee()
print("[0] = ...")
dept.employees[0] = emp3
session.rollback()

print("del [0]")
del dept.employees[0]
session.rollback()

print("del [0:2]")
del dept.employees[0:2]
session.rollback()

print("[0:2] = ...")
dept.employees[0:2] = [emp3]
session.rollback()

# set
print("discard()")
dept.contractors.discard(con1)
session.rollback()

print("remove()")
dept.contractors.remove(con1)
session.rollback()

print("pop()")
dept.contractors.pop()
session.rollback()

print("clear()")
dept.contractors.clear()
session.rollback()

print("-=")
dept.contractors -= {con1, con2}
session.rollback()

con3 = Contractor()
print("&=")
dept.contractors &= {con3}
session.rollback()

print("^=")
dept.contractors ^= {con1, con2}
session.rollback()

# dict
print("pop()")
dept.executives.pop("finance")
session.rollback()

print("popitem()")
dept.executives.popitem()
session.rollback()

print("clear()")
dept.executives.clear()
session.rollback()

exec3 = Executive(type="finance")
print('["finance"] = ...')
dept.executives["finance"] = exec3
session.rollback()

print('del ["finance"]')
del dept.executives["finance"]
session.rollback()

exec4 = Executive(type="operations")
print("update()")
dept.executives.update({"finance": exec3, "operations": exec4})
session.rollback()
```

The following table summarizes the current behavior:

method               |list    |set     |dict
---------------------|--------|--------|--------
`remove`             |after   |before  |-
`clear`              |before  |before\*|before
`pop/popitem`        |after   |after   |after
`dict.pop`           |-       |-       |before
`discard`            |-       |before  |-
`__setitem__`        |before  |-       |before
`__setitem__` (slice)|before\*|-       |-
`__delitem__`        |before  |-       |before
`__delitem__` (slice)|before  |-       |-
`__isub__`           |-       |before\*|-
`__iand__`           |-       |before\*|-
`__ixor__`           |-       |before\*|-
`update`             |-       |-       |before\*

\* before\* means the event fires before the removal of the current item, but after the removal of the previous item

Ideally they should all behave like either "before" or "before\*" (but not both, for consistency). Technically it's not really a bug because the documentation doesn't prescribe any one particular behavior, but the inconsistency is annoying. For me personally, the motivating use case is being able to intercept *all* changes to a particular instance and act before any changes are made; currently my use case is always okay for `append`/`set`, but only sometimes for `remove`.