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

GraphQL specification violation in how Juniper handled field error propagation inside of fragment spreads #1287

Open
cmtm opened this issue Oct 4, 2024 · 1 comment
Assignees
Labels
bug Something isn't working k::design Related to overall design and/or architecture p::major Major priority for implementation
Milestone

Comments

@cmtm
Copy link

cmtm commented Oct 4, 2024

Describe the bug
I believe Juniper isn't spec compliant in how it's handling field error propagation.
Relevant excerpt from the GraphQL spec about how to handle field errors on non-nullable fields.

Since Non-Null type fields cannot be null, field errors are propagated to be handled by the parent field. If the parent field may be null then it resolves to null, otherwise if it is a Non-Null type, the field error is further propagated to its parent field.

Juniper follows this behavior when the query doesn't have any fragments, but if the field error only occurs in a fragment, "null" won't propagate to the closest nullable ancestor field, but instead seems to just make the fields in the fragment null, while not affecting fields outside of the fragment.

My interpretation of the GraphQL spec is that the way a query is partitioned into fragments shouldn't affect the query response. I'm not completely sure of this, but I also can't find any mention of how fragments should change the way field errors are handled.

To Reproduce
Unit test demonstrating divergence in behavior

use juniper::{graphql_object, EmptyMutation, EmptySubscription, Variables};

struct MyObject;

#[graphql_object]
impl MyObject {
    fn erroring_field() -> Result<i32, &'static str> {
        Err("This field always errors")
    }
}

// Arbitrary context data.
struct Ctx;

impl juniper::Context for Ctx {}

struct Query;

#[graphql_object]
#[graphql(context = Ctx)]
impl Query {
    fn my_object() -> MyObject {
        MyObject {}
    }

    fn just_a_field() -> i32 {
        3
    }
}

type Schema = juniper::RootNode<'static, Query, EmptyMutation<Ctx>, EmptySubscription<Ctx>>;

fn main() {
    let execute_query = |query: &str| {
        let ctx = Ctx;
        juniper::execute_sync(
            query,
            None,
            &Schema::new(Query, EmptyMutation::new(), EmptySubscription::new()),
            &Variables::new(),
            &ctx,
        )
        .unwrap()
    };

    let (res, _) = execute_query("{ myObject { erroringField } justAField }");
    let (res_fragment, _) = execute_query(
        "query { myObject { ...MyObjectFragment } justAField } fragment MyObjectFragment on MyObject { erroringField }",
    );

    assert_eq!(res, res_fragment);
}

Expected behavior
Expect field errors that occur inside fragments to propagate nullability to their closest nullable ancestor, even if that ancestor is outside of the fragment itself.
equivalently:
Expect above code to not panic.

Additional context
N/A

@cmtm cmtm added bug Something isn't working needs-triage labels Oct 4, 2024
@tyranron tyranron added p::major Major priority for implementation and removed needs-triage labels Oct 29, 2024
@tyranron tyranron self-assigned this Oct 29, 2024
@tyranron tyranron added this to the 0.17.0 milestone Oct 29, 2024
@tyranron
Copy link
Member

@cmtm thanks for finding it. Will definitely look into this before cutting a new release.

@tyranron tyranron added the k::design Related to overall design and/or architecture label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working k::design Related to overall design and/or architecture p::major Major priority for implementation
Projects
None yet
Development

No branches or pull requests

2 participants