Opened 3 weeks ago

Closed 2 weeks ago

#35868 closed Bug (fixed)

'collectstatic' management command inappropriately eating AttributeError exceptions

Reported by: Brett G Owned by: Peter Ruszel
Component: contrib.staticfiles Version: 5.0
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Brett G)

Recently, Django 5.0 removed the django.utils.timezone.utc alias to datetime.timezone.utc.

I've written a custom file storage class that implements a get_modified_time method which, predictably, used the django.utils.timezone.utc alias. Use of this alias raises an AttributeError in Django 5.0.

The code in 'django/contrib/staticfiles/management/commands/collectstatic.py' reads as follows:

try:
    # When was the target file modified last time?
    target_last_modified = self.storage.get_modified_time(prefixed_path)
except (OSError, NotImplementedError, AttributeError):
    # The storage doesn't support get_modified_time() or failed
    pass
else:
    try:
        # When was the source file modified last time?
        source_last_modified = source_storage.get_modified_time(path)
    except (OSError, NotImplementedError, AttributeError):
        pass

I assume the exception catch there is meant to ignore AttributeError exceptions from custom file storage classes that do not implement get_modified_time. This is inappropriate - custom file storage classes can be written by users, and the get_modified_time method may raise AttributeError for a completely unrelated reason.

The net effect of this is that due to the AttributeError raised by my own get_modified_time method, when collectstatic was run, all static files were copied, regardless of modification time, as collectstatic silently ignored this error. This was difficult to pin down.

Checking for the presence of a get_modified_time attribute using duck typing (eg. hasattr(self.storage, 'get_modified_time')) may be more appropriate than eating such a generic exception.

Change History (7)

comment:1 by Brett G, 3 weeks ago

Description: modified (diff)

comment:2 by Brett G, 3 weeks ago

Description: modified (diff)

comment:3 by Sarah Boyce, 3 weeks ago

Triage Stage: UnreviewedAccepted

Looks like the catching of the AttributeError was added back in 4f1ac8f5f15f08f34dc06c7442cec50a7af31c45 (which was to align to a change in django-storages https://github.com/jezdez/django-staticfiles/commit/b87a3531fbf4f18fea0633310410f8710cd0de39)

As a custom storage backend should inherit from django.core.files.storage.Storage (and this is documented https://docs.djangoproject.com/en/5.1/howto/custom-file-storage/#how-to-write-a-custom-storage-class), I feel like it will always have get_modified_time if folks are following the docs.
I believe it is safe to remove this

comment:4 by Peter Ruszel, 2 weeks ago

Owner: set to Peter Ruszel
Status: newassigned

comment:5 by Peter Ruszel, 2 weeks ago

Has patch: set

comment:6 by Sarah Boyce, 2 weeks ago

Triage Stage: AcceptedReady for checkin

comment:7 by Sarah Boyce <42296566+sarahboyce@…>, 2 weeks ago

Resolution: fixed
Status: assignedclosed

In cf9da6f:

Fixed #35868 -- Removed unneeded AttributeError catching in collectstatic's delete_file().

Note: See TracTickets for help on using tickets.
Back to Top