Skip to content

Commit 51e53d7

Browse files
JoviDeCroockJoviDeCroock
authored
Add safe schema change for descriptions changing (#4275)
This adds a new safe schema-change when a description changes of any schema-member. Do note that there are some drive-by fixes in here as I noticed a bug in safe-argument changes where we'd just always report the changes rather than actually see whether something changed 😅 Resolves #4245
1 parent 670c26c commit 51e53d7

File tree

2 files changed

+238
-3
lines changed

2 files changed

+238
-3
lines changed

‎src/utilities/__tests__/findSchemaChanges-test.ts‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,23 @@ describe('findSchemaChanges', () => {
2222
]);
2323
});
2424

25+
it('should detect a type changing description', () => {
26+
const newSchema = buildSchema(`
27+
"New Description"
28+
type Type1
29+
`);
30+
31+
const oldSchema = buildSchema(`
32+
type Type1
33+
`);
34+
expect(findSchemaChanges(oldSchema, newSchema)).to.deep.equal([
35+
{
36+
type: SafeChangeType.DESCRIPTION_CHANGED,
37+
description: 'Description of Type1 has changed to "New Description".',
38+
},
39+
]);
40+
});
41+
2542
it('should detect if a field was added', () => {
2643
const oldSchema = buildSchema(`
2744
type Query {
@@ -43,6 +60,30 @@ describe('findSchemaChanges', () => {
4360
]);
4461
});
4562

63+
it('should detect a field changing description', () => {
64+
const oldSchema = buildSchema(`
65+
type Query {
66+
foo: String
67+
bar: String
68+
}
69+
`);
70+
71+
const newSchema = buildSchema(`
72+
type Query {
73+
foo: String
74+
"New Description"
75+
bar: String
76+
}
77+
`);
78+
expect(findSchemaChanges(oldSchema, newSchema)).to.deep.equal([
79+
{
80+
type: SafeChangeType.DESCRIPTION_CHANGED,
81+
description:
82+
'Description of field Query.bar has changed to "New Description".',
83+
},
84+
]);
85+
});
86+
4687
it('should detect if a default value was added', () => {
4788
const oldSchema = buildSchema(`
4889
type Query {
@@ -84,6 +125,30 @@ describe('findSchemaChanges', () => {
84125
]);
85126
});
86127

128+
it('should detect if an arg value changes description', () => {
129+
const oldSchema = buildSchema(`
130+
type Query {
131+
foo(x: String!): String
132+
}
133+
`);
134+
135+
const newSchema = buildSchema(`
136+
type Query {
137+
foo(
138+
"New Description"
139+
x: String!
140+
): String
141+
}
142+
`);
143+
expect(findSchemaChanges(oldSchema, newSchema)).to.deep.equal([
144+
{
145+
type: SafeChangeType.DESCRIPTION_CHANGED,
146+
description:
147+
'Description of argument Query.foo(x) has changed to "New Description".',
148+
},
149+
]);
150+
});
151+
87152
it('should detect if a directive was added', () => {
88153
const oldSchema = buildSchema(`
89154
type Query {
@@ -106,6 +171,31 @@ describe('findSchemaChanges', () => {
106171
]);
107172
});
108173

174+
it('should detect if a directive changes description', () => {
175+
const oldSchema = buildSchema(`
176+
directive @Foo on FIELD_DEFINITION
177+
178+
type Query {
179+
foo: String
180+
}
181+
`);
182+
183+
const newSchema = buildSchema(`
184+
"New Description"
185+
directive @Foo on FIELD_DEFINITION
186+
187+
type Query {
188+
foo: String
189+
}
190+
`);
191+
expect(findSchemaChanges(oldSchema, newSchema)).to.deep.equal([
192+
{
193+
description: 'Description of @Foo has changed to "New Description".',
194+
type: SafeChangeType.DESCRIPTION_CHANGED,
195+
},
196+
]);
197+
});
198+
109199
it('should detect if a directive becomes repeatable', () => {
110200
const oldSchema = buildSchema(`
111201
directive @Foo on FIELD_DEFINITION
@@ -177,4 +267,94 @@ describe('findSchemaChanges', () => {
177267
},
178268
]);
179269
});
270+
271+
it('should detect if a directive arg changes description', () => {
272+
const oldSchema = buildSchema(`
273+
directive @Foo(
274+
arg1: String
275+
) on FIELD_DEFINITION
276+
277+
type Query {
278+
foo: String
279+
}
280+
`);
281+
282+
const newSchema = buildSchema(`
283+
directive @Foo(
284+
"New Description"
285+
arg1: String
286+
) on FIELD_DEFINITION
287+
288+
type Query {
289+
foo: String
290+
}
291+
`);
292+
expect(findSchemaChanges(oldSchema, newSchema)).to.deep.equal([
293+
{
294+
description:
295+
'Description of @Foo(Foo) has changed to "New Description".',
296+
type: SafeChangeType.DESCRIPTION_CHANGED,
297+
},
298+
]);
299+
});
300+
301+
it('should detect if an enum member changes description', () => {
302+
const oldSchema = buildSchema(`
303+
enum Foo {
304+
TEST
305+
}
306+
307+
type Query {
308+
foo: String
309+
}
310+
`);
311+
312+
const newSchema = buildSchema(`
313+
enum Foo {
314+
"New Description"
315+
TEST
316+
}
317+
318+
type Query {
319+
foo: String
320+
}
321+
`);
322+
expect(findSchemaChanges(oldSchema, newSchema)).to.deep.equal([
323+
{
324+
description:
325+
'Description of enum value Foo.TEST has changed to "New Description".',
326+
type: SafeChangeType.DESCRIPTION_CHANGED,
327+
},
328+
]);
329+
});
330+
331+
it('should detect if an input field changes description', () => {
332+
const oldSchema = buildSchema(`
333+
input Foo {
334+
x: String
335+
}
336+
337+
type Query {
338+
foo: String
339+
}
340+
`);
341+
342+
const newSchema = buildSchema(`
343+
input Foo {
344+
"New Description"
345+
x: String
346+
}
347+
348+
type Query {
349+
foo: String
350+
}
351+
`);
352+
expect(findSchemaChanges(oldSchema, newSchema)).to.deep.equal([
353+
{
354+
description:
355+
'Description of input-field Foo.x has changed to "New Description".',
356+
type: SafeChangeType.DESCRIPTION_CHANGED,
357+
},
358+
]);
359+
});
180360
});

