ardatan / graphql-tools

:wrench: Utility library for GraphQL to build, stitch and mock GraphQL schemas in the SDL-first approach
https://www.graphql-tools.com
MIT License
5.35k stars 814 forks source link

Improve handling of selections that would only consist of `__typename` #6456

Open lukastaegert opened 2 months ago

lukastaegert commented 2 months ago

Is your feature request related to a problem? Please describe.

In 33e8146e33aa17790ee76d14e52f62c684ee1b16, you implemented a change that aborted the query with an error if it would habe __typename in its selection set. That is good, as it finally allows us to handle a long-standing issue with our setup, but we would suggest to further refine the behavior.

In our project, we use a custom solution based on stitchSchemas and especially delegateToSchema to implement a strangler pattern where we want to gradually replace an old service A with a new service B behind a common GraphQL gateway. Here, the schema of B is mostly a subset of the schema of A. If stitchSchemas determines that a top-level field is supported by both services, then we use delegateToSchema to send the request to both services and then implement a custom merge logic to merge the results.

There is one problem though. Assume we have these schemas:

# Service A
type Query {
  foo: Foo
}

type Foo {
  first: String
  second: String
}

# Service B
type Query {
  foo: Foo
}

type Foo {
  first: String
}

And this query:

query {
  foo {
    __typename
    second
  }
}

stichSchemas will determine that both services implement Query.foo and we will therefore pass the query to delegateToSchema, which in turn will find out that we want to send this query to B:

query {
  foo {
    __typename
  }
}

With the old logic, this generated a lot of useless requests while the new logic now throws an error that we can catch and handle! (Technically, this also means that this patch release was a breaking change for us, but we do not mind, we pinned the dependency for now)

However, catching this error means we need to rely on the exact wording of the error message as we still want to handle other errors as before. Also, using errors as expected control flow like this probably also carries some performance penalty (though we did not test this).

Describe the solution you'd like

We want a clean way to ignore requests that would only select __typename of any field. Ideally, these fields would return null. It would be ok for this behavior to be opt-in via option. Alternatively, there could be a hook to handle these cases.

Describe alternatives you've considered

If we want to stick with throwing errors, it would also help to add a stable error code to the error that we can test for.

Additional context

See above. If you think our approach could be improved in some way that would avoid this problem differently, let us know!