Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#26297 closed Bug (fixed)

collectstatic --clear throws NotImplementedError, "This backend doesn't support absolute paths."

Reported by: Topher Owned by: Berker Peksag
Component: contrib.staticfiles Version: 1.9
Severity: Normal Keywords: clear notimplementederror path
Cc: berker.peksag@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

storage.py docstrings state of storage.path()

Storage systems that can't be accessed using open() should *not* implement this method.

collectstatic.py does not catch this to use the implemented storage.delete() method on line 221

full_path = self.storage.path(fpath)
if not os.path.exists(full_path) and os.path.lexists(full_path):
    # Delete broken symlinks
    os.unlink(full_path)
else:
    self.storage.delete(fpath)

Patch?:

try:
    full_path = self.storage.path(fpath)
    if not os.path.exists(full_path) and os.path.lexists(full_path):
        # Delete broken symlinks
        os.unlink(full_path)
    else:
        self.storage.delete(fpath)
except NotImplementedError:
    self.storage.delete(fpath)

Change History (7)

comment:1 by Tim Graham, 9 years ago

Description: modified (diff)
Easy pickings: unset
Needs tests: set
Triage Stage: UnreviewedAccepted

I think something like this would make it more obvious where the exception is expected:

try:
    full_path = self.storage.path(fpath)
except NotImplementedError:
    self.storage.delete(fpath)
else:
    if not os.path.exists(full_path) and os.path.lexists(full_path):
        # Delete broken symlinks
        os.unlink(full_path)
    else:
        self.storage.delete(fpath)

A test is also needed.

comment:2 by Topher, 9 years ago

Easy pickings: set
Needs tests: unset
Triage Stage: AcceptedUnreviewed

I may be wrong, I don't think a test would be feasible as testing would require simulating multiple storage backends that aren't currently tested?

Off topic, I'm starting to wonder why collectstatic has any symlink logic at all. Shouldn't staticfiles offer a symlink storage backend to be used, if the functionality was desired, instead?

comment:3 by Tim Graham, 9 years ago

Easy pickings: unset
Needs tests: set
Triage Stage: UnreviewedAccepted

I can't think of a reason why creating a subclass of a storage backend that raises NotImplementedError to test this wouldn't be feasible, but I didn't look into the details.

Maybe the symlink logic added in 0922bbf18d3ae8f37e1823df2dbc270d33334548 isn't ideal. Feel free to submit a patch if you feel it could be done better.

comment:4 by Berker Peksag, 9 years ago

Cc: berker.peksag@… added
Needs tests: unset
Owner: changed from nobody to Berker Peksag
Status: newassigned

comment:5 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:6 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In 28bcff82:

Fixed #26297 -- Fixed collectstatic --clear crash if storage doesn't implement path().

comment:7 by Tim Graham <timograham@…>, 9 years ago

In b4bb2ad1:

[1.9.x] Fixed #26297 -- Fixed collectstatic --clear crash if storage doesn't implement path().

Backport of 28bcff82c5ed4694f4761c303294ffafbd7096ce from master

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