Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[bug] FileNotFoundError when trying to delete an image which links a non existing file #140

Open
nemesifier opened this issue Apr 27, 2021 · 11 comments
Open

[bug] FileNotFoundError when trying to delete an image which links a non existing file #140

nemesifier opened this issue Apr 27, 2021 · 11 comments
Labels
bug Something isn't working good first issue Good for newcomers Hacktoberfest Easy issues for attracting Hacktoberfest participants.

Comments

@nemesifier
Copy link
Member

nemesifier commented Apr 27, 2021 •
edited
Loading

This happens mostly during testing, but may happen during migrations as well.

How to replicate:

  • create a firmware build and upload an image
  • delete the image from the filesystem (not from openwisp)
  • delete the image from openwisp

Result:

Traceback (most recent call last):
  File "/lib/python3.8/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/lib/python3.8/site-packages/django/core/handlers/base.py", line 181, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/lib/python3.8/site-packages/django/contrib/admin/options.py", line 614, in wrapper
    return self.admin_site.admin_view(view)(*args, **kwargs)
  File "/lib/python3.8/site-packages/django/utils/decorators.py", line 130, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/lib/python3.8/site-packages/django/views/decorators/cache.py", line 44, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "/lib/python3.8/site-packages/django/contrib/admin/sites.py", line 233, in inner
    return view(request, *args, **kwargs)
  File "/openwisp-firmware-upgrader/openwisp_firmware_upgrader/admin.py", line 159, in change_view
    return super().change_view(request, object_id, form_url, extra_context)
  File "/lib/python3.8/site-packages/django_reversion-3.0.7-py3.8.egg/reversion/admin.py", line 154, in change_view
    return super().change_view(request, object_id, form_url, extra_context)
  File "/lib/python3.8/site-packages/django/contrib/admin/options.py", line 1656, in change_view
    return self.changeform_view(request, object_id, form_url, extra_context)
  File "/lib/python3.8/site-packages/django/utils/decorators.py", line 43, in _wrapper
    return bound_method(*args, **kwargs)
  File "/lib/python3.8/site-packages/django/utils/decorators.py", line 130, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/lib/python3.8/site-packages/django/contrib/admin/options.py", line 1534, in changeform_view
    return self._changeform_view(request, object_id, form_url, extra_context)
  File "/lib/python3.8/site-packages/django/contrib/admin/options.py", line 1579, in _changeform_view
    if all_valid(formsets) and form_validated:
  File "/lib/python3.8/site-packages/django/forms/formsets.py", line 464, in all_valid
    valid &= formset.is_valid()
  File "/lib/python3.8/site-packages/django/forms/formsets.py", line 308, in is_valid
    self.errors
  File "/lib/python3.8/site-packages/django/forms/formsets.py", line 288, in errors
    self.full_clean()
  File "/lib/python3.8/site-packages/django/forms/formsets.py", line 336, in full_clean
    form_errors = form.errors
  File "/lib/python3.8/site-packages/django/forms/forms.py", line 172, in errors
    self.full_clean()
  File "/lib/python3.8/site-packages/django/forms/forms.py", line 376, in full_clean
    self._post_clean()
  File "/lib/python3.8/site-packages/django/forms/models.py", line 405, in _post_clean
    self.instance.full_clean(exclude=exclude, validate_unique=False)
  File "/lib/python3.8/site-packages/django/db/models/base.py", line 1209, in full_clean
    self.clean_fields(exclude=exclude)
  File "/lib/python3.8/site-packages/django/db/models/base.py", line 1251, in clean_fields
    setattr(self, f.attname, f.clean(raw_value, self))
  File "/lib/python3.8/site-packages/django_private_storage-2.2.2-py3.8.egg/private_storage/fields.py", line 55, in clean
    file = data.file
  File "/lib/python3.8/site-packages/django/db/models/fields/files.py", line 43, in _get_file
    self._file = self.storage.open(self.name, 'rb')
  File "/lib/python3.8/site-packages/django/core/files/storage.py", line 36, in open
    return self._open(name, mode)
  File "/lib/python3.8/site-packages/django/core/files/storage.py", line 231, in _open
    return File(open(self.path(name), mode))

