un1t / django-cleanup

Automatically deletes old file for FileField and ImageField. It also deletes files on models instance deletion.
MIT License
1.11k stars 79 forks source link

instance.field_name is not set anymore after updating the file #27

Closed pchiquet closed 8 years ago

pchiquet commented 9 years ago

The post_save signal mess with instance.field_name value when deleting the old file:

>>> product = Product.objects.create(image=image1)
>>> product.image = 'new.jpg'
>>> product.save()
>>> product.image
None

since 0.4.0 release

pchiquet commented 9 years ago

https://github.com/un1t/django-cleanup/pull/28

vinnyrose commented 9 years ago

I can't reproduce this, though I am working against newer code so it's possible I fixed it unintentionally... is this bug reliable with tox?

pchiquet commented 9 years ago

I ran tox -epy27-django{16,17,18,19} and test_replace_file fails for all django versions.

(not tested with python3)

$ tox -epy27-django{16,17,18,19}
py27-django16 runtests: commands[0] | py.test -v --cov-report=term-missing --cov=django_cleanup django_cleanup
django_cleanup/testapp/test_all.py::test_replace_file FAILED

py27-django17 runtests: commands[0] | py.test -v --cov-report=term-missing --cov=django_cleanup django_cleanup
django_cleanup/testapp/test_all.py::test_replace_file FAILED

py27-django18 runtests: commands[0] | py.test -v --cov-report=term-missing --cov=django_cleanup django_cleanup
django_cleanup/testapp/test_all.py::test_replace_file FAILED

py27-django19 runtests: commands[0] | py.test -v --cov-report=term-missing --cov=django_cleanup django_cleanup
django_cleanup/testapp/test_all.py::test_replace_file FAILED
vinnyrose commented 9 years ago

I was able to reproduce it, turns out I was hiding this with a bug in my code (fixed now). I don't think your proposed fix will work in django 1.9+ on a transactional database since the delete may not be run until later with from django.db.transaction import on_commit.

I can actually fix this in my version of the code since I am passing the instance directly into the delete_file command. Originally this was to fix the loss of storage classes after a pickle/unpickle, but this fix can fit in there as well. I am going to try to put in a PR tonight with my fixes so far.

un1t commented 8 years ago

I merged PR, so it should work now.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.