Skip to content

Commit 410cb49

Browse files
andimarekandimarek
committed
handle fields better
1 parent fee3be3 commit 410cb49

3 files changed

Lines changed: 260 additions & 23 deletions

File tree

‎src/main/java/graphql/schema/diffing/SchemaDiffing.java‎

Lines changed: 134 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
package graphql.schema.diffing;
22

33
import com.google.common.collect.BiMap;
4+
import com.google.common.collect.HashBasedTable;
45
import com.google.common.collect.HashBiMap;
56
import com.google.common.collect.HashMultiset;
7+
import com.google.common.collect.ImmutableList;
68
import com.google.common.collect.Multiset;
79
import com.google.common.collect.Multisets;
10+
import com.google.common.collect.Table;
811
import com.google.common.util.concurrent.AtomicDouble;
912
import com.google.common.util.concurrent.AtomicDoubleArray;
1013
import graphql.schema.GraphQLSchema;
@@ -21,21 +24,15 @@
2124
import java.util.Objects;
2225
import java.util.PriorityQueue;
2326
import java.util.Set;
24-
import java.util.concurrent.ArrayBlockingQueue;
25-
import java.util.concurrent.BlockingQueue;
2627
import java.util.concurrent.Callable;
2728
import java.util.concurrent.ExecutorService;
2829
import java.util.concurrent.Executors;
2930
import java.util.concurrent.ForkJoinPool;
3031
import java.util.concurrent.LinkedBlockingQueue;
31-
import java.util.concurrent.Semaphore;
3232
import java.util.concurrent.TimeUnit;
33-
import java.util.concurrent.atomic.AtomicBoolean;
3433
import java.util.concurrent.atomic.AtomicReference;
35-
import java.util.concurrent.locks.Condition;
36-
import java.util.concurrent.locks.Lock;
37-
import java.util.concurrent.locks.ReentrantLock;
3834
import java.util.stream.Collectors;
35+
import java.util.stream.Stream;
3936

4037
import static graphql.Assert.assertTrue;
4138
import static graphql.schema.diffing.SchemaGraphFactory.DIRECTIVE;
@@ -117,33 +114,123 @@ private void diffNamedList(Collection<Vertex> sourceVertices,
117114
}
118115
}
119116

