aaugustin / django-resto

REplicated STOrage for Django, file backends that mirror media files to several servers over HTTP [UNMAINTAINED]
https://django-resto.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
104 stars 8 forks source link

Add flag to DistributedStorage and HybridStorage to allow overwrite #5

Closed max-arnold closed 11 years ago

max-arnold commented 11 years ago

And the last patch allows to write files in place without checking their presence (eliminates HEAD request) and generating suffixes like _1. We rely on this heavily in production to save user avatars and thumbnails with fixed filename and path.

Sorry for the lack of tests.

max-arnold commented 11 years ago

Also there is small annoyance in the logs with this one:

        logger.warning("PUT on existing file %s on %s.", name, host)

To fix it I can pass overwrite flag down the line to DefaultTransport.create()

aaugustin commented 11 years ago

Two questions:

1) After applying your patch, how is the overwrite parameter set? In Django, DEFAULT_FILE_STORAGE is instanciated without arguments, and for this reason I have a setting for every other argument. 2) Ignoring django-resto for a moment, how would you implement the equivalent feature in Django? I think you have to subclass FileSystemStorage.

Looking at the patch, it's just replacing methods, or adding code at the beginning of existing methods. This means it can easily be implemented as a subclass. You see where I'm going :)

At the very high level, the storage engines provided by django-resto target behavior- and feature-parity with those provided by Django. Once you're storing your media files on multiple servers, you're entering a territory where everyone has some custom needs, and my answer to that is: "django-resto provides robust and well tested base storage classes, subclass them to adjust to your specific needs."

In general, I'm happy to make changes:

However, I'd really like to keep django-resto simple, and I'm very conservative with feature requests.

max-arnold commented 11 years ago

1) After applying your patch, how is the overwrite parameter set? In Django, DEFAULT_FILE_STORAGE is instanciated without arguments, and for this reason I have a setting for every other argument

I prefer to explicitly assign overwrite parameter to field's storage where it is necessary (I do not want it to be default).

Looking at the patch, it's just replacing methods, or adding code at the beginning of existing methods. This means it can easily be implemented as a subclass. You see where I'm going :)

Yes, it is easy to do via subclassing and I understand where you are going :)

Actually, my main motivation behind this pull request is to simplify test coverage and switch to upstream django-resto. My private fork is not covered by tests and I want to fix this. In order to do this I need to use http_server.py and probably subclass tests from django-resto. But I do not consider them to be part of public and stable API and expect things to break in the future. The other way is to duplicate http_server and some tests to cover my subclasses, which violates DRY principle.

max-arnold commented 11 years ago

Basically it all boils down to stabilization of http_server interface as the documented way for users to use it in test coverage. I will be happy to use it to test not only customized storage, but also several application-specific model fields which use it and have some logic for thumbnail generation.

aaugustin commented 11 years ago

django-resto is stable, I bumped the version number to 1.0 a few months ago. But I haven't specifically documented which APIs are stable. I should do that.

max-arnold commented 11 years ago

Ok, I'll switch to unpatched django-resto, subclass it to add overwrite flag and will try to reuse django_resto.http_server to test my subclasses. If the need in additional extensibility points arises (especially in http_server), I'll open another issue.

Thanks for quick and detailed responses to pull requests!

aaugustin commented 11 years ago

For the record, I've documented the stable APIs in https://github.com/aaugustin/django-resto/pull/6, and http_server is now considered stable. (I changed the API to stop the server because it was stupid.)

max-arnold commented 11 years ago

Thanks!