django-athumb
django-athumb copied to clipboard
gtaylor
Use django-storages, at least as parent class
Looking through the changes between S3BotoStorage in this repo and in django-storages, I wasn't sure what the significant changes were. Would it be possible to use the django-storages backend with this project? or if there are important changes to that backend, then maybe install django-storages and subclass their version?
That way it would be easier to see what this storage does in comparison their version.
It's definitely possible, and the way to go moving forward. Back when we forked off from django-storages, their boto S3 backend had some issues that I didn't have the time or will to deal with then. As the project has grown, it's likely that the situation has improved greatly, but I haven't had the time/motivation to see where things stood today.
I would gladly accept research and/or pull requests conducted by others, though. I am unlikely to tackle this anytime soon, since things are working pretty well right now. It's probably not a massive project, but care needs to be taken to prevent backwards incompatibilities.
If the current upstream django-storages already has support for what #22 is doing, that would be perhaps be additional motivation.
If anyone wants to take a stab at this, I'd love to see it happen. I may suggest preserving compatibility by making our S3BotoStorage a sub-class of django-storages. We'd probably need to mixin or find another way to do this:
https://github.com/gtaylor/django-athumb/blob/master/athumb/backends/s3boto.py#L216
That's technically not part of the Django storage API's "interface", but has been a useful addition for us. It may make sense to see if this fits upstream in django-storages, since it's more of a general-purpose thing.
Sure I'll take a stab this. It worries me a little not to have any tests, because it is hard to know if maybe upgrading django-storages will break anything, but why don't I just try that first, and if I am feeling motivated start writing some tests as well.
It's not a 100% substitute for a good test suite, but we can run your branch on our staging servers at Pathwright to see if anything turns up during our regular QA process.
Does url_as_attachment get the url for a certain S3 key? I am not terribly familiar with storage backends, so I don't see exactly what this function does...
Yes. It tells S3 to generate a link that will have a different Content-Disposition header in the response ('attachment'). Worse case, do your thing and leave me to deal with that method. I may try to figure out a better way to approach this.
Do you need to override the url method so that S3 doesn't have to be queried for the URL? The current url method, does this already when there is a custom domain, but it does seem to query S3 for when there isn't a custom domain. I personally do have a custom domain, so don't need get need this to be overridden. Do you guys use a custom S3 domain?
If you're talking about this: https://github.com/gtaylor/django-athumb/blob/master/athumb/backends/s3boto.py#L260
That's really the whole point of the S3BotoStorage_AllPublic backend. It's going to naively spit something out regardless of what you're doing. It won't bother with any of the signature generation that Boto does, and there is no possibility of contacting S3 for anything.
So what we really need to do is just make S3BotoStorage a sub-class of the upstream django-storages class, remove basically everything from it, and see what breaks.