bradleyg / django-s3direct

Directly upload files to S3 compatible services with Django.
MIT License
661 stars 234 forks source link

Always stores full path in DB, making storage not portable #154

Open bluppfisk opened 6 years ago

bluppfisk commented 6 years ago

If the full path is stored and the project needs to be transferred to someone else, you need to change all filename entries in the database. It would be better to store only the filename and let other parts depend on the environment config

bradleyg commented 6 years ago

Yes, totally agree. Anybody keen on submitting a pull request for this? :)

chubz commented 6 years ago

@bradleyg What would we need to change?

I'm looking at the lib here and a way in for this would be: Changing finishUpload where link and url are set? Adding value_from_datadict in the widget? It would be better to do it in the widget since a setting could then trigger intended behavior.

I've dug this up: https://stackoverflow.com/questions/33202644/django-and-s3-direct-uploads But it seems that poster of this question deals in having a view and assigning cleaned data himself.

I'd propose this be automated, in a sense that either the js part or the widget part changes.

chubz commented 6 years ago

So my personal reason to experiment with this is trying to connect s3direct with django-storages. My understanding is that django storages has a location attribute much like s3 direct has destination. These two are something that can connect these. Now the thing is django storages looks at keys from the location, so if these two match they need to be removed when saving the key here for file to be accessible.

As for the widget it might be okay to hook a settings to this like S3DIRECT_STORE_FULL_URL If this is false then the filename would be saved.

def value_from_datadict(self, data, files, name):
        """
        Given a dictionary of data and this widget's name, return the value
        of this widget or None if it's not provided.
        """
        # should check a setting STORE_KEY
        url = super(S3DirectWidget, self).value_from_datadict(data, files, name)
        if url and not getattr(settings, 'S3DIRECT_STORE_FULL_URL', True):
            try:
                # splits on the last / and take just the file name
                return url.rsplit('/', 1)[-1]
            except IndexError:
                raise ValueError('There is something worng with the file path')
        return url

Main issue I have with this is that someone else might need different "path" saved. Maybe using the path of their destination etc.

It could be that a few settings options could be usefull here:

S3DIRECT_STORE_FULL_URL = True # preserves default behavior
S3DIRECT_STORE_FILENAME = False # if this is overridden only filename is stored (django-storages comapt)
S3DIRECT_STORE_DESTINATION = False # if we need to store full path cutting bucket URL

@bradleyg any thoughts on this proposition?

roxel commented 6 years ago

@chubz I think you idea might work, but I suggest to resolve this problem on a different level, i.e. in Field. It would be nice to use plain models.FileField in model and then in form add a field like:

document = S3DirectFileField(
    dest='document_destination',
    max_length=255,
    label="Document",
)

That field could then not only save relative path but also use S3DirectWidget behavior in production environments and FileField behavior in development/locally.

In my case I needed that kind of behavior, but only solution I found was to:

The problem was that in order to properly display path I had to create absolute path in forms.initial (when using S3DirectURLField) and overwrite S3DirectWidget render method to have absolute path under file_url. It's all not very clean... :(

@chubz @bradleyg Do you have some ideas on how to do it better?

getup8 commented 4 years ago

Also very interested in this. IMHO it doesn't make sense to return the full URL from the JS, just the key should be returned and then the user could provide a function (via a setting) to save it however they wanted. Even with the default behavior, it might make more sense to still just return the key and then construct the full URL in python.

@bradleyg do you have any thoughts on how we might implement this?

Looks like the value is set in JS here: https://github.com/bradleyg/django-s3direct/blob/master/src/index.js#L75

I think I like @chubz idea to just edit value_from_datadict at the widget level since that would get implemented in either a model or custom form implementation. I think the render method would need to be adjusted slightly as well but not by much.