‎src/utilities/findSchemaChanges.ts‎

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ export type DangerousChangeType =
7070
(typeof DangerousChangeType)[keyof typeof DangerousChangeType];
7171

7272
export const SafeChangeType = {
73+
DESCRIPTION_CHANGED: 'DESCRIPTION_CHANGED' as const,
7374
TYPE_ADDED: 'TYPE_ADDED' as const,
7475
OPTIONAL_INPUT_FIELD_ADDED: 'OPTIONAL_INPUT_FIELD_ADDED' as const,
7576
OPTIONAL_ARG_ADDED: 'OPTIONAL_ARG_ADDED' as const,
@@ -194,6 +195,15 @@ function findDirectiveChanges(
194195
});
195196
}
196197

198+
for (const [oldArg, newArg] of argsDiff.persisted) {
199+
if (oldArg.description !== newArg.description) {
200+
schemaChanges.push({
201+
type: SafeChangeType.DESCRIPTION_CHANGED,
202+
description: `Description of @${oldDirective.name}(${oldDirective.name}) has changed to "${newArg.description}".`,
203+
});
204+
}
205+
}
206+
197207
if (oldDirective.isRepeatable && !newDirective.isRepeatable) {
198208
schemaChanges.push({
199209
type: BreakingChangeType.DIRECTIVE_REPEATABLE_REMOVED,
@@ -206,6 +216,13 @@ function findDirectiveChanges(
206216
});
207217
}
208218

219+
if (oldDirective.description !== newDirective.description) {
220+
schemaChanges.push({
221+
type: SafeChangeType.DESCRIPTION_CHANGED,
222+
description: `Description of @${oldDirective.name} has changed to "${newDirective.description}".`,
223+
});
224+
}
225+
209226
for (const location of oldDirective.locations) {
210227
if (!newDirective.locations.includes(location)) {
211228
schemaChanges.push({
@@ -256,6 +273,13 @@ function findTypeChanges(
256273
}
257274

258275
for (const [oldType, newType] of typesDiff.persisted) {
276+
if (oldType.description !== newType.description) {
277+
schemaChanges.push({
278+
type: SafeChangeType.DESCRIPTION_CHANGED,
279+
description: `Description of ${oldType.name} has changed to "${newType.description}".`,
280+
});
281+
}
282+
259283
if (isEnumType(oldType) && isEnumType(newType)) {
260284
schemaChanges.push(...findEnumTypeChanges(oldType, newType));
261285
} else if (isUnionType(oldType) && isUnionType(newType)) {
@@ -328,14 +352,21 @@ function findInputObjectTypeChanges(
328352
`Field ${oldType}.${oldField.name} changed type from ` +
329353
`${String(oldField.type)} to ${String(newField.type)}.`,
330354
});
331-
} else {
355+
} else if (oldField.type.toString() !== newField.type.toString()) {
332356
schemaChanges.push({
333357
type: SafeChangeType.FIELD_CHANGED_KIND_SAFE,
334358
description:
335359
`Field ${oldType}.${oldField.name} changed type from ` +
336360
`${String(oldField.type)} to ${String(newField.type)}.`,
337361
});
338362
}
363+
364+
if (oldField.description !== newField.description) {
365+
schemaChanges.push({
366+
type: SafeChangeType.DESCRIPTION_CHANGED,
367+
description: `Description of input-field ${newType}.${newField.name} has changed to "${newField.description}".`,
368+
});
369+
}
339370
}
340371

341372
return schemaChanges;
@@ -368,7 +399,7 @@ function findUnionTypeChanges(
368399
function findEnumTypeChanges(
369400
oldType: GraphQLEnumType,
370401
newType: GraphQLEnumType,
371-
): Array<BreakingChange | DangerousChange> {
402+
): Array<SchemaChange> {
372403
const schemaChanges = [];
373404
const valuesDiff = diff(oldType.getValues(), newType.getValues());
374405

@@ -386,6 +417,15 @@ function findEnumTypeChanges(
386417
});
387418
}
388419

420+
for (const [oldValue, newValue] of valuesDiff.persisted) {
421+
if (oldValue.description !== newValue.description) {
422+
schemaChanges.push({
423+
type: SafeChangeType.DESCRIPTION_CHANGED,
424+
description: `Description of enum value ${oldType}.${oldValue.name} has changed to "${newValue.description}".`,
425+
});
426+
}
427+
}
428+
389429
return schemaChanges;
390430
}
391431

@@ -459,6 +499,13 @@ function findFieldChanges(
459499
`${String(oldField.type)} to ${String(newField.type)}.`,
460500
});
461501
}
502+
503+
if (oldField.description !== newField.description) {
504+
schemaChanges.push({
505+
type: SafeChangeType.DESCRIPTION_CHANGED,
506+
description: `Description of field ${oldType}.${oldField.name} has changed to "${newField.description}".`,
507+
});
508+
}
462509
}
463510

