save() raises FileNotFoundError

[mnacharov]

Hello, Ilya!

We are using this lib in a while. In most cases it works perfect! Saves lots of space..

Resently my colleague found small issue with manual file removal.. If somewhere in code I already delete a file (or it is disappeared after django created an object) then .save() will raise FileNotFoundError

>>> obj = MyModel.object.all().first()
>>> obj.data.delete()
>>> obj.data = None
>>> obj.save()  # raises FileNotFoundError

May be it will be better to catch this exception inside this application? In case of, for example, using another library, which is know nothing about django_cleanup?

I'm ready to create pull request if you don't mind

[un1t]

Hi! PR would be greate, especially if you provide tests for this case.

[mnacharov]

Ok. I will do that! some day.. =)

[vinnyrose]

Why can't you catch the exception elsewhere? Perhaps someplace where you could best handle the exception? It's generally not the best decision to outright ignore an exception.

[vinnyrose]

I have had this exception before, and it warranted immediate attention and a fix to the server setup. Without this exception, the resulting 500 error, the resulting email notification, this error would have gone on unnoticed until a customer / client / user notified us, which is unacceptable. (<--see my next comment) If you are interacting with another library that is messing with files you should wrap those interactions with your own try catch, as you would be able to best handle the exception there instead of declaring that this exception is no longer exceptional because of an edge case. I am a strong NO on an PR for ignoring this exception.

Perhaps instead we can read the return value from cleanup_pre_delete and if we receive a False value we do not proceed with the delete and the cleanup_post_delete would not be called. But if the return value is None or True we continue as usual. Then in cleanup_pre_delete someone can determine if the file can be deleted or not. We may need to send additional information in the signal, such as the model and field being operated on.

[mnacharov]

@vinnyrose Can you give me an example, where raising this exception is essential?

[mnacharov]

I guess we are talking about two different cases... My point is: If we saves a model object with a file field containg None and file already was deleted that this is defenetely not exceptional. Or I missing something?

[vinnyrose]

I must have misremembered when I had this exception before, it must of been from something else entirely. I'm saying this because I just found that the default storage class in django will never raise this exception. Which must mean that you are using a different storage class from the default, is this correct?

So this leads to this point: You may have noticed that this library does not include the os library. This is because we are agnostic of the underlying storage. We have no idea if the file we are tracking is stored on disk, or in a database, or in a server on the other side of the world. In your case the exception is FileNotFoundError, but can you guarantee that this is the exception for every FileStorage class that will ever exists? I don't think you can. So we should not be catching an Exception that is specific to your Storage class. Perhaps the storage class that you are using should ignore this exception?

[mnacharov]

I guess you are right. I will find the actual reason of this exception and then come back with explanations

Thank you!

[vinnyrose]

I am going to add a bare except and log a warning that an exception happened in the upcoming version.

[vinnyrose]

Fixed in fff32f5.

[mnacharov]

Thanks!

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.