Opened 5 years ago

Closed 5 years ago

#14814 closed (wontfix)

Check for file binding in FieldFile.delete()

Reported by: dimier Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: field file, FieldFile
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


Atteched patch adds a simple check for file association in FieldFile.delete method.

Attachments (1)

r14751_field_file.diff (487 bytes) - added by dimier 5 years ago.

Download all attachments as: .zip

Change History (7)

Changed 5 years ago by dimier

comment:1 Changed 5 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

I don't understand the point of this patch. It would simply make calling FieldFile.delete() a no-op, rather than an error. But it should be an error, since calling an instance method without passing an instance is a mistake.

If there is something I've missed, please explain and re-open.

comment:2 Changed 5 years ago by lukeplant

Sorry, that should be: it would make FieldFile.delete(None) a no-op. It isn't obvious why you would want to do that (instead of just, um, not calling that method).

comment:3 Changed 5 years ago by Alex

self evaluates to False if there's no file attached to the wrapper.

comment:4 Changed 5 years ago by dimier

Thanks, lukeplant and Alex.

Of course, I didn't call FieldFile.delete() directly.

First, calling delete() without associated file will raise OSError exception:

OSError: [Errno 21] Is a directory: '/home/dimier/testproject/media'

And no exception will be raised if I do album.cover_img = None before delete(). It's ambiguously, I think.

Second, the point of this patch is to minimize number of checks to delete a file.

For example, if I want to update an album cover I need to write the following code:

if album.cover_img:
album.cover_img = new_cover_path

In some models in my project I have several image fields needed to update and I get/set them through getattr/setattr, so without described check the code become more readable.
If I just set album.cover_img to a new value without delete(), previous associated file will not be deleted.

So, the following will be simpler:

album.cover_img = new_cover_path

This small patch is just my first experience in contribution to open-source and I have another improvements to publish. If the patch is useless, just leave the ticket closed.

comment:5 Changed 5 years ago by dimier

  • Resolution invalid deleted
  • Status changed from closed to reopened

comment:6 Changed 5 years ago by russellm

  • Resolution set to wontfix
  • Status changed from reopened to closed

I am in agreement with Luke and Alex. It isn't a good thing to be silently hiding an error check.

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