From f201681bf806a2c46c4ee8b2533287421327a302 Mon Sep 17 00:00:00 2001 From: Benjie Date: Thu, 9 Feb 2023 01:51:45 +0000 Subject: [PATCH] Fix ambiguity around when schema definition may be omitted (#3839) This PR implements the tests and fixes necessary for [Spec RFC #987](https://github.com/graphql/graphql-spec/pull/987). This PR is made of three main commits: 1. Test printing a schema that has `Query`, `Mutation` and `Virus` types, but only supports `query` operations (via the `Query` type) and does _not_ support `mutation` operations. 2. Test parsing this same schema text, and assert that the schema does not have a mutation type. 3. Fix the printing of the schema. Co-authored-by: Lee Byron --- src/__testUtils__/viralSDL.ts | 19 ++++++ src/__testUtils__/viralSchema.ts | 44 +++++++++++++ .../__tests__/buildASTSchema-test.ts | 11 ++++ src/utilities/__tests__/printSchema-test.ts | 6 ++ src/utilities/printSchema.ts | 65 +++++++++---------- 5 files changed, 110 insertions(+), 35 deletions(-) create mode 100644 src/__testUtils__/viralSDL.ts create mode 100644 src/__testUtils__/viralSchema.ts diff --git a/src/__testUtils__/viralSDL.ts b/src/__testUtils__/viralSDL.ts new file mode 100644 index 0000000000..e66ff169a7 --- /dev/null +++ b/src/__testUtils__/viralSDL.ts @@ -0,0 +1,19 @@ +export const viralSDL = `\ +schema { + query: Query +} + +type Query { + viruses: [Virus!] +} + +type Virus { + name: String! + knownMutations: [Mutation!]! +} + +type Mutation { + name: String! + geneSequence: String! +}\ +`; diff --git a/src/__testUtils__/viralSchema.ts b/src/__testUtils__/viralSchema.ts new file mode 100644 index 0000000000..62613dc710 --- /dev/null +++ b/src/__testUtils__/viralSchema.ts @@ -0,0 +1,44 @@ +import { + GraphQLList, + GraphQLNonNull, + GraphQLObjectType, +} from '../type/definition.js'; +import { GraphQLString } from '../type/scalars.js'; +import { GraphQLSchema } from '../type/schema.js'; + +const Mutation = new GraphQLObjectType({ + name: 'Mutation', + fields: { + name: { + type: new GraphQLNonNull(GraphQLString), + }, + geneSequence: { + type: new GraphQLNonNull(GraphQLString), + }, + }, +}); + +const Virus = new GraphQLObjectType({ + name: 'Virus', + fields: { + name: { + type: new GraphQLNonNull(GraphQLString), + }, + knownMutations: { + type: new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(Mutation))), + }, + }, +}); + +const Query = new GraphQLObjectType({ + name: 'Query', + fields: { + viruses: { + type: new GraphQLList(new GraphQLNonNull(Virus)), + }, + }, +}); + +export const viralSchema = new GraphQLSchema({ + query: Query, +}); diff --git a/src/utilities/__tests__/buildASTSchema-test.ts b/src/utilities/__tests__/buildASTSchema-test.ts index 204ceb800c..6e152af36c 100644 --- a/src/utilities/__tests__/buildASTSchema-test.ts +++ b/src/utilities/__tests__/buildASTSchema-test.ts @@ -2,6 +2,7 @@ import { assert, expect } from 'chai'; import { describe, it } from 'mocha'; import { dedent } from '../../__testUtils__/dedent.js'; +import { viralSDL } from '../../__testUtils__/viralSDL.js'; import type { Maybe } from '../../jsutils/Maybe.js'; @@ -1092,4 +1093,14 @@ describe('Schema Builder', () => { 'Unknown type: "UnknownType".', ); }); + + it('correctly processes viral schema', () => { + const schema = buildSchema(viralSDL); + expect(schema.getQueryType()).to.contain({ name: 'Query' }); + expect(schema.getType('Virus')).to.contain({ name: 'Virus' }); + expect(schema.getType('Mutation')).to.contain({ name: 'Mutation' }); + // Though the viral schema has a 'Mutation' type, it is not used for the + // 'mutation' operation. + expect(schema.getMutationType()).to.equal(undefined); + }); }); diff --git a/src/utilities/__tests__/printSchema-test.ts b/src/utilities/__tests__/printSchema-test.ts index 7248d747b6..29799a4881 100644 --- a/src/utilities/__tests__/printSchema-test.ts +++ b/src/utilities/__tests__/printSchema-test.ts @@ -2,6 +2,8 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; import { dedent, dedentString } from '../../__testUtils__/dedent.js'; +import { viralSchema } from '../../__testUtils__/viralSchema.js'; +import { viralSDL } from '../../__testUtils__/viralSDL.js'; import { DirectiveLocation } from '../../language/directiveLocation.js'; @@ -867,4 +869,8 @@ describe('Type System Printer', () => { } `); }); + it('prints viral schema correctly', () => { + const printed = printSchema(viralSchema); + expect(printed).to.equal(viralSDL); + }); }); diff --git a/src/utilities/printSchema.ts b/src/utilities/printSchema.ts index 527cf4be17..0b9b1638d7 100644 --- a/src/utilities/printSchema.ts +++ b/src/utilities/printSchema.ts @@ -70,28 +70,28 @@ function printFilteredSchema( } function printSchemaDefinition(schema: GraphQLSchema): Maybe { - if (schema.description == null && isSchemaOfCommonNames(schema)) { - return; - } - - const operationTypes = []; - const queryType = schema.getQueryType(); - if (queryType) { - operationTypes.push(` query: ${queryType.name}`); - } - const mutationType = schema.getMutationType(); - if (mutationType) { - operationTypes.push(` mutation: ${mutationType.name}`); - } - const subscriptionType = schema.getSubscriptionType(); - if (subscriptionType) { - operationTypes.push(` subscription: ${subscriptionType.name}`); + + // Special case: When a schema has no root operation types, no valid schema + // definition can be printed. + if (!queryType && !mutationType && !subscriptionType) { + return; } - return printDescription(schema) + `schema {\n${operationTypes.join('\n')}\n}`; + // Only print a schema definition if there is a description or if it should + // not be omitted because of having default type names. + if (schema.description || !hasDefaultRootOperationTypes(schema)) { + return ( + printDescription(schema) + + 'schema {\n' + + (queryType ? ` query: ${queryType.name}\n` : '') + + (mutationType ? ` mutation: ${mutationType.name}\n` : '') + + (subscriptionType ? ` subscription: ${subscriptionType.name}\n` : '') + + '}' + ); + } } /** @@ -107,25 +107,20 @@ function printSchemaDefinition(schema: GraphQLSchema): Maybe { * } * ``` * - * When using this naming convention, the schema description can be omitted. + * When using this naming convention, the schema description can be omitted so + * long as these names are only used for operation types. + * + * Note however that if any of these default names are used elsewhere in the + * schema but not as a root operation type, the schema definition must still + * be printed to avoid ambiguity. */ -function isSchemaOfCommonNames(schema: GraphQLSchema): boolean { - const queryType = schema.getQueryType(); - if (queryType && queryType.name !== 'Query') { - return false; - } - - const mutationType = schema.getMutationType(); - if (mutationType && mutationType.name !== 'Mutation') { - return false; - } - - const subscriptionType = schema.getSubscriptionType(); - if (subscriptionType && subscriptionType.name !== 'Subscription') { - return false; - } - - return true; +function hasDefaultRootOperationTypes(schema: GraphQLSchema): boolean { + /* eslint-disable eqeqeq */ + return ( + schema.getQueryType() == schema.getType('Query') && + schema.getMutationType() == schema.getType('Mutation') && + schema.getSubscriptionType() == schema.getType('Subscription') + ); } export function printType(type: GraphQLNamedType): string {