Skip to content

Commit db4cfdc

Browse files
leebyronleebyron
committed
Merge branch 'nullable_change_warning' of https://github.com/aolesky/graphql-js into aolesky-nullable_change_warning
2 parents a3afb98 + c167f26 commit db4cfdc

File tree

2 files changed

+173
-47
lines changed

2 files changed

+173
-47
lines changed

‎src/utilities/__tests__/findBreakingChanges-test.js

+120-4
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ import {
2525
DangerousChangeType,
2626
findBreakingChanges,
2727
findDangerousChanges,
28-
findFieldsThatChangedType,
28+
findFieldsThatChangedTypeOnObjectOrInterfaceTypes,
29+
findFieldsThatChangedTypeOnInputObjectTypes,
2930
findRemovedTypes,
3031
findTypesRemovedFromUnions,
3132
findTypesAddedToUnions,
@@ -260,7 +261,10 @@ describe('findBreakingChanges', () => {
260261
description: 'Type1.field18 changed type from [[Int!]!] to [[Int!]].',
261262
},
262263
];
263-
expect(findFieldsThatChangedType(oldSchema, newSchema)).to.eql(
264+
expect(findFieldsThatChangedTypeOnObjectOrInterfaceTypes(
265+
oldSchema,
266+
newSchema
267+
)).to.eql(
264268
expectedFieldChanges
265269
);
266270
});
@@ -437,7 +441,10 @@ describe('findBreakingChanges', () => {
437441
'[[Int!]!].',
438442
},
439443
];
440-
expect(findFieldsThatChangedType(oldSchema, newSchema)).to.eql(
444+
expect(findFieldsThatChangedTypeOnInputObjectTypes(
445+
oldSchema,
446+
newSchema
447+
).breakingChanges).to.eql(
441448
expectedFieldChanges,
442449
);
443450
}
@@ -487,7 +494,10 @@ describe('findBreakingChanges', () => {
487494
'InputType1 was added.',
488495
},
489496
];
490-
expect(findFieldsThatChangedType(oldSchema, newSchema)).to.eql(
497+
expect(findFieldsThatChangedTypeOnInputObjectTypes(
498+
oldSchema,
499+
newSchema
500+
).breakingChanges).to.eql(
491501
expectedFieldChanges,
492502
);
493503
});
@@ -1526,6 +1536,56 @@ describe('findDangerousChanges', () => {
15261536
]);
15271537
});
15281538

