feat(object_store): publicly expose client error

[kyle-mccarthy]

Which issue does this PR close?

Closes #5345.

Rationale for this change

Exposing the client error publicly allows accessing the status code of the response associated with the error. This gives the user more control over error handling, such as special handling for authn/authz errors.

let err = store.head(&path).await.unwrap_err();
let source = err
    .source()
    .unwrap()
    .downcast_ref::<object_store::ClientError>().unwrap();
let status = source.status().unwrap();

What changes are included in this PR?

expose client::Error as ClientError

Are there any user-facing changes?

Yes

Notes

• Should the error type or its variants be renamed?
• Alternatively, a PermissionDenied variant could be added to crate::Error.
• Alternatively, source could be changed to reqwest::Error when the client error is converted to crate::Error. (Would allow downcasting directly to reqwest::Error.)

[alamb]

Thanks @kyle-mccarthy 🙏

It seems like that since the errors are now public the CI demands some of the methods be documented: https://github.com/apache/arrow-rs/actions/runs/10167031274/job/28118484246?pr=6158

[kyle-mccarthy]

@alamb I noticed that the main error variant suppresses this warning via #[allow(missing_docs)], is that acceptable here too?

Also, I have a branch that adds a PermissionDenied variant, like the original ticket discussed. Let me know if this approach is preferable and I can repoint this PR.

[alamb]

@alamb I noticed that the main error variant suppresses this warning via #[allow(missing_docs)], is that acceptable here too?

Is there a reason not to document them? For example would they add no value over the names of the variants themselves?

In my simple mind more documentation is always better than less (though I realize not everyone agrees with this assesment).

[kyle-mccarthy]

@alamb Not at all, I added comments to the error enum!

Note that I restricted the visibility of the error() method to the crate, I couldn't think of a valid reason to expose it publicly. Also, I had to conditionally compile it to resolve a failing check due to dead code.

[alamb]

Looks good to me -- thank you @kyle-mccarthy

[tustvold]

If we do this I think we should make the variants opaque, and expose information with accessors. Otherwise it is going to be very hard to evolve this without introducing breaking changes

[alamb]

I think keeping the underlying error opaque is pretty hard to use and clearly there appears to be a need to get finer grained information about what the error was

Otherwise it is going to be very hard to evolve this without introducing breaking changes

Perhaps another possibility is to mark it as [#non_exhaustive] https://doc.rust-lang.org/reference/attributes/type_system.html

If we do this I think we should make the variants opaque, and expose information with accessors.

Are you suggesting something like

impl Error {
  fn is_redirect(&self) -> bool {..}
  fn status_code(&self) -> Option<StatusCode> {..}
...
}

[tustvold]

Are you suggesting something like

Yes, this is also what the underlying HTTP clients do as well. My aprehension is we're exposing a fair amount of internal details around how we perform requests, retries, etc... that seems undesirable.

non-exhaustive would help for adding new variants, but we'd be unable to change existing. If say we wanted to add more context to a retry error message, this would be a breaking change

[tustvold]

I'm rather apprehensive about this as it exposes a fair bit of internal implementation details. Perhaps we could take a step back and address the motivating use-case, as then we can potentially develop a more targeted solution? The original ticket was centered on permission denied errors, and the proposal was to add a variant to represent this, is this the same request or something different?

[alamb]

cc @crepererum and @thinkharderdev for your thoughts

[alamb]

I think keeping the underlying error opaque is pretty hard to use and clearly there appears to be a need to get finer grained information about what the error was

Otherwise it is going to be very hard to evolve this without introducing breaking changes

Perhaps another possibility is to mark it as [#non_exhaustive] https://doc.rust-lang.org/reference/attributes/type_system.html

If we do this I think we should make the variants opaque, and expose information with accessors.

Are you suggesting something like

impl Error {
  fn is_redirect(&self) -> bool {..}
  fn status_code(&self) -> Option<StatusCode> {..}
...
}

[alamb]

If someone has time to design a better error abstraction for the client I would be happy to try and review it. However, I don't think I would likely have the time do to so however

[tustvold]

My personal feeling is the better abstraction is to add a permission denied variant to the top-level error, as originally stated on the ticket. This will work across all stores, is easily discoverable and easy to understand. The problem is for historical reasons this isn't marked non-exhaustive and so adding this would be a breaking change.

So either we defer this to the next breaking release, or we do something in the middleground to expose this variant in a non-breaking fashion.

One stopgap option could simply be to add an opaque PermissionError that the generic error can be downcast to.

It all really depends on what the use-case is and how acute the need is

[kyle-mccarthy]

@tustvold I understand your concerns and feel like they are valid. I have a branch that takes an alternative approach and surfaces permission problems though a PermissionDenied variant on the top level object_store::Error enum.

I am primarily wanting to be able able to identify permission related problems, so either direction would help me.

However, I think the core problem would still exist if just the PermissionDenied variant was added. The core problem being that object_store uses private types (albeit type erased) in the public source field of object_store::Error::Generic variant. This forces downstream users of the crate to handle all generic errors the same way, or resort to parsing the stringified error message.

[tustvold]

This forces downstream users of the crate to handle all generic errors the same way, or resort to parsing the stringified error message.

Right, the alternative being we expose public types and those types then become part of the public API, but in a somewhat obfuscated manner. This gets particularly spicy if one exposes foreign types, which we probably do in some places, then not only are these crates part of your public API but if those crate's make a breaking release any error handling code will happily build against a different version, and just silently not work 😄

The approach as it stands is to have explicit semantic error categories, this gives a lot of freedom to make changes to the implementations, and has a lot of precedent in the ecosystem (e.g. hyper, reqwest) and the standard library (std::io::Error), but does come with downsides should a particular semantic category be omitted.

Edit: I've created #6165 as I think it is something we should do anyway

[crepererum]

This PR kinda sidesteps / ignores the discussion in #5345 that -- more or less -- concluded that we do NOT want to expose too many implementation details.

[alamb]

This PR kinda sidesteps / ignores the discussion in #5345 that -- more or less -- concluded that we do NOT want to expose too many implementation details.

So what is the path forward here?

Users continue to parse the text of the error messages if they need more detail on what error occured?

Someone (who?) creates a more detailed error categorization?

Something else?

[crepererum]

Someone (who?) creates a more detailed error categorization?

This, by whoever steps up and does the work.

let's get consensus before merging

[kyle-mccarthy]

Closing this in favor of #6194.