Fix subschemas merge

[isaackhlam]

🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.

Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of the pull request.

Description

Bug fix for on graphql query is mutation, stitched schema executed query rather than mutation.
On test v1, There is no *User in the field Query so it returns null when we use Mutation to query to server.
On test v2, We add *User to the field Query, We can see it return operation type to be Query even we query with Mutation.
The bug is caused by hardcode 'query' in stitched schema, changing to info.operation.operation can fix the bug.
Detain explanation in #4508

• Add failing test for the bug

• Fix the bug by changing 'query' to operation: info.operation.operation

Co-authored-by:

• @kftsehk
• @Abbywpy

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

Screenshots/Sandbox (if appropriate/relevant):

https://www.graphql-tools.com/docs/schema-stitching/stitch-type-merging#merging-flow
If such field name is not defined in Query of stitched schema, the merged resolver fails silently returning null result.

How Has This Been Tested?

To reproduce

cd gateway
npm i
pip install awscli aws-sam-cli
sam local start-api -t ./local.yml -l message.log --host 0.0.0.0 -p 3000

query in both test case

mutation {
  # or oneUser, which zeroValue becomes null
  zeroUser(id: 1) {
    __typename
    id
    zeroValue
    oneValue
    user {
      __typename
      id
      zeroValue
      oneValue
    }
  }
}

• Has Mutation.*User, not Query.*User

Result

{
  "data": {
    "zeroUser": {
      "__typename": "User",
      "id": "1",
      "zeroValue": "0: User: mutation",
      "oneValue": null,
      "user": {
        "__typename": "User",
        "id": "1",
        "zeroValue": "0: User: mutation",
        "oneValue": null
      }
    }
  }
}

• Added Query.*User to both server

Result

{
  "data": {
    "zeroUser": {
      "__typename": "User",
      "id": "1",
      "zeroValue": "0: User: mutation",
      "oneValue": "1: User: query",
      "user": {
        "__typename": "User",
        "id": "1",
        "zeroValue": "0: User: mutation",
        "oneValue": "1: User: query"
      }
    }
  }
}

On both tests we expected result to be

{
  "data": {
    "zeroUser": {
      "__typename": "User",
      "id": "1",
      "zeroValue": "0: User: mutation",
      "oneValue": "1: User: mutation",
      "user": {
        "__typename": "User",
        "id": "1",
        "zeroValue": "0: User: mutation",
        "oneValue": "1: User: mutation"
      }
    }
  }
}

Test Environment:

• OS: Linux 4.18.0-372.9.1.el8.x86_64
• @graphql-tools/graphql-file-loader: ^7.3.14
• @graphql-tools/load: ^7.5.13
• @graphql-tools/schema: ^8.3.13
• @graphql-tools/stitch: ^8.6.12
• @graphql-yoga/node: ^2.8.0
• @vendia/serverless-express: ^4.8.0
• NodeJS: v14.19.3

Checklist:

• I have followed the CONTRIBUTING doc and the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests and linter rules pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

[changeset-bot]

⚠️ No Changeset found

Latest commit: b491287

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

@isaackhlam is attempting to deploy a commit to the The Guild Team on Vercel.

A member of the Team first needs to authorize it.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
graphql-tools	❌ Failed (Inspect)		Jul 2, 2022 at 3:22PM (UTC)

[yaacovCR]

Definitely a bug, but we probably need to make sure to only send mutations for root fields?

I have to look over more closely when I am able.

[kftsehk]

@yaacovCR There are quite some discussions, and also comments suggesting nested mutation is not supported in graphql spec
graphql/graphql-js#221
graphql/graphql-spec#252
However, as I am aware, the graphql spec only requires that root mutation selection set is executed serially instead of in parallel, because it is expected that side-effect are present in root mutation.