117+
private void diffNamedList(Set<String> sourceNames,
118+
Set<String> targetNames,
119+
List<String> deleted,
120+
List<String> inserted,
121+
List<String> same) {
122+
for (String sourceName : sourceNames) {
123+
if (targetNames.contains(sourceName)) {
124+
same.add(sourceName);
125+
} else {
126+
deleted.add(sourceName);
127+
}
128+
}
129+
130+
for (String targetName : targetNames) {
131+
if (!sourceNames.contains(targetName)) {
132+
inserted.add(targetName);
133+
}
134+
}
135+
}
136+
120137
List<EditOperation> diffImpl(SchemaGraph sourceGraph, SchemaGraph targetGraph) throws InterruptedException {
121138
int sizeDiff = sourceGraph.size() - targetGraph.size();
122139
System.out.println("graph diff: " + sizeDiff);
123140
Map<String, Set<Vertex>> isolatedSourceVertices = new LinkedHashMap<>();
124141
Map<String, Set<Vertex>> isolatedTargetVertices = new LinkedHashMap<>();
142+
Table<String, String, Set<Vertex>> isolatedTargetVerticesForFields = HashBasedTable.create();
143+
Table<String, String, Set<Vertex>> isolatedSourceVerticesForFields = HashBasedTable.create();
125144
for (String type : SchemaGraphFactory.ALL_TYPES) {
126-
Collection<Vertex> sourceVertices = sourceGraph.getVerticesByType(type).stream().filter(vertex -> !vertex.isBuiltInType()).collect(Collectors.toList());
127-
Collection<Vertex> targetVertices = targetGraph.getVerticesByType(type).stream().filter(vertex -> !vertex.isBuiltInType()).collect(Collectors.toList());
128-
if (sourceVertices.size() > targetVertices.size()) {
129-
isolatedTargetVertices.put(type, Vertex.newArtificialNodes(sourceVertices.size() - targetVertices.size(), "target-artificial-" + type + "-"));
130-
} else if (targetVertices.size() > sourceVertices.size()) {
131-
isolatedSourceVertices.put(type, Vertex.newArtificialNodes(targetVertices.size() - sourceVertices.size(), "source-artificial-" + type + "-"));
132-
}
133-
if (isNamedType(type)) {
134-
ArrayList<Vertex> deleted = new ArrayList<>();
135-
ArrayList<Vertex> inserted = new ArrayList<>();
136-
HashBiMap<Vertex, Vertex> same = HashBiMap.create();
137-
diffNamedList(sourceVertices, targetVertices, deleted, inserted, same);
138-
System.out.println(" " + type + " deleted " + deleted.size() + " inserted" + inserted.size() + " same " + same.size());
145+
if (FIELD.equals(type)) {
146+
147+
Stream<Vertex> sourceVerticesStream = sourceGraph.getVerticesByType(type).stream().filter(vertex -> !vertex.isBuiltInType());
148+
Stream<Vertex> targetVerticesStream = targetGraph.getVerticesByType(type).stream().filter(vertex -> !vertex.isBuiltInType());
149+
Map<String, ImmutableList<Vertex>> sourceFieldsByContainer = FpKit.groupingBy(sourceVerticesStream, v -> {
150+
Vertex fieldsContainer = getFieldsContainer(v, sourceGraph);
151+
return fieldsContainer.getType() + "-" + fieldsContainer.get("name");
152+
});
153+
Map<String, ImmutableList<Vertex>> targetFieldsByContainer = FpKit.groupingBy(targetVerticesStream, v -> {
154+
Vertex fieldsContainer = getFieldsContainer(v, targetGraph);
155+
return fieldsContainer.getType() + "-" + fieldsContainer.get("name");
156+
});
157+
158+
List<String> deletedContainers = new ArrayList<>();
159+
List<String> insertedContainers = new ArrayList<>();
160+
List<String> sameContainers = new ArrayList<>();
161+
diffNamedList(sourceFieldsByContainer.keySet(), targetFieldsByContainer.keySet(), deletedContainers, insertedContainers, sameContainers);
162+
163+
for (String sameContainer : sameContainers) {
164+
int ix = sameContainer.indexOf("-");
165+
String containerName = sameContainer.substring(ix + 1);
166+
// we have this container is source and target
167+
ImmutableList<Vertex> sourceVertices = sourceFieldsByContainer.get(sameContainer);
168+
ImmutableList<Vertex> targetVertices = targetFieldsByContainer.get(sameContainer);
169+
170+
ArrayList<Vertex> deletedFields = new ArrayList<>();
171+
ArrayList<Vertex> insertedFields = new ArrayList<>();
172+
HashBiMap<Vertex, Vertex> sameFields = HashBiMap.create();
173+
diffNamedList(sourceVertices, targetVertices, deletedFields, insertedFields, sameFields);
174+
175+
if (deletedFields.size() > insertedFields.size()) {
176+
Set<Vertex> newTargetVertices = Vertex.newArtificialNodes(deletedFields.size() - insertedFields.size(), "target-artificial-" + type + "-");
177+
for (Vertex deletedField : deletedFields) {
178+
isolatedTargetVerticesForFields.put(containerName, deletedField.get("name"), newTargetVertices);
179+
}
180+
} else if (insertedFields.size() > deletedFields.size()) {
181+
Set<Vertex> newSourceVertices = Vertex.newArtificialNodes(insertedFields.size() - deletedFields.size(), "source-artificial-" + type + "-");
182+
for (Vertex insertedField : insertedFields) {
183+
isolatedSourceVerticesForFields.put(containerName, insertedField.get("name"), newSourceVertices);
184+
}
185+
}
186+
}
187+
188+
Set<Vertex> insertedFields = new LinkedHashSet<>();
189+
Set<Vertex> deletedFields = new LinkedHashSet<>();
190+
for (String insertedContainer : insertedContainers) {
191+
insertedFields.addAll(targetFieldsByContainer.get(insertedContainer));
192+
}
193+
for (String deletedContainer : deletedContainers) {
194+
deletedFields.addAll(sourceFieldsByContainer.get(deletedContainer));
195+
}
196+
if (deletedFields.size() > insertedFields.size()) {
197+
isolatedTargetVertices.put(type, Vertex.newArtificialNodes(deletedFields.size() - insertedFields.size(), "target-artificial-FIELD-"));
198+
} else if (insertedFields.size() > deletedFields.size()) {
199+
isolatedSourceVertices.put(type, Vertex.newArtificialNodes(insertedFields.size() - deletedFields.size(), "source-artificial-FIELD-"));
200+
}
201+
202+
} else {
203+
Collection<Vertex> sourceVertices = sourceGraph.getVerticesByType(type).stream().filter(vertex -> !vertex.isBuiltInType()).collect(Collectors.toList());
204+
Collection<Vertex> targetVertices = targetGraph.getVerticesByType(type).stream().filter(vertex -> !vertex.isBuiltInType()).collect(Collectors.toList());
205+
if (sourceVertices.size() > targetVertices.size()) {
206+
isolatedTargetVertices.put(type, Vertex.newArtificialNodes(sourceVertices.size() - targetVertices.size(), "target-artificial-" + type + "-"));
207+
} else if (targetVertices.size() > sourceVertices.size()) {
208+
isolatedSourceVertices.put(type, Vertex.newArtificialNodes(targetVertices.size() - sourceVertices.size(), "source-artificial-" + type + "-"));
209+
}
139210
}
211+
// if (isNamedType(type)) {
212+
// ArrayList<Vertex> deleted = new ArrayList<>();
213+
// ArrayList<Vertex> inserted = new ArrayList<>();
214+
// HashBiMap<Vertex, Vertex> same = HashBiMap.create();
215+
// diffNamedList(sourceVertices, targetVertices, deleted, inserted, same);
216+
// System.out.println(" " + type + " deleted " + deleted.size() + " inserted" + inserted.size() + " same " + same.size());
217+
// }
140218
}
219+
141220
for (Map.Entry<String, Set<Vertex>> entry : isolatedSourceVertices.entrySet()) {
142221
sourceGraph.addVertices(entry.getValue());
143222
}
144223
for (Map.Entry<String, Set<Vertex>> entry : isolatedTargetVertices.entrySet()) {
145224
targetGraph.addVertices(entry.getValue());
146225
}
226+
for (String containerName : isolatedSourceVerticesForFields.rowKeySet()) {
227+
Map<String, Set<Vertex>> row = isolatedSourceVerticesForFields.row(containerName);
228+
sourceGraph.addVertices(row.values().iterator().next());
229+
}
230+
for (String containerName : isolatedTargetVerticesForFields.rowKeySet()) {
231+
Map<String, Set<Vertex>> row = isolatedTargetVerticesForFields.row(containerName);
232+
targetGraph.addVertices(row.values().iterator().next());
233+
}
147234

148235
Set<Vertex> isolatedBuiltInSourceVertices = new LinkedHashSet<>();
149236
Set<Vertex> isolatedBuiltInTargetVertices = new LinkedHashSet<>();
@@ -154,7 +241,7 @@ List<EditOperation> diffImpl(SchemaGraph sourceGraph, SchemaGraph targetGraph) t
154241
isolatedBuiltInTargetVertices.addAll(targetGraph.addIsolatedVertices(sourceGraph.size() - targetGraph.size(), "target-artificial-builtin-"));
155242
}
156243
assertTrue(sourceGraph.size() == targetGraph.size());
157-
IsolatedInfo isolatedInfo = new IsolatedInfo(isolatedSourceVertices, isolatedTargetVertices, isolatedBuiltInSourceVertices, isolatedBuiltInTargetVertices);
244+
IsolatedInfo isolatedInfo = new IsolatedInfo(isolatedSourceVertices, isolatedTargetVertices, isolatedBuiltInSourceVertices, isolatedBuiltInTargetVertices, isolatedSourceVerticesForFields, isolatedTargetVerticesForFields);
158245

159246
int graphSize = sourceGraph.size();
160247
System.out.println("graph size: " + graphSize);
@@ -612,12 +699,19 @@ static class IsolatedInfo {
612699
Map<String, Set<Vertex>> isolatedTargetVertices;
613700
Set<Vertex> isolatedBuiltInSourceVertices;
614701
Set<Vertex> isolatedBuiltInTargetVertices;
702+
Table<String, String, Set<Vertex>> isolatedSourceVerticesForFields;
703+
Table<String, String, Set<Vertex>> isolatedTargetVerticesForFields;
615704

616-
public IsolatedInfo(Map<String, Set<Vertex>> isolatedSourceVertices, Map<String, Set<Vertex>> isolatedTargetVertices, Set<Vertex> isolatedBuiltInSourceVertices, Set<Vertex> isolatedBuiltInTargetVertices) {
705+
public IsolatedInfo(Map<String, Set<Vertex>> isolatedSourceVertices, Map<String, Set<Vertex>> isolatedTargetVertices, Set<Vertex> isolatedBuiltInSourceVertices,
706+
Set<Vertex> isolatedBuiltInTargetVertices,
707+
Table<String, String, Set<Vertex>> isolatedSourceVerticesForFields,
708+
Table<String, String, Set<Vertex>> isolatedTargetVerticesForFields) {
617709
this.isolatedSourceVertices = isolatedSourceVertices;
618710
this.isolatedTargetVertices = isolatedTargetVertices;
619711
this.isolatedBuiltInSourceVertices = isolatedBuiltInSourceVertices;
620712
this.isolatedBuiltInTargetVertices = isolatedBuiltInTargetVertices;
713+
this.isolatedSourceVerticesForFields = isolatedSourceVerticesForFields;
714+
this.isolatedTargetVerticesForFields = isolatedTargetVerticesForFields;
621715
}
622716
}
623717

@@ -640,18 +734,36 @@ private boolean isMappingPossible(Vertex v,
640734
Map<String, Set<Vertex>> isolatedTargetVertices = isolatedInfo.isolatedTargetVertices;
641735
Set<Vertex> isolatedBuiltInSourceVertices = isolatedInfo.isolatedBuiltInSourceVertices;
642736
Set<Vertex> isolatedBuiltInTargetVertices = isolatedInfo.isolatedBuiltInTargetVertices;
737+
Table<String, String, Set<Vertex>> isolatedSourceVerticesForFields = isolatedInfo.isolatedSourceVerticesForFields;
738+
Table<String, String, Set<Vertex>> isolatedTargetVerticesForFields = isolatedInfo.isolatedTargetVerticesForFields;
643739

644740
if (v.isArtificialNode()) {
645741
if (u.isBuiltInType()) {
646742
return isolatedBuiltInSourceVertices.contains(v);
647743
} else {
744+
if (u.getType().equals(FIELD)) {
745+
Vertex fieldsContainer = getFieldsContainer(u, targetGraph);
746+
String containerName = fieldsContainer.get("name");
747+
String fieldName = u.get("name");
748+
if (isolatedSourceVerticesForFields.contains(containerName, fieldName)) {
749+
return isolatedSourceVerticesForFields.get(containerName, fieldName).contains(v);
750+
}
751+
}
648752
return isolatedSourceVertices.getOrDefault(u.getType(), emptySet()).contains(v);
649753
}
650754
}
651755
if (u.isArtificialNode()) {
652756
if (v.isBuiltInType()) {
653757
return isolatedBuiltInTargetVertices.contains(u);
654758
} else {
759+
if (v.getType().equals(FIELD)) {
760+
Vertex fieldsContainer = getFieldsContainer(v, sourceGraph);
761+
String containerName = fieldsContainer.get("name");
762+
String fieldName = u.get("name");
763+
if (isolatedTargetVerticesForFields.contains(containerName, fieldName)) {
764+
return isolatedTargetVerticesForFields.get(containerName, fieldName).contains(u);
765+
}
766+
}
655767
return isolatedTargetVertices.getOrDefault(v.getType(), emptySet()).contains(u);
656768
}
657769
}

‎src/main/java/graphql/util/FpKit.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ public static <T, NewKey> Map<NewKey, ImmutableList<T>> groupingBy(Collection<T>
4848
return list.stream().collect(Collectors.groupingBy(function, LinkedHashMap::new, mapping(Function.identity(), ImmutableList.toImmutableList())));
4949
}
5050

51+
public static <T, NewKey> Map<NewKey, ImmutableList<T>> groupingBy(Stream<T> stream, Function<T, NewKey> function) {
52+
return stream.collect(Collectors.groupingBy(function, LinkedHashMap::new, mapping(Function.identity(), ImmutableList.toImmutableList())));
53+
}
5154
public static <T, NewKey> Map<NewKey, T> groupingByUniqueKey(Collection<T> list, Function<T, NewKey> keyFunction) {
5255
return list.stream().collect(Collectors.toMap(
5356
keyFunction,
@@ -56,6 +59,14 @@ public static <T, NewKey> Map<NewKey, T> groupingByUniqueKey(Collection<T> list,
5659
LinkedHashMap::new)
5760
);
5861
}
62+
public static <T, NewKey> Map<NewKey, T> groupingByUniqueKey(Stream<T> stream, Function<T, NewKey> keyFunction) {
63+
return stream.collect(Collectors.toMap(
64+
keyFunction,
65+
identity(),
66+
throwingMerger(),
67+
LinkedHashMap::new)
68+
);
69+
}
5970

6071
private static <T> BinaryOperator<T> throwingMerger() {
6172
return (u, v) -> {

‎src/test/groovy/graphql/schema/diffing/SchemaDiffingTest.groovy‎

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,120 @@ class SchemaDiffingTest extends Specification {
5353

5454
}
5555

56+
def "adding fields and rename and delete"() {
57+
given:
58+
def schema1 = schema("""
59+
type Query {
60+
hello: String
61+
toDelete:String
62+
newField: String
63+
newField2: String
64+
}
65+
type Mutation {
66+
unchanged: Boolean
67+
unchanged2: Other
68+
}
69+
type Other {
70+
id: ID
71+
}
72+
""")
73+
def schema2 = schema("""
74+
type Query {
75+
helloRenamed: String
76+
newField: String
77+
newField2: String
78+
}
79+
type Mutation {
80+
unchanged: Boolean
81+
unchanged2: Other
82+
}
83+
type Other {
84+
id: ID
85+
}
86+
""")
87+
88+
when:
89+
def diff = new SchemaDiffing().diffGraphQLSchema(schema1, schema2)
90+
diff.each { println it }
91+
then:
92+
diff.size() == 6
93+
94+
}
95+
96+
def "remove field and rename type"() {
97+
given:
98+
def schema1 = schema("""
99+
type Query {
100+
foo: Foo
101+
}
102+
type Foo {
103+
bar: Bar
104+
toDelete:String
105+
}
106+
type Bar {
107+
id: ID
108+
name: String
109+
}
110+
""")
111+
def schema2 = schema("""
112+
type Query {
113+
foo: FooRenamed
114+
}
115+
type FooRenamed {
116+
bar: Bar
117+
}
118+
type Bar {
119+
id: ID
120+
name: String
121+
}
122+
""")
123+
124+
when:
125+
def diff = new SchemaDiffing().diffGraphQLSchema(schema1, schema2)
126+
diff.each { println it }
127+
then:
128+
diff.size() == 7
129+
130+
}
131+
132+
def "renamed field and added field and type"() {
133+
given:
134+
def schema1 = schema("""
135+
type Query {
136+
foo: Foo
137+
}
138+
type Foo {
139+
foo:String
140+
}
141+
""")
142+
def schema2 = schema("""
143+
type Query {
144+
foo: Foo
145+
}
146+
type Foo {
147+
fooRenamed:String
148+
bar: Bar
149+
}
150+
type Bar {
151+
id: String
152+
name: String
153+
}
154+
""")
155+
156+
when:
157+
def diff = new SchemaDiffing().diffGraphQLSchema(schema1, schema2)
158+
diff.each { println it }
159+
then:
160+
/**
161+
* 1: Changed Field
162+
* 2: New Object
163+
* 3-8: Three new Fields + DummyTypes
164+
* 9-17: Edges from Object to new Fields (3) + Edges from Field to Dummy Type (3) + Edges from DummyType to String
165+
* */
166+
diff.size() == 17
167+
168+
}
169+
56170

57171
def "test two field renames one type rename"() {
58172
given:
@@ -371,7 +485,7 @@ class SchemaDiffingTest extends Specification {
371485
def diffing = new SchemaDiffing()
372486
when:
373487
def diff = diffing.diffGraphQLSchema(schema1, schema2)
374-
for(EditOperation editOperation: diff) {
488+
for (EditOperation editOperation : diff) {
375489
println editOperation
376490
}
377491

0 commit comments

Comments
 (0)