amol- / depot

Toolkit for storing files and attachments in web applications
MIT License
161 stars 41 forks source link

`_SQLAMutationTracker` didn't track objects that deleted by `relationship.remove()` #46

Closed philiptzou closed 5 years ago

philiptzou commented 7 years ago

As the title described. I have a secondary relationship to another child table via an associative table. I can delete a child from the parent by using parentObj.children.remove(childObj). However SQLAlchemy didn't put childObj into its session.deleted set. _SQLAMutationTracker relies on that property to aware the deletion of objects.

As a result, the record was deleted from database but not depot storage.

href commented 5 years ago

We've discovered the same issue in a code-base using table based generic associations.

We worked around the issue by adding our own files to the list of files to be deleted. We do this whenever the model holding these files is deleted:

from sqlalchemy import event
from sqlalchemy.orm import object_session

class File(Base):
    ...
    reference = Column(UploadedFileField())

@event.listens_for(File, 'after_delete')
def after_delete(mapper, connection, target):
    session = object_session(target)

    session._depot_old = getattr(session, '_depot_old', set())
    session._depot_old.update(target.reference.files)

I would assume this could be adopted for filedepot itself, but I'm not sure it is an issue that it has to solve. Anyway, maybe this helps someone stumbling upon this.

amol- commented 5 years ago

https://github.com/amol-/depot/commit/098ac91b10a23ae0f108ed2fa30fbff447dbf475 should provide support for relationship.remove. Objects deleted that way do not figure as deleted until the session flush completed.

Can you confirm that change solves the issue for you?

href commented 5 years ago

I ran the tests I wrote for this against your master without our workaround and I can confirm it works!

Thanks for adding a fix on your side.