object-store: status code from client errors are not always accessible

[thinkharderdev]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

When we get a client error (HTTP status code 4xx), the status code is not always accessible from client code.

                                return Err(match status.is_client_error() {
                                    true => match r.text().await {
                                        Ok(body) => {
                                            Error::Client {
                                                body: Some(body).filter(|b| !b.is_empty()),
                                                status,
                                            }
                                        }
                                        Err(e) => {
                                            Error::Reqwest {
                                                retries,
                                                max_retries,
                                                elapsed: now.elapsed(),
                                                retry_timeout,
                                                source: e,
                                            }
                                        }
                                    }
                                    false => Error::Reqwest {
                                        retries,
                                        max_retries,
                                        elapsed: now.elapsed(),
                                        retry_timeout,
                                        source: e,
                                    }
                                });

if there is a response body, we ultimately return a generic error with source of type object_store::client::retry::Error::Client

Since object_store::client::retry::Error is crate private you cannot downcast the source to this in client code and hence cannot access the status code

Describe the solution you'd like

The status code should be accessible since it can be important to error handling logic in application code

Describe alternatives you've considered

Additional context

[thinkharderdev]

Note that there is currently a workaround since the you can parse the status code from the error display format but you have to do something like this which is quite hacky :)

pub fn is_permission_denied_error(error: &object_store::Error) -> bool {
    match error {
        object_store::Error::Generic { source, .. } => {
            let mut inner: &dyn StdError = source.as_ref();
            loop {
                if let Some(err) = inner.downcast_ref::<reqwest::Error>() {
                    return matches!(err.status(), Some(StatusCode::FORBIDDEN));
                }

                if format!("{inner}").contains("Client error with status 403") {
                    return true;
                }

                if let Some(err) = inner.source() {
                    inner = err;
                } else {
                    return false;
                }
            }
        }
        _ => false,
    }
}

[tustvold]

I don't think we should expose the retry error, but we could conceivably expose a status method on object_store::Error. However, this would be specific to HTTP based stores, and wouldn't work for LocalFileSystem.

Some other options might be:

• Add method to object_store that returns std::io::ErrorKind
• Change generic variant to std::io::Error
• Add a new variant for permission errors to object_store::Error (and make the enum non exhaustive at the same time)

Of these the first has the advantage of not being a breaking change, but is perhaps a little odd. The second would likely be very disruptive. The third would likely be the most consistent with the current approach, but would be a breaking change.

To be honest I am not wedded to any particular approach, they're all some measure of not amazing 😅

[crepererum]

I agree that exposing the HTTP status code directly breaks the abstraction that object_store offers. If you wanna do HTTP shenanigans, use an HTTP client. Also exposing dyn Errors that you can downcast is technically a semantic-versioning backdoor / issue, because changing errors (e.g. due to implementation changes or indirect library upgrades) would easily slip through reviews or automated checks, but will technically break downstream code.

That said there's semantic information like "permission denied" that can be important. The option mentioned by @tustvold work all fulfill this goal but have varying API "smoothness":

Add method to object_store that returns std::io::ErrorKind

Then why do we have error variants at all?

Change generic variant to std::io::Error

Sounds good, question is if we ever need any features not covered by this error type. Using a more standard error sounds IMHO better than the current state. That's breaking of course.

Add a new variant for permission errors to object_store::Error (and make the enum non exhaustive at the same time)

That's consistent w/ the current handling. I think we should make "new variants" not breaking in the future by decorating the error types with "non exhaustive" (stable for ages by now).

[tustvold]

Then why do we have error variants at all?

Some variants we expose don't have corresponding ErrorKind, e.g. Precondition, but I have debated exposing io::Error in the past

[crepererum]

Then why do we have error variants at all?

Some variants we expose don't have corresponding ErrorKind, e.g. Precondition, but I have debated exposing io::Error in the past

Then I think we shouldn't use ErrorKind. While it's non-breaking, it creates a second error enumeration that is also partly incomplete. To work with the errors, you then have to handle both enumerations (the actual error enum and the kind) and both only tell you half of the story or may even provide conflicting information.

[thinkharderdev]

What about adding a enum object_store::ErrorKind and then have

pub enum Error {
    #[snafu(display("Generic {} error: {}", store, source))]
    Generic {
        store: &'static str,
        kind: Option<ErrorKind>,
        source: Box<dyn std::error::Error + Send + Sync + 'static>,
    },
    ...
}

This is somewhat redundant because object_store::ErrorKind would be almost the same as std::io::ErrorKind but would give us flexibility to be precise,

This would of course also be a breaking change as written above but could also be done as a non-breaking change if we internalized it as a method, so

pub enum ErrorKind {
  InvalidInput,
  IO,
  PermissionDenied,
  PreconditionFailed,
  ...
}

impl From<&StatusCode> for ErrorKind {
   ... map HTTP status codes to kinds
}

impl Error {
   pub fn kind(&self) -> Option<object_store::ErrorKind> {
      match self {
          Error::Generic { source, .. } => {
            match source.downcast_ref::<retry::Error>() {
               Some(retry::Error::Client { status, .. }) => return Some(status.into()),
               ...,
               None => ()
            };

            ... other mappings ....
          }
      }

   }
}

[tustvold]

I think an ErrorKind abstraction combined with an opaque struct, similar to io::Error, is how I would implement this if I were writing the crate today. However, from a functionality perspective this is not materially different from the current approach using enum variants and would represent a fairly major breaking change. I therefore think adding a Permission error variant is probably my preferred path forward here

[thinkharderdev]

Coming back to this. I agree that maybe exposing HTTP status codes breaks the abstraction, but I think the more basic issue is that we don't expose PermissionDenied errors in a way that is easy to determine in client code. It seems like this should be a first-class error variant? All object stores that are supported have some notion of permissions.

[crepererum]

Coming back to this. I agree that maybe exposing HTTP status codes breaks the abstraction, but I think the more basic issue is that we don't expose PermissionDenied errors in a way that is easy to determine in client code. It seems like this should be a first-class error variant? All object stores that are supported have some notion of permissions.

Sounds reasonable to me.