Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Writing null or empty string to FileField does not empty the reference #937

Closed
wbbradley opened this issue Jun 14, 2013 · 15 comments
Closed
Labels

Comments

@wbbradley
Copy link

It does not seem possible to use PATCH on a FileField to null out the field. Is this by design?

@tomchristie
Copy link
Member

Not really by design no, not sure if this needs addressing or not.
I expect the underlying issue is that when partial updates are used there's no real differentiation between fields with None values and fields that are not present. Maybe worth digging around to see if this is also an issue with other None-able fields.

@tomchristie
Copy link
Member

Duplicated by #949. Let's follow up against that ticket.

@tomchristie
Copy link
Member

Not sure if this is a dup or not afterall, will reopen for now.

@tomchristie tomchristie reopened this Jul 11, 2013
@aburgel
Copy link
Contributor

aburgel commented Jul 24, 2013

I've run into this issue as well.

When using FileField, in WritableField.field_from_native the field data is retrieved from files and not data, meaning it comes from the parsed multipart data, not from the request body data. If you're submitting null or an empty string to that field, it won't be in the multipart data, so it will effectively look like no data was submitted.

I think the fix is to fall back on data for that field if its not found in files. If something other than null or empty string is submitted, then the validation on FileField will catch it.

Does that make sense?

@tomchristie
Copy link
Member

Fixed via #1003.

@fletchowns
Copy link
Contributor

I was trying out the new version that has this change and noticed my ImageFields were being inadvertently emptied out.

I totally understand the reasoning behind this change but I think it may have had unintended consequences. For example, if you are viewing a specific record through the browsable API and click the PUT button on the HTML form at the bottom, you've now destroyed data without even thinking about it, because the FileField is rendered without a file selected, so it gets emptied out when the form is submitted.

I'm not sure if there's a better way of going about this or not, just wanted to share my experience. I definitely like the idea of having a built in way to clear out these fields so I can get rid of my custom code to handle that in pre_save.

@wbbradley
Copy link
Author

Interesting. I wonder if we can pre-fill the form with the filename, to avoid it from 'changing'? Just a thought... note that I haven't thought about this problem in a few months now...

@fletchowns
Copy link
Contributor

Hmmm possibly, I can't figure out a way to get it to work though. I think browser security restrictions may prevent us from going down that route.

I can't think of a way to do this without using magical fields that don't actually exist on the model. There's gotta be some clever solution out there!

@fletchowns
Copy link
Contributor

Anybody have any other thoughts on this one?

@fletchowns
Copy link
Contributor

@tomchristie Any thoughts on this? It seems odd that you can no longer use the browsable API to update a record that has a FileField on it without having to upload the same file every time.

@tomchristie
Copy link
Member

@fletchowns I don't really have much spare time to look into it right now - it'd be great if someone else was able to take ownership of investigating the issue and pushing through a fix.

@martinmaillard
Copy link
Contributor

Hi,

I hit this issue in my code and it seems like a real problem: it basically makes PUT requests unusable on views containing a FileField/ImageField.

Is it reasonable to expect that sending a PUT request with the content of a GET request on the same resource should have no effect ? If this is the case, the problem comes from the asymmetry between FileField.to_native and FileField.from_native. If to_native returns the file name, sending the file name in the PUT request should not clear out the file. In my use case, I override to_native to serve the URL of the image instead of the file name. Sending this URL back in a PUT request should not affect the stored file.

I'm not very familiar with the codebase yet. Do you think this is a good solution ? And is it possible to implement ?

@wbbradley
Copy link
Author

The semantics of a null value are equally ambiguous, in my opinion. Perhaps
you want to be using PATCH? Files and Web Form REST semantics are not
clearly defined, AFAIK.

-W

On Tue, Apr 22, 2014 at 8:10 AM, Martin Maillard
notifications@github.comwrote:

Hi,

I hit this issue in my code and it seems like a real problem: it basically
makes PUT requests unusable on views containing a FileField/ImageField.

Is it reasonable to expect that sending a PUT request with the content of
a GET request on the same resource should have no effect ? If this is the
case, the problem comes from the asymmetry between FileField.to_nativeand FileField.from_native.
Ifto_nativereturns the file name, sending the file name in the PUT
request should not clear out the file. In my use case, I overrideto_native`
to serve the URL of the image instead of the file name. Sending this URL
back in a PUT request should not affect the stored file.

I'm not very familiar with the codebase yet. Do you think this is a good
solution ? And is it possible to implement ?


Reply to this email directly or view it on GitHubhttps://github.com//issues/937#issuecomment-41051627
.

@litchfield
Copy link

litchfield commented Jul 11, 2018

We're using PATCH with FileFields/ImageFields etc, so need a way of clearing the individual field. On our frontend the files are objects, so more natural to express empty state as null rather than an empty string.

Here's our hack, applied to our base serializer --

    def get_extra_kwargs(self):
        extra_kwargs = super().get_extra_kwargs()
        for f in self.Meta.model._meta.get_fields():
            # Hack to always allow_null on FileFields
            if isinstance(f, FileField):
                kwargs = extra_kwargs.get(f.name, {})
                kwargs['allow_null'] = True
                extra_kwargs[f.name] = kwargs
        return extra_kwargs```

@johncpang
Copy link

We're using PATCH with FileFields/ImageFields etc, so need a way of clearing the individual field. On our frontend the files are objects, so more natural to express empty state as null rather than an empty string.

Here's our hack, applied to our base serializer --

    def get_extra_kwargs(self):
        extra_kwargs = super().get_extra_kwargs()
        for f in self.Meta.model._meta.get_fields():
            # Hack to always allow_null on FileFields
            if isinstance(f, FileField):
                kwargs = extra_kwargs.get(f.name, {})
                kwargs['allow_null'] = True
                extra_kwargs[f.name] = kwargs
        return extra_kwargs```

I verified, this works. However, I also found that another solution which might be a bit better.

Copy from the source, in serializers add "allow_null=True" to the ImageField / FileField:

class PartSerializer(serializers.ModelSerializer):
    image = serializers.ImageField(max_length=None, allow_empty_file=True, allow_null=True, required=False)

    class Meta:
        model = Part
        fields = ('id', 'image')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants