You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "thinkharderdev (via GitHub)" <gi...@apache.org> on 2024/01/30 13:00:34 UTC
[I] object-store: status code from client errors are not always accessible [arrow-rs]
thinkharderdev opened a new issue, #5345:
URL: https://github.com/apache/arrow-rs/issues/5345
**Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
<!--
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
(This section helps Arrow developers understand the context and *why* for this feature, in addition to the *what*)
-->
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**
<!--
A clear and concise description of what you want to happen.
-->
The status code should be accessible since it can be important to error handling logic in application code
**Describe alternatives you've considered**
<!--
A clear and concise description of any alternative solutions or features you've considered.
-->
**Additional context**
<!--
Add any other context or screenshots about the feature request here.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-1916959674
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 Error`s 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-1917326089
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-1916904880
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 external 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 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-2049489784
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed issue #5345: object-store: status code from client errors are not always accessible
URL: https://github.com/apache/arrow-rs/issues/5345
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-1916965312
> Then why do we have error variants at all?
Some variants we expose don't have corresponding `ErrorKind`, e.g. Precondition
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-1917313262
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 ....
}
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-2049496104
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-1916803527
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,
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] object-store: status code from client errors are not always accessible [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on issue #5345:
URL: https://github.com/apache/arrow-rs/issues/5345#issuecomment-1916975966
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org