Skip to content

Commit

Permalink
Correctly report skipped concrete union/interface types
Browse files Browse the repository at this point in the history 
Closes gh-962
  • Loading branch information
@rstoyanchev
rstoyanchev committed May 3, 2024
1 parent 9bcd824 commit dceb3af
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 24 deletions.
53 changes: 41 additions & 12 deletions ...g-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,14 +220,15 @@ else if (typePair.outputType() instanceof GraphQLInterfaceType interfaceType) {
// Can we inspect GraphQL type?
if (!(graphQlType instanceof GraphQLFieldsContainer fieldContainer)) {
if (isNotScalarOrEnumType(graphQlType)) {
this.reportBuilder.skippedType(graphQlType, parent, field, "Unsupported schema type");
this.reportBuilder.skippedType(graphQlType, parent, field, "Unsupported schema type", false);
}
continue;
}

// Can we inspect the Class?
if (currentResolvableType.resolve(Object.class) == Object.class) {
this.reportBuilder.skippedType(graphQlType, parent, field, "No class information");
boolean isDerived = !graphQlType.equals(typePair.outputType());
this.reportBuilder.skippedType(graphQlType, parent, field, "No class information", isDerived);
continue;
}

Expand Down Expand Up @@ -723,7 +724,9 @@ private final class ReportBuilder {

private final MultiValueMap<DataFetcher<?>, String> unmappedArguments = new LinkedMultiValueMap<>();

private final List<SchemaReport.SkippedType> skippedTypes = new ArrayList<>();
private final List<DefaultSkippedType> skippedTypes = new ArrayList<>();

private final List<DefaultSkippedType> candidateSkippedTypes = new ArrayList<>();

void unmappedField(FieldCoordinates coordinates) {
this.unmappedFields.add(coordinates);
Expand All @@ -737,19 +740,44 @@ void unmappedArgument(DataFetcher<?> dataFetcher, List<String> arguments) {
this.unmappedArguments.put(dataFetcher, arguments);
}

void skippedType(GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field, String reason) {
DefaultSkippedType skippedType = DefaultSkippedType.create(type, parent, field);
void skippedType(
GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field,
String reason, boolean isDerivedType) {

DefaultSkippedType skippedType = DefaultSkippedType.create(type, parent, field, reason);

if (!isDerivedType) {
skippedType(skippedType);
return;
}

// Keep skipped union member or interface implementing types aside to the end.
// Use of concrete types elsewhere may provide more information.

this.candidateSkippedTypes.add(skippedType);
}

private void skippedType(DefaultSkippedType skippedType) {
if (logger.isDebugEnabled()) {
logger.debug("Skipping '" + skippedType + "': " + reason);
logger.debug("Skipping '" + skippedType + "': " + skippedType.reason());
}
this.skippedTypes.add(skippedType);
}

SchemaReport build() {

this.candidateSkippedTypes.forEach((skippedType) -> {
if (skippedType.type() instanceof GraphQLFieldsContainer fieldsContainer) {
if (SchemaMappingInspector.this.inspectedTypes.contains(fieldsContainer.getName())) {
return;
}
}
skippedType(skippedType);
});

return new DefaultSchemaReport(
this.unmappedFields, this.unmappedRegistrations, this.unmappedArguments, this.skippedTypes);
}

}


Expand All @@ -768,7 +796,7 @@ private final class DefaultSchemaReport implements SchemaReport {

DefaultSchemaReport(
List<FieldCoordinates> unmappedFields, Map<FieldCoordinates, DataFetcher<?>> unmappedRegistrations,
MultiValueMap<DataFetcher<?>, String> unmappedArguments, List<SkippedType> skippedTypes) {
MultiValueMap<DataFetcher<?>, String> unmappedArguments, List<DefaultSkippedType> skippedTypes) {

this.unmappedFields = Collections.unmodifiableList(unmappedFields);
this.unmappedRegistrations = Collections.unmodifiableMap(unmappedRegistrations);
Expand Down Expand Up @@ -834,17 +862,18 @@ private String formatUnmappedFields() {
* Default implementation of a {@link SchemaReport.SkippedType}.
*/
private record DefaultSkippedType(
GraphQLType type, FieldCoordinates fieldCoordinates) implements SchemaReport.SkippedType {
GraphQLType type, FieldCoordinates fieldCoordinates, String reason)
implements SchemaReport.SkippedType {

@Override
public String toString() {
return (type instanceof GraphQLNamedType namedType) ? namedType.getName() : type.toString();
return (this.type instanceof GraphQLNamedType named) ? named.getName() : this.type.toString();
}

public static DefaultSkippedType create(
GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field) {
GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field, String reason) {

return new DefaultSkippedType(type, FieldCoordinates.coordinates(parent, field));
return new DefaultSkippedType(type, FieldCoordinates.coordinates(parent, field), reason);
}
}

Expand Down
73 changes: 61 additions & 12 deletions ...src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorUnionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

import org.springframework.graphql.data.method.annotation.Argument;
import org.springframework.graphql.data.method.annotation.QueryMapping;
import org.springframework.graphql.execution.SchemaMappingInspector.ClassResolver;
import org.springframework.stereotype.Controller;
Expand All @@ -35,16 +36,20 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest

private static final String schema = """
type Query {
search: [SearchResult!]!
search: [SearchResult]
article(id: ID): Article
}
union SearchResult = Photo | Video
union SearchResult = Article | Photo | Video
type Photo {
height: Int
width: Int
}
type Video {
title: String
}
type Article {
content: String
}
""";


Expand All @@ -55,8 +60,10 @@ class UnmappedFields {
void reportUnmappedFieldsByCheckingReturnTypePackage() {
SchemaReport report = inspectSchema(schema, SearchController.class);
assertThatReport(report)
.hasSkippedTypeCount(0)
.hasUnmappedFieldCount(3)
.hasSkippedTypeCount(1)
.containsSkippedTypes("Article")
.hasUnmappedFieldCount(4)
.containsUnmappedFields("Query", "article")
.containsUnmappedFields("Photo", "height", "width")
.containsUnmappedFields("Video", "title");
}
Expand All @@ -65,17 +72,20 @@ void reportUnmappedFieldsByCheckingReturnTypePackage() {
void reportUnmappedFieldsByCheckingControllerTypePackage() {
SchemaReport report = inspectSchema(schema, ObjectSearchController.class);
assertThatReport(report)
.hasSkippedTypeCount(0)
.hasUnmappedFieldCount(3)
.hasSkippedTypeCount(1)
.containsSkippedTypes("Article")
.hasUnmappedFieldCount(4)
.containsUnmappedFields("Query", "article")
.containsUnmappedFields("Photo", "height", "width")
.containsUnmappedFields("Video", "title");
}


sealed interface ResultItem permits Photo, Video { }
record Photo() implements ResultItem { }
record Video() implements ResultItem { }

sealed interface ResultItem permits Photo, Video { }

@Controller
static class SearchController {

Expand Down Expand Up @@ -108,8 +118,10 @@ void classNameFunction() {
SearchController.class);

assertThatReport(report)
.hasSkippedTypeCount(0)
.hasUnmappedFieldCount(3)
.hasSkippedTypeCount(1)
.containsSkippedTypes("Article")
.hasUnmappedFieldCount(4)
.containsUnmappedFields("Query", "article")
.containsUnmappedFields("Photo", "height", "width")
.containsUnmappedFields("Video", "title");
}
Expand All @@ -124,8 +136,11 @@ void classNameTypeResolver() {
SearchController.class);

assertThatReport(report)
.hasUnmappedFieldCount(2).containsUnmappedFields("Photo", "height", "width")
.hasSkippedTypeCount(1).containsSkippedTypes("Video");
.hasSkippedTypeCount(2)
.containsSkippedTypes("Article", "Video")
.hasUnmappedFieldCount(3)
.containsUnmappedFields("Query", "article")
.containsUnmappedFields("Photo", "height", "width");
}

sealed interface ResultItem permits PhotoImpl, VideoImpl { }
Expand All @@ -149,7 +164,7 @@ class SkippedTypes {
@Test
void reportSkippedImplementations() {
SchemaReport report = inspectSchema(schema, SearchController.class);
assertThatReport(report).hasSkippedTypeCount(2).containsSkippedTypes("Photo", "Video");
assertThatReport(report).hasSkippedTypeCount(3).containsSkippedTypes("Article", "Photo", "Video");
}

interface ResultItem { }
Expand All @@ -164,4 +179,38 @@ List<ResultItem> search() {
}
}


@Nested
class CandidateSkippedTypes {

// A union member type is only a candidate to be skipped until the inspection is done.
// Use of the concrete type elsewhere may provide more information.

@Test
void candidateNotSkippedIfConcreteUseElsewhere() {
SchemaReport report = inspectSchema(schema, SearchController.class);
assertThatReport(report).hasSkippedTypeCount(2).containsSkippedTypes("Photo", "Video");
}

@Controller
static class SearchController {

@QueryMapping
List<Object> search() {
throw new UnsupportedOperationException();
}

@QueryMapping
Article article(@Argument Long id) {
throw new UnsupportedOperationException();
}
}
}


/**
* Declared outside {@link CandidateSkippedTypes}, so the union lookup won't find it.
*/
private record Article(String content) { }

}

0 comments on commit dceb3af

Please sign in to comment.