From 477724fe56df8b94293d16bb246511edf4d4f8aa Mon Sep 17 00:00:00 2001 From: squidfunk Date: Thu, 1 Feb 2018 17:01:06 +0100 Subject: [PATCH 1/6] Added flag for mergeTypes to merge all types --- src/merge_types.js | 15 ++++++++------- test/graphql/other/mergeable_custom_type.js | 9 +++++++++ test/merge_types.test.js | 16 ++++++++++++++++ 3 files changed, 33 insertions(+), 7 deletions(-) create mode 100644 test/graphql/other/mergeable_custom_type.js diff --git a/src/merge_types.js b/src/merge_types.js index e05afeb..2088aca 100644 --- a/src/merge_types.js +++ b/src/merge_types.js @@ -31,9 +31,9 @@ const _addCommentsToAST = (nodes, flatten = true) => { return astWithComments; }; -const _makeRestDefinitions = defs => +const _makeRestDefinitions = (defs, all = false) => defs - .filter(def => _isNonMergeableTypeDefinition(def) && !isObjectSchemaDefinition(def)) + .filter(def => (_isNonMergeableTypeDefinition(def) && !all) && !isObjectSchemaDefinition(def)) .map((def) => { if (isObjectTypeDefinition(def)) { return { @@ -45,10 +45,10 @@ const _makeRestDefinitions = defs => return def; }); -const _makeMergedDefinitions = (defs) => { +const _makeMergedDefinitions = (defs, all = false) => { // TODO: This function can be cleaner! const groupedMergableDefinitions = defs - .filter(_isMergeableTypeDefinition) + .filter(def => _isMergeableTypeDefinition(def) || all) .reduce( (mergableDefs, def) => { const name = def.name.value; @@ -92,7 +92,7 @@ const _makeDocumentWithDefinitions = definitions => ({ const printDefinitions = defs => print(_makeDocumentWithDefinitions(defs)); -const mergeTypes = (types) => { +const mergeTypes = (types, options = { all: false }) => { const allDefs = types .map((type) => { if (typeof type === 'string') { @@ -103,8 +103,9 @@ const mergeTypes = (types) => { .map(ast => ast.definitions) .reduce((defs, newDef) => [...defs, ...newDef], []); - const mergedDefs = _makeMergedDefinitions(allDefs); - const rest = _addCommentsToAST(_makeRestDefinitions(allDefs), false).map(printDefinitions); + const mergedDefs = _makeMergedDefinitions(allDefs, options.all); + const rest = _addCommentsToAST(_makeRestDefinitions(allDefs, options.all), false) + .map(printDefinitions); const schemaDefs = allDefs.filter(isObjectSchemaDefinition); const schema = printDefinitions([makeSchema(mergedDefs, schemaDefs), ...mergedDefs]); diff --git a/test/graphql/other/mergeable_custom_type.js b/test/graphql/other/mergeable_custom_type.js new file mode 100644 index 0000000..87e2159 --- /dev/null +++ b/test/graphql/other/mergeable_custom_type.js @@ -0,0 +1,9 @@ +export default ` + type Custom { + id: ID! + } + type Custom { + name: String + age: Int + } +`; diff --git a/test/merge_types.test.js b/test/merge_types.test.js index cb3aba7..c09ee56 100644 --- a/test/merge_types.test.js +++ b/test/merge_types.test.js @@ -7,6 +7,7 @@ import vendorType from './graphql/types/vendor_type'; import personEntityType from './graphql/types/person_entity_type'; import personSearchType from './graphql/types/person_search_type'; import customType from './graphql/other/custom_type'; +import mergeableCustomType from './graphql/other/mergeable_custom_type'; import simpleQueryType from './graphql/other/simple_query_type'; const normalizeWhitespace = str => str.replace(/\s+/g, ' ').trim(); @@ -131,6 +132,21 @@ describe('mergeTypes', () => { expect(separateTypes).toContain(expectedCustomType); }); + it('includes merged customType', () => { + const types = [mergeableCustomType]; + const mergedTypes = mergeTypes(types, { all: true }); + const expectedCustomType = normalizeWhitespace(` + type Custom { + id: ID! + name: String + age: Int + } + `); + const separateTypes = normalizeWhitespace(mergedTypes); + + expect(separateTypes).toContain(expectedCustomType); + }); + it('returns minimal schema', () => { const types = [customType]; const mergedTypes = mergeTypes(types); From 6ee3a719077b1e193cdc85208ea8aba452004a65 Mon Sep 17 00:00:00 2001 From: squidfunk Date: Thu, 1 Feb 2018 17:31:11 +0100 Subject: [PATCH 2/6] Only merge custom types, not enums --- src/merge_types.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/merge_types.js b/src/merge_types.js index 2088aca..2fc340a 100644 --- a/src/merge_types.js +++ b/src/merge_types.js @@ -5,10 +5,10 @@ import print from './utilities/astPrinter'; import { isObjectTypeDefinition, isObjectSchemaDefinition } from './utilities/astHelpers'; import { makeSchema, mergeableTypes } from './utilities/makeSchema'; -const _isMergeableTypeDefinition = def => - isObjectTypeDefinition(def) && mergeableTypes.includes(def.name.value); +const _isMergeableTypeDefinition = (def, all) => + isObjectTypeDefinition(def) && (mergeableTypes.includes(def.name.value) || all); -const _isNonMergeableTypeDefinition = def => !_isMergeableTypeDefinition(def); +const _isNonMergeableTypeDefinition = (def, all) => !_isMergeableTypeDefinition(def, all); const _makeCommentNode = value => ({ kind: 'Comment', value }); @@ -33,7 +33,7 @@ const _addCommentsToAST = (nodes, flatten = true) => { const _makeRestDefinitions = (defs, all = false) => defs - .filter(def => (_isNonMergeableTypeDefinition(def) && !all) && !isObjectSchemaDefinition(def)) + .filter(def => _isNonMergeableTypeDefinition(def, all) && !isObjectSchemaDefinition(def)) .map((def) => { if (isObjectTypeDefinition(def)) { return { @@ -48,7 +48,7 @@ const _makeRestDefinitions = (defs, all = false) => const _makeMergedDefinitions = (defs, all = false) => { // TODO: This function can be cleaner! const groupedMergableDefinitions = defs - .filter(def => _isMergeableTypeDefinition(def) || all) + .filter(def => _isMergeableTypeDefinition(def, all)) .reduce( (mergableDefs, def) => { const name = def.name.value; From 9620c4529f8e4c445d25415d1e47fbc28c61b26d Mon Sep 17 00:00:00 2001 From: squidfunk Date: Mon, 5 Feb 2018 12:06:57 +0100 Subject: [PATCH 3/6] Added unit tests for custom type merging --- test/graphql/other/custom_type/conflicting.js | 10 +++ test/graphql/other/custom_type/disjoint.js | 15 +++++ .../{custom_type.js => custom_type/index.js} | 0 .../matching.js} | 1 + test/merge_types.test.js | 67 ++++++++++++++----- 5 files changed, 76 insertions(+), 17 deletions(-) create mode 100644 test/graphql/other/custom_type/conflicting.js create mode 100644 test/graphql/other/custom_type/disjoint.js rename test/graphql/other/{custom_type.js => custom_type/index.js} (100%) rename test/graphql/other/{mergeable_custom_type.js => custom_type/matching.js} (88%) diff --git a/test/graphql/other/custom_type/conflicting.js b/test/graphql/other/custom_type/conflicting.js new file mode 100644 index 0000000..30bc44c --- /dev/null +++ b/test/graphql/other/custom_type/conflicting.js @@ -0,0 +1,10 @@ +export default ` + type Custom { + id: ID! + age: Int + } + type Custom { + name: String + age: String + } +`; diff --git a/test/graphql/other/custom_type/disjoint.js b/test/graphql/other/custom_type/disjoint.js new file mode 100644 index 0000000..216e9ff --- /dev/null +++ b/test/graphql/other/custom_type/disjoint.js @@ -0,0 +1,15 @@ +export default ` + type Custom { + id: ID! + } + type Custom { + name: String + age: Int + } + type Query { + foo: Int + } + type Query { + foo: Int + } +`; diff --git a/test/graphql/other/custom_type.js b/test/graphql/other/custom_type/index.js similarity index 100% rename from test/graphql/other/custom_type.js rename to test/graphql/other/custom_type/index.js diff --git a/test/graphql/other/mergeable_custom_type.js b/test/graphql/other/custom_type/matching.js similarity index 88% rename from test/graphql/other/mergeable_custom_type.js rename to test/graphql/other/custom_type/matching.js index 87e2159..676e0d5 100644 --- a/test/graphql/other/mergeable_custom_type.js +++ b/test/graphql/other/custom_type/matching.js @@ -1,6 +1,7 @@ export default ` type Custom { id: ID! + age: Int } type Custom { name: String diff --git a/test/merge_types.test.js b/test/merge_types.test.js index c09ee56..836c928 100644 --- a/test/merge_types.test.js +++ b/test/merge_types.test.js @@ -7,7 +7,11 @@ import vendorType from './graphql/types/vendor_type'; import personEntityType from './graphql/types/person_entity_type'; import personSearchType from './graphql/types/person_search_type'; import customType from './graphql/other/custom_type'; -import mergeableCustomType from './graphql/other/mergeable_custom_type'; +import disjointCustomTypes from './graphql/other/custom_type/disjoint'; +import matchingCustomTypes from './graphql/other/custom_type/matching'; +import conflictingCustomTypes from './graphql/other/custom_type/conflicting'; + + import simpleQueryType from './graphql/other/simple_query_type'; const normalizeWhitespace = str => str.replace(/\s+/g, ' ').trim(); @@ -117,7 +121,7 @@ describe('mergeTypes', () => { }); describe('when only single custom type is passed', () => { - it('includes customType', () => { + it('includes custom type', () => { const types = [customType]; const mergedTypes = mergeTypes(types); const expectedCustomType = normalizeWhitespace(` @@ -132,21 +136,6 @@ describe('mergeTypes', () => { expect(separateTypes).toContain(expectedCustomType); }); - it('includes merged customType', () => { - const types = [mergeableCustomType]; - const mergedTypes = mergeTypes(types, { all: true }); - const expectedCustomType = normalizeWhitespace(` - type Custom { - id: ID! - name: String - age: Int - } - `); - const separateTypes = normalizeWhitespace(mergedTypes); - - expect(separateTypes).toContain(expectedCustomType); - }); - it('returns minimal schema', () => { const types = [customType]; const mergedTypes = mergeTypes(types); @@ -197,6 +186,50 @@ describe('mergeTypes', () => { }); }); + describe('when custom type is present twice', () => { + it('merges disjoint custom types', () => { + const types = [disjointCustomTypes]; + const mergedTypes = mergeTypes(types, { all: true }); + const expectedCustomType = normalizeWhitespace(` + type Custom { + id: ID! + name: String + age: Int + } + `); + const separateTypes = normalizeWhitespace(mergedTypes); + expect(separateTypes).toContain(expectedCustomType); + }); + + it('merges custom types with matching definitions', () => { + const types = [matchingCustomTypes]; + const mergedTypes = mergeTypes(types, { all: true }); + const expectedCustomType = normalizeWhitespace(` + type Custom { + id: ID! + name: String + age: Int + } + `); + const separateTypes = normalizeWhitespace(mergedTypes); + expect(separateTypes).toContain(expectedCustomType); + }); + + it('throws on custom types with conflicting definitions', () => { + const types = [conflictingCustomTypes]; + const mergedTypes = mergeTypes(types, { all: true }); + const expectedCustomType = normalizeWhitespace(` + type Custom { + id: ID! + name: String + age: Int + } + `); + const separateTypes = normalizeWhitespace(mergedTypes); + expect(separateTypes).toContain(expectedCustomType); + }); + }); + it('includes schemaType', () => { const types = [clientType, productType]; const mergedTypes = mergeTypes(types); From f47a4eaff9baac22ff1cfce8c91a41728ee6386d Mon Sep 17 00:00:00 2001 From: squidfunk Date: Mon, 5 Feb 2018 12:55:47 +0100 Subject: [PATCH 4/6] Added field definition merging --- src/merge_types.js | 19 +++++++++++++++---- test/graphql/other/custom_type/disjoint.js | 6 ------ test/merge_types.test.js | 15 ++++----------- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/merge_types.js b/src/merge_types.js index 2fc340a..5e367fc 100644 --- a/src/merge_types.js +++ b/src/merge_types.js @@ -45,6 +45,20 @@ const _makeRestDefinitions = (defs, all = false) => return def; }); +const _makeMergedFieldDefinitions = (merged, candidate) => _addCommentsToAST(candidate.fields) + .reduce((fields, field) => { + const original = merged.fields.find(base => base.name.value === field.name.value); + if (!original) { + fields.push(field); + } else if (field.type.name.value !== original.type.name.value) { + throw new Error( + `Conflicting types for ${merged.name.value}.${field.name.value}: ` + + `${field.type.name.value} != ${original.type.name.value}`, + ); // TODO: + } + return fields; + }, merged.fields); + const _makeMergedDefinitions = (defs, all = false) => { // TODO: This function can be cleaner! const groupedMergableDefinitions = defs @@ -67,10 +81,7 @@ const _makeMergedDefinitions = (defs, all = false) => { ...mergableDefs, [name]: { ...mergableDefs[name], - fields: [ - ...mergableDefs[name].fields, - ..._addCommentsToAST(def.fields), - ], + fields: _makeMergedFieldDefinitions(mergableDefs[name], def), }, }; }, { diff --git a/test/graphql/other/custom_type/disjoint.js b/test/graphql/other/custom_type/disjoint.js index 216e9ff..87e2159 100644 --- a/test/graphql/other/custom_type/disjoint.js +++ b/test/graphql/other/custom_type/disjoint.js @@ -6,10 +6,4 @@ export default ` name: String age: Int } - type Query { - foo: Int - } - type Query { - foo: Int - } `; diff --git a/test/merge_types.test.js b/test/merge_types.test.js index 836c928..2d0fd5c 100644 --- a/test/merge_types.test.js +++ b/test/merge_types.test.js @@ -207,8 +207,8 @@ describe('mergeTypes', () => { const expectedCustomType = normalizeWhitespace(` type Custom { id: ID! - name: String age: Int + name: String } `); const separateTypes = normalizeWhitespace(mergedTypes); @@ -217,16 +217,9 @@ describe('mergeTypes', () => { it('throws on custom types with conflicting definitions', () => { const types = [conflictingCustomTypes]; - const mergedTypes = mergeTypes(types, { all: true }); - const expectedCustomType = normalizeWhitespace(` - type Custom { - id: ID! - name: String - age: Int - } - `); - const separateTypes = normalizeWhitespace(mergedTypes); - expect(separateTypes).toContain(expectedCustomType); + expect(() => { + mergeTypes(types, { all: true }); + }).toThrow(expect.any(Error)); }); }); From 0e5984249177a4309c7de016f254d10f14136bdb Mon Sep 17 00:00:00 2001 From: squidfunk Date: Mon, 5 Feb 2018 13:07:03 +0100 Subject: [PATCH 5/6] Added tests for root-level type merging --- src/merge_types.js | 2 +- test/graphql/other/query_type/conflicting.js | 14 +++++++ test/graphql/other/query_type/disjoint.js | 13 +++++++ test/graphql/other/query_type/matching.js | 15 ++++++++ test/merge_types.test.js | 39 +++++++++++++++++++- 5 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 test/graphql/other/query_type/conflicting.js create mode 100644 test/graphql/other/query_type/disjoint.js create mode 100644 test/graphql/other/query_type/matching.js diff --git a/src/merge_types.js b/src/merge_types.js index 5e367fc..94a0868 100644 --- a/src/merge_types.js +++ b/src/merge_types.js @@ -54,7 +54,7 @@ const _makeMergedFieldDefinitions = (merged, candidate) => _addCommentsToAST(can throw new Error( `Conflicting types for ${merged.name.value}.${field.name.value}: ` + `${field.type.name.value} != ${original.type.name.value}`, - ); // TODO: + ); } return fields; }, merged.fields); diff --git a/test/graphql/other/query_type/conflicting.js b/test/graphql/other/query_type/conflicting.js new file mode 100644 index 0000000..c52d2dd --- /dev/null +++ b/test/graphql/other/query_type/conflicting.js @@ -0,0 +1,14 @@ +export default ` + type Client { + id: ID! + name: String + age: Int + } + type Query { + getClient(id: ID!): Client + } + type Query { + getClient(id: ID!): Boolean + deleteClient(id: ID!): Client + } +`; diff --git a/test/graphql/other/query_type/disjoint.js b/test/graphql/other/query_type/disjoint.js new file mode 100644 index 0000000..090510b --- /dev/null +++ b/test/graphql/other/query_type/disjoint.js @@ -0,0 +1,13 @@ +export default ` + type Client { + id: ID! + name: String + age: Int + } + type Query { + getClient(id: ID!): Client + } + type Query { + deleteClient(id: ID!): Client + } +`; diff --git a/test/graphql/other/query_type/matching.js b/test/graphql/other/query_type/matching.js new file mode 100644 index 0000000..a252374 --- /dev/null +++ b/test/graphql/other/query_type/matching.js @@ -0,0 +1,15 @@ +export default ` + type Client { + id: ID! + name: String + age: Int + } + type Query { + getClient(id: ID!): Client + deleteClient(id: ID!): Client + } + type Query { + getClient(id: ID!): Client + deleteClient(id: ID!): Client + } +`; diff --git a/test/merge_types.test.js b/test/merge_types.test.js index 2d0fd5c..03bf96c 100644 --- a/test/merge_types.test.js +++ b/test/merge_types.test.js @@ -11,8 +11,10 @@ import disjointCustomTypes from './graphql/other/custom_type/disjoint'; import matchingCustomTypes from './graphql/other/custom_type/matching'; import conflictingCustomTypes from './graphql/other/custom_type/conflicting'; - import simpleQueryType from './graphql/other/simple_query_type'; +import disjointQueryTypes from './graphql/other/query_type/disjoint'; +import matchingQueryTypes from './graphql/other/query_type/matching'; +import conflictingQueryTypes from './graphql/other/query_type/conflicting'; const normalizeWhitespace = str => str.replace(/\s+/g, ' ').trim(); @@ -120,6 +122,41 @@ describe('mergeTypes', () => { }); }); + describe('when query type is present twice', () => { + it('merges disjoint query types', () => { + const types = [disjointQueryTypes]; + const mergedTypes = mergeTypes(types); + const expectedSchemaType = normalizeWhitespace(` + type Query { + getClient(id: ID!): Client + deleteClient(id: ID!): Client + } + `); + const separateTypes = normalizeWhitespace(mergedTypes); + expect(separateTypes).toContain(expectedSchemaType); + }); + + it('merges query types with matching definitions', () => { + const types = [matchingQueryTypes]; + const mergedTypes = mergeTypes(types); + const expectedSchemaType = normalizeWhitespace(` + type Query { + getClient(id: ID!): Client + deleteClient(id: ID!): Client + } + `); + const separateTypes = normalizeWhitespace(mergedTypes); + expect(separateTypes).toContain(expectedSchemaType); + }); + + it('throws on query types with conflicting definitions', () => { + const types = [conflictingQueryTypes]; + expect(() => { + mergeTypes(types); + }).toThrow(expect.any(Error)); + }); + }); + describe('when only single custom type is passed', () => { it('includes custom type', () => { const types = [customType]; From a74044906e6e3fd62f3c42152538b06470916cf1 Mon Sep 17 00:00:00 2001 From: squidfunk Date: Mon, 5 Feb 2018 14:34:39 +0100 Subject: [PATCH 6/6] Fixed parse error for comment nodes --- src/merge_types.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/merge_types.js b/src/merge_types.js index 94a0868..b97823d 100644 --- a/src/merge_types.js +++ b/src/merge_types.js @@ -47,7 +47,9 @@ const _makeRestDefinitions = (defs, all = false) => const _makeMergedFieldDefinitions = (merged, candidate) => _addCommentsToAST(candidate.fields) .reduce((fields, field) => { - const original = merged.fields.find(base => base.name.value === field.name.value); + const original = merged.fields.find(base => base.name && typeof base.name.value !== 'undefined' && + field.name && typeof field.name.value !== 'undefined' && + base.name.value === field.name.value); if (!original) { fields.push(field); } else if (field.type.name.value !== original.type.name.value) {