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

Error handling audit #81

Closed
BrynCooke opened this issue Jul 6, 2021 · 8 comments
Closed

Error handling audit #81

BrynCooke opened this issue Jul 6, 2021 · 8 comments

Comments

@BrynCooke
Copy link
Contributor

Create a comprehensive list of all errors.
Ensure that error messages have:

  1. Information about what happened in a user friendly language.
  2. Information about what to do about the error in user friendly language.

Sensitive information must not be leaked.
Consider metrics for infrastructure errors.

@o0Ignition0o o0Ignition0o transferred this issue from another repository Nov 4, 2021
o0Ignition0o added a commit that referenced this issue Nov 24, 2021
fixes #81

Xtask was running npm install instead of npm clean-install, while the cache was looking at the package-lock.json checksum.This means linux and osx builds never would hit the cache, and upload to wrong keys.
@garypen garypen changed the title Error handing audit Error handling audit Apr 1, 2022
@Geal Geal added the api/1.0 label Jul 8, 2022
@Geal
Copy link
Contributor

Geal commented Jul 8, 2022

putting that in the api-1.0 effort, although I think it's mostly done?

@o0Ignition0o
Copy link
Contributor

Not sure if it belongs in here, but error handling in native plugins is pretty cumbersome as well. Is there an issue tracking this specifically?

@abernix
Copy link
Member

abernix commented Jul 26, 2022

@o0Ignition0o I don't think there is an issue for that (at least not that I'm aware of)

@SimonSapin
Copy link
Contributor

#1487 make some error enums private, but the remaining ones are still in need of an audit. I think some variants are not used anymore. And the enums should probably be made #[non_exhaustive] so we can add variants after 1.0.

@SimonSapin
Copy link
Contributor

SimonSapin commented Aug 11, 2022

In the spirit of #1142 (comment), we generally want to use a Vec<apollo_router::graphql::Error> in SomethingResponse value, rather than return Result::Err in Tower services.

What if we never used Result::Err, and make the error type std::convert::Infallible (an empty enum, impossible to construct) instead of BoxError?

@SimonSapin
Copy link
Contributor

make the error type std::convert::Infallible

Let’s… not. tower::Buffer uses BoxError, tower_test::Mock uses BoxError. Building types from the http crate is fallible. and And probably more constraints I haven’t found yet

@garypen
Copy link
Contributor

garypen commented Aug 18, 2022

If we wish to improve the quality of our generated error documentation:

In case you want to have an independent doc comment, the #[displaydoc("...") atrribute may be used on the variant or struct to override it.

We could use this mechanism to, for instance, number all of our errors: e.g.: router000001, etc...

@SimonSapin SimonSapin added this to the v1.0.0-alpha.0 milestone Aug 23, 2022
@SimonSapin
Copy link
Contributor

#1621 completes the audit of error-handling-related Rust APIs for 1.0. Removing the label and milestone, but leaving this issue open as in 1.x we’ll want to audit the presentation / formatting of errors in GraphQL responses and in logs, as well as maybe add error codes.

@SimonSapin SimonSapin removed this from the v1.0.0-alpha.0 milestone Aug 25, 2022
@SimonSapin SimonSapin removed the 1.0 label Aug 25, 2022
@abernix abernix modified the milestones: v1.0.0-alpha.0, v1.0.0-alpha.1, post-v1.x Aug 29, 2022
@abernix abernix removed this from the v1.x.x milestone Oct 17, 2022
@abernix abernix closed this as completed May 18, 2023
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

No branches or pull requests

6 participants