graphql-tools icon indicating copy to clipboard operation
graphql-tools copied to clipboard

Import Fails to Import Interface of Union's Types Across Multiple Files

Open tlivings opened this issue 2 years ago • 9 comments

Issue workflow progress

Progress of the issue based on the Contributor Workflow

  • [x] 1. The issue provides a reproduction available on Github, Stackblitz or CodeSandbox
  • [x] 2. A failing test has been provided
  • [ ] 3. A local solution has been provided
  • [ ] 4. A pull request is pending review

Describe the bug

~Note: I am reporting a bug but have not been able to reproduce in an isolated fashion yet.~ See https://github.com/tlivings/graphql-tools/tree/reproduce-5436 failing test.

Attempting to load schema using the GraphQLFileLoader and import statements.

Use case: A type's field is a union of types that implement interfaces.

Error: Unknown type "XXXX". Did you mean "XXXXXX", ...?
          at assertValidSDL (xxxx/node_modules/graphql/validation/validate.js:135:11)
          at buildASTSchema (xxxx/node_modules/graphql/utilities/buildASTSchema.js:44:34)
          at makeExecutableSchema (xxxx/node_modules/@graphql-tools/schema/cjs/makeExecutableSchema.js:73:47)
          at mergeSchemas (xxxx/node_modules/@graphql-tools/schema/cjs/merge-schemas.js:32:63)
          at getSchemaFromSources (xxxx/node_modules/@graphql-tools/load/cjs/schema.js:57:46)
          at loadSchema (xxxx/node_modules/@graphql-tools/load/cjs/schema.js:19:12)

To Reproduce Steps to reproduce the behavior:

See the failing test case for https://github.com/tlivings/graphql-tools/tree/reproduce-5436.

Expected behavior

Environment:

  • @graphql-tools/load: 7.8.14

tlivings avatar Jul 19 '23 23:07 tlivings

Updated — I think I have a failing test for when using unions:

# schema
import U from 'u.graphql'

type A {
  field: U
}
# u.graphql

interface I {
  field3: String
}

type A {
  field1: String
}

type B implements I {
  field2: String
  field3: String
}

union U = A | B

If a nested union's type implements an interface that interface won't be imported...

tlivings avatar Jul 20 '23 20:07 tlivings

Here is a failing test: https://github.com/tlivings/graphql-tools/tree/reproduce-5436

tlivings avatar Jul 20 '23 20:07 tlivings

Hi @ardatan — is there anything additional I can provide? The way I reproduced was in the posted fork and then opening in codespaces and running the test.

I've also been trying to debug this but haven't found root cause yet. If you have any insight into the issue, I'd be happy to work on a PR!

tlivings avatar Jul 25 '23 16:07 tlivings

Updated the example in issue

tlivings avatar Jul 27 '23 17:07 tlivings

I have a potential fix I am trying out. It seems like addDefinition does not recursively try adding transitive dependencies.

tlivings avatar Jul 28 '23 14:07 tlivings

Basically:

 // Call addDefinition recursively to add all dependent documents
if ('name' in definition && definition.name) {
  const documentName = definition.name.value;
  const dependencyDefinitionForDocument = allImportedDefinitionsMap.get(documentName);
  dependencyDefinitionForDocument?.forEach(node => {
    if (node !== definition) {
      addDefinition(node, definitionName, definitionSet);
    }
});

For the test case's union, one of the types has an interface which will not be present in allImportedDefinitionsMap.get(documentName) and as a result, fail.

Only potentialTransitiveDefinitionsMap will contain the definitions it depends on.

tlivings avatar Jul 28 '23 16:07 tlivings

Thank you for all the details you've given so far. I tried to extract the tests from the commits in the branch you shared, but I couldn't. Could you please create a PR with this failing test?

ardatan avatar Jul 31 '23 15:07 ardatan

Done ☝️

tlivings avatar Jul 31 '23 16:07 tlivings

I've taken a stab at solving this issue within this code base: https://github.com/tlivings/graphql-import

It is indeed fixed, but likely doesn't support import the same way and is not typescript (which allows me easier manipulation of graphql ASTs etc).

I don't know if / when I will be able to see if I can apply something similar to the issue here though. I might simply offer it as an alternative graphql file loader for now (although currently it is unpublished).

tlivings avatar Dec 05 '23 15:12 tlivings