Fix type change and directive deletion problems in schema diffing #3102
Conversation
| 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)) { |
There was a problem hiding this comment.
Fixes a bug where deleted directives were considered deleted union members.
| 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)) { |
There was a problem hiding this comment.
Fixes a bug where deleted directives were considered deleted enum values.
| def appliedDirective = (changes.enumDifferences["E"] as EnumModification).getDetails(AppliedDirectiveDeletion) | ||
| def diff = changes.enumDifferences["E"] as EnumModification | ||
|
|
||
| diff.getDetails().size() == 1 |
There was a problem hiding this comment.
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.
| } | ||
| String newType = getTypeFromEdgeLabel(editOperation.getTargetEdge()); | ||
| EditOperation deletedTypeEdgeOperation = findDeletedEdge(inputField, editOperations, mapping); | ||
| EditOperation deletedTypeEdgeOperation = findDeletedEdge(inputField, editOperations, mapping, edge -> edge.getLabel().contains("type=")); |
There was a problem hiding this comment.
Is "type=" strings a good idea here?
There was a problem hiding this comment.
Well I reckon a better solution is to store a relationship enum that tracks how the two vertices were related to each other.
Andi suggested this is fine for now. Or did I misunderstand in our meeting? @andimarek
There was a problem hiding this comment.
Yes the edge having type= as indicator that this is an "type edge" is ok for now.
But: can we make this is bit more dry and contain this logic a bit?
We have already getDefaultValueFromEdgeLabel helper method. I would recommend to add another helper method like "isTypeEdge" and use that.
| } | ||
| String newType = getTypeFromEdgeLabel(editOperation.getTargetEdge()); | ||
| EditOperation deletedTypeEdgeOperation = findDeletedEdge(inputField, editOperations, mapping); | ||
| EditOperation deletedTypeEdgeOperation = findDeletedEdge(inputField, editOperations, mapping, edge -> edge.getLabel().contains("type=")); |
There was a problem hiding this comment.
Yes the edge having type= as indicator that this is an "type edge" is ok for now.
But: can we make this is bit more dry and contain this logic a bit?
We have already getDefaultValueFromEdgeLabel helper method. I would recommend to add another helper method like "isTypeEdge" and use that.
4 participants
I think at some point the Edges to retain more information from the original schema e.g. relationship information. Ideally not in a
String.It's possible that future changes to the GraphQL spec could change the schema. A lot of this code relies on implicit assumptions that could be broken.
e.g. instead of saying I want to only look at union members, I have to assume that there are only union members and applied directives. Then ignore the applied directives.
Either way, it works now.