[admin] Fixed FileNotFoundError #140 #180

AbhigyaShridhar · 2022-02-22T11:40:17Z

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:

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
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

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 openwisp#140

pandafy

Good work @AbhigyaShridhar! Read my comments below.
Also, it will be good to add a test here.

pandafy · 2022-02-22T17:34:22Z

openwisp_firmware_upgrader/admin.py

+        for imageObject in qs:
+            if imageObject.file is not None:
+                path = imageObject.file.path
+                if not os.path.isfile(path):
+                    try:
+                        type = imageObject.type
+                        imageObject.delete()
+                        logger.warning(f"Image object {type} was removed")
+                    except Exception:
+                        pass
+        return qs


Looping over individual FirmwareImage object will make system very slow. Instead of this, the issue recommended catching the exception when it occurs(on delete operation) and delete the related FirmwareImage.

pandafy · 2022-02-22T17:37:30Z

openwisp_firmware_upgrader/admin.py

+            if type(e).__name__ == "FileNotFoundError":
+                self.message_user(
+                    request, "Please reload the page", level=logging.ERROR
+                )
+            else:
+                self.message_user(
+                    request,
+                    "Image objects have been removed or form data didn't validate",
+                    level=logging.ERROR,
+                )
+                return HttpResponseRedirect(request.path)


Instead of showing this error to the user, log the error with logger.exception.

Delete the FirmwareImage object for which the error was raised.

Call the change_view method again (recursion)

We want to handle this case without making it obvious to the user.

OK, I understand

nemesifier · 2022-03-08T16:30:01Z

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

AbhigyaShridhar · 2022-04-20T17:26:06Z

@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.

nemesifier · 2022-04-20T17:42:40Z

@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.

…sp#140 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) Fixes openwisp#140

AbhigyaShridhar · 2022-04-26T15:37:56Z

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 · 2022-04-26T15:40:56Z

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)

…sp#140 Added a try/catch block in the change_view of `BuildAdmin` class. If a `FileNotFoundError` is caught, the relavant file/directory will be temperorily re-created just before recursively calling change_view again. Fixes openwisp#140

…sp#140 Added a try/catch block in the return statement in `BuildAdmin` `change_view` If a `FileNotFoundError` is caught, the file/directory from the error path is recreated temperorily before recursively calling `change_view` again, to complete the delete operation without throwing any errors. Fixes openwisp#140

nemesifier

Thanks for the investigation @AbhigyaShridhar!

I don't understand where is the code which automatically deletes the firmware image object?
Moreover, the test below doesn't do much, right?

I am not inclined to merge this solution because it doesn't feel right, if we cannot just catch the exception and automatically delete the object, doing the other steps of creating a file do not look like something we should pursue because they may cause more harm than good and we will just have to live with the problem.

nemesifier · 2022-04-29T20:16:45Z

openwisp_firmware_upgrader/admin.py

+        try:
+            return super().change_view(request, object_id, form_url, extra_context)
+        except FileNotFoundError as e:
+            path = e.filename


Can't we just delete the object here, add a message for the user with the message framework and redirect to the admin list page?

Keep in mind this is an exceptional case which happens only if someone messes with the filesystem.

pandafy · 2022-06-10T16:01:05Z

@AbhigyaShridhar @ping!

pandafy · 2024-02-22T14:42:31Z

Closing this PR because of inactivity.

pandafy self-requested a review February 22, 2022 11:46

pandafy added this to In progress in OpenWISP Priorities for next releases via automation Feb 22, 2022

pandafy moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Feb 22, 2022

pandafy requested changes Feb 22, 2022

View reviewed changes

OpenWISP Priorities for next releases automation moved this from Ready for review/testing to In progress Feb 22, 2022

nemesifier closed this Mar 8, 2022

OpenWISP Priorities for next releases automation moved this from In progress to Done Mar 8, 2022

AbhigyaShridhar added 2 commits April 5, 2022 00:10

Merge branch 'openwisp:master' into issues/140-FileNotFound-exception

ba7ac15

Merge branch 'openwisp:master' into issues/140-FileNotFound-exception

c19f1cd

codesankalp reopened this Apr 26, 2022

OpenWISP Priorities for next releases automation moved this from Done to In progress Apr 26, 2022

AbhigyaShridhar added 2 commits April 26, 2022 22:38

nemesifier reviewed Apr 29, 2022

View reviewed changes

pandafy closed this Feb 22, 2024

OpenWISP Priorities for next releases automation moved this from In progress to Done Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[admin] Fixed FileNotFoundError #140 #180

[admin] Fixed FileNotFoundError #140 #180

AbhigyaShridhar commented Feb 22, 2022

pandafy left a comment

pandafy Feb 22, 2022

pandafy Feb 22, 2022

AbhigyaShridhar Feb 22, 2022

nemesifier commented Mar 8, 2022

AbhigyaShridhar commented Apr 20, 2022

nemesifier commented Apr 20, 2022

AbhigyaShridhar commented Apr 26, 2022 •

edited

AbhigyaShridhar commented Apr 26, 2022 •

edited

nemesifier left a comment

nemesifier Apr 29, 2022

pandafy commented Jun 10, 2022

pandafy commented Feb 22, 2024

[admin] Fixed FileNotFoundError #140 #180

[admin] Fixed FileNotFoundError #140 #180

Conversation

AbhigyaShridhar commented Feb 22, 2022

pandafy left a comment

Choose a reason for hiding this comment

pandafy Feb 22, 2022

Choose a reason for hiding this comment

pandafy Feb 22, 2022

Choose a reason for hiding this comment

AbhigyaShridhar Feb 22, 2022

Choose a reason for hiding this comment

nemesifier commented Mar 8, 2022

AbhigyaShridhar commented Apr 20, 2022

nemesifier commented Apr 20, 2022

AbhigyaShridhar commented Apr 26, 2022 • edited

AbhigyaShridhar commented Apr 26, 2022 • edited

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier Apr 29, 2022

Choose a reason for hiding this comment

pandafy commented Jun 10, 2022

pandafy commented Feb 22, 2024

AbhigyaShridhar commented Apr 26, 2022 •

edited

AbhigyaShridhar commented Apr 26, 2022 •

edited