search

jazzband / django-queued-storage

Provides a proxy for Django storage backends that allows you to upload files locally and eventually serve them remotely
http://django-queued-storage.rtfd.org/
BSD 3-Clause "New" or "Revised" License
317 stars 62 forks source link

django 1.10 support #22

Closed thijstriemstra closed 7 years ago

thijstriemstra commented 8 years ago

django 1.10a1 was released and this PR aims to make it compatible.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.4%) to 87.31% when pulling ed1d0a0900a62f7d19b6afac68b536c80ed13f8a on thijstriemstra:master into 883bdb439c9b3953ab7d513eddf769b945065b9d on jazzband:master.

thijstriemstra commented 8 years ago

Anyone an idea why it's failing with:

AttributeError: 'QueuedEncodeSystemStorage' object has no attribute 'generate_filename'
ashwoods commented 8 years ago

Django 1.10 introduced a new method: https://docs.djangoproject.com/en/1.10/ref/files/storage/#django.core.files.storage.Storage.generate_filename

thijstriemstra commented 8 years ago

Implemented generate_filename by using the default django 1.10a1 implementation, and also added the required max_length argument, but now getting this error:

 SuspiciousFileOperation: The joined path (/tmp/tmp93y495/queued_storage.txt) is located outside of the base path component (/tmp/tmpY5ycks)
coveralls commented 8 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 1a86ba05b8a966309fc66af4828f8be1336aa6fb on thijstriemstra:master into 883bdb439c9b3953ab7d513eddf769b945065b9d on jazzband:master.

ashwoods commented 8 years ago

Sorry for taking so long to look into this. In django 1.x there has been a storage refactor, which includes a slash check on file paths.

https://code.djangoproject.com/wiki/FileStorageRefactor#Backwards-incompatibilechanges

Saving files outside of MEDIA_ROOT is not possible using the Storage API. So the tests have to be refactored to use relative file path names only.

the offending lines are 30 and 31

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling db499c2a35372180b8e61aa1784036faa341bffb on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 12decb68e5f27f612c9f754fe3d651ab688c2c8a on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

egregors commented 8 years ago

Any progress for this Pull request? 1.10 is already released.

ashwoods commented 8 years ago

I'll get around to it today. Thx for the reminder!

egregors commented 7 years ago

How can we help you to resolve this PR?

ashwoods commented 7 years ago

Pretty much fix the tests and make sure it works like i think it works. This week was too eventfull for me. https://travis-ci.org/jazzband/django-queued-storage/jobs/139383703#L357

thijstriemstra commented 7 years ago

make sure it works like i think it works.

hehe

chandanand commented 7 years ago

Any update on this? Waiting to use it with Django 1.10.

thijstriemstra commented 7 years ago

Waiting? Why not help finish this?

onekiloparsec commented 7 years ago

I'll be glad to help finish this. But I am puzzled with travis errors. Most of them are of the type E SuspiciousFileOperation: The joined path (/tmp/tmp1J0df4/queued_storage.txt) is located outside of the base path component (/tmp/tmpKaaMSf), but only for some combinations of Django/Python.

onekiloparsec commented 7 years ago

I just discovered I can join Jazzband and thus directly work on this PR. Nice. Changes to come.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 868dcdddbb05337ce693337446120b4c4c13cb22 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 868dcdddbb05337ce693337446120b4c4c13cb22 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 868dcdddbb05337ce693337446120b4c4c13cb22 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 868dcdddbb05337ce693337446120b4c4c13cb22 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 868dcdddbb05337ce693337446120b4c4c13cb22 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 868dcdddbb05337ce693337446120b4c4c13cb22 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 868dcdddbb05337ce693337446120b4c4c13cb22 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 59ca6ae8fe7241a9abd25bce90dbb35e617ba3b1 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 59ca6ae8fe7241a9abd25bce90dbb35e617ba3b1 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 86.567% when pulling 59ca6ae8fe7241a9abd25bce90dbb35e617ba3b1 on thijstriemstra:master into 42e9f4c61fb7ff0cedd56b10cef45f8eb2cbae50 on jazzband:master.

onekiloparsec commented 7 years ago

@thijstriemstra Would you agree to let me push to that PR directly? It would be much easier for me rather than playing in another branch. We could then mutualise efforts. I've made some tests in branch pr/22, but to no avail so far. I will investigate more. This project is key for my development, and I need support for v3 of S3Boto right away.

onekiloparsec commented 7 years ago

