Opened 14 years ago

Last modified 7 years ago

#13327 new Cleanup/optimization

FileField/ImageField accessor methods throw unnecessary exceptions when they are blank or null.

Reported by: kevin.howerton@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Alex Gaynor)

Many of the accessor methods on the FileField call this method:

    def _require_file(self):
        if not self:
            raise ValueError("The '%s' attribute has no file associated with it." % self.field.name)

This method defeats the blank or null features, and has 0 benefit.

The preferred behavior should be that it raises an error if the field is set, but the file does not exist. However if you try to retrieve a URL or path from a file-field it should return None if the field is Null or if the field is blank. Not sure what the reasoning behind throwing an error here, is but it seems extremely dis-advantageous in practice requiring every image or file-field that can be blank/null to have if/else statement wrapped around it in templates to prevent 500 errors.

Change History (9)

comment:1 by Alex Gaynor, 14 years ago

Description: modified (diff)

Please use preview.

comment:2 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedDesign decision needed

This has the potential to be a fairly major change in behavior, so it may not be possible to make this change. However, I can see the point you are making. Determining the right way forward will require some discussion on django-dev.

comment:3 by Apreche, 14 years ago

I completely agree that this needs to change.

If you set an Image or File Field to blank/null, then it is expected that there will often be no actual file in the file system. Obviously if there is an operation that requires reading the file from the file system, such as checking the size of the file, then it is obvious that there would be IO errors or file not found exceptions. However, for operations such as getting the path or url, there is no reason to check the file system at all. If the field is null, then these operations should simply return None.

As the submitter has pointed out, the current system requires that you put conditionals around every image field in every template in order to avoid errors that prevent the entire page from loading just because one image is missing. With the suggested change, the pages would load without error, and maybe a few relatively harmless broken images.

comment:4 by Julien Phalip, 13 years ago

Severity: Normal
Type: Cleanup/optimization

comment:5 by Thejaswi Puthraya, 13 years ago

Component: UncategorizedDatabase layer (models, ORM)
Easy pickings: unset
UI/UX: unset

comment:6 by Anssi Kääriäinen, 11 years ago

Triage Stage: Design decision neededAccepted

I agree a change here seems like a good idea. The backwards compatibility needs more investigation than my 2 minute review...

comment:7 by Tim Graham, 8 years ago

#26380 is a duplicate.

comment:8 by Mehdy Haghy, 8 years ago

any workaround to handle this error?

I get this error in DRF model serializer so I can't suppress it when a FileField is null in database.
considering the model definition, null values should not cause exception but it does

   class Repository(models.Model):
     owner = models.ForeignKey('auth.User', related_name='file_owner')
     file_content= models.FileField(upload_to='repository',null=True, blank=True)
Last edited 8 years ago by Mehdy Haghy (previous) (diff)

comment:9 by Elias Nygren, 7 years ago

Some workarounds:

The model property:

@property
def image_url(self):
    if self.image and hasattr(self.image, 'url'):
        return self.image.url

The Template if (bad, breaks DRY):

{% if person.image %}
    <img src="{{ person.image.url }}">
{% endif %}

The check (bad, breaks DRY):

person.profile_pic.url if person.profile_pic else ''

IMHO all of the above are quite horrible compared to if we could just fix the root problem:

django/db/models/fields/files.py

    def _require_file(self):
        if not self:
            raise ValueError("The '%s' attribute has no file associated with it." % self.field.name)

    @property
    def url(self):
        self._require_file()
        return self.storage.url(self.name)

with something equivalent to (not a working fix):

    @property
    def url(self):
        if self:  # only pass to storage.url if there is a file
            return self.storage.url(self.name)
        return None # (or empty string?)
Note: See TracTickets for help on using tickets.
Back to Top