Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot get useful information when an error occurs #30

Closed
wiktor-k opened this issue Aug 28, 2024 · 8 comments · Fixed by #31
Closed

Cannot get useful information when an error occurs #30

wiktor-k opened this issue Aug 28, 2024 · 8 comments · Fixed by #31

Comments

@wiktor-k
Copy link
Contributor

Hi,

I've observed the following behaviour: when using nethsm-sdk-rs and an API error occurs we're not getting real cause but rather error in serde: EOF while parsing a value at line 1 column 0.

As one practical example is the backup function (although I've seen it in other cases too).

If I'm not mistaken the actual issue is that on an API failure the code wants to deserialize the body of the response (in default_api.rs file: https://github.com/Nitrokey/nethsm-sdk-rs/blob/main/src/apis/default_api.rs#L2509) and the deserialization fails because the request has empty content (content-length: 0). (The deserialization in the backup case wants to create a SystemBackupPostError, looking at the source it's not clear how that would work without correct JSON in the error-case API response https://github.com/Nitrokey/nethsm-sdk-rs/blob/main/src/apis/default_api.rs#L561-L566).

It's quite possible I'm holding it wrong somehow so any suggestions are welcome!

(Our internal ticket: https://gitlab.archlinux.org/archlinux/signstar/-/issues/29 CC @dvzrv)

robin-nitrokey added a commit that referenced this issue Sep 9, 2024
Previously, we the generated code did not properly take into account the
HTTP status of error responses.  We would either return
*Error::UnknownError (if the response message was set) or a serde error
otherwise.

With this patch, we map the status codes to the corresponding error
variants and skip deserialization if the response body is empty.

Fixes: #30
@robin-nitrokey
Copy link
Member

Thanks for the report! Please check if #31 helps in this case. Also, if you know what kind of input triggers the problem, I can add a test case for it.

robin-nitrokey added a commit that referenced this issue Sep 9, 2024
Previously, the generated code did not properly take into account the
HTTP status of error responses.  We would either return
*Error::UnknownError (if the response message was set) or a serde error
otherwise.

With this patch, we map the status codes to the corresponding error
variants and skip deserialization if the response body is empty.

Fixes: #30
@wiktor-k
Copy link
Contributor Author

wiktor-k commented Sep 9, 2024

Also, if you know what kind of input triggers the problem, I can add a test case for it.

I think I know how to trigger that manually from a command line but let me get back to that and report back. Thanks for your time! 👋

@wiktor-k
Copy link
Contributor Author

Hi @robin-nitrokey,

I've tested it on one use case and it looks quite a lot better - instead of serde error we're getting nethsm_sdk_rs::apis::Error::ResponseError! Of course we need to map the status code 401 into a variant on our end but it's possible now.

By the way I've noticed that the error message is basically stuffed in the header: "www-authenticate": "Basic realm=\"invalid authorization\"". (I wonder if there's a more standard approach for passing error messages to the client in the works, the Error types are really tiny)

As far as I'm concerned we could consider this closed and if I notice it doesn't work in some other place I'm happy to create a new issue.

Thanks for your help and time! 👋

@robin-nitrokey
Copy link
Member

Thanks for the feedback! Generally, most endpoints return error messages as part of the body in a JSON document. But as this is not part of the OpenAPI spec, the generated error types don’t contain these strings. I’m not sure why there is no error message in the body in this case. My guess is that this is due to the control flow – authorization is handled by a lower level than the NetHSM business logic, and probably that level does not return error messages.

@wiktor-k
Copy link
Contributor Author

I’m not sure why there is no error message in the body in this case. My guess is that this is due to the control flow – authorization is handled by a lower level than the NetHSM business logic, and probably that level does not return error messages.

Thanks for confirming this! I'm planning to handle and save the error message string in the content body so I guess the login failure issue was not such a great use case... Now I need to look for another operation that can fail 🤔

@robin-nitrokey
Copy link
Member

I see. Maybe this test case is useful for you – if you call a normal operation on an unprovisioned instance, you should see an error message:

let err = default_api::keys_get(&config, None).err().unwrap();
match err {
Error::ResponseError(content) => {
assert_eq!(content.status, 412);
let err = String::from_utf8_lossy(&content.content);
assert!(
err.contains("Service not available"),
"unexpected error message: {err}"
);

To do it properly, you would need to decode error.content as JSON and try to access the msg (?) field.

@wiktor-k
Copy link
Contributor Author

To do it properly, you would need to decode error.content as JSON and try to access the msg (?) field.

I think it's called message:

Error: NetHsm(Api("Adding user failed: Status code: 412: {\"message\":\"Service not available\"}"))

I'm going through the ticket that was reported by @dvzrv and it seems we've seen the "serde error" only in case of login failure 🤔 The other errors are currently properly packed in an Error::ResponseError. One way or another the idea of parsing returned JSON is going to make it quite more readable.

I think it's good to go!

I've got one more thing but I'll create a separate ticket for that.

Thanks for your help and time! 🙇

@robin-nitrokey
Copy link
Member

Yes, the login error triggered that problem specifically because it had no body, so it could not be deserialized into a ResponseError.

robin-nitrokey added a commit that referenced this issue Sep 18, 2024
Previously, the generated code did not properly take into account the
HTTP status of error responses.  We would either return
*Error::UnknownError (if the response message was set) or a serde error
otherwise.

With this patch, we map the status codes to the corresponding error
variants and skip deserialization if the response body is empty.

Fixes: #30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants