Skip to content

Commit

Permalink
Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transf…
Browse files Browse the repository at this point in the history 
…orms (#2906)

* add a test to reproduce GraphQLTypeReference issue in transforms

* Added extra test for schema transform

* Fixes the bug where dangling actual references are not used during schema rebuild

* Extra test to ensure that field referenced types are still ok

* Got carried away and got too precise.   A changed type should not be referenced

* Updated to include other types that have replaceable types

* Added non null and list wrapper tests

Co-authored-by: Marc-Andre Giroux <mgiroux@netflix.com>
  • Loading branch information
@bbakerman
bbakerman and Marc-Andre Giroux committed Aug 11, 2022
1 parent faf6337 commit fb507ad
Show file tree
Hide file tree
Showing 4 changed files with 310 additions and 25 deletions.
18 changes: 11 additions & 7 deletions src/main/java/graphql/schema/GraphQLTypeUtil.java
View file
Expand Up @@ -196,12 +196,7 @@ public static <T extends GraphQLType> T unwrapOneAs(GraphQLType type) {
* @return the underlying type
*/
public static GraphQLUnmodifiedType unwrapAll(GraphQLType type) {
while (true) {
if (isNotWrapped(type)) {
return (GraphQLUnmodifiedType) type;
}
type = unwrapOne(type);
}
return unwrapAllAs(type);
}

/**
Expand All @@ -215,7 +210,16 @@ public static GraphQLUnmodifiedType unwrapAll(GraphQLType type) {
*/
public static <T extends GraphQLType> T unwrapAllAs(GraphQLType type) {
//noinspection unchecked
return (T) unwrapAll(type);
return (T) unwrapAllImpl(type);
}

private static GraphQLType unwrapAllImpl(GraphQLType type) {
while (true) {
if (isNotWrapped(type)) {
return type;
}
type = unwrapOne(type);
}
}


Expand Down
93 changes: 76 additions & 17 deletions src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java
View file
Expand Up @@ -3,11 +3,16 @@
import com.google.common.collect.ImmutableMap;
import graphql.AssertException;
import graphql.Internal;
import graphql.schema.GraphQLAppliedDirectiveArgument;
import graphql.schema.GraphQLArgument;
import graphql.schema.GraphQLEnumType;
import graphql.schema.GraphQLFieldDefinition;
import graphql.schema.GraphQLInputObjectField;
import graphql.schema.GraphQLInputObjectType;
import graphql.schema.GraphQLInterfaceType;
import graphql.schema.GraphQLList;
import graphql.schema.GraphQLNamedType;
import graphql.schema.GraphQLNonNull;
import graphql.schema.GraphQLObjectType;
import graphql.schema.GraphQLScalarType;
import graphql.schema.GraphQLSchemaElement;
Expand All @@ -21,13 +26,17 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.TreeMap;
import java.util.function.Supplier;

import static graphql.schema.GraphQLTypeUtil.unwrapAllAs;
import static graphql.util.TraversalControl.CONTINUE;
import static java.lang.String.format;

@Internal
public class GraphQLTypeCollectingVisitor extends GraphQLTypeVisitorStub {

private final Map<String, GraphQLNamedType> result = new LinkedHashMap<>();
private final Map<String, GraphQLNamedType> indirectStrongReferences = new LinkedHashMap<>();

public GraphQLTypeCollectingVisitor() {
}
Expand All @@ -36,14 +45,14 @@ public GraphQLTypeCollectingVisitor() {
public TraversalControl visitGraphQLEnumType(GraphQLEnumType node, TraverserContext<GraphQLSchemaElement> context) {
assertTypeUniqueness(node, result);
save(node.getName(), node);
return super.visitGraphQLEnumType(node, context);
return CONTINUE;
}

@Override
public TraversalControl visitGraphQLScalarType(GraphQLScalarType node, TraverserContext<GraphQLSchemaElement> context) {
assertTypeUniqueness(node, result);
save(node.getName(), node);
return super.visitGraphQLScalarType(node, context);
return CONTINUE;
}

@Override
Expand All @@ -53,7 +62,7 @@ public TraversalControl visitGraphQLObjectType(GraphQLObjectType node, Traverser
} else {
save(node.getName(), node);
}
return super.visitGraphQLObjectType(node, context);
return CONTINUE;
}

@Override
Expand All @@ -63,7 +72,7 @@ public TraversalControl visitGraphQLInputObjectType(GraphQLInputObjectType node,
} else {
save(node.getName(), node);
}
return super.visitGraphQLInputObjectType(node, context);
return CONTINUE;
}

@Override
Expand All @@ -73,23 +82,48 @@ public TraversalControl visitGraphQLInterfaceType(GraphQLInterfaceType node, Tra
} else {
save(node.getName(), node);
}

return super.visitGraphQLInterfaceType(node, context);
return CONTINUE;
}

@Override
public TraversalControl visitGraphQLUnionType(GraphQLUnionType node, TraverserContext<GraphQLSchemaElement> context) {
assertTypeUniqueness(node, result);
save(node.getName(), node);
return super.visitGraphQLUnionType(node, context);
return CONTINUE;
}

@Override
public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext<GraphQLSchemaElement> context) {
saveIndirectStrongReference(node::getType);
return CONTINUE;
}

