django-cms / django-filer

File and Image Management Application for django
https://django-filer.readthedocs.io/
Other
1.76k stars 577 forks source link

fix: django-storage 1.14 complains about files being opened twice when copying #1418

Closed fsbraun closed 1 year ago

fsbraun commented 1 year ago

Description

While the root cause of the bug #1417 might be in django-storages 1.14, this quick-fix ensures that any ValueError raised opening a file for copying does not crash the file application with a server error.

Related resources

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (ac3985d) 75.21% compared to head (a4cfe26) 75.22%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1418 +/- ## ======================================= Coverage 75.21% 75.22% ======================================= Files 75 75 Lines 3454 3455 +1 Branches 555 555 ======================================= + Hits 2598 2599 +1 Misses 691 691 Partials 165 165 ``` | [Files Changed](https://app.codecov.io/gh/django-cms/django-filer/pull/1418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=django-cms) | Coverage Δ | | |---|---|---| | [filer/admin/folderadmin.py](https://app.codecov.io/gh/django-cms/django-filer/pull/1418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=django-cms#diff-ZmlsZXIvYWRtaW4vZm9sZGVyYWRtaW4ucHk=) | `75.65% <100.00%> (ø)` | | | [filer/models/filemodels.py](https://app.codecov.io/gh/django-cms/django-filer/pull/1418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=django-cms#diff-ZmlsZXIvbW9kZWxzL2ZpbGVtb2RlbHMucHk=) | `88.69% <100.00%> (+0.04%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vinitkumar commented 1 year ago

@fsbraun Is it possible to have a test with this patch?

fsbraun commented 1 year ago

@vinitkumar I know! I've been thinking about it, but I do not see a way of testing this without the specific implementation of django-storage's S3Storage on an AWS instance. I frankly believe that the underlying issue is between Django and django-storage, but it's hard to tell. Since thetry except block.

Any idea how to test this?

vinitkumar commented 1 year ago

@fsbraun I don't know. Seems hard to implement in the test. Have you tried testing this patch with a real s3 backend?

fsbraun commented 1 year ago

@vinitkumar I tested it on my staging setup with AWS: Without the fix, copying files fails and a copy of the first file remains in the source folder. After fixing it, copying works fine.

The fix also does not affect local file storage (which just runs seek(0) in case of a reopen).