From 0e78ce88b3e0a50874e4d4baa5f82d1dd31164c1 Mon Sep 17 00:00:00 2001 From: AbhigyaShridhar Date: Tue, 22 Feb 2022 17:04:06 +0530 Subject: [PATCH 1/3] [admin] Fixed FileNotFoundError #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 #140 --- openwisp_firmware_upgrader/admin.py | 50 +++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/openwisp_firmware_upgrader/admin.py b/openwisp_firmware_upgrader/admin.py index e39f7128..a85ceec0 100644 --- a/openwisp_firmware_upgrader/admin.py +++ b/openwisp_firmware_upgrader/admin.py @@ -1,4 +1,5 @@ import logging +import os from datetime import timedelta import reversion @@ -7,7 +8,7 @@ from django.conf import settings from django.contrib import admin, messages from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME -from django.shortcuts import redirect +from django.shortcuts import HttpResponseRedirect, redirect from django.template.response import TemplateResponse from django.urls import resolve, reverse from django.utils.safestring import mark_safe @@ -84,6 +85,37 @@ def has_change_permission(self, request, obj=None): return False return True + def get_parent_object(self, request): + """ + method to get the instance from model' s parent class + in context with the ForeignKey relation + """ + resolved = resolve(request.path_info) + if resolved.kwargs: + return self.parent_model.objects.get(pk=resolved.kwargs["object_id"]) + return None + + def get_queryset(self, request): + """ + overriding queryset to remove any FirmwareImage instances, + where the image file has been manually deleted by the user + from the file system + """ + qs = super(FirmwareImageInline, self).get_queryset(request) + build = self.get_parent_object(request) + qs = qs.filter(build=build) + 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 + class CategoryFilter(MultitenantOrgFilter): multitenant_lookup = 'organization_id__in' @@ -192,7 +224,21 @@ def change_view(self, request, object_id, form_url='', extra_context=None): extra_context = extra_context or {} upgrade_url = f'{app_label}_build_changelist' extra_context.update({'upgrade_url': upgrade_url}) - return super().change_view(request, object_id, form_url, extra_context) + # preventing change_view to throw an error/exception + try: + return super().change_view(request, object_id, form_url, extra_context) + except Exception as e: + 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) class UpgradeOperationForm(forms.ModelForm): From a3e1263cb3b7366048793ddfdbfb1e0eb1fc25ba Mon Sep 17 00:00:00 2001 From: AbhigyaShridhar Date: Tue, 26 Apr 2022 20:36:05 +0530 Subject: [PATCH 2/3] [admin] fixed FileNotFoundError while deleting a FirmwareImage #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 #140 --- openwisp_firmware_upgrader/admin.py | 62 +++++-------------- .../tests/test_models.py | 6 ++ 2 files changed, 23 insertions(+), 45 deletions(-) diff --git a/openwisp_firmware_upgrader/admin.py b/openwisp_firmware_upgrader/admin.py index 3cc464c7..116fcebe 100644 --- a/openwisp_firmware_upgrader/admin.py +++ b/openwisp_firmware_upgrader/admin.py @@ -8,7 +8,7 @@ from django.conf import settings from django.contrib import admin, messages from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME -from django.shortcuts import HttpResponseRedirect, redirect +from django.shortcuts import redirect from django.template.response import TemplateResponse from django.urls import resolve, reverse from django.utils.safestring import mark_safe @@ -80,37 +80,6 @@ def has_change_permission(self, request, obj=None): return False return True - def get_parent_object(self, request): - """ - method to get the instance from model' s parent class - in context with the ForeignKey relation - """ - resolved = resolve(request.path_info) - if resolved.kwargs: - return self.parent_model.objects.get(pk=resolved.kwargs["object_id"]) - return None - - def get_queryset(self, request): - """ - overriding queryset to remove any FirmwareImage instances, - where the image file has been manually deleted by the user - from the file system - """ - qs = super(FirmwareImageInline, self).get_queryset(request) - build = self.get_parent_object(request) - qs = qs.filter(build=build) - 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 - class CategoryFilter(MultitenantOrgFilter): multitenant_lookup = 'organization_id__in' @@ -211,21 +180,24 @@ def change_view(self, request, object_id, form_url='', extra_context=None): extra_context = extra_context or {} upgrade_url = f'{app_label}_build_changelist' extra_context.update({'upgrade_url': upgrade_url}) - # preventing change_view to throw an error/exception + # preventing change_view to throw an error try: return super().change_view(request, object_id, form_url, extra_context) - except Exception as e: - 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) + except FileNotFoundError as e: + path = e.filename + directories = path.split('/') + n = len(directories) + dirPath = "" + for i in range(0, n - 1): + if i > 0: + dirPath = dirPath + '/' + dirPath = dirPath + directories[i] + if not os.path.isdir(dirPath): + os.mkdir(dirPath) + f = open(e.filename, "w+") + f.write("To be deleted") + f.close() + return self.change_view(request, object_id, form_url, extra_context) class UpgradeOperationForm(forms.ModelForm): diff --git a/openwisp_firmware_upgrader/tests/test_models.py b/openwisp_firmware_upgrader/tests/test_models.py index 723c4b16..c66c21ba 100644 --- a/openwisp_firmware_upgrader/tests/test_models.py +++ b/openwisp_firmware_upgrader/tests/test_models.py @@ -132,6 +132,12 @@ def test_device_fw_image_changed(self, *args): self.assertEqual(UpgradeOperation.objects.count(), 1) self.assertEqual(BatchUpgradeOperation.objects.count(), 0) + def test_device_fw_image_deleted(self, *args): + with mock.patch( + f'{self.app_label}.models.UpgradeOperation.upgrade', return_value=None + ): + pass + def test_device_fw_created(self, *args): with mock.patch( f'{self.app_label}.models.UpgradeOperation.upgrade', return_value=None From 739f0a2354a5926321494913918fd5825938b6c2 Mon Sep 17 00:00:00 2001 From: AbhigyaShridhar Date: Tue, 26 Apr 2022 20:36:05 +0530 Subject: [PATCH 3/3] [admin] fixed FileNotFoundError while deleting a FirmwareImage #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 #140 --- openwisp_firmware_upgrader/admin.py | 62 +++++-------------- .../tests/test_models.py | 6 ++ 2 files changed, 23 insertions(+), 45 deletions(-) diff --git a/openwisp_firmware_upgrader/admin.py b/openwisp_firmware_upgrader/admin.py index 3cc464c7..116fcebe 100644 --- a/openwisp_firmware_upgrader/admin.py +++ b/openwisp_firmware_upgrader/admin.py @@ -8,7 +8,7 @@ from django.conf import settings from django.contrib import admin, messages from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME -from django.shortcuts import HttpResponseRedirect, redirect +from django.shortcuts import redirect from django.template.response import TemplateResponse from django.urls import resolve, reverse from django.utils.safestring import mark_safe @@ -80,37 +80,6 @@ def has_change_permission(self, request, obj=None): return False return True - def get_parent_object(self, request): - """ - method to get the instance from model' s parent class - in context with the ForeignKey relation - """ - resolved = resolve(request.path_info) - if resolved.kwargs: - return self.parent_model.objects.get(pk=resolved.kwargs["object_id"]) - return None - - def get_queryset(self, request): - """ - overriding queryset to remove any FirmwareImage instances, - where the image file has been manually deleted by the user - from the file system - """ - qs = super(FirmwareImageInline, self).get_queryset(request) - build = self.get_parent_object(request) - qs = qs.filter(build=build) - 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 - class CategoryFilter(MultitenantOrgFilter): multitenant_lookup = 'organization_id__in' @@ -211,21 +180,24 @@ def change_view(self, request, object_id, form_url='', extra_context=None): extra_context = extra_context or {} upgrade_url = f'{app_label}_build_changelist' extra_context.update({'upgrade_url': upgrade_url}) - # preventing change_view to throw an error/exception + # preventing change_view to throw an error try: return super().change_view(request, object_id, form_url, extra_context) - except Exception as e: - 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) + except FileNotFoundError as e: + path = e.filename + directories = path.split('/') + n = len(directories) + dirPath = "" + for i in range(0, n - 1): + if i > 0: + dirPath = dirPath + '/' + dirPath = dirPath + directories[i] + if not os.path.isdir(dirPath): + os.mkdir(dirPath) + f = open(e.filename, "w+") + f.write("To be deleted") + f.close() + return self.change_view(request, object_id, form_url, extra_context) class UpgradeOperationForm(forms.ModelForm): diff --git a/openwisp_firmware_upgrader/tests/test_models.py b/openwisp_firmware_upgrader/tests/test_models.py index 723c4b16..c66c21ba 100644 --- a/openwisp_firmware_upgrader/tests/test_models.py +++ b/openwisp_firmware_upgrader/tests/test_models.py @@ -132,6 +132,12 @@ def test_device_fw_image_changed(self, *args): self.assertEqual(UpgradeOperation.objects.count(), 1) self.assertEqual(BatchUpgradeOperation.objects.count(), 0) + def test_device_fw_image_deleted(self, *args): + with mock.patch( + f'{self.app_label}.models.UpgradeOperation.upgrade', return_value=None + ): + pass + def test_device_fw_created(self, *args): with mock.patch( f'{self.app_label}.models.UpgradeOperation.upgrade', return_value=None