Skip to content

Commit 2f54794

Browse files
dondonzdondonz
committed
gnhf #2: Implemented allocation and collection overhead reductions in overlapping field validation: replaced ImmutableList path copies with ArrayList stack, switched LinkedHashSet to HashSet for dedup sets, shared visitedFragments across merge loop, and added single-element entry fast-paths.
1 parent e0ca061 commit 2f54794

File tree

1 file changed

+52
-32
lines changed

1 file changed

+52
-32
lines changed

‎src/main/java/graphql/validation/OperationValidator.java‎

Lines changed: 52 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@
7878
import java.util.Set;
7979
import java.util.function.Predicate;
8080

81-
import static graphql.collect.ImmutableKit.addToList;
8281
import static graphql.collect.ImmutableKit.emptyList;
8382
import static graphql.schema.GraphQLTypeUtil.isEnum;
8483
import static graphql.schema.GraphQLTypeUtil.isInput;
@@ -310,9 +309,9 @@ public class OperationValidator implements DocumentVisitor {
310309
private @Nullable Map<String, VariableDefinition> variableDefinitionMap;
311310

312311
// --- State: OverlappingFieldsCanBeMerged ---
313-
private final Set<Set<FieldAndType>> sameResponseShapeChecked = new LinkedHashSet<>();
314-
private final Set<Set<FieldAndType>> sameForCommonParentsChecked = new LinkedHashSet<>();
315-
private final Set<Set<Field>> conflictsReported = new LinkedHashSet<>();
312+
private final Set<Set<FieldAndType>> sameResponseShapeChecked = new HashSet<>();
313+
private final Set<Set<FieldAndType>> sameForCommonParentsChecked = new HashSet<>();
314+
private final Set<Set<Field>> conflictsReported = new HashSet<>();
316315

317316
// --- State: LoneAnonymousOperation ---
318317
private boolean hasAnonymousOp = false;
@@ -1169,7 +1168,7 @@ private void validateOverlappingFieldsCanBeMerged(OperationDefinition operationD
11691168

11701169
private void overlappingFieldsImpl(SelectionSet selectionSet, @Nullable GraphQLOutputType graphQLOutputType) {
11711170
Map<String, Set<FieldAndType>> fieldMap = new LinkedHashMap<>(selectionSet.getSelections().size());
1172-
Set<String> visitedFragments = new LinkedHashSet<>();
1171+
Set<String> visitedFragments = new HashSet<>();
11731172
overlappingFields_collectFields(fieldMap, selectionSet, graphQLOutputType, visitedFragments);
11741173
List<Conflict> conflicts = findConflicts(fieldMap);
11751174
for (Conflict conflict : conflicts) {
@@ -1198,10 +1197,9 @@ private void overlappingFields_collectFieldsForFragmentSpread(Map<String, Set<Fi
11981197
if (fragment == null) {
11991198
return;
12001199
}
1201-
if (visitedFragments.contains(fragment.getName())) {
1200+
if (!visitedFragments.add(fragment.getName())) {
12021201
return;
12031202
}
1204-
visitedFragments.add(fragment.getName());
12051203
GraphQLType graphQLType = TypeFromAST.getTypeFromAST(validationContext.getSchema(), fragment.getTypeCondition());
12061204
overlappingFields_collectFields(fieldMap, fragment.getSelectionSet(), graphQLType, visitedFragments);
12071205
}
@@ -1230,56 +1228,78 @@ private void overlappingFields_collectFieldsForField(Map<String, Set<FieldAndTyp
12301228

12311229
private List<Conflict> findConflicts(Map<String, Set<FieldAndType>> fieldMap) {
12321230
List<Conflict> result = new ArrayList<>();
1233-
sameResponseShapeByName(fieldMap, emptyList(), result);
1234-
sameForCommonParentsByName(fieldMap, emptyList(), result);
1231+
ArrayList<String> pathStack = new ArrayList<>();
1232+
sameResponseShapeByName(fieldMap, pathStack, result);
1233+
sameForCommonParentsByName(fieldMap, pathStack, result);
12351234
return result;
12361235
}
12371236

1238-
private void sameResponseShapeByName(Map<String, Set<FieldAndType>> fieldMap, ImmutableList<String> currentPath, List<Conflict> conflictsResult) {
1237+
private void sameResponseShapeByName(Map<String, Set<FieldAndType>> fieldMap, ArrayList<String> pathStack, List<Conflict> conflictsResult) {
12391238
for (Map.Entry<String, Set<FieldAndType>> entry : fieldMap.entrySet()) {
1240-
if (sameResponseShapeChecked.contains(entry.getValue())) {
1239+
Set<FieldAndType> fieldAndTypes = entry.getValue();
1240+
if (sameResponseShapeChecked.contains(fieldAndTypes)) {
12411241
continue;
12421242
}
1243-
ImmutableList<String> newPath = addToList(currentPath, entry.getKey());
1244-
sameResponseShapeChecked.add(entry.getValue());
1245-
Conflict conflict = requireSameOutputTypeShape(newPath, entry.getValue());
1246-
if (conflict != null) {
1247-
conflictsResult.add(conflict);
1248-
continue;
1243+
sameResponseShapeChecked.add(fieldAndTypes);
1244+
if (fieldAndTypes.size() > 1) {
1245+
pathStack.add(entry.getKey());
1246+
Conflict conflict = requireSameOutputTypeShape(pathStack, fieldAndTypes);
1247+
if (conflict != null) {
1248+
conflictsResult.add(conflict);
1249+
pathStack.remove(pathStack.size() - 1);
1250+
continue;
1251+
}
1252+
Map<String, Set<FieldAndType>> subSelections = mergeSubSelections(fieldAndTypes);
1253+
if (!subSelections.isEmpty()) {
1254+
sameResponseShapeByName(subSelections, pathStack, conflictsResult);
1255+
}
1256+
pathStack.remove(pathStack.size() - 1);
1257+
} else {
1258+
// Single field can't conflict with itself, but still recurse into sub-selections
1259+
Map<String, Set<FieldAndType>> subSelections = mergeSubSelections(fieldAndTypes);
1260+
if (!subSelections.isEmpty()) {
1261+
pathStack.add(entry.getKey());
1262+
sameResponseShapeByName(subSelections, pathStack, conflictsResult);
1263+
pathStack.remove(pathStack.size() - 1);
1264+
}
12491265
}
1250-
Map<String, Set<FieldAndType>> subSelections = mergeSubSelections(entry.getValue());
1251-
sameResponseShapeByName(subSelections, newPath, conflictsResult);
12521266
}
12531267
}
12541268

12551269
private Map<String, Set<FieldAndType>> mergeSubSelections(Set<FieldAndType> sameNameFields) {
12561270
Map<String, Set<FieldAndType>> fieldMap = new LinkedHashMap<>();
1271+
Set<String> visitedFragments = new HashSet<>();
12571272
for (FieldAndType fieldAndType : sameNameFields) {
12581273
if (fieldAndType.field.getSelectionSet() != null) {
1259-
Set<String> visitedFragments = new LinkedHashSet<>();
12601274
overlappingFields_collectFields(fieldMap, fieldAndType.field.getSelectionSet(), fieldAndType.graphQLType, visitedFragments);
12611275
}
12621276
}
12631277
return fieldMap;
12641278
}
12651279

1266-
private void sameForCommonParentsByName(Map<String, Set<FieldAndType>> fieldMap, ImmutableList<String> currentPath, List<Conflict> conflictsResult) {
1280+
private void sameForCommonParentsByName(Map<String, Set<FieldAndType>> fieldMap, ArrayList<String> pathStack, List<Conflict> conflictsResult) {
12671281
for (Map.Entry<String, Set<FieldAndType>> entry : fieldMap.entrySet()) {
1268-
List<Set<FieldAndType>> groups = groupByCommonParents(entry.getValue());
1269-
ImmutableList<String> newPath = addToList(currentPath, entry.getKey());
1282+
Set<FieldAndType> fieldAndTypes = entry.getValue();
1283+
List<Set<FieldAndType>> groups = groupByCommonParents(fieldAndTypes);
1284+
pathStack.add(entry.getKey());
12701285
for (Set<FieldAndType> group : groups) {
12711286
if (sameForCommonParentsChecked.contains(group)) {
12721287
continue;
12731288
}
12741289
sameForCommonParentsChecked.add(group);
1275-
Conflict conflict = requireSameNameAndArguments(newPath, group);
1276-
if (conflict != null) {
1277-
conflictsResult.add(conflict);
1278-
continue;
1290+
if (group.size() > 1) {
1291+
Conflict conflict = requireSameNameAndArguments(pathStack, group);
1292+
if (conflict != null) {
1293+
conflictsResult.add(conflict);
1294+
continue;
1295+
}
12791296
}
12801297
Map<String, Set<FieldAndType>> subSelections = mergeSubSelections(group);
1281-
sameForCommonParentsByName(subSelections, newPath, conflictsResult);
1298+
if (!subSelections.isEmpty()) {
1299+
sameForCommonParentsByName(subSelections, pathStack, conflictsResult);
1300+
}
12821301
}
1302+
pathStack.remove(pathStack.size() - 1);
12831303
}
12841304
}
12851305

@@ -1324,7 +1344,7 @@ private boolean isInterfaceOrUnion(@Nullable GraphQLType type) {
13241344
return type instanceof GraphQLInterfaceType || type instanceof GraphQLUnionType;
13251345
}
13261346

1327-
private @Nullable Conflict requireSameNameAndArguments(ImmutableList<String> path, Set<FieldAndType> fieldAndTypes) {
1347+
private @Nullable Conflict requireSameNameAndArguments(List<String> path, Set<FieldAndType> fieldAndTypes) {
13281348
if (fieldAndTypes.size() <= 1) {
13291349
return null;
13301350
}
@@ -1351,7 +1371,7 @@ private boolean isInterfaceOrUnion(@Nullable GraphQLType type) {
13511371
return null;
13521372
}
13531373

1354-
private String pathToString(ImmutableList<String> path) {
1374+
private String pathToString(List<String> path) {
13551375
return String.join("/", path);
13561376
}
13571377

@@ -1380,7 +1400,7 @@ private boolean sameArguments(List<Argument> arguments1, @Nullable List<Argument
13801400
return null;
13811401
}
13821402

1383-
private @Nullable Conflict requireSameOutputTypeShape(ImmutableList<String> path, Set<FieldAndType> fieldAndTypes) {
1403+
private @Nullable Conflict requireSameOutputTypeShape(List<String> path, Set<FieldAndType> fieldAndTypes) {
13841404
if (fieldAndTypes.size() <= 1) {
13851405
return null;
13861406
}
@@ -1430,7 +1450,7 @@ private boolean sameArguments(List<Argument> arguments1, @Nullable List<Argument
14301450
return null;
14311451
}
14321452

1433-
private Conflict mkNotSameTypeError(ImmutableList<String> path, List<Field> fields, @Nullable GraphQLType typeA, @Nullable GraphQLType typeB) {
1453+
private Conflict mkNotSameTypeError(List<String> path, List<Field> fields, @Nullable GraphQLType typeA, @Nullable GraphQLType typeB) {
14341454
String name1 = typeA != null ? simplePrint(typeA) : "null";
14351455
String name2 = typeB != null ? simplePrint(typeB) : "null";
14361456
String reason = i18n(FieldsConflict, "OverlappingFieldsCanBeMerged.differentReturnTypes", pathToString(path), name1, name2);

0 commit comments

Comments
 (0)