Solution:

  • catch the exception
  • log it with logger.exception() (it's an exceptional situation which needs to be logged in case anyone wants to debug)
  • avoid any other failure to ensure the FirmwareImage object gets deleted successfully from the DB
@nemesifier nemesifier added bug Something isn't working good first issue Good for newcomers labels Apr 27, 2021
@prachipancholi
Copy link

Hey, can you brief me what is the issue to be fix?

@nemesifier
Copy link
Member Author

Hey, can you brief me what is the issue to be fix?

@prachipancholi what is it that is not clear in the issue description?

@hanif-ali
Copy link
Contributor

I am working on this and have some doubts:

  • Should we silently delete the FirmwareImage after logging the exception or keep it as it is?
  • Instead of logging, we can also raise a ValidationError so that the user gets a message. What do you think of this approach?

@ghost
Copy link

ghost commented Aug 29, 2021

  • Should we silently delete the FirmwareImage after logging the exception or keep it as it is?

If the user intended to delete the FirmwareImage in the first place then I'm +1 for automatically deleting it.

@nemesifier nemesifier added the Hacktoberfest Easy issues for attracting Hacktoberfest participants. label Sep 21, 2021
@AbhigyaShridhar
Copy link
Contributor

Hey @nemesisdesign I think I can figure this one out! May I start working on it?

@nemesifier
Copy link
Member Author

I think it would be obvious that the FirmwareImage would have to be deleted, I mean, the original intention of the user was to delete it, now there's an exception, if the exception is catched the flow will resume normally and the image will be deleted. I have upgraded the issue description to reflect that.

Anyone who wants to work on this, don't ask for permission, just send a PR!

@nemesifier
Copy link
Member Author

It may be good to work on #170 before working on this, because this issue may disappear automatically if #170 is done.

@pandafy pandafy added this to the Release 0.2.0 milestone Jan 26, 2022
AbhigyaShridhar added a commit to AbhigyaShridhar/openwisp-firmware-upgrader that referenced this issue Feb 22, 2022
@AbhigyaShridhar
[admin] Fixed FileNotFoundError openwisp#140
0e78ce8 
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
@AbhigyaShridhar AbhigyaShridhar mentioned this issue Feb 22, 2022
Closed
AbhigyaShridhar added a commit to AbhigyaShridhar/openwisp-firmware-upgrader that referenced this issue Apr 26, 2022
@AbhigyaShridhar
[admin] fixed FileNotFoundError while deleting a FirmwareImage openwi…
a3e1263 
…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 added a commit to AbhigyaShridhar/openwisp-firmware-upgrader that referenced this issue Apr 26, 2022
@AbhigyaShridhar
[admin] fixed FileNotFoundError while deleting a FirmwareImage openwi…
739f0a2 
…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
AbhigyaShridhar added a commit to AbhigyaShridhar/openwisp-firmware-upgrader that referenced this issue Apr 26, 2022
@AbhigyaShridhar
[admin] Fixed FileNotFoundError while deleting a FirmwareImage openwi…
ba9e1cf 
…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 nemesifier removed this from the Release 1.0.0 milestone May 5, 2022
@Vihar214
Copy link

Vihar214 commented Dec 4, 2022 •
edited
Loading

Hello, Is there anyone working on this issue if not then can you assign this issue to Me?

@tspare
Copy link
Contributor

tspare commented Mar 30, 2023 •
edited by nemesifier
Loading

I think this is fixed. I could not reproduce this issue. This is my repo version:

commit 9c7840c (HEAD -> master, origin/master, origin/HEAD)
Date: Mon Jan 30 17:24:02 2023 -0300

@Ashish8329
Copy link

Hey @nemesifier , is this issue is open or need to fix?

@nemesifier
Copy link
Member Author

@Ashish8329 I am not sure if this issue is still valid, testing if you can reproduce it would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers Hacktoberfest Easy issues for attracting Hacktoberfest participants.
Projects
OpenWISP Contributor's Board
Status: In progress
OpenWISP Priorities for next releases
Status: To do
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[admin] Fixed FileNotFoundError #140 AbhigyaShridhar/openwisp-firmware-upgrader
8 participants
@nemesifier @pandafy @tspare @hanif-ali @AbhigyaShridhar @prachipancholi @Vihar214 @Ashish8329