464511
return schemaChanges;
@@ -484,6 +531,7 @@ function findArgChanges(
484531
oldArg.type,
485532
newArg.type,
486533
);
534+
487535
if (!isSafe) {
488536
schemaChanges.push({
489537
type: BreakingChangeType.ARG_CHANGED_KIND,
@@ -520,14 +568,21 @@ function findArgChanges(
520568
type: SafeChangeType.ARG_DEFAULT_VALUE_ADDED,
521569
description: `${oldType}.${oldField.name}(${oldArg.name}:) added a defaultValue ${newValueStr}.`,
522570
});
523-
} else {
571+
} else if (oldArg.type.toString() !== newArg.type.toString()) {
524572
schemaChanges.push({
525573
type: SafeChangeType.ARG_CHANGED_KIND_SAFE,
526574
description:
527575
`Argument ${oldType}.${oldField.name}(${oldArg.name}:) has changed type from ` +
528576
`${String(oldArg.type)} to ${String(newArg.type)}.`,
529577
});
530578
}
579+
580+
if (oldArg.description !== newArg.description) {
581+
schemaChanges.push({
582+
type: SafeChangeType.DESCRIPTION_CHANGED,
583+
description: `Description of argument ${oldType}.${oldField.name}(${oldArg.name}) has changed to "${newArg.description}".`,
584+
});
585+
}
531586
}
532587

533588
for (const newArg of argsDiff.added) {

0 commit comments

Comments
 (0)