I would argue that correctness of execution should not be based on mutation being executed serially, any non-trivial application will run on more than a single threaded graphql server. In fact, even query is not strictly free of side-effect, consider it may have the ability to populate a cache at some level, making subsequent calls eventually consistent.
There are also other way to guarentee serial execution, which is to establish parent/child relation between fields, as in example from graphql/graphql-spec#252:

{
  user(id:"1") {
    postToTimeline(commentContent:"Happy Birthday Mark!") { # This is a serial field
      comment {
        addReaction(reaction: LIKE) # This is another serial field, that adds a reaction into the comment
      }
    }
  }
}

On the other hand, it is important to correctly pass the operation type to stitched schema at any level, as there may be different requirements for running query and mutation, e.g. consistency, authentication, data source etc.

Our use case uses nested mutation, being aware of the consequence that mutation and data query can run in parallel.

mutation {
  post(id:"1") { # Post
    postComment(comment:"Hello world") { # Post!
       comments { # [Comment!]!, correct usage, result includes new comment
         id
      }
    }
    allComments { # [Comment!]!, undefined behavior, can return data with / without new comment
      id
    }
  }
}

We implements guards to operations only meant for mutation, and refuse to process if executed in query. This is meant to warn for programming mistake.

[yaacovCR]

Ok, so not a bug? my mistake, we don't currently support nested mutations?

Although we could consider a feature request, I am not sure how this would work in practice, as sometimes for subfields u may want to stitch to queries, sometimes to mutations, so u cannot always go by the overall original operation type. Maybe some additional config could support this?

I have to look more closely still at your example.

[kftsehk]

Sorry that the request in graphql/graphql-spec#252 might have mislead you, our case consist of only erither whole operation on query, or whole operation on mutation, however, mutations containing side-effect is not always at the root field.

I think mutation side-effect at root field is not required by graphql spec, as long as serial execution is not needed.

I would like to highlight 2 comments in the spec request thread:

graphql/graphql-spec#252 (comment)

supporting "nested mutations" is entirely possible within the existing GraphQL spec.

graphql/graphql-spec#252 (comment)

any field on the GraphQL schema can have side effects. But then you don't get the nice feature of the fields executing in serial rather than in parallel

Our bug reported here shows that mutation is being forwarded to query, which is not the behavior as if the same schema is run in a monolithic server.

[yaacovCR]

First of all, thanks for contributing! We appreciate the effort to improve the library. There is also a lot of interesting ideas here with regard to order of execution, which is a topic that probably could be fleshed out in greater depth overall, so it's great to see how these ideas are being utilized in the wild, especially in the context of schema stitching.

I'm just going to address a few things, some are in relation to comments you have made, and in the end with respect to the specifics of the PR.

I would argue that correctness of execution should not be based on mutation being executed serially, any non-trivial application will run on more than a single threaded graphql server....
There are also other way to guarentee serial execution, which is to establish parent/child relation between fields, as in example from graphql/graphql-spec#252:

Within a mutation operation, root fields are definitely executed serially. Sadly, although nested "mutations" can be utilized simply by nesting the Mutation type, multiple nested mutations no longer have that guarantee of serial execution. As long as you only nest a single mutation, you should be safe.

This comment is in line with what you quote:

any field on the GraphQL schema can have side effects. But then you don't get the nice feature of the fields executing in serial rather than in parallel

So I believe we are in agreement as to what the spec currently supports! On to your specific case:

Our bug reported here shows that mutation is being forwarded to query, which is not the behavior as if the same schema is run in a monolithic server.

The gateway is correctly forwarding the root fields as mutations to the subschema. I think we can agree on that. It is only that when type merging occurs, it is assumed that all merged types (including merged root types) are accessed via query root fields (accessors).

Putting aside whether this is a "fix" or "feature implementation" (because who cares, thank you for pointing out this edge case!!!!), I just want to highlight that any implementation cannot simply infer the accessor's parent root type from the original operation, because that would break a mutation in which we are stitching in fields from a query. Your test cases include mutations where we are using mutation accessors in the presence of a query accessor, but not a case (the usual case, I believe!) where we actually want to start from a mutation and use a query accessor.

