Handle deleted buckets gracefully

[jadkik]

About this change - What it does

Introduce the ensure_object_store_available parameter to allow creating a transfer object without necessarily creating the backing resources.

When that flag is passed the constructor will not attempt to create missing resources (such as storage buckets), which would allow consumers of the API to handle error cases without having to expose exceptions from rohmu's implementation details.

Instead they can call verify_object_storage to check for existence/access, and/or create_object_store_if_needed to create the bucket/container if it doesn't already exist.

Resolves: #xxxxx

Why this way

This provides a nicer interface to handle potential errors related to network issues/permission issues/etc outside of the constructor. It keeps the current behaviour by default for backwards compatibility.

It's a first step towards untangling the I/O from the initialization process.

This approach avoids adding additional exceptions thrown in the constructor, or alternatively relying even more on the content of the error message string.

An earlier draft of the PR was relying on an initialized flag, but new exceptions and methods are more flexible and provide a nicer API.

[rikonen]

The pattern here seems rather weird. Shouldn't we rather declare proper exception class for the case of failing to create a bucket and raise that instead of the current behavior of leaking random provider specific exceptions?

[jadkik]

@rikonen the reasoning was that:

1. this would be a step in the right direction to decouple the bucket creation logic out of the constructor
2. a new exception type that consumers of the library don't expect would be a backwards-incompatible change (i.e. some places that rely on catching Azure's 403 to handle things gracefully would suddenly start crashing)

[rikonen]

It is true that to make it not backward compatible some parameter to control the behavior would still be needed, and that would mean the interface is still a bit awkward. The initialized property is perhaps the thing that feels most problematic with the current approach. How about instead of using that we'd just ask the constructor not to perform any checks related to buckets and add a separate create_bucket_if_missing() method, which would raise a consistent exception?

So you'd have something like

transfer = get_transfer(..., skip_bucket_init=True)
try:
    transfer.create_bucket_if_missing()
except BucketCreateException as ex:
    ...

where the try/except block is optional and can be skipped if you know the bucket will exist.

[jadkik]

@rikonen

I picked the names to avoid the name bucket as I was modifying the base class that has Sftp and Local variants which don't have that concept and Swift which calls it a container. But then again with my names you'd have a parameter and property whose names are vague and do nothing in some cases.

We could have a solution that looks roughly like this:

def get_transfer(storage_config: Config, ensure_object_store_available: bool = True) -> BaseTransfer[Any]:
    """
    By default, any transfer object will make sure it can establish a connection and setup the necessary storage on initialization.
    When ensure_object_store_available is False, the backing storage (e.g. bucket) is not validated or created until first use.
    """
    transfer_obj = get_transfer_from_model(..., ensure_object_store_available=ensure_object_store_available)
    return transfer_obj


class BaseTransfer:
    def __init__(self, ..., ensure_object_store_available: bool = True):
        ...
        if ensure_object_store_available:
            self._verify_object_storage_unwrapped()
            self._create_object_store_if_needed_unwrapped()

    def _verify_object_storage_unwrapped(self):
        self.client.get(...)

    def verify_object_storage(self):
        try:
            self._verify_object_storage_unwrapped()
        except (HttpError, RuntimeError):
            raise TransferObjectStoreInitializationError()

    def _create_object_store_if_needed_unwrapped(self):
        self.client.post(...)

    def create_object_store_if_needed(self):
        try:
            self._create_object_store_if_needed_unwrapped()
        except (HttpError, RuntimeError):
            raise TransferObjectStoreCreateError()


class TransferObjectStoreConfigurationError(rohmu.errors.Error):
    """Raised when a transient network or permission issue does not allow us to validate access to the object store"""
    ...


class TransferObjectStoreMissingError(rohmu.errors.Error):
    """Raised when we know for sure the bucket is missing"""
    ...


class TransferObjectStoreCreateError(rohmu.errors.Error):
    """When we fail to create the object store, regardless of the underlying cause"""
    ...

With such an implementation, we preserve the behaviour of get_transfer(config): check and create buckets, do not wrap exceptions.

And it would cover our use case with such a usage:

transfer = get_transfer(config, ensure_object_store_available=False)
try:
    transfer.verify_object_storage()
except TransferObjectStoreConfigurationError:
    # This is probably a misconiguration or a misconfiguration, retry later
    ...
except TransferObjectStoreMissingError:
    # The bucket was deleted, do not treat this as an error
    pass

# We do not call create_object_store_if_needed() at all in this case

In a next major version, the current users would start using the API as follows to keep the current behviour:

transfer = get_transfer(..., ensure_object_store_available=False)
try:
    transfer.verify_object_storage()
    transfer.create_object_store_if_needed()
except rohmu.errors.Error:  # or more specific subclasses, do not catch google-specific or aws-specific exceptions 
    ...

# Operate on the transfer object as usual

[rikonen]

That suggestion sounds good to me

[Prime541]

LGTM

[rikonen]

I didn't read this fully but the interface looks reasonable

[jadkik]

Fixing this separately in #191

[jadkik]

I reverted the type: ignore comment and addressed it in #191

I also found a bug in the Azure changes related to permissions of tokens and pushed a fix for that (see diff)