Skip to content

Fix type change and directive deletion problems in schema diffing #3102

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

Merged
andimarek merged 3 commits into from
Mar 1, 2023
Merged

Fix type change and directive deletion problems in schema diffing #3102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6d08f9
Fix type change and directive deletion problems in schema diffing
gnawf Mar 1, 2023
c3a5497
Remove redundant check
gnawf Mar 1, 2023
fb695ef
Add helper to check whether edge is a type
gnawf Mar 1, 2023
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
33 changes: 19 additions & 14 deletions src/main/java/graphql/schema/diffing/ana/EditOperationAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.function.Predicate;

import static graphql.Assert.assertTrue;
import static graphql.schema.diffing.ana.SchemaDifference.AppliedDirectiveAddition;
Expand Down Expand Up @@ -593,7 +594,7 @@ private void handleUnionMemberChanges(List<EditOperation> editOperations, Mappin
break;
case DELETE_EDGE:
Edge oldEdge = editOperation.getSourceEdge();
if (oldEdge.getFrom().isOfType(SchemaGraph.UNION)) {
if (oldEdge.getFrom().isOfType(SchemaGraph.UNION) && !oldEdge.getTo().isOfType(SchemaGraph.APPLIED_DIRECTIVE)) {
Copy link
Contributor Author

@gnawf gnawf Mar 1, 2023

Choose a reason for hiding this comment

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

Fixes a bug where deleted directives were considered deleted union members.

handleUnionMemberDeleted(editOperation);
}
break;
Expand All @@ -606,13 +607,13 @@ private void handleEnumValuesChanges(List<EditOperation> editOperations, Mapping
switch (editOperation.getOperation()) {
case INSERT_EDGE:
Edge newEdge = editOperation.getTargetEdge();
if (newEdge.getFrom().isOfType(SchemaGraph.ENUM)) {
if (newEdge.getFrom().isOfType(SchemaGraph.ENUM) && newEdge.getTo().isOfType(SchemaGraph.ENUM_VALUE)) {
handleEnumValueAdded(editOperation);
}
break;
case DELETE_EDGE:
Edge oldEdge = editOperation.getSourceEdge();
if (oldEdge.getFrom().isOfType(SchemaGraph.ENUM)) {
if (oldEdge.getFrom().isOfType(SchemaGraph.ENUM) && oldEdge.getTo().isOfType(SchemaGraph.ENUM_VALUE)) {
Copy link
Contributor Author

@gnawf gnawf Mar 1, 2023

Choose a reason for hiding this comment

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

Fixes a bug where deleted directives were considered deleted enum values.

handleEnumValueDeleted(editOperation);
}
break;
Expand Down Expand Up @@ -932,7 +933,7 @@ private void typeEdgeInsertedForInputField(EditOperation
return;
}
String newType = getTypeFromEdgeLabel(editOperation.getTargetEdge());
EditOperation deletedTypeEdgeOperation = findDeletedEdge(inputField, editOperations, mapping);
EditOperation deletedTypeEdgeOperation = findDeletedEdge(inputField, editOperations, mapping, this::isTypeEdge);
String oldType = getTypeFromEdgeLabel(deletedTypeEdgeOperation.getSourceEdge());
InputObjectFieldTypeModification inputObjectFieldTypeModification = new InputObjectFieldTypeModification(inputField.getName(), oldType, newType);
getInputObjectModification(inputObject.getName()).getDetails().add(inputObjectFieldTypeModification);
Expand Down Expand Up @@ -963,7 +964,7 @@ private void typeEdgeInsertedForArgument(EditOperation
String newType = getTypeFromEdgeLabel(editOperation.getTargetEdge());
// this means we have an existing object changed its type
// and there must be a deleted edge with the old type information
EditOperation deletedTypeEdgeOperation = findDeletedEdge(argument, editOperations, mapping);
EditOperation deletedTypeEdgeOperation = findDeletedEdge(argument, editOperations, mapping, this::isTypeEdge);
String oldType = getTypeFromEdgeLabel(deletedTypeEdgeOperation.getSourceEdge());
ObjectFieldArgumentTypeModification objectFieldArgumentTypeModification = new ObjectFieldArgumentTypeModification(field.getName(), argument.getName(), oldType, newType);
getObjectModification(object.getName()).getDetails().add(objectFieldArgumentTypeModification);
Expand All @@ -985,7 +986,7 @@ private void typeEdgeInsertedForArgument(EditOperation
String newType = getTypeFromEdgeLabel(editOperation.getTargetEdge());
// this means we have an existing object changed its type
// and there must be a deleted edge with the old type information
EditOperation deletedTypeEdgeOperation = findDeletedEdge(argument, editOperations, mapping);
EditOperation deletedTypeEdgeOperation = findDeletedEdge(argument, editOperations, mapping, this::isTypeEdge);
String oldType = getTypeFromEdgeLabel(deletedTypeEdgeOperation.getSourceEdge());
InterfaceFieldArgumentTypeModification interfaceFieldArgumentTypeModification = new InterfaceFieldArgumentTypeModification(field.getName(), argument.getName(), oldType, newType);
getInterfaceModification(interfaze.getName()).getDetails().add(interfaceFieldArgumentTypeModification);
Expand All @@ -1000,7 +1001,7 @@ private void typeEdgeInsertedForArgument(EditOperation
return;
}
String newType = getTypeFromEdgeLabel(editOperation.getTargetEdge());
EditOperation deletedTypeEdgeOperation = findDeletedEdge(argument, editOperations, mapping);
EditOperation deletedTypeEdgeOperation = findDeletedEdge(argument, editOperations, mapping, this::isTypeEdge);
String oldType = getTypeFromEdgeLabel(deletedTypeEdgeOperation.getSourceEdge());
DirectiveArgumentTypeModification directiveArgumentTypeModification = new DirectiveArgumentTypeModification(argument.getName(), oldType, newType);
getDirectiveModification(directive.getName()).getDetails().add(directiveArgumentTypeModification);
Expand All @@ -1025,11 +1026,10 @@ private void typeEdgeInsertedForField(EditOperation
String newType = getTypeFromEdgeLabel(editOperation.getTargetEdge());
// this means we have an existing object changed its type
// and there must be a deleted edge with the old type information
EditOperation deletedTypeEdgeOperation = findDeletedEdge(field, editOperations, mapping);
EditOperation deletedTypeEdgeOperation = findDeletedEdge(field, editOperations, mapping, this::isTypeEdge);
String oldType = getTypeFromEdgeLabel(deletedTypeEdgeOperation.getSourceEdge());
ObjectFieldTypeModification objectFieldTypeModification = new ObjectFieldTypeModification(field.getName(), oldType, newType);
getObjectModification(object.getName()).getDetails().add(objectFieldTypeModification);

} else {
assertTrue(objectOrInterface.isOfType(SchemaGraph.INTERFACE));
Vertex interfaze = objectOrInterface;
Expand All @@ -1042,22 +1042,23 @@ private void typeEdgeInsertedForField(EditOperation
String newType = getTypeFromEdgeLabel(editOperation.getTargetEdge());
// this means we have an existing object changed its type
// and there must be a deleted edge with the old type information
EditOperation deletedTypeEdgeOperation = findDeletedEdge(field, editOperations, mapping);
EditOperation deletedTypeEdgeOperation = findDeletedEdge(field, editOperations, mapping, this::isTypeEdge);
String oldType = getTypeFromEdgeLabel(deletedTypeEdgeOperation.getSourceEdge());
InterfaceFieldTypeModification interfaceFieldTypeModification = new InterfaceFieldTypeModification(field.getName(), oldType, newType);
getInterfaceModification(interfaze.getName()).getDetails().add(interfaceFieldTypeModification);

}
}


private EditOperation findDeletedEdge(Vertex targetVertexFrom, List<EditOperation> editOperations, Mapping
mapping) {
private EditOperation findDeletedEdge(Vertex targetVertexFrom,
List<EditOperation> editOperations,
Mapping mapping,
Predicate<Edge> edgePredicate) {
Vertex sourceVertexFrom = mapping.getSource(targetVertexFrom);
for (EditOperation editOperation : editOperations) {
if (editOperation.getOperation() == EditOperation.Operation.DELETE_EDGE) {
Edge deletedEdge = editOperation.getSourceEdge();
if (deletedEdge.getFrom() == sourceVertexFrom) {
if (deletedEdge.getFrom() == sourceVertexFrom && edgePredicate.test(deletedEdge)) {
return editOperation;
}
}
Expand Down Expand Up @@ -1199,6 +1200,10 @@ private String getDefaultValueFromEdgeLabel(Edge edge) {
return defaultValue;
}

private boolean isTypeEdge(Edge edge) {
String label = edge.getLabel();
return label.startsWith("type=");
}

private void interfaceImplementationDeleted(Edge deletedEdge) {
Vertex from = deletedEdge.getFrom();
Expand Down
13 changes: 10 additions & 3 deletions src/test/groovy/graphql/schema/diffing/ana/EditOperationAnalyzerAppliedDirectivesTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,11 @@ class EditOperationAnalyzerAppliedDirectivesTest extends Specification {
def changes = calcDiff(oldSdl, newSdl)
then:
changes.enumDifferences["E"] instanceof EnumModification
def appliedDirective = (changes.enumDifferences["E"] as EnumModification).getDetails(AppliedDirectiveDeletion)
def diff = changes.enumDifferences["E"] as EnumModification

diff.getDetails().size() == 1
Copy link
Contributor Author

@gnawf gnawf Mar 1, 2023

Choose a reason for hiding this comment

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

I haven't had time to change the rest, but the tests should really ensure all details are checked.

In this test we previously had an undetected change where the deleted directive also introduced a enum value removed change detail.


def appliedDirective = diff.getDetails(AppliedDirectiveDeletion)
(appliedDirective[0].locationDetail as AppliedDirectiveEnumLocation).name == "E"
appliedDirective[0].name == "d"
}
Expand Down Expand Up @@ -778,7 +782,6 @@ class EditOperationAnalyzerAppliedDirectivesTest extends Specification {
appliedDirective[0].name == "d"
}


def "applied directive deleted union"() {
given:
def oldSdl = '''
Expand All @@ -802,8 +805,12 @@ class EditOperationAnalyzerAppliedDirectivesTest extends Specification {
when:
def changes = calcDiff(oldSdl, newSdl)
then:
changes.unionDifferences.keySet() == ["U"] as Set
changes.unionDifferences["U"] instanceof UnionModification
def appliedDirective = (changes.unionDifferences["U"] as UnionModification).getDetails(AppliedDirectiveDeletion)
def diff = changes.unionDifferences["U"] as UnionModification
diff.details.size() == 1

def appliedDirective = diff.getDetails(AppliedDirectiveDeletion)
(appliedDirective[0].locationDetail as AppliedDirectiveUnionLocation).name == "U"
appliedDirective[0].name == "d"
}
Expand Down
Loading