From 2e54ebac73414f10c093e10ad0d0771dae9296cc Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Sat, 26 Sep 2020 17:14:09 +0300 Subject: [PATCH 1/8] feat(merge): add onFieldTypeConflict to fix field type conflicts manually --- packages/merge/src/typedefs-mergers/fields.ts | 31 +++++++++++++------ .../src/typedefs-mergers/merge-typedefs.ts | 17 ++++++++++ 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/packages/merge/src/typedefs-mergers/fields.ts b/packages/merge/src/typedefs-mergers/fields.ts index 6591156edb8..0c68c036e1d 100644 --- a/packages/merge/src/typedefs-mergers/fields.ts +++ b/packages/merge/src/typedefs-mergers/fields.ts @@ -5,17 +5,30 @@ import { mergeDirectives } from './directives'; import { isNotEqual, compareNodes } from '@graphql-tools/utils'; import { mergeArguments } from './arguments'; -function fieldAlreadyExists(fieldsArr: ReadonlyArray, otherField: any): boolean { - const result: FieldDefinitionNode | null = fieldsArr.find(field => field.name.value === otherField.name.value); +type FieldDefNode = FieldDefinitionNode | InputValueDefinitionNode; +export type OnFieldTypeConflict = (existingField: FieldDefNode, otherField: FieldDefNode) => void; + +function fieldAlreadyExists( + fieldsArr: ReadonlyArray, + otherField: FieldDefNode, + onFieldTypeConflict?: OnFieldTypeConflict +): boolean { + const result: FieldDefinitionNode | FieldDefNode | null = fieldsArr.find( + field => field.name.value === otherField.name.value + ); if (result) { const t1 = extractType(result.type); const t2 = extractType(otherField.type); if (t1.name.value !== t2.name.value) { - throw new Error( - `Field "${otherField.name.value}" already defined with a different type. Declared as "${t1.name.value}", but you tried to override with "${t2.name.value}"` - ); + if (onFieldTypeConflict) { + onFieldTypeConflict(result, otherField); + } else { + throw new Error( + `Field "${otherField.name.value}" already defined with a different type. Declared as "${t1.name.value}", but you tried to override with "${t2.name.value}"` + ); + } } } @@ -31,10 +44,10 @@ export function mergeFields f.name.value === (field as any).name.value); - if (config && config.throwOnConflict) { + if (config?.throwOnConflict) { preventConflicts(type, existing, field, false); } else { preventConflicts(type, existing, field, true); @@ -51,10 +64,10 @@ export function mergeFields !config.exclusions.includes(`${type.name.value}.${field.name.value}`)); } return result; diff --git a/packages/merge/src/typedefs-mergers/merge-typedefs.ts b/packages/merge/src/typedefs-mergers/merge-typedefs.ts index e604c2c0f9b..49d98ba0fab 100644 --- a/packages/merge/src/typedefs-mergers/merge-typedefs.ts +++ b/packages/merge/src/typedefs-mergers/merge-typedefs.ts @@ -3,6 +3,7 @@ import { isSourceTypes, isStringTypes, isSchemaDefinition } from './utils'; import { MergedResultMap, mergeGraphQLNodes } from './merge-nodes'; import { resetComments, printWithComments } from './comments'; import { createSchemaDefinition, printSchemaWithDirectives } from '@graphql-tools/utils'; +import { OnFieldTypeConflict } from './fields'; type Omit = Pick>; type CompareFn = (a: T, b: T) => number; @@ -57,6 +58,22 @@ export interface Config { exclusions?: string[]; sort?: boolean | CompareFn; convertExtensions?: boolean; + /** + * Called if types of the same fields are different + * + * Default: false + * + * @example: + * Given: + * ```graphql + * type User { a: String } + * type User { a: Int } + * ``` + * + * Instead of throwing `already defined with a different type` error, + * `onFieldTypeConflict` function is called. + */ + onFieldTypeConflict?: OnFieldTypeConflict; } /** From 4ae305896c7e433f3156b99b9ccd08ef8e539793 Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Sat, 26 Sep 2020 17:34:37 +0300 Subject: [PATCH 2/8] Fix CI --- .github/workflows/automerge.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/automerge.yml b/.github/workflows/automerge.yml index 1dee9567a40..7afb75292d4 100644 --- a/.github/workflows/automerge.yml +++ b/.github/workflows/automerge.yml @@ -32,4 +32,4 @@ jobs: env: GITHUB_TOKEN: '${{ secrets.GITHUB_TOKEN }}' MERGE_METHOD: 'squash' - LABELS: automerge + MERGE_LABELS: automerge From 4e0b910ad638d3a4f9b482f984e321eac780c528 Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Sun, 27 Sep 2020 17:45:03 +0300 Subject: [PATCH 3/8] improvements --- packages/merge/src/typedefs-mergers/fields.ts | 65 +++++++++---------- 1 file changed, 29 insertions(+), 36 deletions(-) diff --git a/packages/merge/src/typedefs-mergers/fields.ts b/packages/merge/src/typedefs-mergers/fields.ts index 0c68c036e1d..8cd0eaa0915 100644 --- a/packages/merge/src/typedefs-mergers/fields.ts +++ b/packages/merge/src/typedefs-mergers/fields.ts @@ -6,33 +6,14 @@ import { isNotEqual, compareNodes } from '@graphql-tools/utils'; import { mergeArguments } from './arguments'; type FieldDefNode = FieldDefinitionNode | InputValueDefinitionNode; -export type OnFieldTypeConflict = (existingField: FieldDefNode, otherField: FieldDefNode) => void; +export type OnFieldTypeConflict = (existingField: FieldDefNode, otherField: FieldDefNode) => FieldDefNode | void; -function fieldAlreadyExists( - fieldsArr: ReadonlyArray, - otherField: FieldDefNode, - onFieldTypeConflict?: OnFieldTypeConflict -): boolean { +function fieldAlreadyExists(fieldsArr: ReadonlyArray, otherField: FieldDefNode): FieldDefNode { const result: FieldDefinitionNode | FieldDefNode | null = fieldsArr.find( field => field.name.value === otherField.name.value ); - if (result) { - const t1 = extractType(result.type); - const t2 = extractType(otherField.type); - - if (t1.name.value !== t2.name.value) { - if (onFieldTypeConflict) { - onFieldTypeConflict(result, otherField); - } else { - throw new Error( - `Field "${otherField.name.value}" already defined with a different type. Declared as "${t1.name.value}", but you tried to override with "${t2.name.value}"` - ); - } - } - } - - return !!result; + return result; } export function mergeFields( @@ -44,18 +25,9 @@ export function mergeFields f.name.value === (field as any).name.value); - - if (config?.throwOnConflict) { - preventConflicts(type, existing, field, false); - } else { - preventConflicts(type, existing, field, true); - } - - if (isNonNullTypeNode(field.type) && !isNonNullTypeNode(existing.type)) { - existing.type = field.type; - } + let existing: any = fieldAlreadyExists(result, field); + if (existing) { + existing = preventConflicts(type, existing, field, !config?.throwOnConflict, config?.onFieldTypeConflict); existing.arguments = mergeArguments(field['arguments'] || [], existing.arguments || [], config); existing.directives = mergeDirectives(field.directives, existing.directives, config); @@ -77,16 +49,37 @@ function preventConflicts( type: { name: NameNode }, a: FieldDefinitionNode | InputValueDefinitionNode, b: FieldDefinitionNode | InputValueDefinitionNode, - ignoreNullability = false + ignoreNullability = false, + onFieldTypeConflict: OnFieldTypeConflict ) { const aType = printTypeNode(a.type); const bType = printTypeNode(b.type); if (isNotEqual(aType, bType)) { - if (safeChangeForFieldType(a.type, b.type, ignoreNullability) === false) { + const t1 = extractType(a.type); + const t2 = extractType(b.type); + + if (t1.name.value !== t2.name.value) { + if (onFieldTypeConflict) { + return onFieldTypeConflict(a, b) || a; + } + throw new Error( + `Field "${b.name.value}" already defined with a different type. Declared as "${t1.name.value}", but you tried to override with "${t2.name.value}"` + ); + } + if (!safeChangeForFieldType(a.type, b.type, ignoreNullability)) { + if (onFieldTypeConflict) { + return onFieldTypeConflict(a, b) || a; + } throw new Error(`Field '${type.name.value}.${a.name.value}' changed type from '${aType}' to '${bType}'`); } } + + if (isNonNullTypeNode(b.type) && !isNonNullTypeNode(a.type)) { + (a as any).type = b.type; + } + + return a; } function safeChangeForFieldType(oldType: TypeNode, newType: TypeNode, ignoreNullability = false): boolean { From f62723991d3a33d17e949faf00a0263166156968 Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Mon, 28 Sep 2020 01:39:53 +0300 Subject: [PATCH 4/8] Call onFieldTypeConflict no matter what --- packages/merge/src/typedefs-mergers/fields.ts | 57 ++++++++++--------- packages/merge/tests/merge-typedefs.spec.ts | 56 +++++++++++++++++- 2 files changed, 84 insertions(+), 29 deletions(-) diff --git a/packages/merge/src/typedefs-mergers/fields.ts b/packages/merge/src/typedefs-mergers/fields.ts index 8cd0eaa0915..91c7cf7ff5c 100644 --- a/packages/merge/src/typedefs-mergers/fields.ts +++ b/packages/merge/src/typedefs-mergers/fields.ts @@ -1,23 +1,39 @@ import { Config } from './merge-typedefs'; -import { FieldDefinitionNode, InputValueDefinitionNode, TypeNode, NameNode } from 'graphql'; +import { FieldDefinitionNode, InputValueDefinitionNode, TypeNode, NameNode, NamedTypeNode } from 'graphql'; import { extractType, isWrappingTypeNode, isListTypeNode, isNonNullTypeNode, printTypeNode } from './utils'; import { mergeDirectives } from './directives'; import { isNotEqual, compareNodes } from '@graphql-tools/utils'; import { mergeArguments } from './arguments'; type FieldDefNode = FieldDefinitionNode | InputValueDefinitionNode; -export type OnFieldTypeConflict = (existingField: FieldDefNode, otherField: FieldDefNode) => FieldDefNode | void; +export type OnFieldTypeConflict = ( + existingField: FieldDefNode, + otherField: FieldDefNode, + type: NamedTypeNode, + config: Config +) => FieldDefNode; -function fieldAlreadyExists(fieldsArr: ReadonlyArray, otherField: FieldDefNode): FieldDefNode { - const result: FieldDefinitionNode | FieldDefNode | null = fieldsArr.find( - field => field.name.value === otherField.name.value - ); +function fieldAlreadyExists(fieldsArr: ReadonlyArray, otherField: FieldDefNode): [FieldDefNode, number] { + const resultIndex: number | null = fieldsArr.findIndex(field => field.name.value === otherField.name.value); - return result; + return [resultIndex > -1 ? fieldsArr[resultIndex] : null, resultIndex]; } +const defaultOnFieldTypeConflict: OnFieldTypeConflict = ( + f1: FieldDefNode, + f2: FieldDefNode, + type: NamedTypeNode, + config: Config +) => { + const newField: any = preventConflicts(type, f1, f2, !config?.throwOnConflict); + newField.arguments = mergeArguments(f2['arguments'] || [], f1['arguments'] || [], config); + newField.directives = mergeDirectives(f2.directives, f1.directives, config); + newField.description = f2.description || f1.description; + return newField; +}; + export function mergeFields( - type: { name: NameNode }, + type: NamedTypeNode, f1: ReadonlyArray, f2: ReadonlyArray, config: Config @@ -25,13 +41,10 @@ export function mergeFields { expect(mergeDirectives(directivesOne, directivesTwo, config)).toEqual(directivesTwo); }); - }) + }); + it('should call onFieldTypeConflict if there are two different types', () => { + const onFieldTypeConflict = jest.fn().mockImplementation((_, r) => r); + const typeDefs1 = parse(/* GraphQL */` + type Query { + foo: Int + } + `); + const typeDefs2 = parse(/* GraphQL */` + type Query { + foo: String + } + `); + const mergedTypeDefs = mergeTypeDefs([typeDefs1, typeDefs2], { + onFieldTypeConflict, + }); + expect(print(onFieldTypeConflict.mock.calls[0][0])).toContain('foo: Int'); + expect(print(onFieldTypeConflict.mock.calls[0][1])).toContain('foo: String'); + expect(print(mergedTypeDefs)).toBeSimilarGqlDoc(/* GraphQL */` + schema { + query: Query + } + + type Query { + foo: String + } + `); + }); it('should call onFieldTypeConflict if there are two same types but with different nullability', () => { + const onFieldTypeConflict = jest.fn().mockImplementation((_, r) => r); + const typeDefs1 = parse(/* GraphQL */` + type Query { + foo: String! + } + `); + const typeDefs2 = parse(/* GraphQL */` + type Query { + foo: String + } + `); + const mergedTypeDefs = mergeTypeDefs([typeDefs1, typeDefs2], { + onFieldTypeConflict, + }); + expect(print(onFieldTypeConflict.mock.calls[0][0])).toContain('foo: String!'); + expect(print(onFieldTypeConflict.mock.calls[0][1])).toContain('foo: String'); + expect(print(mergedTypeDefs)).toBeSimilarGqlDoc(/* GraphQL */` + schema { + query: Query + } + + type Query { + foo: String + } + `); + }); }); From 64c1f5bfb7bb1106e6c04461558fcb5d46112da7 Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Mon, 28 Sep 2020 01:42:48 +0300 Subject: [PATCH 5/8] Fix typings --- packages/merge/src/typedefs-mergers/fields.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/merge/src/typedefs-mergers/fields.ts b/packages/merge/src/typedefs-mergers/fields.ts index 91c7cf7ff5c..574ac15c62e 100644 --- a/packages/merge/src/typedefs-mergers/fields.ts +++ b/packages/merge/src/typedefs-mergers/fields.ts @@ -6,10 +6,11 @@ import { isNotEqual, compareNodes } from '@graphql-tools/utils'; import { mergeArguments } from './arguments'; type FieldDefNode = FieldDefinitionNode | InputValueDefinitionNode; +type NamedDefNode = { name: NameNode }; export type OnFieldTypeConflict = ( existingField: FieldDefNode, otherField: FieldDefNode, - type: NamedTypeNode, + type: NamedDefNode, config: Config ) => FieldDefNode; @@ -33,7 +34,7 @@ const defaultOnFieldTypeConflict: OnFieldTypeConflict = ( }; export function mergeFields( - type: NamedTypeNode, + type: NamedDefNode, f1: ReadonlyArray, f2: ReadonlyArray, config: Config From 26be83f130fa436bf43cf4d4f110237a1814603f Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Mon, 28 Sep 2020 01:56:14 +0300 Subject: [PATCH 6/8] Fix build --- packages/merge/src/typedefs-mergers/fields.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/merge/src/typedefs-mergers/fields.ts b/packages/merge/src/typedefs-mergers/fields.ts index 574ac15c62e..aa0c43ff205 100644 --- a/packages/merge/src/typedefs-mergers/fields.ts +++ b/packages/merge/src/typedefs-mergers/fields.ts @@ -1,5 +1,5 @@ import { Config } from './merge-typedefs'; -import { FieldDefinitionNode, InputValueDefinitionNode, TypeNode, NameNode, NamedTypeNode } from 'graphql'; +import { FieldDefinitionNode, InputValueDefinitionNode, TypeNode, NameNode } from 'graphql'; import { extractType, isWrappingTypeNode, isListTypeNode, isNonNullTypeNode, printTypeNode } from './utils'; import { mergeDirectives } from './directives'; import { isNotEqual, compareNodes } from '@graphql-tools/utils'; @@ -23,7 +23,7 @@ function fieldAlreadyExists(fieldsArr: ReadonlyArray, otherField: const defaultOnFieldTypeConflict: OnFieldTypeConflict = ( f1: FieldDefNode, f2: FieldDefNode, - type: NamedTypeNode, + type: NamedDefNode, config: Config ) => { const newField: any = preventConflicts(type, f1, f2, !config?.throwOnConflict); From ef2c632d24d74196204f76b5fbdc9225d6c35d66 Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Mon, 28 Sep 2020 12:13:38 +0300 Subject: [PATCH 7/8] merge fields except types if there is onFieldTypeConflict --- packages/merge/src/typedefs-mergers/fields.ts | 25 ++++++------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/packages/merge/src/typedefs-mergers/fields.ts b/packages/merge/src/typedefs-mergers/fields.ts index aa0c43ff205..d65aa340e22 100644 --- a/packages/merge/src/typedefs-mergers/fields.ts +++ b/packages/merge/src/typedefs-mergers/fields.ts @@ -20,19 +20,6 @@ function fieldAlreadyExists(fieldsArr: ReadonlyArray, otherField: return [resultIndex > -1 ? fieldsArr[resultIndex] : null, resultIndex]; } -const defaultOnFieldTypeConflict: OnFieldTypeConflict = ( - f1: FieldDefNode, - f2: FieldDefNode, - type: NamedDefNode, - config: Config -) => { - const newField: any = preventConflicts(type, f1, f2, !config?.throwOnConflict); - newField.arguments = mergeArguments(f2['arguments'] || [], f1['arguments'] || [], config); - newField.directives = mergeDirectives(f2.directives, f1.directives, config); - newField.description = f2.description || f1.description; - return newField; -}; - export function mergeFields( type: NamedDefNode, f1: ReadonlyArray, @@ -44,8 +31,12 @@ export function mergeFields Date: Mon, 28 Sep 2020 12:17:50 +0300 Subject: [PATCH 8/8] fix tests --- packages/merge/src/typedefs-mergers/fields.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/merge/src/typedefs-mergers/fields.ts b/packages/merge/src/typedefs-mergers/fields.ts index d65aa340e22..4696ab48a1d 100644 --- a/packages/merge/src/typedefs-mergers/fields.ts +++ b/packages/merge/src/typedefs-mergers/fields.ts @@ -63,7 +63,7 @@ function preventConflicts(a: FieldDefNode, b: FieldDefNode, type: { name: NameNo `Field "${b.name.value}" already defined with a different type. Declared as "${t1.name.value}", but you tried to override with "${t2.name.value}"` ); } - if (!safeChangeForFieldType(a.type, b.type, !config.throwOnConflict)) { + if (!safeChangeForFieldType(a.type, b.type, !config?.throwOnConflict)) { throw new Error(`Field '${type.name.value}.${a.name.value}' changed type from '${aType}' to '${bType}'`); } }