So, I think we need to simply introduce another field on the accessor specification to indicate the operation, i.e. besides specifying the fieldName to get at the merged type for a subschema, you would also have to specify the operation or the rootType. What do you think?

[kftsehk]

Hi @yaacovCR, thanks for your quick reply, surely schema stitching, particularly remote schema, is kind of new ideas and deserves some discussion.

There are two cases below, imo in terms of correctness, Behavior B is implementation-wise more correct since stitched schema will reproduce the info object as if the execution is non-stitched. However, the difference is in the info object, the structure and data of info afaik is not specified by the spec, maybe a lot of graphql user doesn't care about that too, and it will require stitched schema to be implemented in a special way having same reachability from Query and Mutation.

Behavior A is more DX friendly to new graphql stitching user, as said, maybe a lot of graphql user doesn't care about the info object at all, and following a mutation then query pattern means subsequent stitched data would return from Query of stitched schema.

Also I have not came up with a sensible scenario where we start with Query from gateway, and requires to forward to Mutation of stitched schema.

I would propose to add a boolean value like forwardRootTypeToStitchedSchema, that

• false: [default if unset] Forward anything to query
• true: Forward gateway mutation to stitched Mutation, gateway query to stitched Query

Behavior A: Mutation at root then query stitched fields not available from mutation's return

I just want to highlight that any implementation cannot simply infer the accessor's parent root type from the original operation, because that would break a mutation in which we are stitching in fields from a query

I think you mean the case that

Gateway:

type Query {
  queryFoo: Foo!
}
type Mutation {
  mutateFoo: Foo!
}
type Foo {
  id: ID!
  fooField: String!
}

Stitched schema:

type Query {
  queryFooFromStitch: Foo!
}
type Foo {
  id: ID!
  stitchedFooField: String!
}

where a query like

mutation {
  mutateFoo {
    id
    fooField
    stitchedFooField
  }
}

will break stitchedFooField if stitched schema's operation is inferred from gateway operation.

It is a valid concern since a lot of graphql users may have already organized their mutation following the pattern that mutations are only at root mutation type, i.e. Mutation.mutateSomething.no.more.mutation

Behavior B: Migrate from decomposing a single schema, which resolvers depends on info object

This is the case we are encountering, In the scenario of single schema, info object received by all resolver will be either query or mutation, and afaik there is no graphql implementation that would lead to a mix of two.

In this case, the graph of Query is the same as Mutation, that is also the reason why mutation is in non-root type

Gateway:

type Query {
  foo: Foo!
}
type Mutation {
  foo: Foo!
}
type Foo {
  id: ID!
  fooField: String!
  mutate: Foo! @mutation
}

Stitched schema:

type Query {
  fooFromStitch: Foo!
}
type Mutation {
  fooFromStitch: Foo!
}
type Foo {
  id: ID!
  stitchedFooField: String!
}

In the scenario of schema stitching, if the gateway specifies a fixed RootType or operation in accessor, we always end up having scenario having different info.operation in gateway and stitched schema.

[yaacovCR]

Question: Does your nested mutation resolver do something different when included within a query operation? Like throw an error?

[kftsehk]

it does in our case to throw a "mutation in query error" that, we forbids resolver intended for mutation being run in query

It is a way to require developers using client library to explicitly use the .mutate() call, or submit mutation {} query. There are different ways client/server library may cache the result, and an explicit mutation is a good way to get rid of all those.

Edit: e.g. in apollo client and server, query is considered cachable on whole query / field level, on both client and server-side, I am not particularly favorable towards apollo, but anyway it seem quite a lot of developer uses it because of its features.

[yaacovCR]

I imagine it would require custom merge resolvers that modify info only as necessary based on the directive.