1539+
it('should detect if a nullable field was added to an input', () => {
1540+
const oldInputType = new GraphQLInputObjectType({
1541+
name: 'InputType1',
1542+
fields: {
1543+
field1: {
1544+
type: GraphQLString,
1545+
},
1546+
},
1547+
});
1548+
1549+
const newInputType = new GraphQLInputObjectType({
1550+
name: 'InputType1',
1551+
fields: {
1552+
field1: {
1553+
type: GraphQLString,
1554+
},
1555+
field2: {
1556+
type: GraphQLInt,
1557+
},
1558+
},
1559+
});
1560+
1561+
const oldSchema = new GraphQLSchema({
1562+
query: queryType,
1563+
types: [
1564+
oldInputType,
1565+
]
1566+
});
1567+
1568+
const newSchema = new GraphQLSchema({
1569+
query: queryType,
1570+
types: [
1571+
newInputType,
1572+
]
1573+
});
1574+
1575+
const expectedFieldChanges = [
1576+
{
1577+
type: DangerousChangeType.NULLABLE_INPUT_FIELD_ADDED,
1578+
description: 'A nullable field field2 on input type ' +
1579+
'InputType1 was added.',
1580+
},
1581+
];
1582+
1583+
expect(findFieldsThatChangedTypeOnInputObjectTypes(
1584+
oldSchema,
1585+
newSchema
1586+
).dangerousChanges).to.eql(expectedFieldChanges);
1587+
});
1588+
15291589
it('should find all dangerous changes', () => {
15301590
const enumThatGainsAValueOld = new GraphQLEnumType({
15311591
name: 'EnumType1',
@@ -1668,4 +1728,60 @@ describe('findDangerousChanges', () => {
16681728
expectedDangerousChanges
16691729
);
16701730
});
1731+
1732+
it('should detect if a nullable field argument was added', () => {
1733+
const oldType = new GraphQLObjectType({
1734+
name: 'Type1',
1735+
fields: {
1736+
field1: {
1737+
type: GraphQLString,
1738+
args: {
1739+
arg1: {
1740+
type: GraphQLString,
1741+
},
1742+
},
1743+
},
1744+
},
1745+
});
1746+
1747+
const newType = new GraphQLObjectType({
1748+
name: 'Type1',
1749+
fields: {
1750+
field1: {
1751+
type: GraphQLString,
1752+
args: {
1753+
arg1: {
1754+
type: GraphQLString,
1755+
},
1756+
arg2: {
1757+
type: GraphQLString,
1758+
},
1759+
},
1760+
},
1761+
},
1762+
});
1763+
1764+
const oldSchema = new GraphQLSchema({
1765+
query: queryType,
1766+
types: [
1767+
oldType,
1768+
]
1769+
});
1770+
1771+
const newSchema = new GraphQLSchema({
1772+
query: queryType,
1773+
types: [
1774+
newType,
1775+
]
1776+
});
1777+
1778+
expect(
1779+
findArgChanges(oldSchema, newSchema).dangerousChanges
1780+
).to.eql([
1781+
{
1782+
type: DangerousChangeType.NULLABLE_ARG_ADDED,
1783+
description: 'A nullable arg arg2 on Type1.field1 was added',
1784+
},
1785+
]);
1786+
});
16711787
});

‎src/utilities/findBreakingChanges.js

+53-43
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ export const DangerousChangeType = {
4646
VALUE_ADDED_TO_ENUM: 'VALUE_ADDED_TO_ENUM',
4747
INTERFACE_ADDED_TO_OBJECT: 'INTERFACE_ADDED_TO_OBJECT',
4848
TYPE_ADDED_TO_UNION: 'TYPE_ADDED_TO_UNION',
49+
NULLABLE_INPUT_FIELD_ADDED: 'NULLABLE_INPUT_FIELD_ADDED',
50+
NULLABLE_ARG_ADDED: 'NULLABLE_ARG_ADDED',
4951
};
5052

5153
export type BreakingChange = {
@@ -69,7 +71,9 @@ export function findBreakingChanges(
6971
return [
7072
...findRemovedTypes(oldSchema, newSchema),
7173
...findTypesThatChangedKind(oldSchema, newSchema),
72-
...findFieldsThatChangedType(oldSchema, newSchema),
74+
...findFieldsThatChangedTypeOnObjectOrInterfaceTypes(oldSchema, newSchema),
75+
...findFieldsThatChangedTypeOnInputObjectTypes(oldSchema, newSchema)
76+
.breakingChanges,
7377
...findTypesRemovedFromUnions(oldSchema, newSchema),
7478
...findValuesRemovedFromEnums(oldSchema, newSchema),
7579
...findArgChanges(oldSchema, newSchema).breakingChanges,
@@ -90,6 +94,8 @@ export function findDangerousChanges(
9094
...findValuesAddedToEnums(oldSchema, newSchema),
9195
...findInterfacesAddedToObjectTypes(oldSchema, newSchema),
9296
...findTypesAddedToUnions(oldSchema, newSchema),
97+
...findFieldsThatChangedTypeOnInputObjectTypes(oldSchema, newSchema)
98+
.dangerousChanges
9399
];
94100
}
95101

@@ -224,12 +230,20 @@ export function findArgChanges(
224230
const oldArgDef = oldArgs.find(
225231
arg => arg.name === newArgDef.name
226232
);
227-
if (!oldArgDef && newArgDef.type instanceof GraphQLNonNull) {
228-
breakingChanges.push({
229-
type: BreakingChangeType.NON_NULL_ARG_ADDED,
230-
description: `A non-null arg ${newArgDef.name} on ` +
231-
`${newType.name}.${fieldName} was added`,
232-
});
233+
if (!oldArgDef) {
234+
if (newArgDef.type instanceof GraphQLNonNull) {
235+
breakingChanges.push({
236+
type: BreakingChangeType.NON_NULL_ARG_ADDED,
237+
description: `A non-null arg ${newArgDef.name} on ` +
238+
`${newType.name}.${fieldName} was added`,
239+
});
240+
} else {
241+
dangerousChanges.push({
242+
type: DangerousChangeType.NULLABLE_ARG_ADDED,
243+
description: `A nullable arg ${newArgDef.name} on ` +
244+
`${newType.name}.${fieldName} was added`,
245+
});
246+
}
233247
}
234248
});
235249
});
@@ -263,30 +277,14 @@ function typeKindName(type: GraphQLNamedType): string {
263277
throw new TypeError('Unknown type ' + type.constructor.name);
264278
}
265279

266-
/**
267-
* Given two schemas, returns an Array containing descriptions of any breaking
268-
* changes in the newSchema related to the fields on a type. This includes if
269-
* a field has been removed from a type, if a field has changed type, or if
270-
* a non-null field is added to an input type.
271-
*/
272-
export function findFieldsThatChangedType(
273-
oldSchema: GraphQLSchema,
274-
newSchema: GraphQLSchema
275-
): Array<BreakingChange> {
276-
return [
277-
...findFieldsThatChangedTypeOnObjectOrInterfaceTypes(oldSchema, newSchema),
278-
...findFieldsThatChangedTypeOnInputObjectTypes(oldSchema, newSchema),
279-
];
280-
}
281-
282-
function findFieldsThatChangedTypeOnObjectOrInterfaceTypes(
280+
export function findFieldsThatChangedTypeOnObjectOrInterfaceTypes(
283281
oldSchema: GraphQLSchema,
284282
newSchema: GraphQLSchema,
285283
): Array<BreakingChange> {
286284
const oldTypeMap = oldSchema.getTypeMap();
287285
const newTypeMap = newSchema.getTypeMap();
288286

289-
const breakingFieldChanges = [];
287+
const breakingChanges = [];
290288
Object.keys(oldTypeMap).forEach(typeName => {
291289
const oldType = oldTypeMap[typeName];
292290
const newType = newTypeMap[typeName];
@@ -303,7 +301,7 @@ function findFieldsThatChangedTypeOnObjectOrInterfaceTypes(
303301
Object.keys(oldTypeFieldsDef).forEach(fieldName => {
304302
// Check if the field is missing on the type in the new schema.
305303
if (!(fieldName in newTypeFieldsDef)) {
306-
breakingFieldChanges.push({
304+
breakingChanges.push({
307305
type: BreakingChangeType.FIELD_REMOVED,
308306
description: `${typeName}.${fieldName} was removed.`,
309307
});
@@ -319,7 +317,7 @@ function findFieldsThatChangedTypeOnObjectOrInterfaceTypes(
319317
const newFieldTypeString = isNamedType(newFieldType) ?
320318
newFieldType.name :
321319
newFieldType.toString();
322-
breakingFieldChanges.push({
320+
breakingChanges.push({
323321
type: BreakingChangeType.FIELD_CHANGED_KIND,
324322
description: `${typeName}.${fieldName} changed type from ` +
325323
`${oldFieldTypeString} to ${newFieldTypeString}.`,
@@ -328,17 +326,21 @@ function findFieldsThatChangedTypeOnObjectOrInterfaceTypes(
328326
}
329327
});
330328
});
331-
return breakingFieldChanges;
329+
return breakingChanges;
332330
}
333331

334332
export function findFieldsThatChangedTypeOnInputObjectTypes(
335333
oldSchema: GraphQLSchema,
336334
newSchema: GraphQLSchema
337-
): Array<BreakingChange> {
335+
): {
336+
breakingChanges: Array<BreakingChange>,
337+
dangerousChanges: Array<DangerousChange>
338+
} {
338339
const oldTypeMap = oldSchema.getTypeMap();
339340
const newTypeMap = newSchema.getTypeMap();
340341

341-
const breakingFieldChanges = [];
342+
const breakingChanges = [];
343+
const dangerousChanges = [];
342344
Object.keys(oldTypeMap).forEach(typeName => {
343345
const oldType = oldTypeMap[typeName];
344346
const newType = newTypeMap[typeName];
@@ -354,7 +356,7 @@ export function findFieldsThatChangedTypeOnInputObjectTypes(
354356
Object.keys(oldTypeFieldsDef).forEach(fieldName => {
355357
// Check if the field is missing on the type in the new schema.
356358
if (!(fieldName in newTypeFieldsDef)) {
357-
breakingFieldChanges.push({
359+
breakingChanges.push({
358360
type: BreakingChangeType.FIELD_REMOVED,
359361
description: `${typeName}.${fieldName} was removed.`,
360362
});
@@ -371,29 +373,37 @@ export function findFieldsThatChangedTypeOnInputObjectTypes(
371373
const newFieldTypeString = isNamedType(newFieldType) ?
372374
newFieldType.name :
373375
newFieldType.toString();
374-
breakingFieldChanges.push({
376+
breakingChanges.push({
375377
type: BreakingChangeType.FIELD_CHANGED_KIND,
376378
description: `${typeName}.${fieldName} changed type from ` +
377379
`${oldFieldTypeString} to ${newFieldTypeString}.`,
378380
});
379381
}
380382
}
381383
});
382-
// Check if a non-null field was added to the input object type
384+
// Check if a field was added to the input object type
383385
Object.keys(newTypeFieldsDef).forEach(fieldName => {
384-
if (
385-
!(fieldName in oldTypeFieldsDef) &&
386-
newTypeFieldsDef[fieldName].type instanceof GraphQLNonNull
387-
) {
388-
breakingFieldChanges.push({
389-
type: BreakingChangeType.NON_NULL_INPUT_FIELD_ADDED,
390-
description: `A non-null field ${fieldName} on ` +
391-
`input type ${newType.name} was added.`,
392-
});
386+
if (!(fieldName in oldTypeFieldsDef)) {
387+
if (newTypeFieldsDef[fieldName].type instanceof GraphQLNonNull) {
388+
breakingChanges.push({
389+
type: BreakingChangeType.NON_NULL_INPUT_FIELD_ADDED,
390+
description: `A non-null field ${fieldName} on ` +
391+
`input type ${newType.name} was added.`,
392+
});
393+
} else {
394+
dangerousChanges.push({
395+
type: DangerousChangeType.NULLABLE_INPUT_FIELD_ADDED,
396+
description: `A nullable field ${fieldName} on ` +
397+
`input type ${newType.name} was added.`,
398+
});
399+
}
393400
}
394401
});
395402
});
396-
return breakingFieldChanges;
403+
return {
404+
breakingChanges,
405+
dangerousChanges,
406+
};
397407
}
398408

399409
function isChangeSafeForObjectOrInterfaceField(

0 commit comments

Comments
 (0)