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

Federated queries are not correctly resolved if one of subgraphs is unavailable #177

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kroupacz
Copy link

@kroupacz kroupacz commented Nov 24, 2024

Imagine a simple federated query:

query {
  testNestedField {
    subgraph1 { # Everything inside this field is resolved by subgraph1
      testSuccessQuery {
        id
        email
        sub1
      }
    }
    subgraph2 { # Everything inside this field is resolved by subgraph2
      testSuccessQuery {
        id
        email
        sub2
      }
    }
  }
}

If subraph2 is unavailable, the query should be resolved as follows:

{
  testNestedField: {
    subgraph1: {
      testSuccessQuery: {
        id: 'user1',
        email: '[email protected]',
        sub1: true,
      },
    },
    subgraph2: {
      testSuccessQuery: null
    },
  },
}

In case of a subgrah2 outage, Apollo Gateway will resolve the above-mentioned query exactly as shown in the example. Unfortunately, GraphQL Yoga used as gateway does not work properly in this case and the result is also strangely affected by non/nullable parent fields even if the actual query result has always nullable type.

I didn't find the exact location where the problem occurs, but I would guess that it's somewhere in the @graphql-tools/executor-http package.

Explanation of the tests which can be found in this PR.
The first two test cases in describe('EXPECTED RESPONSE - EVERYTHING OK') when subgraph2 actually returns an error from its resolver return the expected response. These are just example tests of how it should work and result should be the same even if the subgrah2 is down.

The other 4 test cases in describe('UNEXPECTED RESPONSE - FAILING TESTS') when subgraph2 is actually unavailable always return a different and wrong response.

  • test subgraph2_Nullable.testErrorQuery - subgraph2 is unavailable

    • unexpected response data and errors got completely lost somewhere along the way
  • test subgraph2_NonNullable.testErrorQuery - subgraph2 is unavailable

    • unexpected response data and wrong error due to NonNullable "parent" field (GQL result type of testErrorQuery is actually nullable)
  • test subgraph2_Nullable.testErrorQuery only - subgraph2 is unavailable

    • unexpected response data, but the error is almost correct. Only error path is incomplete and should be ['testNestedField', 'subgraph2_Nullable', 'testErrorQuery']
  • test subgraph2_NonNullable.testErrorQuery only - subgraph2 is unavailable

    • unexpected response data, but the error is almost correct. Only error path is incomplete and should be ['testNestedField', 'subgraph2_NonNullable', 'testErrorQuery']

@kroupacz kroupacz force-pushed the bug/unavailable-subgraph branch from 7bb64ac to 46e2791 Compare November 24, 2024 18:47
@ardatan
Copy link
Member

ardatan commented Nov 24, 2024

Thanks for creating the PR, we'll look into it but it seems something is wrong with your setup.

@kroupacz kroupacz force-pushed the bug/unavailable-subgraph branch from 46e2791 to 81ad9fa Compare November 25, 2024 06:39
@kroupacz
Copy link
Author

Hello @ardatan,
thanks for quick reply.
I pushed the fix for my TestEnvironment.ts. It should work now in your CI environment.
I would like to point out that my "failing tests" are written in such a way that they are not failing 😇, but are described in detail in comments where it should be clear what is wrong.

const result: FormattedExecutionResult = await response.json();

expect(response.status).toBe(200);
expect(result.data).toMatchObject(unexpectedData);
Copy link
Author

@kroupacz kroupacz Nov 25, 2024

Choose a reason for hiding this comment

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

@ardatan This is interesting. If this test runs on Bun, it correctly receive expectedNullableData mentioned above. 🤔 https://github.com/graphql-hive/gateway/actions/runs/12004650429/job/33463064378?pr=177#step:4:1519

@ardatan ardatan force-pushed the bug/unavailable-subgraph branch from 81ad9fa to fd79091 Compare December 3, 2024 11:19
@kroupacz
Copy link
Author

Hello @ardatan,
have you been able to investigate the issue yet?
Do you need any additional information which could help you solve the issue?
Thanks you in advance for your reply.
Sincerely,
Tomáš

@ardatan ardatan force-pushed the bug/unavailable-subgraph branch from fd79091 to eaeff3c Compare December 14, 2024 15:47
@ardatan
Copy link
Member

ardatan commented Feb 13, 2025

After taking another look at the issue, I noticed few things;

If Subgraph 2 is down, subgraph2 field should be null with errors not testSuccessQuery one.
Same for Subgraph 1 and subgraph1 field.

Because;

{
  testNestedField: {
    subgraph1: {
      testSuccessQuery: {
        id: 'user1',
        email: '[email protected]',
        sub1: true,
      },
    },
    subgraph2: {
      testSuccessQuery: null
    },
  },
}

This means subgraph2 field is available but testSuccessQuery field under of it is not.

@kroupacz
Copy link
Author

I understand your deduction, but I don't think it's correct in this case. If I run the same tests against a real Apollo Gateway, the result is correct in my opinion, or rather it behaves as the GraphQL specification says https://spec.graphql.org/draft/#sel-GAPHRPTPCAACEzBg6S

 If the field which experienced an error was declared as Non-Null, the null result will bubble up to the next nullable field

And the first non-nullable field is always testSuccessQuery or testErrorQuery.

However, if I run the tests against GraphQL Yoga (used as a gateway), it behaves differently and in my opinion incorrectly as you can see in the tests.

@ardatan
Copy link
Member

ardatan commented Feb 18, 2025

Let's think of an example outside Federation then;

We have a following schema;

type Query {
  foo: Foo
}

type Foo {
  bar: Bar
}

type Bar {
  id: ID
}

and we have the following resolvers;

const resolvers = {
  Query: {
    foo: () => ({})
  },
  Foo: {
    bar: () => null,
  },
  Bar: {
    id: () => 'BAR_ID'
  }
}

What do you expect when you make the following request?

{
  foo {
    bar {
      id
    }
  }
}

This one right? because bar is null, and there is no root value to put id.

{
  "data": {
     "foo": {
        "bar": null
      }
  }
}

When we go back to our example, subgraph2 is null and there is no object to add testSuccessQuery.

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.

2 participants