Well, I've struggled during my whole morning with this SuspiciousOperation error. And there is something I don't get.

thijstriemstra commented 7 years ago

Push directly, absolutely!

onekiloparsec commented 7 years ago

Ok thanks! But you have to explicitly set it up. (normally, small checkbox on the right-hand side of this conversation)

thijstriemstra commented 7 years ago

Done.

onekiloparsec commented 7 years ago

Thanks! I'm still struggling to understand basics of Django Storage. Definitely, tests have to be rewritten for some parts. I'll push when I can achieve actually something useful.

ashwoods commented 7 years ago

@onekiloparsecI think I fell a little bit into too much work bias as I have been wanting to refactor the app for a while, but never get to it because it's a lot of work ;)

While the refactor in itself would simplify the code and it would be pretty easy, maintaining backward compatibility is another issue.

I don't really see why the QueuedStorage methods have to map to the django storage at all, being a simple facade to a single object that most of the time share the same structure.

So you achieve this by overriding a few magic methods, and would abstract the whole storage logic away. I could make a prototype after work, but could really appreciate some help making sure I don't break anything that already works ;)

onekiloparsec commented 7 years ago

Thanks for your input. I am not sure to follow you completely. The use of a so-well-named Facade pattern is fairly natural to me in this context. You want to be able to switch from one type of storage to another, queued or not, without worrying about breaking things up.

However, the fact that you can achieve this by using a wrapping object composing two underlying Storage objects, or with a brand new (subclass of the base?) Storage class, is a matter of implementation, and discussion.

So far, as far as I understand, the former has been chosen. Which makes testing easier too, IMHO.

onekiloparsec commented 7 years ago

Ok, I've been able to track down the problem, and I have a basic working Dango-1.10+ test class. Will push asap once I have cleaned up the stuff.

onekiloparsec commented 7 years ago

I spoke too soon. I'm investigating the use of the mock library now.

ashwoods commented 7 years ago

I've pushed a possible fix for the suspicious error in django-110 branch

onekiloparsec commented 7 years ago

I think I managed to get it working. But I found some strange things along the way. I changed in particular the parameters of the Celery tasks. Strangely, they were given parameters to recreate new instance of Storage classes, rather than using those already built.

I will soon push the new implementation, once all tests pass.

ashwoods commented 7 years ago

@onekiloparsec aware of this branch: https://github.com/jazzband/django-queued-storage/tree/django-110 ?

onekiloparsec commented 7 years ago

@ashwoods yes, I'm aware of this branch, thanks for the reminder. To me, the additional commit on this branch is already integrated into this PR code (indeed, these methods were missing), but it doesn't fix the issue of failing tests with django-110. Moreover, in this PR, I am addressing quite a few more details (in addition of running all tests with more version of Django and Python).

onekiloparsec commented 7 years ago

Hm, it seems, I'have pushed in the wrong branch (pr/22). I'll clean things up asap.

onekiloparsec commented 7 years ago

I've changed my pr branch to actually point to django-110 branch, and just pushed (and removed pr/22 branch). To me this PR is now obsolete, and if you agree and all tests pass, we could merge django-110 into master.

ashwoods commented 7 years ago

@onekiloparsec any reason to remove the tox-travis integration?

onekiloparsec commented 7 years ago

No at all! I'm sorry, I noticed I may have messed up a little bit with tox (with which I'm not entirely confortable). Please, correct me, if necessary! But note that I've taken the liberty to merge django-110 into master already.

ashwoods commented 7 years ago

I'd wait with the merge, as there are a few notes I want to make on the changes.

onekiloparsec commented 7 years ago

Sorry @ashwoods if I moved forward a bit too quickly. I saw this project a bit sleeping (we are all very busy!), and I thought I could move forward without asking. But of course, I'm eager to participate along the spirit of the project. I'll push to master right now.

ashwoods commented 7 years ago

why push to master? the tests are not eve passing?

onekiloparsec commented 7 years ago

I noticed that GitHub mentioned a commit in this PR thread, but the associated checks don't pass. My mistake was to try to push directly to this PR. But I didn't managed to do so. So I pushed to the branch django-110, and I merged after checking that all tests passes on this branch.

onekiloparsec commented 7 years ago

And to finish the story: after the successful tests on django-110, I merged into master, and then I added 2 commits. And the last one indeed, make the tests fail again. Sorry.

I hope confusion will decrease, sorry for that. I won't push anything that would increase it again. Let me know what I can do/help to sync on the same state.