From d71b45d60ef40740833b9f76e654d16c175eb844 Mon Sep 17 00:00:00 2001 From: Daniel Tschinder Date: Fri, 16 Feb 2018 00:15:19 +0100 Subject: [PATCH] Find breaking directive changes ref: graphql/graphql-js#1152 --- src/Utils/FindBreakingChanges.php | 183 ++++++++++++--- tests/Utils/FindBreakingChangesTest.php | 286 ++++++++++++++++++++++-- 2 files changed, 419 insertions(+), 50 deletions(-) diff --git a/src/Utils/FindBreakingChanges.php b/src/Utils/FindBreakingChanges.php index 6c67aa1..631fd69 100644 --- a/src/Utils/FindBreakingChanges.php +++ b/src/Utils/FindBreakingChanges.php @@ -5,10 +5,12 @@ namespace GraphQL\Utils; +use GraphQL\Type\Definition\Directive; use GraphQL\Type\Definition\EnumType; use GraphQL\Type\Definition\InputObjectType; use GraphQL\Type\Definition\InterfaceType; use GraphQL\Type\Definition\ListOfType; +use GraphQL\Type\Definition\NamedType; use GraphQL\Type\Definition\NonNull; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\ScalarType; @@ -30,6 +32,10 @@ class FindBreakingChanges const BREAKING_CHANGE_NON_NULL_ARG_ADDED = 'NON_NULL_ARG_ADDED'; const BREAKING_CHANGE_NON_NULL_INPUT_FIELD_ADDED = 'NON_NULL_INPUT_FIELD_ADDED'; const BREAKING_CHANGE_INTERFACE_REMOVED_FROM_OBJECT = 'INTERFACE_REMOVED_FROM_OBJECT'; + const BREAKING_CHANGE_DIRECTIVE_REMOVED = 'DIRECTIVE_REMOVED'; + const BREAKING_CHANGE_DIRECTIVE_ARG_REMOVED = 'DIRECTIVE_ARG_REMOVED'; + const BREAKING_CHANGE_DIRECTIVE_LOCATION_REMOVED = 'DIRECTIVE_LOCATION_REMOVED'; + const BREAKING_CHANGE_NON_NULL_DIRECTIVE_ARG_ADDED = 'NON_NULL_DIRECTIVE_ARG_ADDED'; const DANGEROUS_CHANGE_ARG_DEFAULT_VALUE = 'ARG_DEFAULT_VALUE_CHANGE'; const DANGEROUS_CHANGE_VALUE_ADDED_TO_ENUM = 'VALUE_ADDED_TO_ENUM'; @@ -53,7 +59,11 @@ class FindBreakingChanges self::findTypesRemovedFromUnions($oldSchema, $newSchema), self::findValuesRemovedFromEnums($oldSchema, $newSchema), self::findArgChanges($oldSchema, $newSchema)['breakingChanges'], - self::findInterfacesRemovedFromObjectTypes($oldSchema, $newSchema) + self::findInterfacesRemovedFromObjectTypes($oldSchema, $newSchema), + self::findRemovedDirectives($oldSchema, $newSchema), + self::findRemovedDirectiveArgs($oldSchema, $newSchema), + self::findAddedNonNullDirectiveArgs($oldSchema, $newSchema), + self::findRemovedDirectiveLocations($oldSchema, $newSchema) ); } @@ -283,8 +293,8 @@ class FindBreakingChanges $isSafe = self::isChangeSafeForObjectOrInterfaceField($oldFieldType, $newfieldType); if (!$isSafe) { - $oldFieldTypeString = self::isNamedType($oldFieldType) ? $oldFieldType->name : $oldFieldType; - $newFieldTypeString = self::isNamedType($newfieldType) ? $newfieldType->name : $newfieldType; + $oldFieldTypeString = $oldFieldType instanceof NamedType ? $oldFieldType->name : $oldFieldType; + $newFieldTypeString = $newfieldType instanceof NamedType ? $newfieldType->name : $newfieldType; $breakingChanges[] = ['type' => self::BREAKING_CHANGE_FIELD_CHANGED, 'description' => "${typeName}->${fieldName} changed type from ${oldFieldTypeString} to ${newFieldTypeString}."]; } } @@ -323,11 +333,11 @@ class FindBreakingChanges ]; } else { $oldFieldType = $oldTypeFieldsDef[$fieldName]->getType(); - $newfieldType = $newTypeFieldsDef[$fieldName]->getType(); - $isSafe = self::isChangeSafeForInputObjectFieldOrFieldArg($oldFieldType, $newfieldType); + $newFieldType = $newTypeFieldsDef[$fieldName]->getType(); + $isSafe = self::isChangeSafeForInputObjectFieldOrFieldArg($oldFieldType, $newFieldType); if (!$isSafe) { - $oldFieldTypeString = self::isNamedType($oldFieldType) ? $oldFieldType->name : $oldFieldType; - $newFieldTypeString = self::isNamedType($newfieldType) ? $newfieldType->name : $newfieldType; + $oldFieldTypeString = $oldFieldType instanceof NamedType ? $oldFieldType->name : $oldFieldType; + $newFieldTypeString = $newFieldType instanceof NamedType ? $newFieldType->name : $newFieldType; $breakingChanges[] = [ 'type' => self::BREAKING_CHANGE_FIELD_CHANGED, 'description' => "${typeName}->${fieldName} changed type from ${oldFieldTypeString} to ${newFieldTypeString}."]; @@ -361,9 +371,9 @@ class FindBreakingChanges Type $oldType, Type $newType ) { - if (self::isNamedType($oldType)) { + if ($oldType instanceof NamedType) { // if they're both named types, see if their names are equivalent - return (self::isNamedType($newType) && $oldType->name === $newType->name) + return ($newType instanceof NamedType && $oldType->name === $newType->name) // moving from nullable to non-null of the same underlying type is safe || ($newType instanceof NonNull && self::isChangeSafeForObjectOrInterfaceField( @@ -387,16 +397,16 @@ class FindBreakingChanges /** * @param Type $oldType - * @param Schema $newSchema + * @param Type $newType * * @return bool */ private static function isChangeSafeForInputObjectFieldOrFieldArg( - Type $oldType, Type $newType - ) - { - if (self::isNamedType($oldType)) { - return self::isNamedType($newType) && $oldType->name === $newType->name; + Type $oldType, + Type $newType + ) { + if ($oldType instanceof NamedType) { + return $newType instanceof NamedType && $oldType->name === $newType->name; } elseif ($oldType instanceof ListOfType) { return $newType instanceof ListOfType && self::isChangeSafeForInputObjectFieldOrFieldArg($oldType->getWrappedType(), $newType->getWrappedType()); } elseif ($oldType instanceof NonNull) { @@ -583,20 +593,135 @@ class FindBreakingChanges return $breakingChanges; } - /** - * @param Type $type - * - * @return bool - */ - private static function isNamedType(Type $type) + public static function findRemovedDirectives(Schema $oldSchema, Schema $newSchema) { - return ( - $type instanceof ScalarType || - $type instanceof ObjectType || - $type instanceof InterfaceType || - $type instanceof UnionType || - $type instanceof EnumType || - $type instanceof InputObjectType - ); + $removedDirectives = []; + + $newSchemaDirectiveMap = self::getDirectiveMapForSchema($newSchema); + foreach($oldSchema->getDirectives() as $directive) { + if (!isset($newSchemaDirectiveMap[$directive->name])) { + $removedDirectives[] = [ + 'type' => self::BREAKING_CHANGE_DIRECTIVE_REMOVED, + 'description' => "{$directive->name} was removed", + ]; + } + } + + return $removedDirectives; + } + + public static function findRemovedArgsForDirectives(Directive $oldDirective, Directive $newDirective) + { + $removedArgs = []; + $newArgMap = self::getArgumentMapForDirective($newDirective); + foreach((array) $oldDirective->args as $arg) { + if (!isset($newArgMap[$arg->name])) { + $removedArgs[] = $arg; + } + } + + return $removedArgs; + } + + public static function findRemovedDirectiveArgs(Schema $oldSchema, Schema $newSchema) + { + $removedDirectiveArgs = []; + $oldSchemaDirectiveMap = self::getDirectiveMapForSchema($oldSchema); + + foreach($newSchema->getDirectives() as $newDirective) { + if (!isset($oldSchemaDirectiveMap[$newDirective->name])) { + continue; + } + + foreach(self::findRemovedArgsForDirectives($oldSchemaDirectiveMap[$newDirective->name], $newDirective) as $arg) { + $removedDirectiveArgs[] = [ + 'type' => self::BREAKING_CHANGE_DIRECTIVE_ARG_REMOVED, + 'description' => "{$arg->name} was removed from {$newDirective->name}", + ]; + } + } + + return $removedDirectiveArgs; + } + + public static function findAddedArgsForDirective(Directive $oldDirective, Directive $newDirective) + { + $addedArgs = []; + $oldArgMap = self::getArgumentMapForDirective($oldDirective); + foreach((array) $newDirective->args as $arg) { + if (!isset($oldArgMap[$arg->name])) { + $addedArgs[] = $arg; + } + } + + return $addedArgs; + } + + public static function findAddedNonNullDirectiveArgs(Schema $oldSchema, Schema $newSchema) + { + $addedNonNullableArgs = []; + $oldSchemaDirectiveMap = self::getDirectiveMapForSchema($oldSchema); + + foreach($newSchema->getDirectives() as $newDirective) { + if (!isset($oldSchemaDirectiveMap[$newDirective->name])) { + continue; + } + + foreach(self::findAddedArgsForDirective($oldSchemaDirectiveMap[$newDirective->name], $newDirective) as $arg) { + if (!$arg->getType() instanceof NonNull) { + continue; + } + $addedNonNullableArgs[] = [ + 'type' => self::BREAKING_CHANGE_NON_NULL_DIRECTIVE_ARG_ADDED, + 'description' => "A non-null arg {$arg->name} on directive {$newDirective->name} was added", + ]; + } + } + + return $addedNonNullableArgs; + } + + public static function findRemovedLocationsForDirective(Directive $oldDirective, Directive $newDirective) + { + $removedLocations = []; + $newLocationSet = array_flip($newDirective->locations); + foreach($oldDirective->locations as $oldLocation) { + if (!array_key_exists($oldLocation, $newLocationSet)) { + $removedLocations[] = $oldLocation; + } + } + + return $removedLocations; + } + + public static function findRemovedDirectiveLocations(Schema $oldSchema, Schema $newSchema) + { + $removedLocations = []; + $oldSchemaDirectiveMap = self::getDirectiveMapForSchema($oldSchema); + + foreach($newSchema->getDirectives() as $newDirective) { + if (!isset($oldSchemaDirectiveMap[$newDirective->name])) { + continue; + } + + foreach(self::findRemovedLocationsForDirective($oldSchemaDirectiveMap[$newDirective->name], $newDirective) as $location) { + $removedLocations[] = [ + 'type' => self::BREAKING_CHANGE_DIRECTIVE_LOCATION_REMOVED, + 'description' => "{$location} was removed from {$newDirective->name}", + ]; + } + } + + return $removedLocations; + } + + private static function getDirectiveMapForSchema(Schema $schema) + { + return Utils::keyMap($schema->getDirectives(), function ($dir) { return $dir->name; }); + } + + private static function getArgumentMapForDirective(Directive $directive) + { + return Utils::keyMap($directive->args ?: [], function ($arg) { return $arg->name; }); } } diff --git a/tests/Utils/FindBreakingChangesTest.php b/tests/Utils/FindBreakingChangesTest.php index bfc79a9..b60301d 100644 --- a/tests/Utils/FindBreakingChangesTest.php +++ b/tests/Utils/FindBreakingChangesTest.php @@ -1,7 +1,10 @@ 'DirectiveThatRemovesArg', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + 'args' => FieldArgument::createMap([ + 'arg1' => [ + 'name' => 'arg1', + ], + ]), + ]); + $directiveThatRemovesArgNew = new Directive([ + 'name' => 'DirectiveThatRemovesArg', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + ]); + $nonNullDirectiveAddedOld = new Directive([ + 'name' => 'NonNullDirectiveAdded', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + ]); + $nonNullDirectiveAddedNew = new Directive([ + 'name' => 'NonNullDirectiveAdded', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + 'args' => FieldArgument::createMap([ + 'arg1' => [ + 'name' => 'arg1', + 'type' => Type::nonNull(Type::boolean()), + ], + ]), + ]); + $directiveRemovedLocationOld = new Directive([ + 'name' => 'Directive Name', + 'locations' => [DirectiveLocation::FIELD_DEFINITION, DirectiveLocation::QUERY], + ]); + $directiveRemovedLocationNew = new Directive([ + 'name' => 'Directive Name', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + ]); + $oldSchema = new Schema([ 'query' => $this->queryType, - 'types' => - [ - 'TypeThatGetsRemoved' => $typeThatGetsRemoved, - 'TypeThatChangesType' => $typeThatChangesTypeOld, - 'TypeThatHasBreakingFieldChanges' => $typeThatHasBreakingFieldChangesOld, - 'UnionTypeThatLosesAType' => $unionTypeThatLosesATypeOld, - 'EnumTypeThatLosesAValue' => $enumTypeThatLosesAValueOld, - 'ArgThatChanges' => $argThatChanges, - 'TypeThatLosesInterface' => $typeThatLosesInterfaceOld - ] + 'types' => [ + 'TypeThatGetsRemoved' => $typeThatGetsRemoved, + 'TypeThatChangesType' => $typeThatChangesTypeOld, + 'TypeThatHasBreakingFieldChanges' => $typeThatHasBreakingFieldChangesOld, + 'UnionTypeThatLosesAType' => $unionTypeThatLosesATypeOld, + 'EnumTypeThatLosesAValue' => $enumTypeThatLosesAValueOld, + 'ArgThatChanges' => $argThatChanges, + 'TypeThatLosesInterface' => $typeThatLosesInterfaceOld + ], + 'directives' => [ + $directiveThatIsRemoved, + $directiveThatRemovesArgOld, + $nonNullDirectiveAddedOld, + $directiveRemovedLocationOld, + ] ]); $newSchema = new Schema([ 'query' => $this->queryType, - 'types' => - [ - 'TypeThatChangesType' => $typeThatChangesTypeNew, - 'TypeThatHasBreakingFieldChanges' => $typeThatHasBreakingFieldChangesNew, - 'UnionTypeThatLosesAType' => $unionTypeThatLosesATypeNew, - 'EnumTypeThatLosesAValue' => $enumTypeThatLosesAValueNew, - 'ArgThatChanges' => $argChanged, - 'TypeThatLosesInterface' => $typeThatLosesInterfaceNew, - 'Interface1' => $interface1 - ] + 'types' => [ + 'TypeThatChangesType' => $typeThatChangesTypeNew, + 'TypeThatHasBreakingFieldChanges' => $typeThatHasBreakingFieldChangesNew, + 'UnionTypeThatLosesAType' => $unionTypeThatLosesATypeNew, + 'EnumTypeThatLosesAValue' => $enumTypeThatLosesAValueNew, + 'ArgThatChanges' => $argChanged, + 'TypeThatLosesInterface' => $typeThatLosesInterfaceNew, + 'Interface1' => $interface1 + ], + 'directives' => [ + $directiveThatRemovesArgNew, + $nonNullDirectiveAddedNew, + $directiveRemovedLocationNew, + ] ]); $expectedBreakingChanges = [ @@ -1206,13 +1255,208 @@ class FindBreakingChangesTest extends \PHPUnit_Framework_TestCase [ 'type' => FindBreakingChanges::BREAKING_CHANGE_INTERFACE_REMOVED_FROM_OBJECT, 'description' => 'TypeThatLosesInterface1 no longer implements interface Interface1.', + ], + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_DIRECTIVE_REMOVED, + 'description' => 'skip was removed', + ], + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_DIRECTIVE_ARG_REMOVED, + 'description' => 'arg1 was removed from DirectiveThatRemovesArg', + ], + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_NON_NULL_DIRECTIVE_ARG_ADDED, + 'description' => 'A non-null arg arg1 on directive NonNullDirectiveAdded was added', + ], + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_DIRECTIVE_LOCATION_REMOVED, + 'description' => 'QUERY was removed from Directive Name', ] ]; $this->assertEquals($expectedBreakingChanges, FindBreakingChanges::findBreakingChanges($oldSchema, $newSchema)); } - // findDangerousChanges tests below here + /** + * @it should detect if a directive was explicitly removed + */ + public function testShouldDetectIfADirectiveWasExplicitlyRemoved() + { + $oldSchema = new Schema([ + 'directives' => [Directive::skipDirective(), Directive::includeDirective()], + ]); + + $newSchema = new Schema([ + 'directives' => [Directive::skipDirective()], + ]); + + $includeDirective = Directive::includeDirective(); + + $expectedBreakingChanges = [ + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_DIRECTIVE_REMOVED, + 'description' => "{$includeDirective->name} was removed", + ] + ]; + + $this->assertEquals($expectedBreakingChanges, FindBreakingChanges::findRemovedDirectives($oldSchema, $newSchema)); + } + + /** + * @it should detect if a directive was implicitly removed + */ + public function testShouldDetectIfADirectiveWasImplicitlyRemoved() + { + $oldSchema = new Schema([]); + + $newSchema = new Schema([ + 'directives' => [Directive::skipDirective(), Directive::includeDirective()], + ]); + + $deprecatedDirective = Directive::deprecatedDirective(); + + $expectedBreakingChanges = [ + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_DIRECTIVE_REMOVED, + 'description' => "{$deprecatedDirective->name} was removed", + ] + ]; + + $this->assertEquals($expectedBreakingChanges, FindBreakingChanges::findRemovedDirectives($oldSchema, $newSchema)); + } + + /** + * @it should detect if a directive argument was removed + */ + public function testShouldDetectIfADirectiveArgumentWasRemoved() + { + $oldSchema = new Schema([ + 'directives' => [ + new Directive([ + 'name' => 'DirectiveWithArg', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + 'args' => FieldArgument::createMap([ + 'arg1' => [ + 'name' => 'arg1', + ], + ]), + ]) + ], + ]); + + $newSchema = new Schema([ + 'directives' => [ + new Directive([ + 'name' => 'DirectiveWithArg', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + ]) + ], + ]); + + $expectedBreakingChanges = [ + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_DIRECTIVE_ARG_REMOVED, + 'description' => "arg1 was removed from DirectiveWithArg", + ] + ]; + + $this->assertEquals($expectedBreakingChanges, FindBreakingChanges::findRemovedDirectiveArgs($oldSchema, $newSchema)); + } + + /** + * @it should detect if a non-nullable directive argument was added + */ + public function testShouldDetectIfANonNullableDirectiveArgumentWasAdded() + { + $oldSchema = new Schema([ + 'directives' => [ + new Directive([ + 'name' => 'DirectiveName', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + ]) + ], + ]); + + $newSchema = new Schema([ + 'directives' => [ + new Directive([ + 'name' => 'DirectiveName', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + 'args' => FieldArgument::createMap([ + 'arg1' => [ + 'name' => 'arg1', + 'type' => Type::nonNull(Type::boolean()), + ], + ]), + ]) + ], + ]); + + $expectedBreakingChanges = [ + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_NON_NULL_DIRECTIVE_ARG_ADDED, + 'description' => "A non-null arg arg1 on directive DirectiveName was added", + ] + ]; + + $this->assertEquals($expectedBreakingChanges, FindBreakingChanges::findAddedNonNullDirectiveArgs($oldSchema, $newSchema)); + } + + /** + * @it should detect locations removed from a directive + */ + public function testShouldDetectLocationsRemovedFromADirective() + { + $d1 = new Directive([ + 'name' => 'Directive Name', + 'locations' => [DirectiveLocation::FIELD_DEFINITION, DirectiveLocation::QUERY], + ]); + + $d2 = new Directive([ + 'name' => 'Directive Name', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + ]); + + $this->assertEquals([DirectiveLocation::QUERY], FindBreakingChanges::findRemovedLocationsForDirective($d1, $d2)); + } + + /** + * @it should detect locations removed directives within a schema + */ + public function testShouldDetectLocationsRemovedDirectiveWithinASchema() + { + $oldSchema = new Schema([ + 'directives' => [ + new Directive([ + 'name' => 'Directive Name', + 'locations' => [ + DirectiveLocation::FIELD_DEFINITION, + DirectiveLocation::QUERY + ], + ]) + ], + ]); + + $newSchema = new Schema([ + 'directives' => [ + new Directive([ + 'name' => 'Directive Name', + 'locations' => [DirectiveLocation::FIELD_DEFINITION], + ]) + ], + ]); + + $expectedBreakingChanges = [ + [ + 'type' => FindBreakingChanges::BREAKING_CHANGE_DIRECTIVE_LOCATION_REMOVED, + 'description' => "QUERY was removed from Directive Name", + ] + ]; + + $this->assertEquals($expectedBreakingChanges, FindBreakingChanges::findRemovedDirectiveLocations($oldSchema, $newSchema)); + } + + // DESCRIBE: findDangerousChanges public function testFindDangerousArgChanges() {