From 24819fa8ee2c21abfc91d991bd6ed0376d70544e Mon Sep 17 00:00:00 2001 From: Manuel Spigolon Date: Tue, 1 Mar 2022 10:48:30 +0000 Subject: [PATCH 1/4] fix: manage duplicate types --- lib/gateway.js | 22 ++++-- test/gateway/fix-743.js | 168 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 5 deletions(-) create mode 100644 test/gateway/fix-743.js diff --git a/lib/gateway.js b/lib/gateway.js index 60b253ef..05c6f1c2 100644 --- a/lib/gateway.js +++ b/lib/gateway.js @@ -119,19 +119,23 @@ function isDefaultType (type) { * posts: [Post] * } */ -function defineResolvers (schema, typeToServiceMap, serviceMap, typeFieldsToService) { +function defineResolvers (schema, typeToServiceMap, serviceMap, typeFieldsToService, duplicatedTypes) { const types = schema.getTypeMap() + function getService (key) { + return typeToServiceMap[key] || (duplicatedTypes[key] && duplicatedTypes[key][0]) || null + } + for (const type of Object.values(types)) { if (isObjectType(type) && !isDefaultType(type.name)) { - const serviceForType = typeToServiceMap[type.name] + const serviceForType = getService(type.name) for (const field of Object.values(type.getFields())) { const fieldType = getNamedType(field.type) if (fieldType.astNode && fieldType.astNode.kind === Kind.ENUM_TYPE_DEFINITION) continue const fieldName = field.name if (!isScalarType(fieldType)) { - const serviceForFieldType = typeToServiceMap[fieldType] + const serviceForFieldType = getService(fieldType) /* istanbul ignore else */ if ( (serviceForFieldType === null && serviceForType !== null) || @@ -319,6 +323,7 @@ async function buildGateway (gatewayOpts, app) { const typeToServiceMap = {} const typeFieldsToService = {} let allTypes = [] + const duplicatedTypes = {} const factory = new Factory() app.decorateReply(kEntityResolvers) app.addHook('onRequest', async function (req, reply) { @@ -328,7 +333,13 @@ async function buildGateway (gatewayOpts, app) { for (const [service, serviceDefinition] of Object.entries(serviceMap)) { for (const type of serviceDefinition.types) { allTypes.push(serviceDefinition.schema.getTypeMap()[type]) - typeToServiceMap[type] = service + if (!typeToServiceMap[type]) { + typeToServiceMap[type] = service + } else if (!duplicatedTypes[type]) { + duplicatedTypes[type] = [typeToServiceMap[type], service] + } else { + duplicatedTypes[type].push(service) + } } for (const [type, fields] of Object.entries(serviceDefinition.extensionTypeMap)) { @@ -410,7 +421,7 @@ async function buildGateway (gatewayOpts, app) { typeToServiceMap[typeName] = null } - defineResolvers(schema, typeToServiceMap, serviceMap, typeFieldsToService) + defineResolvers(schema, typeToServiceMap, serviceMap, typeFieldsToService, duplicatedTypes) return { schema, @@ -494,6 +505,7 @@ async function buildGateway (gatewayOpts, app) { typeToServiceMap.Mutation = null typeToServiceMap.Subscription = null + // todo const valueTypes = findValueTypes(allTypes) for (const typeName of valueTypes) { typeToServiceMap[typeName] = null diff --git a/test/gateway/fix-743.js b/test/gateway/fix-743.js new file mode 100644 index 00000000..d1c741f0 --- /dev/null +++ b/test/gateway/fix-743.js @@ -0,0 +1,168 @@ +'use strict' + +const { test } = require('tap') +const Fastify = require('fastify') +const GQL = require('../..') + +async function buildService () { + const app = Fastify() + const schema = ` + extend type Query { + list: PageInfo + } + + type User @key(fields: "id") { + id: ID! + name: String + } + + type PageInfo { + edges: [User] + } + ` + + const resolvers = { + Query: { + list: () => { + return { + edges: [ + { id: 1, name: 'Davide' }, + { id: 2, name: 'Fiorello' } + ] + } + } + } + } + + app.register(GQL, { + schema, + resolvers, + federationMetadata: true, + allowBatchedQueries: true + }) + + return app +} + +async function buildServiceExternal () { + const app = Fastify() + const schema = ` + type PageInfo { + edges: [User] + } + + type User @key(fields: "id") @extends { + id: ID! @external + } + ` + + const resolvers = { + // Query: { + // listx: () => {} + // } + } + + app.register(GQL, { + schema, + resolvers, + federationMetadata: true, + allowBatchedQueries: true + }) + + return app +} + +async function buildProxy (port1, port2) { + const proxy = Fastify() + + proxy.register(GQL, { + graphiql: true, + gateway: { + services: [ + { + name: 'ext1', + url: `http://localhost:${port1}/graphql` + }, + { + name: 'ext2', + url: `http://localhost:${port2}/graphql` + } + ] + }, + pollingInterval: 2000 + }) + + return proxy +} + +test('federated node should be able to return aliased value if the type is declared in multiple places', async (t) => { + const port1 = 3040 + const serviceOne = await buildService() + await serviceOne.listen(port1) + t.teardown(() => { serviceOne.close() }) + + const port2 = 3041 + const serviceTwo = await buildServiceExternal() + await serviceTwo.listen(port2) + t.teardown(() => { serviceTwo.close() }) + + const serviceProxy = await buildProxy(port1, port2) + await serviceProxy.ready() + t.teardown(() => { serviceProxy.close() }) + + let res = await serviceProxy.inject({ + method: 'POST', + url: '/graphql', + body: { + query: `{ + list { edges { id name } } + }` + } + }) + + t.same(res.json(), { + data: { + list: { + edges: [ + { + id: '1', + name: 'Davide' + }, + { + id: '2', + name: 'Fiorello' + } + ] + } + } + }) + + res = await serviceProxy.inject({ + method: 'POST', + url: '/graphql', + body: { + query: `{ + list { + items: edges { id name } + } + }` + } + }) + + t.same(res.json(), { + data: { + list: { + items: [ + { + id: '1', + name: 'Davide' + }, + { + id: '2', + name: 'Fiorello' + } + ] + } + } + }) +}) From ead28337109ff55a96276e46a56f484fa808e701 Mon Sep 17 00:00:00 2001 From: Manuel Spigolon Date: Tue, 1 Mar 2022 11:16:53 +0000 Subject: [PATCH 2/4] fix: select the best service node --- lib/gateway.js | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/gateway.js b/lib/gateway.js index 05c6f1c2..c5616916 100644 --- a/lib/gateway.js +++ b/lib/gateway.js @@ -122,8 +122,17 @@ function isDefaultType (type) { function defineResolvers (schema, typeToServiceMap, serviceMap, typeFieldsToService, duplicatedTypes) { const types = schema.getTypeMap() - function getService (key) { - return typeToServiceMap[key] || (duplicatedTypes[key] && duplicatedTypes[key][0]) || null + function getService (key, priorityService) { + const service = typeToServiceMap[key] + if (service || !duplicatedTypes[key]) { + return service + } + + const duplicatedType = + duplicatedTypes[key].find(s => s === priorityService) || + duplicatedTypes[key][0] || + null + return duplicatedType } for (const type of Object.values(types)) { @@ -135,7 +144,7 @@ function defineResolvers (schema, typeToServiceMap, serviceMap, typeFieldsToServ if (fieldType.astNode && fieldType.astNode.kind === Kind.ENUM_TYPE_DEFINITION) continue const fieldName = field.name if (!isScalarType(fieldType)) { - const serviceForFieldType = getService(fieldType) + const serviceForFieldType = getService(fieldType, serviceForType) /* istanbul ignore else */ if ( (serviceForFieldType === null && serviceForType !== null) || @@ -479,11 +488,18 @@ async function buildGateway (gatewayOpts, app) { this._serviceSDLs = _serviceSDLs allTypes = [] + const duplicatedTypes = {} for (const [service, serviceDefinition] of Object.entries(serviceMap)) { for (const type of serviceDefinition.types) { allTypes.push(serviceDefinition.schema.getTypeMap()[type]) - typeToServiceMap[type] = service + if (!typeToServiceMap[type]) { + typeToServiceMap[type] = service + } else if (!duplicatedTypes[type]) { + duplicatedTypes[type] = [typeToServiceMap[type], service] + } else { + duplicatedTypes[type].push(service) + } } for (const [type, fields] of Object.entries(serviceDefinition.extensionTypeMap)) { @@ -511,7 +527,7 @@ async function buildGateway (gatewayOpts, app) { typeToServiceMap[typeName] = null } - defineResolvers(schema, typeToServiceMap, serviceMap, typeFieldsToService) + defineResolvers(schema, typeToServiceMap, serviceMap, typeFieldsToService, duplicatedTypes) return schema }, From afa76873510edef52a5b3155bf752f7f8ac0d96c Mon Sep 17 00:00:00 2001 From: Manuel Spigolon Date: Tue, 1 Mar 2022 11:39:13 +0000 Subject: [PATCH 3/4] fix: last service wins on duplicated type --- lib/gateway.js | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/gateway.js b/lib/gateway.js index c5616916..281dfb50 100644 --- a/lib/gateway.js +++ b/lib/gateway.js @@ -342,13 +342,14 @@ async function buildGateway (gatewayOpts, app) { for (const [service, serviceDefinition] of Object.entries(serviceMap)) { for (const type of serviceDefinition.types) { allTypes.push(serviceDefinition.schema.getTypeMap()[type]) - if (!typeToServiceMap[type]) { - typeToServiceMap[type] = service - } else if (!duplicatedTypes[type]) { - duplicatedTypes[type] = [typeToServiceMap[type], service] - } else { - duplicatedTypes[type].push(service) + if (typeToServiceMap[type]) { + if (!duplicatedTypes[type]) { + duplicatedTypes[type] = [typeToServiceMap[type], service] + } else { + duplicatedTypes[type].push(service) + } } + typeToServiceMap[type] = service } for (const [type, fields] of Object.entries(serviceDefinition.extensionTypeMap)) { @@ -493,13 +494,14 @@ async function buildGateway (gatewayOpts, app) { for (const [service, serviceDefinition] of Object.entries(serviceMap)) { for (const type of serviceDefinition.types) { allTypes.push(serviceDefinition.schema.getTypeMap()[type]) - if (!typeToServiceMap[type]) { - typeToServiceMap[type] = service - } else if (!duplicatedTypes[type]) { - duplicatedTypes[type] = [typeToServiceMap[type], service] - } else { - duplicatedTypes[type].push(service) + if (typeToServiceMap[type]) { + if (!duplicatedTypes[type]) { + duplicatedTypes[type] = [typeToServiceMap[type], service] + } else { + duplicatedTypes[type].push(service) + } } + typeToServiceMap[type] = service } for (const [type, fields] of Object.entries(serviceDefinition.extensionTypeMap)) { From 1a309534694a2b2ef8788c9f09fee0098580daef Mon Sep 17 00:00:00 2001 From: Manuel Spigolon Date: Tue, 1 Mar 2022 14:53:07 +0000 Subject: [PATCH 4/4] wip: node service selector --- lib/gateway.js | 15 +++++++++++---- test/gateway/type-redefined.js | 4 ++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/gateway.js b/lib/gateway.js index 281dfb50..133b03c3 100644 --- a/lib/gateway.js +++ b/lib/gateway.js @@ -124,7 +124,7 @@ function defineResolvers (schema, typeToServiceMap, serviceMap, typeFieldsToServ function getService (key, priorityService) { const service = typeToServiceMap[key] - if (service || !duplicatedTypes[key]) { + if (service || !duplicatedTypes[key] || priorityService === false) { return service } @@ -137,14 +137,19 @@ function defineResolvers (schema, typeToServiceMap, serviceMap, typeFieldsToServ for (const type of Object.values(types)) { if (isObjectType(type) && !isDefaultType(type.name)) { - const serviceForType = getService(type.name) + const serviceForType = getService(type) for (const field of Object.values(type.getFields())) { const fieldType = getNamedType(field.type) if (fieldType.astNode && fieldType.astNode.kind === Kind.ENUM_TYPE_DEFINITION) continue const fieldName = field.name if (!isScalarType(fieldType)) { - const serviceForFieldType = getService(fieldType, serviceForType) + // TODO: if the field type is/contains an external type, + // the getService function should skip the duplicated types logic + const isExternal = fieldType.astNode.isExternal + + const serviceForFieldType = getService(fieldType, isExternal) + /* istanbul ignore else */ if ( (serviceForFieldType === null && serviceForType !== null) || @@ -156,7 +161,9 @@ function defineResolvers (schema, typeToServiceMap, serviceMap, typeFieldsToServ * In this case, the field type is a value type and the type is neither a query, mutation nor a subscription. * - Or there is a service for the type and a service for the field type and both refer to the same service. */ - field.resolve = (parent, args, context, info) => parent && parent[info.path.key] + field.resolve = (parent, args, context, info) => { + return parent && parent[info.path.key] + } } else if (serviceForType === null) { /** * If the return type of a query, subscription or mutation is a value type, its service is undefined or null, e.g. for diff --git a/test/gateway/type-redefined.js b/test/gateway/type-redefined.js index 1b839070..b13c4f05 100644 --- a/test/gateway/type-redefined.js +++ b/test/gateway/type-redefined.js @@ -128,12 +128,12 @@ async function buildProxy (port1, port2) { } test('federated node should be able to redefine type', async (t) => { - const port1 = 3027 + const port1 = 3030 const serviceOne = await buildService() await serviceOne.listen(port1) t.teardown(() => { serviceOne.close() }) - const port2 = 3028 + const port2 = 3031 const serviceTwo = await buildServiceExternal() await serviceTwo.listen(port2) t.teardown(() => { serviceTwo.close() })