Skip to content

Commit

Permalink
Correctly detect required/optional args/fields
Browse files Browse the repository at this point in the history 
Disscussed here: graphql#1465 (comment)
  • Loading branch information
@IvanGoncharov
IvanGoncharov committed Aug 29, 2018
1 parent 74d1e94 commit da7ef46
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 50 deletions.
52 changes: 31 additions & 21 deletions src/utilities/__tests__/findBreakingChanges-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ describe('findBreakingChanges', () => {
).to.eql(expectedFieldChanges);
});

it('should detect if a non-null field is added to an input type', () => {
it('should detect if a required field is added to an input type', () => {
const oldSchema = buildSchema(`
input InputType1 {
field1: String
Expand All @@ -348,7 +348,8 @@ describe('findBreakingChanges', () => {
input InputType1 {
field1: String
requiredField: Int!
optionalField: Boolean
optionalField1: Boolean
optionalField2: Boolean! = false
}
type Query {
Expand All @@ -358,10 +359,9 @@ describe('findBreakingChanges', () => {

const expectedFieldChanges = [
{
type: BreakingChangeType.NON_NULL_INPUT_FIELD_ADDED,
type: BreakingChangeType.REQUIRED_INPUT_FIELD_ADDED,
description:
'A non-null field requiredField on input type ' +
'InputType1 was added.',
'A required field requiredField on input type InputType1 was added.',
},
];
expect(
Expand Down Expand Up @@ -609,7 +609,7 @@ describe('findBreakingChanges', () => {
]);
});

it('should detect if a non-null field argument was added', () => {
it('should detect if a required field argument was added', () => {
const oldSchema = buildSchema(`
type Type1 {
field1(arg1: String): String
Expand All @@ -622,7 +622,12 @@ describe('findBreakingChanges', () => {

const newSchema = buildSchema(`
type Type1 {
field1(arg1: String, newRequiredArg: String!, newOptionalArg: Int): String
field1(
arg1: String,
newRequiredArg: String!
newOptionalArg1: Int
newOptionalArg2: Int! = 0
): String
}
type Query {
Expand All @@ -632,8 +637,8 @@ describe('findBreakingChanges', () => {

expect(findArgChanges(oldSchema, newSchema).breakingChanges).to.eql([
{
type: BreakingChangeType.NON_NULL_ARG_ADDED,
description: 'A non-null arg newRequiredArg on Type1.field1 was added',
type: BreakingChangeType.REQUIRED_ARG_ADDED,
description: 'A required arg newRequiredArg on Type1.field1 was added',
},
]);
});
Expand Down Expand Up @@ -882,9 +887,9 @@ describe('findBreakingChanges', () => {
description: 'arg1 was removed from DirectiveThatRemovesArg',
},
{
type: BreakingChangeType.NON_NULL_DIRECTIVE_ARG_ADDED,
type: BreakingChangeType.REQUIRED_DIRECTIVE_ARG_ADDED,
description:
'A non-null arg arg1 on directive NonNullDirectiveAdded was added',
'A required arg arg1 on directive NonNullDirectiveAdded was added',
},
{
type: BreakingChangeType.DIRECTIVE_LOCATION_REMOVED,
Expand Down Expand Up @@ -946,19 +951,24 @@ describe('findBreakingChanges', () => {
]);
});

it('should detect if a non-nullable directive argument was added', () => {
it('should detect if an optional directive argument was added', () => {
const oldSchema = buildSchema(`
directive @DirectiveName on FIELD_DEFINITION
`);

const newSchema = buildSchema(`
directive @DirectiveName(arg1: Boolean!) on FIELD_DEFINITION
directive @DirectiveName(
newRequiredArg: String!
newOptionalArg1: Int
newOptionalArg2: Int! = 0
) on FIELD_DEFINITION
`);

expect(findAddedNonNullDirectiveArgs(oldSchema, newSchema)).to.eql([
{
type: BreakingChangeType.NON_NULL_DIRECTIVE_ARG_ADDED,
description: 'A non-null arg arg1 on directive DirectiveName was added',
type: BreakingChangeType.REQUIRED_DIRECTIVE_ARG_ADDED,
description:
'A required arg newRequiredArg on directive DirectiveName was added',
},
]);
});
Expand Down Expand Up @@ -1131,7 +1141,7 @@ describe('findDangerousChanges', () => {
]);
});

it('should detect if a nullable field was added to an input', () => {
it('should detect if an optional field was added to an input', () => {
const oldSchema = buildSchema(`
input InputType1 {
field1: String
Expand All @@ -1155,9 +1165,9 @@ describe('findDangerousChanges', () => {

const expectedFieldChanges = [
{
type: DangerousChangeType.NULLABLE_INPUT_FIELD_ADDED,
type: DangerousChangeType.OPTIONAL_INPUT_FIELD_ADDED,
description:
'A nullable field field2 on input type InputType1 was added.',
'An optional field field2 on input type InputType1 was added.',
},
];

Expand Down Expand Up @@ -1253,7 +1263,7 @@ describe('findDangerousChanges', () => {
);
});

it('should detect if a nullable field argument was added', () => {
it('should detect if an optional field argument was added', () => {
const oldSchema = buildSchema(`
type Type1 {
field1(arg1: String): String
Expand All @@ -1276,8 +1286,8 @@ describe('findDangerousChanges', () => {

expect(findArgChanges(oldSchema, newSchema).dangerousChanges).to.eql([
{
type: DangerousChangeType.NULLABLE_ARG_ADDED,
description: 'A nullable arg arg2 on Type1.field1 was added',
type: DangerousChangeType.OPTIONAL_ARG_ADDED,
description: 'An optional arg arg2 on Type1.field1 was added',
},
]);
});
Expand Down
59 changes: 30 additions & 29 deletions src/utilities/findBreakingChanges.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import {
isNonNullType,
isListType,
isNamedType,
isRequiredArgument,
isRequiredInputField,
} from '../type/definition';

import type {
Expand All @@ -42,22 +44,22 @@ export const BreakingChangeType = {
VALUE_REMOVED_FROM_ENUM: 'VALUE_REMOVED_FROM_ENUM',
ARG_REMOVED: 'ARG_REMOVED',
ARG_CHANGED_KIND: 'ARG_CHANGED_KIND',
NON_NULL_ARG_ADDED: 'NON_NULL_ARG_ADDED',
NON_NULL_INPUT_FIELD_ADDED: 'NON_NULL_INPUT_FIELD_ADDED',
REQUIRED_ARG_ADDED: 'REQUIRED_ARG_ADDED',
REQUIRED_INPUT_FIELD_ADDED: 'REQUIRED_INPUT_FIELD_ADDED',
INTERFACE_REMOVED_FROM_OBJECT: 'INTERFACE_REMOVED_FROM_OBJECT',
DIRECTIVE_REMOVED: 'DIRECTIVE_REMOVED',
DIRECTIVE_ARG_REMOVED: 'DIRECTIVE_ARG_REMOVED',
DIRECTIVE_LOCATION_REMOVED: 'DIRECTIVE_LOCATION_REMOVED',
NON_NULL_DIRECTIVE_ARG_ADDED: 'NON_NULL_DIRECTIVE_ARG_ADDED',
REQUIRED_DIRECTIVE_ARG_ADDED: 'REQUIRED_DIRECTIVE_ARG_ADDED',
};

export const DangerousChangeType = {
ARG_DEFAULT_VALUE_CHANGE: 'ARG_DEFAULT_VALUE_CHANGE',
VALUE_ADDED_TO_ENUM: 'VALUE_ADDED_TO_ENUM',
INTERFACE_ADDED_TO_OBJECT: 'INTERFACE_ADDED_TO_OBJECT',
TYPE_ADDED_TO_UNION: 'TYPE_ADDED_TO_UNION',
NULLABLE_INPUT_FIELD_ADDED: 'NULLABLE_INPUT_FIELD_ADDED',
NULLABLE_ARG_ADDED: 'NULLABLE_ARG_ADDED',
OPTIONAL_INPUT_FIELD_ADDED: 'OPTIONAL_INPUT_FIELD_ADDED',
OPTIONAL_ARG_ADDED: 'OPTIONAL_ARG_ADDED',
};

export type BreakingChange = {
Expand Down Expand Up @@ -242,24 +244,25 @@ export function findArgChanges(
}
}
}
// Check if a non-null arg was added to the field
// Check if arg was added to the field
for (const newArgDef of newTypeFields[fieldName].args) {
const oldArgs = oldTypeFields[fieldName].args;
const oldArgDef = oldArgs.find(arg => arg.name === newArgDef.name);
if (!oldArgDef) {
if (isNonNullType(newArgDef.type)) {
const argName = newArgDef.name;
if (isRequiredArgument(newArgDef)) {
breakingChanges.push({
type: BreakingChangeType.NON_NULL_ARG_ADDED,
type: BreakingChangeType.REQUIRED_ARG_ADDED,
description:
`A non-null arg ${newArgDef.name} on ` +
`${newType.name}.${fieldName} was added`,
`A required arg ${argName} on ` +
`${typeName}.${fieldName} was added`,
});
} else {
dangerousChanges.push({
type: DangerousChangeType.NULLABLE_ARG_ADDED,
type: DangerousChangeType.OPTIONAL_ARG_ADDED,
description:
`A nullable arg ${newArgDef.name} on ` +
`${newType.name}.${fieldName} was added`,
`An optional arg ${argName} on ` +
`${typeName}.${fieldName} was added`,
});
}
}
Expand Down Expand Up @@ -405,19 +408,19 @@ export function findFieldsThatChangedTypeOnInputObjectTypes(
// Check if a field was added to the input object type
for (const fieldName of Object.keys(newTypeFieldsDef)) {
if (!(fieldName in oldTypeFieldsDef)) {
if (isNonNullType(newTypeFieldsDef[fieldName].type)) {
if (isRequiredInputField(newTypeFieldsDef[fieldName])) {
breakingChanges.push({
type: BreakingChangeType.NON_NULL_INPUT_FIELD_ADDED,
type: BreakingChangeType.REQUIRED_INPUT_FIELD_ADDED,
description:
`A non-null field ${fieldName} on ` +
`input type ${newType.name} was added.`,
`A required field ${fieldName} on ` +
`input type ${typeName} was added.`,
});
} else {
dangerousChanges.push({
type: DangerousChangeType.NULLABLE_INPUT_FIELD_ADDED,
type: DangerousChangeType.OPTIONAL_INPUT_FIELD_ADDED,
description:
`A nullable field ${fieldName} on ` +
`input type ${newType.name} was added.`,
`An optional field ${fieldName} on ` +
`input type ${typeName} was added.`,
});
}
}
Expand Down Expand Up @@ -780,16 +783,14 @@ export function findAddedNonNullDirectiveArgs(
}

for (const arg of findAddedArgsForDirective(oldDirective, newDirective)) {
if (!isNonNullType(arg.type)) {
continue;
if (isRequiredArgument(arg)) {
addedNonNullableArgs.push({
type: BreakingChangeType.REQUIRED_DIRECTIVE_ARG_ADDED,
description:
`A required arg ${arg.name} on directive ` +
`${newDirective.name} was added`,
});
}

addedNonNullableArgs.push({
type: BreakingChangeType.NON_NULL_DIRECTIVE_ARG_ADDED,
description:
`A non-null arg ${arg.name} on directive ` +
`${newDirective.name} was added`,
});
}
}

Expand Down

0 comments on commit da7ef46

Please sign in to comment.