Unspecified variable is becoming null when stitching schemas

[pcorey]

Hi,

I'm working with a local GraphQL schema that has a query (let's call it search) that accepts an optional enum argument, filter:

enum Filter {
  all
  some
}

extend type Query {
  search(filter: FilterEnum): [String!]!
}

This schema is being stitched together with a remote schema using mergeSchemas.

Using graphql-tools@4.0.7, when querying search without providing a value for filter, filter wouldn't be provided as an argument to the search resolvers, as I'd expect. With graphql-tools@5.0.0, if I query search without filter, the search resolver will recieve {filter: null} as its arguments. This is not what I'd expect, and is breaking some assumptions in my code that filter will be undefined, not null, if it's not provided by the client.

If I don't use mergeSchemas, and just use the local schema directly, the code works as I'd expect. filter isn't passed as an argument to the search resolver. This leads me to believe that this is an issue with mergeSchemas and/or schema stitching in general.

@yaacovCR, do you have any ideas about this? I hate to tag you out of the blue, but the changelog mentioned that you did lots of work with schema stitching in 5.0.0.

Thanks!

[yaacovCR]

Sounds similar to yaacovCR#44 (comment)

[yaacovCR]

The flow now should be that it copies the default value when field not specified and should only be null if the default is null. If you could submit a reproduction or a failing test that would be helpful.

[yaacovCR]

Possibly we need to check for undefined here: https://github.com/ardatan/graphql-tools/blob/master/src/delegate/transforms/AddArgumentsAsVariables.ts#L158

[yaacovCR]

With the difference possibly that in v5 the serialization between source and target schema may be converting undefined to null

[yaacovCR]

But I'm not convinced. Would that be a safe change? Can a non optional argument serialize to undefined, and then get dropped when stitching?

Lets see some code to better understand the issue.

[pcorey]

Thanks for getting back to me @yaacovCR. Here's a minimal reproduction. Just run the test suite and you should see the problem. The first test which uses makeExecutableSchema exclusively passes - filter is undefined in the search resolver, not null. The second test uses mergeSchemas and fails because the filter argument is null.

https://github.com/pcorey/graphql-tools-null-reproduction

[yaacovCR]

Thanks! Is the issue also apparent without the use of variables?

[pcorey]

@yaacovCR It looks like if I call search without a filter and without using variables, everything works fine. filter is undefined in the search resolver. Is that what you meant? I just added that as a test and pushed it to the reproduction repo.

[yaacovCR]

Yes, thanks that was exactly my question and is very helpful.

[yaacovCR]

I think the undefined check should be with the original variables here: https://github.com/ardatan/graphql-tools/blob/master/src/delegate/createRequest.ts#L116

[yaacovCR]

We should skip serialization if existing variable value is undefined. This check should probably go in both places, thought about it more, and seems quite unlikely that it should ever be the case that a non-optional argument is parsed internally to undefined.

[yaacovCR]

Also in FilterToSchema!

[yaacovCR]

See #1455 (comment) to test within alpha

[pcorey]

@yaacovCR I tried to test to alpha release, but it looks like something might be broken with the release? The reproduction repo fails because mergeSchemas isn't defined:

➜  graphql-tools-null-reproduction git:(master) ✗ yarn test
yarn run v1.19.1
$ jest --runInBand
 FAIL  ./index.test.js
  ✓ works (23 ms)
  ✕ does not work (2 ms)
  ✕ works without variables (1 ms)

  ● does not work

    TypeError: mergeSchemas is not a function

      46 |
      47 | it("does not work", async () => {
    > 48 |   const schema = mergeSchemas({
         |                  ^
      49 |     schemas: [
      50 |       makeExecutableSchema({
      51 |         typeDefs,

      at Object.<anonymous> (index.test.js:48:18)

  ● works without variables

    TypeError: mergeSchemas is not a function

      63 |
      64 | it("works without variables", async () => {
    > 65 |   const schema = mergeSchemas({
         |                  ^
      66 |     schemas: [
      67 |       makeExecutableSchema({
      68 |         typeDefs,

      at Object.<anonymous> (index.test.js:65:18)

  console.log
    { data: [Object: null prototype] { search: 'result' } }

      at Object.<anonymous> (index.test.js:42:11)

Test Suites: 1 failed, 1 total
Tests:       2 failed, 1 passed, 3 total
Snapshots:   0 total
Time:        1.607 s
Ran all test suites.

And my real project fails to stitch in the remote schema completely due to a executor is not a function error.

Your tests and changes look good to me, so this is probably user error. Any ideas what's up?

[yaacovCR]

Alpha gets ready for v6 and merging of graphql-toolkit and separation of Apollo link out of core functionality,

mergeSchemas => stitchSchemas

You can convert link to executor or subscriber using exported functions linkToExecutor and linkToSubscriber.

I think an executor is the new name for what was a fetcher, with different option name in makeRemoteExecutableSchema

to enable executions and mutations you just have to set an executor but to enable subscriptions you have to set a subscriber. In parallel to how upstream graphql-js has execute and subscribe for local schemas.

[pcorey]

@yaacovCR I see, thanks. Those changes fixed it. Both the reproduction and my actual project are working as expected. I really appreciate all your help!

[yaacovCR]

Thanks for filing this bug!

[pablolopesk8]

I have noticed the same problem on the project I'm working.
We have a lot of optional variables in our frontend. And until the version 4.0.7, it was working good.
We are awaiting the new release anxiously

[yaacovCR]

Closed by #1419.

[adijesori]

Hello @yaacovCR. I'm having the same issue when using v5.0.0.
Do I must upgrade to v6.0.0 to get the fix or there is a patch also to v5.0.0?

[ardatan]

You are using really old version. We recommend you to migrate to the latest one.