Delete old thumbnails from the media/cache directory and the database

[uri-rodberg]

Address issue #721

[codecov]

Codecov Report

Merging #740 (9339cc2) into master (dc5ad37) will decrease coverage by 0.63%.
The diff coverage is 41.93%.

@@            Coverage Diff             @@
##           master     #740      +/-   ##
==========================================
- Coverage   74.11%   73.49%   -0.63%     
==========================================
  Files          30       30              
  Lines        1700     1728      +28     
==========================================
+ Hits         1260     1270      +10     
- Misses        440      458      +18     

Files Changed	Coverage Δ
sorl/thumbnail/kvstores/base.py	87.27% <40.00%> (-7.52%)	⬇️
sorl/thumbnail/management/commands/thumbnail.py	82.69% <40.00%> (-17.31%)	⬇️
sorl/thumbnail/conf/defaults.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[uri-rodberg]

Can you please review this PR?

[uri-rodberg]

Hi, I can confirm that I have tested this new command in production and it works. It deletes thumbnails older than settings.THUMBNAIL_CLEANUP_DELETE_TIMEOUT seconds and doesn't delete thumbnails newer than settings.THUMBNAIL_CLEANUP_DELETE_TIMEOUT seconds.

[claudep]

Instead of having yet another setting, what about allowing the timeout be specified on the command line? It could still default to 10 years when not specified, but python manage.py thumbnail cleanup_delete_timeout 2592000 would be accepted.

[uri-rodberg]

Instead of having yet another setting, what about allowing the timeout be specified on the command line? It could still default to 10 years when not specified, but python manage.py thumbnail cleanup_delete_timeout 2592000 would be accepted.

Hi @claudep,

I asked you on February and you didn't have time, so I had to make a decision. I guess a command line argument can be optional, and if it's not given in the command line then use the setting (which defaults to 10 years). Personally I would prefer to have it as a setting, because in the setting we can use Python to calculate the number of seconds from a given number of days or years, while in the command line we have to know exactly how many seconds we need. So I would suggest to use this PR as it is now, from setting, and you can add a new issue and/or PR to give an optional command line argument, but I think it's better that if it's not given then we use Django's setting. I don't have time to rewrite this PR myself to add a command line argument. What do you think?

[claudep]

I wrote tonight an alternative patch that would avoid new command and setting, by adding an option to clear_delete_referenced. Can you look at it and tell if it would fill your use case?

[claudep]

Sorry, the PR is #745.

[uri-rodberg]

I wrote tonight an alternative patch that would avoid new command and setting, by adding an option to clear_delete_referenced. Can you look at it and tell if it would fill your use case?
Sorry, the PR is #745.

Hi @claudep,

I looked at the code of your patch (didn't run it though). Actually I prefer my patch since it also handles image files that do not exist and also deletes or updates all invalid thumbnail keys. Does your patch handles these cases? I also personally don't like the command continue and I prefer to use if (for example with a boolean flag that will tell the if whether to delete the thumbnail or not). But I like the ISO 8601 duration string supported by django.utils.dateparse.parse_duration in your patch. Maybe you can use my patch but add an optional command line argument using ISO 8601 duration, but keep the option of using settings.THUMBNAIL_CLEANUP_DELETE_TIMEOUT if it's defined and if a command line argument is not given (you may change the default if you want to, or even use None as the default). Or if you prefer you can have 2 commands, one with a required command line argument and one without a command line argument (which will use settings.THUMBNAIL_CLEANUP_DELETE_TIMEOUT). But I prefer very much to use my patch as I think it handles more cases (and by the way I didn't write all of it myself, I worked with 2 programmers that helped me).

By the way, I think clear_delete_referenced will delete almost all thumbnails if run without any command line argument, and its risky - since by mistake I might forget the command line argument, and then all my thumbnails will be deleted, and I prefer very much to use a different command, which does not default to deleting all thumbnails.

Thanks, Uri.

[claudep]

Hi Uri,

Actually I prefer my patch since it also handles image files that do not exist and also deletes or updates all invalid thumbnail keys. Does your patch handles these cases?

For my part, as we already have a command that cleans invalid thumbnail keys, I think it is not useful to do the same in other commands. You can always run the clean command followed by the clear_delete_referenced command to obtain the result you are looking for, in the spirit of one command - one (main) task.

I think clear_delete_referenced will delete almost all thumbnails if run without any command line argument, and its risky

We could mitigate that risk by requiring the --timeout option, and specifiying in the error message that --timeout=0 can be specified to keep the previous behavior.

Hopefully other contributors may now chime in and tell what version of the patch they prefer, as I suppose we won't achieve complete agreement between you an me (that's fine for me to disagree, that does not upset me at all).

[uri-rodberg]

I think clear_delete_referenced will delete almost all thumbnails if run without any command line argument, and its risky

We could mitigate that risk by requiring the --timeout option, and specifiying in the error message that --timeout=0 can be specified to keep the previous behavior.

Yes but I may run the current version which if run without the --timeout option, will delete all my thumbnails.

Hopefully other contributors may now chime in and tell what version of the patch they prefer, as I suppose we won't achieve complete agreement between you an me (that's fine for me to disagree, that does not upset me at all).

OK.

[claudep]

I think clear_delete_referenced will delete almost all thumbnails if run without any command line argument, and its risky

We could mitigate that risk by requiring the --timeout option, and specifiying in the error message that --timeout=0 can be specified to keep the previous behavior.

Yes but I may run the current version which if run without the --timeout option, will delete all my thumbnails.

Not if the --timeout option becomes required.

[uri-rodberg]

Yes but I may run the current version which if run without the --timeout option, will delete all my thumbnails.

Not if the --timeout option becomes required.

I think you don't understand. The current version is 12.9.0. the --timeout option is not required.

[claudep]

Yes but I may run the current version which if run without the --timeout option, will delete all my thumbnails.

Not if the --timeout option becomes required.

I think you don't understand. The current version is 12.9.0. the --timeout option is not required.

But we are talking here about a future version, as this code is not yet merged.

[Pantzan]

Joining late this discussion, I reckon having the option to specify a timeframe within the management command is much clearer, more useful and reduces unnecessary complexity at this stage. I could see Uri's proposal only for example, if a background task or a signal, could be triggered at some point to automate a cleanup process for old assets.