AEB-labs / graphql-weaver

A tool to combine, link and transform GraphQL schemas
MIT License
240 stars 20 forks source link

Links return an error when recommendation service sets typePrefix #49

Closed FX-HAO closed 5 years ago

FX-HAO commented 5 years ago
const schema: GraphQLSchema = await weaveSchemas({
    endpoints: [{
        namespace: 'library',
        typePrefix: "Library",
        url: 'http://example.com/library/graphql'
    }, {
        namespace: 'recommendations',
        typePrefix: "Recommendations",
        url: 'http://example.com/recommendations/graphql',
        fieldMetadata: {
            'Recommendation.song': { // Field song in type Recommendation
                link: {
                    field: 'library.Song', // field Song in namespace library
                    argument: 'id', // argument of library.Song
                    batchMode: false,
                }
            }
        }
     }]
});

When I tried to use this schema, I got an error that includes Unexpected reference to type LibrarySong.

Traceback:

Error: Unexpected reference to type LibrarySong
    at Transformer.findType (/Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-transformer/src/schema-transformer.ts:247:19)
    at TypeResolversTransformer.<anonymous> (/Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-weaver/src/pipeline/abstract-types.ts:115:34)
    at step (/Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-weaver/dist/src/pipeline/abstract-types.js:40:23)
    at Object.next (/Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-weaver/dist/src/pipeline/abstract-types.js:21:53)
    at /Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-weaver/dist/src/pipeline/abstract-types.js:15:71
    at new Promise (<anonymous>)
    at /Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-weaver/dist/src/pipeline/abstract-types.js:11:12
    at /Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-weaver/src/pipeline/abstract-types.ts:108:25
    at /Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-transformer/src/schema-transformer.ts:524:28
    at /Users/haofuxin/shundaojia/graphql-gateway2/node_modules/graphql-transformer/src/schema-transformer.ts:524:28
FX-HAO commented 5 years ago

I think that i figure out what the problem is. https://github.com/AEB-labs/graphql-weaver/blob/94ed46d787709e5d3fb2b6dfdae865e9eb514bcb/src/pipeline/abstract-types.ts#L114-L115 Where name should be replaced with unprefixed name. But I am not familiar with the codebase, i have no idea how to get a map or something that removes the prefix. Now I just do monkey patching to fix the problem.

FX-HAO commented 5 years ago

https://github.com/AEB-labs/graphql-weaver/blob/94ed46d787709e5d3fb2b6dfdae865e9eb514bcb/src/pipeline/pipeline.ts#L23-L25 It seems like we can't get typePrefix in AbstractTypesModule. It's passed through the pipeline at schema initializing. And recommendation service can't get library service's typePrefix.

Yogu commented 5 years ago

Is Song an abstract type (union / interface)?

In theory, the AbstractTypesModule should not need to care about the typePrefix, that's why it is not passed to the module. The type prefix should be removed by TypePrefixModule. However, if we add the type resolver logic to TypePrefixModule, we're basically duplicating the logic of AbstractTypesModule. Maybe it's easier to just pass the typePrefix to AbstractTypesModule. Do you want to prepare a pull request for that?

FX-HAO commented 5 years ago

Song in my use case is an interface type. I use node interface to do links, which saves me a lot of time from writing link interfaces for link objects :P

It seems that context in createPreMergeModules is independent from each other, so how can we get other service's typePrefix and pass it to AbstractTypesModule.

Yogu commented 5 years ago

I was thinking of passing typePrefix to the module constructor, just as for the TypePrefixModule.

FX-HAO commented 5 years ago

I understand, that's what i was thinking too. I'd like to create a PR to fix the issue, it should be a minor change :)

Yogu commented 5 years ago

Merged and released in 0.13.2.