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.

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

trying to delete a non existing blob in gcloud raises a NotFound exception #998

Closed
vchrisb opened this issue Apr 6, 2021 · 4 comments · Fixed by #999
Closed

trying to delete a non existing blob in gcloud raises a NotFound exception #998

vchrisb opened this issue Apr 6, 2021 · 4 comments · Fixed by #999

Comments

@vchrisb
Copy link
Contributor

vchrisb commented Apr 6, 2021

When trying to delete a blob with gcloud as backend, a 404 google.cloud.exceptions.NotFound exception is raised.
This is different when using S3 or Azure.

In Google docs: If the blob isn’t found (backend 404), raises a google.cloud.exceptions.NotFound.
https://googleapis.dev/python/storage/latest/buckets.html#google.cloud.storage.bucket.Bucket.delete_blob

This is due to missing exception handling around the delete_blob call in https://github.com/jschneier/django-storages/blob/master/storages/backends/gcloud.py#L165

    def delete(self, name):
        name = self._normalize_name(clean_name(name))
        self.bucket.delete_blob(name)

The azure backend is wrapping the `delete_blob' call with an exception handling:
https://github.com/jschneier/django-storages/blob/master/storages/backends/azure_storage.py#L228-L234

    def delete(self, name):
        try:
            self.service.delete_blob(
                container_name=self.azure_container,
                blob_name=self._get_valid_path(name),
                timeout=self.timeout)
        except AzureMissingResourceHttpError:
            pass
@sww314
Copy link
Contributor

sww314 commented Apr 15, 2021

Personally, I mostly use the gcloud implementation, so not familiar with the details of the other storages.

However, I do not think delete should mask what the client library returns. It is easy enough to add your own try/except. It is not easy to get the exception if the library hides it.

@vchrisb
Copy link
Contributor Author

vchrisb commented Apr 15, 2021

That might be true for other cases, but not raising an exception is the expected behaviour for a django storage. It currently makes the gcloud backend inconsistent to the other backend implementations and the django native file storage (https://github.com/django/django/blob/main/django/core/files/storage.py#L306).

@jschneier
Copy link
Owner

I find it surprising that the FileSystemStorage works that way, the code comments seem to indicate that it is due to possible concurrent access.

It's not clear to me the storage backends should be suppressing the error either.

@vchrisb
Copy link
Contributor Author

vchrisb commented Sep 15, 2021

@jschneier it may not ideal but it is consistent to all other implementations (including Django's native files storage implementation). Having one that is different makes things complicated and inconsistent.
e.g. Django admin does not have any error handling for that case.

Can you please consider merging the patch or ultimately decline it? In the latter I would at least create a pull request to document the inconsistency.
Thank you for the great package! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants