Add DELETE functionality to imaging handler #592
Conversation
|
This PR should include tests if we are to accept it. I'm not sure I'm for this functionality, though. It's not as simple as removing the original image. What happens when thumbor needs to generate a transformation of the image again? It will just load it anyways and you won't have deleted a thing. Or you'll need to keep track of deleted images and disallow transforms for that image from that point forward. What happens to the existing transforms for that image? Will you delete those as well? I don't see this PR moving forward, sorry. |
|
Deleting an image from the storage is somewhat risky and it doesn’t handle the result_storage purge ( tricky to figure out what is in the result_storage. maybe we could optimize that to store all transforms under a certain key / folder ). Before this get merged it should probably also include options for a username/password combo to prevent anyone from issuing a DELETE on random images |
|
Result storage is optional and configured separately. You can have originals stored and not results. As you've pointed out, the current file hierarchy of result file storage makes that task tricky anyway. That folder hierarchy would have to be revisited in order to make that possible. I consider that a different task, as I don't need purging for results yet. The use case I need this for is that we have bursts of requests of different sizes for the same original over a short period of time. We don't need result storage, as we have another caching layer in front of thumbor. Therefore, it makes sense to cache the originals in thumbor for some time, to avoid it always fetching the original which it already fetched just seconds ago. However, we also have complex purging needs due to the various layers that need to be purged, and therefore we can't just wait for the thumbor expiry of the original to happen. We need to control that purge, which is where the functionality is missing, hence this PR. And to answer your question of what happens when thumbor needs to generate that image again, well, it will 404, since in our case the source URL will be deleted/purged as well, before the original in thumbor is. Essentially, if we don't purge the original in thumbor, it's likely to keep serving content that should be deleted for good. The source is our original storage, and those chained purged situations are when we remove content due to copyright infringement, for example. I don't need to keep a blacklist in thumbor of images I don't want to be transformed again, as those images will be gone by the time thumbor tries again. I'll figure out test coverage, but I hope that my explanations show that this is not as useless as it might look at first glance. |
|
Oh and as for the USERNAME/PASSWORD remark, our thumbor instance won't be public-facing, so that's not a concern for our use case. Visitors hit Varnish, which in turn hits thumbor internally. Purges either take a different route, or have their IP checked when they go through Varnish. |
|
Sorry if I let it seem as though I thought your PR was useless. I don't, at all. This is a recurrent topic actually. We, at globo.com, also get judicial requests for take-downs. That said, I think that providing this functionality in thumbor is not the best way to go, since this is a hard scenario to tackle in a generic manner. What I mean to say is that, as you said, in your specific scenario, it will work just deleting the original. That won't work for a lot of other scenarios, though. As a matter of fact, it does not work for our stock configuration for thumbor. That said, the area I really think thumbor shines is extensibility. All parts of thumbor are extensible, including the tornado App. You just have to pass "-a myapp.app" to thumbor when starting it. This is the way to go for you guys, I think. Since you came forward to share this functionality with everyone else (and I do believe more people require this), my suggestion is that we create a project in thumbor-community (something like thumbor-deletable) and create a new app there that inherits from thumbor's but includes the handler that inherits from thumbor's handler, but also includes deletion. Does that make sense? Instead of changing thumbor to support this specific scenario, we extend it to make sure anyone that needs this scenario gets to use it, while keeping our main thumbor usage the same. Now, if you get to a place where you can't fulfil your needs with thumbor as it is because of some extension point missing, we'll be more than happy to include it here. Just to make sure I am clear, your scenario is not useless. It's not even unusual. We have it here, and we tackle it in the exact way that makes sense for us (we have a custom storage that stores every transform under the original name, and we remove the root folder). Thanks a lot for contributing and I hope I didn't discourage you from helping us improve thumbor. |
|
Here's an example of a similar extension added to thumbor. It uses the thumbor community core app that allows binding new routes / extending the handlers. https://github.com/thumbor-community/shortener |
|
That makes sense, I didn't know extending that way was possible. I'll give that a try, thanks! |
|
@gi11es don't hesitate to contact me if you need help or hit a problem extending with |
Deletion is currently only available to images uploaded via POST, it makes sense to make it available for originals fetched through GET.