Skip to content
This repository has been archived by the owner on Apr 15, 2020. It is now read-only.

Schema delegation doesn't work with query variables in nested fields #46

Closed
ariadne-github opened this issue Mar 24, 2020 · 2 comments
Closed

Comments

@ariadne-github
Copy link

When you use schema stitching combined with delegateToSchema and try to use query variables on a nested part of the delegated schema, the query breaks.

  • Intended outcome: query variables should work just fine even on a delegated schema.

  • Actual outcome: the query breaks with an error like: Variable "$foo" is not defined.

  • How to reproduce the issue:

The error can be reproduced by a simple modification of the test src/test/testStitchingFromSubschemas.ts. I'll report here a series of tests I tried to isolate the problem, just to show the specific case.

If you add a query variable in the test query, like this:

query($myId: ID!) {
  userById(id: $myId) {
    chirps {
      id
      textAlias: text
      author {
        email
      }
    }
  }
}
...
console.error(result.errors) // This is to see the error details

It breaks with the error Variable "$myId" of required type "ID!" was not provided. That is correct. If you then pass the variable value, like this:

const result = await graphql(mergedSchema, query, null, null, { myId: 5 });

It works correctly. The problem arises when the variable is used in a nested query. Add a parameter to a field, even not a required one, and then use it literally:

const chirpTypeDefs = `
  type Chirp {
    id: ID!
    text: String
    authorId: ID!
    author(foo: Boolean): User
  }
`;

...

query($myId: ID!) {
  userById(id: $myId) {
    chirps {
      id
      textAlias: text
      author(foo: true) {
        email
      }
    }
  }
}

This works as expected. But then pass the parameter value by a query variable:

query($myId: ID!, $foo: Boolean) {
  userById(id: $myId) {
    chirps {
      id
      textAlias: text
      author(foo: $foo) {
        email
      }
    }
  }
}

const result = await graphql(mergedSchema, query, null, null, { myId: 5, foo: true });

This produces the following error: Variable "$foo" is not defined. So, as shown before this is not a problem with providing the value, since the error is different, nor with the parameter definition itself, since it works by passing it literally.

In my understanding, this is caused by the mergeSchema operation mapping variableDefinitions uncorrectly, thus losing the outer schema definitions.

I even managed to find the place in the code, but I'm not confident enough in this codebase to submit a pull request. Anyway, I found that the problem lies in the src/delegate/createRequest.ts file, when defining the newVariableDefinitions, which contains remapped values and also new generated ones, but loses the provided definitions in this case. For example, if you change the line 136 like this:

newVariableDefinitions = updatedVariableDefinitions.concat(variableDefinitions)

It works, and also doesn't break any existing tests. But it seems to be a naive solution, so I'd like the repository owner or someone else more faamiliar with the codebase to provide a more solid fix.

This is really important for my company, since we use this package to patch a graphql schema we don't control, and we prefer not forking it, since it seems to be a really trivial bug.

@yaacovCR
Copy link
Owner

@ariadne-github , can you confirm fixed in v9?

@ariadne-github
Copy link
Author

It works correctly, thank you!

yaacovCR added a commit that referenced this issue Mar 26, 2020
yaacovCR added a commit that referenced this issue Mar 26, 2020
yaacovCR added a commit that referenced this issue Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants