Better Errors

[0xDmtri]

Description

Slightly reworked PR #137.

Still not ready as further discussion has to be done before I can finalize it.

Improved error handling and exhaustive handling of all the possible error codes. In original implementation only two variants of enum were handled in conditional manner (which is not robust in this case). The proposed usage of pattern matching via "match" case is a better way of handling errors. I have proposed a new enum variant for an Error that returns the standardized error code instead.

TODO:

1. Decide on typing.
2. Integrate agreed method to tests.

FIXME:

Currently, as we are using both surf and reqwest, the StatusCode is a problem as both crates have their distinct types with different methods (one of the ideas was to use .as_u16() and store it as u16 inside the error enum, however, surf does not have such method implemented).
I tried a few things but seemed to be unable to come up with a unified solution that would pass all the GH workflows.

What I propose?

I propose making our own StatusCode with trait impl for all of the above types of StatusCode. This way, we will be able to have a cleaner code with just a single .into(), instead of adding bunch of feature gated imports etc.

Plz, let me know what you think.

Checklist

• Formatted code using cargo fmt --all
• Linted code using clippy
  • with reqwest feature: cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features serde,derive,reqwest-client-rustls -- -D warnings
  • with surf feature: cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features serde,derive,hyper-client -- -D warnings
• Updated README.md using cargo doc2readme -p influxdb --expand-macros
• Reviewed the diff. Did you leave any print statements or unnecessary comments?
• Any unfinished work that warrants a separate issue captured in an issue with a TODO code comment

[msrd0]

one of the ideas was to use .as_u16() and store it as u16 inside the error enum, however, surf does not have such method implemented

Well, surf uses http_types::StatusCode which is an enum tagged repr(u16), so you can write as u16 and get the status code. Is that what you're looking for?

I propose making our own StatusCode with trait impl for all of the above types of StatusCode.

I am not sure what your plan is with this struct. If it's just a wrapper over u16, I think we'd be better of converting surfs http_types::StatusCode to the proper http::StatusCode every other library uses.

[0xDmtri]

one of the ideas was to use .as_u16() and store it as u16 inside the error enum, however, surf does not have such method implemented

Well, surf uses http_types::StatusCode which is an enum tagged repr(u16), so you can write as u16 and get the status code. Is that what you're looking for?

I propose making our own StatusCode with trait impl for all of the above types of StatusCode.

I am not sure what your plan is with this struct. If it's just a wrapper over u16, I think we'd be better of converting surfs http_types::StatusCode to the proper http::StatusCode every other library uses.

Sure, sure, I was just not sure if you are ok with a primitive. On it.

[0xDmtri]

one of the ideas was to use .as_u16() and store it as u16 inside the error enum, however, surf does not have such method implemented

Well, surf uses http_types::StatusCode which is an enum tagged repr(u16), so you can write as u16 and get the status code. Is that what you're looking for?

I propose making our own StatusCode with trait impl for all of the above types of StatusCode.

I am not sure what your plan is with this struct. If it's just a wrapper over u16, I think we'd be better of converting surfs http_types::StatusCode to the proper http::StatusCode every other library uses.

Pushed

[msrd0]

Thanks for the changes. There is a test failure, could you take a look?

[0xDmtri]

Thanks for the changes. There is a test failure, could you take a look?

I think that failing tests are the result of our new error handling. Currently, you were only handling unauthorized or unauthenticated requests. However, as you can see in the code actions, the error code that causes tests fail is ApiError(400). This has not been handled before.

[msrd0]

Well, this is what CI reports. And there is no 400, just 401.

[0xDmtri]

Well, this is what CI reports. And there is no 400, just 401.

You are looking at something wrong. I literally just went thru all of them and its as I said 400. Plz look at my branch.

[0xDmtri]

@msrd0 have you had a chance to look at my comment above?

[msrd0]

Yes, I did have a look and it seems like you changed the code so that something that already didn't work now actually fails. I had asked @Empty2k12 to take a look but haven't heard back yet.

[0xDmtri]

Yes, I did have a look and it seems like you changed the code so that something that already didn't work now actually fails. I had asked @Empty2k12 to take a look but haven't heard back yet.

if u want, im happy to take it and fix it.

[Empty2k12]

Sorry for the delay. I am taking a look now.

[Empty2k12]

I think I have found the problem. Will push momentarily. We should consider exposing the message field (or full response body) from API errors that InfluxDB gives in the error. For example this was the error that I fixed, but surfacing it took quite some digging in the library code: "{ \"code\":\"invalid\",\"message\":\"failed to parse query: found MEASUREMENT, expected FROM, WHERE at line 1, char 8\"}"

[0xDmtri]

I think I have found the problem. Will push momentarily. We should consider exposing the message field (or full response body) from API errors that InfluxDB gives in the error. For example this was the error that I fixed, but surfacing it took quite some digging in the library code: "{ \"code\":\"invalid\",\"message\":\"failed to parse query: found MEASUREMENT, expected FROM, WHERE at line 1, char 8\"}"

Good idea imho

[Empty2k12]

Looks good. I will create a separate issue for adapting the errors further to include the error message from InfluxDB.