@Override
public TraversalControl visitGraphQLInputObjectField(GraphQLInputObjectField node, TraverserContext<GraphQLSchemaElement> context) {
saveIndirectStrongReference(node::getType);
return CONTINUE;
}

return super.visitGraphQLFieldDefinition(node, context);
@Override
public TraversalControl visitGraphQLArgument(GraphQLArgument node, TraverserContext<GraphQLSchemaElement> context) {
saveIndirectStrongReference(node::getType);
return CONTINUE;
}

@Override
public TraversalControl visitGraphQLAppliedDirectiveArgument(GraphQLAppliedDirectiveArgument node, TraverserContext<GraphQLSchemaElement> context) {
saveIndirectStrongReference(node::getType);
return CONTINUE;
}

private <T> void saveIndirectStrongReference(Supplier<GraphQLType> typeSupplier) {
GraphQLNamedType type = unwrapAllAs(typeSupplier.get());
if (!(type instanceof GraphQLTypeReference)) {
indirectStrongReferences.put(type.getName(), type);
}
}


private boolean isNotTypeReference(String name) {
return result.containsKey(name) && !(result.get(name) instanceof GraphQLTypeReference);
}
Expand All @@ -112,18 +146,43 @@ private void assertTypeUniqueness(GraphQLNamedType type, Map<String, GraphQLName
// do we have an existing definition
if (existingType != null) {
// type references are ok
if (!(existingType instanceof GraphQLTypeReference || type instanceof GraphQLTypeReference))
// object comparison here is deliberate
if (existingType != type) {
throw new AssertException(format("All types within a GraphQL schema must have unique names. No two provided types may have the same name.\n" +
"No provided type may have a name which conflicts with any built in types (including Scalar and Introspection types).\n" +
"You have redefined the type '%s' from being a '%s' to a '%s'",
type.getName(), existingType.getClass().getSimpleName(), type.getClass().getSimpleName()));
}
if (!(existingType instanceof GraphQLTypeReference || type instanceof GraphQLTypeReference)) {
assertUniqueTypeObjects(type, existingType);
}
}
}

private void assertUniqueTypeObjects(GraphQLNamedType type, GraphQLType existingType) {
// object comparison here is deliberate
if (existingType != type) {
throw new AssertException(format("All types within a GraphQL schema must have unique names. No two provided types may have the same name.\n" +
"No provided type may have a name which conflicts with any built in types (including Scalar and Introspection types).\n" +
"You have redefined the type '%s' from being a '%s' to a '%s'",
type.getName(), existingType.getClass().getSimpleName(), type.getClass().getSimpleName()));
}
}

public ImmutableMap<String, GraphQLNamedType> getResult() {
return ImmutableMap.copyOf(new TreeMap<>(result));
Map<String, GraphQLNamedType> types = new TreeMap<>(fixDanglingReplacedTypes(result));
return ImmutableMap.copyOf(types);
}

/**
* It's possible for certain schema edits to create a situation where a field / arg / input field had a type reference, then
* it got replaced with an actual strong reference and then the schema gets edited such that the only reference
* to that type is the replaced strong reference. This edge case means that the replaced reference can be
* missed if it's the only way to get to that type because this visitor asks for the children as original type,
* e.g. the type reference and not the replaced reference.
*
* @param visitedTypes the types collected by this visitor
*
* @return a fixed up map where the only
*/
private Map<String, GraphQLNamedType> fixDanglingReplacedTypes(Map<String, GraphQLNamedType> visitedTypes) {
for (GraphQLNamedType indirectStrongReference : indirectStrongReferences.values()) {
String typeName = indirectStrongReference.getName();
visitedTypes.putIfAbsent(typeName, indirectStrongReference);
}
return visitedTypes;
}
}

0 comments on commit fb507ad

Please sign in to comment.