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

more consistent errors #2178

Merged
merged 13 commits into from
Dec 15, 2022
Merged

more consistent errors #2178

merged 13 commits into from
Dec 15, 2022

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Nov 29, 2022

related to #2101
This is a draft/attempt to have more consistent errors in the router following this spec https://www.apollographql.com/docs/apollo-server/data/errors/ with the error extension code.

Signed-off-by: Benjamin Coenen <[email protected]>
@bnjjj bnjjj self-assigned this Nov 29, 2022
@github-actions

This comment has been minimized.

Copy link
Contributor

@BrynCooke BrynCooke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks good, but let's remove to_shouty_snake_case.

apollo-router/src/error.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the look of this PR. I'd like to see some examples of how errors will look before/after. I can see that a bit from the way tests have changed, but it would be nice to see it written out maybe in an issue or at least on the PR.

If we made this change would it effectively be a router 2.0 change? I mean do we think this is part of our public API?

@bnjjj bnjjj requested a review from BrynCooke December 13, 2022 10:22
@bnjjj
Copy link
Contributor Author

bnjjj commented Dec 13, 2022

@garypen I don't think it should be a 2.0 change. Because we didn't really remove something, we just added more context to errors so it's additive

@bnjjj bnjjj requested review from garypen and Geal December 13, 2022 10:23
@bnjjj bnjjj marked this pull request as ready for review December 13, 2022 10:40
@bnjjj bnjjj merged commit 4b6f62a into dev Dec 15, 2022
@bnjjj bnjjj deleted the bnjjj/fix_2101 branch December 15, 2022 10:20
@abernix abernix added this to the v1.7.0 milestone Dec 22, 2022
@abernix abernix mentioned this pull request Dec 23, 2022
message: String,
locations: Vec<Location>,
path: Option<Path>,
extension_code: T,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new method should be added in the documentation:

    /// Returns a builder that builds a GraphQL [`Error`] from its components.
    ///
    /// Builder methods:
    ///
    /// * `.message(impl Into<`[`String`]`>)`
    ///   Required.
    ///   Sets [`Error::message`].
    ///
    /// * `.locations(impl Into<`[`Vec`]`<`[`Location`]`>>)`
    ///   Optional.
    ///   Sets the entire `Vec` of [`Error::locations`], which defaults to the empty.
    ///
    /// * `.location(impl Into<`[`Location`]`>)`
    ///   Optional, may be called multiple times.
    ///   Adds one item at the end of [`Error::locations`].
    ///
    /// * `.path(impl Into<`[`Path`]`>)`
    ///   Optional.
    ///   Sets [`Error::path`].
    ///
    /// * `.extensions(impl Into<`[`serde_json_bytes::Map`]`<`[`ByteString`]`, `[`Value`]`>>)`
    ///   Optional.
    ///   Sets the entire [`Error::extensions`] map, which defaults to empty.
    ///
    /// * `.extension(impl Into<`[`ByteString`]`>, impl Into<`[`Value`]`>)`
    ///   Optional, may be called multiple times.
    ///   Adds one item to the [`Error::extensions`] map.
    ///
    /// * `.build()`
    ///   Finishes the builder and returns a GraphQL [`Error`].

We need to add .extension_code(impl Into<[String]>)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue #2469 Thanks a lot

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 this pull request may close these issues.

6 participants