From eb1533574750da458d4809f46c4587199761cafc Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Thu, 7 Dec 2017 12:38:39 -0500 Subject: [PATCH 1/3] Add a utility to detect description changes We use this internally for pinging docs team for PR reviews. --- .../__tests__/findDescriptionChanges-test.js | 76 ++++++++++ src/utilities/findDescriptionChanges.js | 142 ++++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 src/utilities/__tests__/findDescriptionChanges-test.js create mode 100644 src/utilities/findDescriptionChanges.js diff --git a/src/utilities/__tests__/findDescriptionChanges-test.js b/src/utilities/__tests__/findDescriptionChanges-test.js new file mode 100644 index 0000000000..456f7768c7 --- /dev/null +++ b/src/utilities/__tests__/findDescriptionChanges-test.js @@ -0,0 +1,76 @@ +/** + * Copyright (c) 2016, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { GraphQLObjectType, GraphQLSchema, GraphQLString } from '../../type'; +import { findDescriptionChanges } from '../findDescriptionChanges'; + +describe('findDescriptionChanges', () => { + const queryType = new GraphQLObjectType({ + name: 'Query', + fields: { + field1: { type: GraphQLString }, + }, + }); + + it('should detect if a description was added to a type', () => { + const typeOld = new GraphQLObjectType({ + name: 'Type', + fields: { + field1: { type: GraphQLString }, + }, + }); + const typeNew = new GraphQLObjectType({ + name: 'Type', + description: 'Something rather', + fields: { + field1: { type: GraphQLString }, + }, + }); + + const oldSchema = new GraphQLSchema({ + query: queryType, + types: [typeOld], + }); + const newSchema = new GraphQLSchema({ + query: queryType, + types: [typeNew], + }); + expect(findDescriptionChanges(oldSchema, newSchema)).to.eql([ + 'Description added on type Type.', + ]); + expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]); + expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]); + }); + + it('should detect if a type with a description was added', () => { + const type = new GraphQLObjectType({ + name: 'Type', + description: 'Something rather', + fields: { + field1: { type: GraphQLString }, + }, + }); + + const oldSchema = new GraphQLSchema({ + query: queryType, + types: [], + }); + const newSchema = new GraphQLSchema({ + query: queryType, + types: [type], + }); + expect(findDescriptionChanges(oldSchema, newSchema)).to.eql([ + 'Description added on new type Type.', + ]); + expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]); + expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]); + }); +}); diff --git a/src/utilities/findDescriptionChanges.js b/src/utilities/findDescriptionChanges.js new file mode 100644 index 0000000000..db1442c84c --- /dev/null +++ b/src/utilities/findDescriptionChanges.js @@ -0,0 +1,142 @@ +/* eslint-disable no-restricted-syntax */ +// @flow + +import { + GraphQLInterfaceType, + GraphQLObjectType, + GraphQLEnumType, + GraphQLInputObjectType, +} from '../type/definition'; +import type { GraphQLFieldMap } from '../type/definition'; +import { GraphQLSchema } from '../type/schema'; + +/** + * Given two schemas, returns an Array containing descriptions of any + * descriptions that are new or changed and need review. + */ +export function findDescriptionChanges( + oldSchema: GraphQLSchema, + newSchema: GraphQLSchema, +): Array { + const oldTypeMap = oldSchema.getTypeMap(); + const newTypeMap = newSchema.getTypeMap(); + + const descriptionChanges: Array = []; + + Object.keys(newTypeMap).forEach(typeName => { + const oldType = oldTypeMap[typeName]; + const newType = newTypeMap[typeName]; + + if (newType.description) { + if (!oldType) { + descriptionChanges.push( + `Description added on new type ${newType.name}.`, + ); + } else if (!oldType.description) { + descriptionChanges.push(`Description added on type ${newType.name}.`); + } else if (oldType.description !== newType.description) { + descriptionChanges.push(`Description changed on type ${newType.name}.`); + } + } + + if (oldType && !(newType instanceof oldType.constructor)) { + return; + } + + if ( + newType instanceof GraphQLObjectType || + newType instanceof GraphQLInterfaceType || + newType instanceof GraphQLInputObjectType + ) { + const oldTypeFields: ?GraphQLFieldMap<*, *> = oldType + ? oldType.getFields() + : null; + const newTypeFields: GraphQLFieldMap<*, *> = newType.getFields(); + + Object.keys(newTypeFields).forEach(fieldName => { + const oldField = oldTypeFields ? oldTypeFields[fieldName] : null; + const newField = newTypeFields[fieldName]; + + if (newField.description) { + if (!oldField) { + descriptionChanges.push( + `Description added on new field ${newType.name}.${newField.name}`, + ); + } else if (!oldField.description) { + descriptionChanges.push( + `Description added on field ${newType.name}.${newField.name}.`, + ); + } else if (oldField.description !== newField.description) { + descriptionChanges.push( + `Description changed on field ${newType.name}.${newField.name}.`, + ); + } + } + + if (!newField.args) { + return; + } + + newField.args.forEach(newArg => { + const oldArg = oldField + ? oldField.args.find(arg => arg.name === newArg.name) + : null; + + if (newArg.description) { + if (!oldArg) { + descriptionChanges.push( + `Description added on new arg ${newType.name}.${ + newField.name + }.${newArg.name}.`, + ); + } else if (!oldArg.description) { + descriptionChanges.push( + `Description added on arg ${newType.name}.${newField.name}.${ + newArg.name + }.`, + ); + } else if (oldArg.description !== newArg.description) { + descriptionChanges.push( + `Description changed on arg ${newType.name}.${newField.name}.${ + newArg.name + }.`, + ); + } + } + }); + }); + } else if (newType instanceof GraphQLEnumType) { + const oldValues = oldType ? oldType.getValues() : null; + const newValues = newType.getValues(); + newValues.forEach(newValue => { + const oldValue = oldValues + ? oldValues.find(value => value.name === newValue.name) + : null; + + if (newValue.description) { + if (!oldValue) { + descriptionChanges.push( + `Description added on enum value ${newType.name}.${ + newValue.name + }.`, + ); + } else if (!oldValue.description) { + descriptionChanges.push( + `Description added on enum value ${newType.name}.${ + newValue.name + }.`, + ); + } else if (oldValue.description !== newValue.description) { + descriptionChanges.push( + `Description changed on enum value ${newType.name}.${ + newValue.name + }.`, + ); + } + } + }); + } + }); + + return descriptionChanges; +} From 3948be96e53d7e4171654e8c084496371af41075 Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Tue, 2 Jan 2018 11:10:33 -0500 Subject: [PATCH 2/3] Refactor for better typing Strongly type the return objects, use invariants in a way to satisfy flow, and DRY up one common function. --- .../__tests__/findDescriptionChanges-test.js | 12 +- src/utilities/findDescriptionChanges.js | 171 ++++++++++-------- 2 files changed, 102 insertions(+), 81 deletions(-) diff --git a/src/utilities/__tests__/findDescriptionChanges-test.js b/src/utilities/__tests__/findDescriptionChanges-test.js index 456f7768c7..b075bf7afc 100644 --- a/src/utilities/__tests__/findDescriptionChanges-test.js +++ b/src/utilities/__tests__/findDescriptionChanges-test.js @@ -43,9 +43,9 @@ describe('findDescriptionChanges', () => { query: queryType, types: [typeNew], }); - expect(findDescriptionChanges(oldSchema, newSchema)).to.eql([ - 'Description added on type Type.', - ]); + expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql( + 'Description added on TYPE Type.', + ); expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]); expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]); }); @@ -67,9 +67,9 @@ describe('findDescriptionChanges', () => { query: queryType, types: [type], }); - expect(findDescriptionChanges(oldSchema, newSchema)).to.eql([ - 'Description added on new type Type.', - ]); + expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql( + 'New TYPE Type added with description.', + ); expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]); expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]); }); diff --git a/src/utilities/findDescriptionChanges.js b/src/utilities/findDescriptionChanges.js index db1442c84c..64eb4f9893 100644 --- a/src/utilities/findDescriptionChanges.js +++ b/src/utilities/findDescriptionChanges.js @@ -9,6 +9,28 @@ import { } from '../type/definition'; import type { GraphQLFieldMap } from '../type/definition'; import { GraphQLSchema } from '../type/schema'; +import invariant from '../jsutils/invariant'; + +export const DescribedObjectType = { + FIELD: 'FIELD', + TYPE: 'TYPE', + ARGUMENT: 'ARGUMENT', + ENUM_VALUE: 'ENUM_VALUE', +}; + +export const DescriptionChangeType = { + OBJECT_ADDED: 'OBJECT_ADDED', + DESCRIPTION_ADDED: 'DESCRIPTION_ADDED', + DESCRIPTION_CHANGED: 'DESCRIPTION_CHANGED', +}; + +export type DescriptionChange = { + object: $Keys, + change: $Keys, + description: string, + oldThing: any, + newThing: any, +}; /** * Given two schemas, returns an Array containing descriptions of any @@ -17,37 +39,32 @@ import { GraphQLSchema } from '../type/schema'; export function findDescriptionChanges( oldSchema: GraphQLSchema, newSchema: GraphQLSchema, -): Array { +): Array { const oldTypeMap = oldSchema.getTypeMap(); const newTypeMap = newSchema.getTypeMap(); - const descriptionChanges: Array = []; + const descriptionChanges: Array = []; Object.keys(newTypeMap).forEach(typeName => { const oldType = oldTypeMap[typeName]; const newType = newTypeMap[typeName]; - if (newType.description) { - if (!oldType) { - descriptionChanges.push( - `Description added on new type ${newType.name}.`, - ); - } else if (!oldType.description) { - descriptionChanges.push(`Description added on type ${newType.name}.`); - } else if (oldType.description !== newType.description) { - descriptionChanges.push(`Description changed on type ${newType.name}.`); - } - } - - if (oldType && !(newType instanceof oldType.constructor)) { - return; - } + descriptionChanges.push( + generateDescriptionChange(newType, oldType, DescribedObjectType.TYPE), + ); if ( newType instanceof GraphQLObjectType || newType instanceof GraphQLInterfaceType || newType instanceof GraphQLInputObjectType ) { + invariant( + !oldType || + oldType instanceof GraphQLObjectType || + oldType instanceof GraphQLInterfaceType || + oldType instanceof GraphQLInputObjectType, + 'Expected oldType to also have fields', + ); const oldTypeFields: ?GraphQLFieldMap<*, *> = oldType ? oldType.getFields() : null; @@ -57,21 +74,13 @@ export function findDescriptionChanges( const oldField = oldTypeFields ? oldTypeFields[fieldName] : null; const newField = newTypeFields[fieldName]; - if (newField.description) { - if (!oldField) { - descriptionChanges.push( - `Description added on new field ${newType.name}.${newField.name}`, - ); - } else if (!oldField.description) { - descriptionChanges.push( - `Description added on field ${newType.name}.${newField.name}.`, - ); - } else if (oldField.description !== newField.description) { - descriptionChanges.push( - `Description changed on field ${newType.name}.${newField.name}.`, - ); - } - } + descriptionChanges.push( + generateDescriptionChange( + newField, + oldField, + DescribedObjectType.FIELD, + ), + ); if (!newField.args) { return; @@ -82,30 +91,20 @@ export function findDescriptionChanges( ? oldField.args.find(arg => arg.name === newArg.name) : null; - if (newArg.description) { - if (!oldArg) { - descriptionChanges.push( - `Description added on new arg ${newType.name}.${ - newField.name - }.${newArg.name}.`, - ); - } else if (!oldArg.description) { - descriptionChanges.push( - `Description added on arg ${newType.name}.${newField.name}.${ - newArg.name - }.`, - ); - } else if (oldArg.description !== newArg.description) { - descriptionChanges.push( - `Description changed on arg ${newType.name}.${newField.name}.${ - newArg.name - }.`, - ); - } - } + descriptionChanges.push( + generateDescriptionChange( + newArg, + oldArg, + DescribedObjectType.ARGUMENT, + ), + ); }); }); } else if (newType instanceof GraphQLEnumType) { + invariant( + !oldType || oldType instanceof GraphQLEnumType, + 'Expected oldType to also have values', + ); const oldValues = oldType ? oldType.getValues() : null; const newValues = newType.getValues(); newValues.forEach(newValue => { @@ -113,30 +112,52 @@ export function findDescriptionChanges( ? oldValues.find(value => value.name === newValue.name) : null; - if (newValue.description) { - if (!oldValue) { - descriptionChanges.push( - `Description added on enum value ${newType.name}.${ - newValue.name - }.`, - ); - } else if (!oldValue.description) { - descriptionChanges.push( - `Description added on enum value ${newType.name}.${ - newValue.name - }.`, - ); - } else if (oldValue.description !== newValue.description) { - descriptionChanges.push( - `Description changed on enum value ${newType.name}.${ - newValue.name - }.`, - ); - } - } + descriptionChanges.push( + generateDescriptionChange( + newValue, + oldValue, + DescribedObjectType.ENUM_VALUE, + ), + ); }); } }); - return descriptionChanges; + return descriptionChanges.filter(Boolean); +} + +function generateDescriptionChange( + newThing, + oldThing, + objectType: $Keys, +): ?DescriptionChange { + if (!newThing.description) { + return; + } + + if (!oldThing) { + return { + object: objectType, + change: DescriptionChangeType.OBJECT_ADDED, + oldThing, + newThing, + description: `New ${objectType} ${newThing.name} added with description.`, + }; + } else if (!oldThing.description) { + return { + object: objectType, + change: DescriptionChangeType.DESCRIPTION_ADDED, + oldThing, + newThing, + description: `Description added on ${objectType} ${newThing.name}.`, + }; + } else if (oldThing.description !== newThing.description) { + return { + object: objectType, + change: DescriptionChangeType.DESCRIPTION_CHANGED, + oldThing, + newThing, + description: `Description changed on ${objectType} ${newThing.name}.`, + }; + } } From 9dd09cd4fe3a749f697d53fe6661bda1acadbcad Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Mon, 2 Apr 2018 12:18:08 -0400 Subject: [PATCH 3/3] Add more tests --- .../__tests__/findDescriptionChanges-test.js | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/utilities/__tests__/findDescriptionChanges-test.js b/src/utilities/__tests__/findDescriptionChanges-test.js index b075bf7afc..d3aec17ab6 100644 --- a/src/utilities/__tests__/findDescriptionChanges-test.js +++ b/src/utilities/__tests__/findDescriptionChanges-test.js @@ -50,6 +50,37 @@ describe('findDescriptionChanges', () => { expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]); }); + it('should detect if a description was changed on a type', () => { + const typeOld = new GraphQLObjectType({ + name: 'Type', + description: 'Something rather', + fields: { + field1: { type: GraphQLString }, + }, + }); + const typeNew = new GraphQLObjectType({ + name: 'Type', + description: 'Something else', + fields: { + field1: { type: GraphQLString }, + }, + }); + + const oldSchema = new GraphQLSchema({ + query: queryType, + types: [typeOld], + }); + const newSchema = new GraphQLSchema({ + query: queryType, + types: [typeNew], + }); + expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql( + 'Description changed on TYPE Type.', + ); + expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]); + expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]); + }); + it('should detect if a type with a description was added', () => { const type = new GraphQLObjectType({ name: 'Type', @@ -73,4 +104,37 @@ describe('findDescriptionChanges', () => { expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]); expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]); }); + + it('should detect if a field with a description was added', () => { + const typeOld = new GraphQLObjectType({ + name: 'Type', + fields: { + field1: { type: GraphQLString }, + }, + }); + const typeNew = new GraphQLObjectType({ + name: 'Type', + fields: { + field1: { type: GraphQLString }, + field2: { + type: GraphQLString, + description: 'Something rather', + }, + }, + }); + + const oldSchema = new GraphQLSchema({ + query: queryType, + types: [typeOld], + }); + const newSchema = new GraphQLSchema({ + query: queryType, + types: [typeNew], + }); + expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql( + 'New FIELD field2 added with description.', + ); + expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]); + expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]); + }); });