Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly detect required/optional args/fields #1492

Merged
merged 1 commit into from
Aug 29, 2018
Merged

Correctly detect required/optional args/fields #1492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 31 additions & 21 deletions src/utilities/__tests__/findBreakingChanges-test.js
View file
Open in desktop
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
Open in desktop
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',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's breaking change but it's very specific API so I don't think it will affect many users especially since most of them just checking for Breaking changes.
Here is few examples:
https://github.com/search?q=NON_NULL_ARG_ADDED+-filename%3AfindBreakingChanges&type=Code

};

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