openwisp-firmware-upgrader icon indicating copy to clipboard operation
openwisp-firmware-upgrader copied to clipboard

[admin] Fixed FileNotFoundError #140

Open AbhigyaShridhar opened this issue 3 years ago • 6 comments
trafficstars

The change_view associated with BuildAdmin class was throwing an exception when trying to delete a FirmwareImage object, when the file associated with that object had already been deleted from the file system.

There were two tasks according to my understanding:

  1. Prevent the website to break and catch the error: the return expression in change_view has been put in a try/catch expression, with instructions/hints/warnings for the user using message_user

  2. If an image file has been deleted from the filesystem, automatically remove the FirmwareImage assiciated with it:

     The 'get_queryset' method for FirmwareImageInline class
    

has been over-written to automatically remove FirmwareImage objects associated with deleted files and the information has been logged with logger.

Fixes #140

AbhigyaShridhar avatar Feb 22 '22 11:02 AbhigyaShridhar

Closing for inactivity, feel free to reopen but please include tests.

nemesifier avatar Mar 08 '22 16:03 nemesifier

@nemesisdesign @pandafy What is the reason for storing images using private_storage instead of using django's 'models.FileField' ? The issue is most probably caused by that.

AbhigyaShridhar avatar Apr 20 '22 17:04 AbhigyaShridhar

@nemesisdesign @pandafy What is the reason for storing images using private_storage instead of using django's 'models.FileField' ? The issue is most probably caused by that.

@AbhigyaShridhar the files must not be reachable without authentication because contain sensitive information.

nemesifier avatar Apr 20 '22 17:04 nemesifier

I have added all the details regarding my approach to the issue in the latest commit message. I want to add a test for this issue. However, to reproduce the issue, the delete method has to be called from a form(which is possible to do using selenium?)

@nemesisdesign @pandafy Can you please point me to some existing test case which I might use for reference? It will help me write a new test.

AbhigyaShridhar avatar Apr 26 '22 15:04 AbhigyaShridhar

I couldn't reproduce this issue with Django's standard models.FileField so I figured that the problem is with the way private_storage is being used. Strangely, there's no problem when delete method is called on an instance of the model. The error is always thrown when the delete method is called on a "modelForm" for an instance, the image file for which has been deleted.

So the issue really is with the private_storage module. To get around the error though, what I have done is:

  • try to return change_view
  • catch the FileNotFoundError if it occurs
  • Get the path from the error itself
  • check if the specified directory exists, if not create one
  • create a temporary file
  • return change_view again (recursion)
  • This time the file is there to be deleted and the model instance is deleted as well

This seems like a "brute force" solution, but since the origin of the issue is an external module, this is the only possible workaround (As we don't want to automatically delete any model instances because it would be strange for the user)

Edit: Had to add the second commit because there was a problem with my commit message (QA checks)

AbhigyaShridhar avatar Apr 26 '22 15:04 AbhigyaShridhar

@AbhigyaShridhar @ping!

pandafy avatar Jun 10 '22 16:06 pandafy

Closing this PR because of inactivity.

pandafy avatar Feb 22 '24 14:02 pandafy