Opened 5 years ago

Closed 5 years ago

Last modified 5 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 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Topher

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 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Berker Peksag

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

comment:5 Changed 5 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:6 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 28bcff82:

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

comment:7 Changed 5 years ago by Tim Graham <timograham@…>

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