Refine nullness checks in the schema report inspection

Prior to this commit, gh-1220 introduced nullness checks in the schema
mapping inspection process. This generally reports all mismatches
between the GraphQL schema nullness information and the corresponding
Java type nullness.

This commit refines the implementation with the following changes:

If an application type is "non-null" and the GraphQL schema type is
"nullable", we should not report that as a problem. Partial responses
is a key GraphQL feature and we should not force developers to make
fields "non-null" just because applications types are "non-null".
If a data fetching error happens, the engine will null out fields in the
hierarchy up until null is a valid value.

The method names and report information now use "errors" instead of
"mismatches" as a consequence.

Closes gh-1344

Author: bclozel

spring-graphql-docs/modules/ROOT/pages/request-execution.adoc

@@ -261,13 +261,18 @@ to inspect schema mappings on startup to ensure the following:
 
 If the application is written in Kotlin, or is using {spring-framework-ref-docs}/core/null-safety.html[Null-safety]
 annotations, further inspections can be performed. The GraphQL schema can declare nullable types (`Book`) and
-non-nullable types (`Book!`). We can ensure that both the schema and the application are in sync when it comes
-to nullability information:
-
-- For schema fields, we can check that the relevant `Class` properties and `DataFetcher` return types with the same
-  nullability.
-- For field arguments, we can ensure that `DataFetcher` parameters have the same nullability
-
+non-nullable types (`Book!`).
+As a result, we can ensure that the application does not break the nullness requirements of the schema.
+
+- When schema fields are non-null, we ensure that the relevant `Class` properties and `DataFetcher` return types
+  are also non-null.
+- When field arguments are non-null, we ensure that `DataFetcher` parameters are also non-null.
+
+The opposite situation is not considered as an error: when the schema has a nullable field `author: Author`
+and the application declares a `@NonNull Author getAuthor();`, the inspector does not raise this as an error.
+Applications should not necessarily make fields non-null in the schema, as any error during a data fetching operation
+will force the GraphQL engine tu null out fields in the hierarchy up until `null` is allowed.
+Partial responses is a key GraphQL feature, so the schema should be designed with nullness in mind.
 
 To enable schema inspection, customize `GraphQlSource.Builder` as shown below.
 In this case the report is simply logged, but you can choose to take any action:

spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java

@@ -78,9 +78,10 @@
  * corresponding Class property.
  * <li>{@code DataFetcher} registrations refer to a schema field that exists.
  * <li>{@code DataFetcher} arguments have matching schema field arguments.
- * <li>The nullness of schema fields matches the nullness of {@link DataFetcher}
- * return types, class properties or class methods.
- * <li>{@code DataFetcher} arguments match the nullness of schema argument types.
+ * <li>The nullness of {@link DataFetcher} return types, class properties or class methods
+ * match, or is more restrictive than, the nullness of schema fields.
+ * <li>The nullness of {@code DataFetcher} arguments match, or is more restrictive than,
+ * the nullness of schema argument types.
  * </ul>
  *
  * <p>Use methods of {@link GraphQlSource.SchemaResourceBuilder} to enable schema
@@ -223,11 +224,11 @@ private void checkFieldsContainer(
 
 	private void checkFieldNullNess(FieldCoordinates fieldCoordinates, Field javaField, Nullness schemaNullness) {
 		Nullness applicationNullness = Nullness.forField(javaField);
-		if (isMismatch(schemaNullness, applicationNullness)) {
+		if (isNullnessError(schemaNullness, applicationNullness)) {
 			DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(javaField,
 					javaField.getDeclaringClass().getSimpleName() + "#" + javaField.getName());
-			this.reportBuilder.fieldNullnessMismatch(fieldCoordinates,
-					new DefaultNullnessMismatch(schemaNullness, applicationNullness, annotatedElement));
+			this.reportBuilder.fieldNullnessError(fieldCoordinates,
+					new DefaultNullnessError(schemaNullness, applicationNullness, annotatedElement));
 
 		}
 	}
@@ -246,10 +247,10 @@ else if (dataFetcher.usesDataLoader() && Map.class.isAssignableFrom(dataFetcherM
 				// we cannot infer nullness if batch loader method returns a Map
 				logger.debug("Skip nullness check for data fetcher '" + dataFetcherMethod.getName() + "' because of batch loading.");
 			}
-			else if (isMismatch(schemaNullness, applicationNullness)) {
+			else if (isNullnessError(schemaNullness, applicationNullness)) {
 				DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(dataFetcherMethod, dataFetcher.getDescription());
-				this.reportBuilder.fieldNullnessMismatch(fieldCoordinates,
-						new DefaultNullnessMismatch(schemaNullness, applicationNullness, annotatedElement));
+				this.reportBuilder.fieldNullnessError(fieldCoordinates,
+						new DefaultNullnessError(schemaNullness, applicationNullness, annotatedElement));
 
 			}
 		}
@@ -270,30 +271,30 @@ private void checkFieldArguments(GraphQLFieldDefinition field, SelfDescribingDat
 	private void checkFieldArgumentsNullness(GraphQLFieldDefinition field, SelfDescribingDataFetcher<?> dataFetcher) {
 		Method dataFetcherMethod = dataFetcher.asMethod();
 		if (dataFetcherMethod != null) {
-			List<SchemaReport.NullnessMismatch> mismatches = new ArrayList<>();
+			List<SchemaReport.NullnessError> nullnessErrors = new ArrayList<>();
 			for (Parameter parameter : dataFetcherMethod.getParameters()) {
 				GraphQLArgument argument = field.getArgument(parameter.getName());
-				if (argument != null) {
+				if (argument != null && argument.getDefinition() != null) {
 					Nullness schemaNullness = resolveNullness(argument.getDefinition().getType());
 					Nullness applicationNullness = Nullness.forMethodParameter(MethodParameter.forParameter(parameter));
-					if (isMismatch(schemaNullness, applicationNullness)) {
-						mismatches.add(new DefaultNullnessMismatch(schemaNullness, applicationNullness, parameter));
+					if (isNullnessError(schemaNullness, applicationNullness)) {
+						nullnessErrors.add(new DefaultNullnessError(schemaNullness, applicationNullness, parameter));
 					}
 				}
 			}
-			if (!mismatches.isEmpty()) {
-				this.reportBuilder.argumentsNullnessMismatches(dataFetcher, mismatches);
+			if (!nullnessErrors.isEmpty()) {
+				this.reportBuilder.argumentsNullnessErrors(dataFetcher, nullnessErrors);
 			}
 		}
 	}
 
 	private void checkReadMethodNullness(FieldCoordinates fieldCoordinates, ResolvableType resolvableType, PropertyDescriptor descriptor, Nullness schemaNullness) {
 		Nullness applicationNullness = Nullness.forMethodReturnType(descriptor.getReadMethod());
-		if (isMismatch(schemaNullness, applicationNullness)) {
+		if (isNullnessError(schemaNullness, applicationNullness)) {
 			DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(descriptor.getReadMethod(),
 					resolvableType.toClass().getSimpleName() + "#" + descriptor.getName());
-			this.reportBuilder.fieldNullnessMismatch(fieldCoordinates,
-					new DefaultNullnessMismatch(schemaNullness, applicationNullness, annotatedElement));
+			this.reportBuilder.fieldNullnessError(fieldCoordinates,
+					new DefaultNullnessError(schemaNullness, applicationNullness, annotatedElement));
 		}
 	}
 
@@ -408,12 +409,11 @@ private Nullness resolveNullness(Type type) {
 		if (type instanceof NonNullType) {
 			return Nullness.NON_NULL;
 		}
-		return  Nullness.NULLABLE;
+		return Nullness.NULLABLE;
 	}
 
-	private boolean isMismatch(Nullness first, Nullness second) {
-		return (first == Nullness.NON_NULL && second == Nullness.NULLABLE) ||
-				(first == Nullness.NULLABLE && second == Nullness.NON_NULL);
+	private boolean isNullnessError(Nullness schemaNullness, Nullness applicationNullness) {
+		return (schemaNullness == Nullness.NON_NULL && applicationNullness == Nullness.NULLABLE);
 	}
 
 
@@ -909,9 +909,9 @@ private final class ReportBuilder {
 
 		private final MultiValueMap<DataFetcher<?>, String> unmappedArguments = new LinkedMultiValueMap<>();
 
-		private final Map<FieldCoordinates, SchemaReport.NullnessMismatch> fieldNullnessMismatches = new LinkedHashMap<>();
+		private final Map<FieldCoordinates, SchemaReport.NullnessError> fieldNullnessErrors = new LinkedHashMap<>();
 
-		private final MultiValueMap<DataFetcher<?>, SchemaReport.NullnessMismatch> argumentsNullnessMismatches = new LinkedMultiValueMap<>();
+		private final MultiValueMap<DataFetcher<?>, SchemaReport.NullnessError> argumentsNullnessErrors = new LinkedMultiValueMap<>();
 
 		private final List<DefaultSkippedType> skippedTypes = new ArrayList<>();
 
@@ -929,12 +929,12 @@ void unmappedArgument(DataFetcher<?> dataFetcher, List<String> arguments) {
 			this.unmappedArguments.put(dataFetcher, arguments);
 		}
 
-		void fieldNullnessMismatch(FieldCoordinates coordinates, SchemaReport.NullnessMismatch nullnessMismatch) {
-			this.fieldNullnessMismatches.put(coordinates, nullnessMismatch);
+		void fieldNullnessError(FieldCoordinates coordinates, SchemaReport.NullnessError nullnessError) {
+			this.fieldNullnessErrors.put(coordinates, nullnessError);
 		}
 
-		void argumentsNullnessMismatches(DataFetcher<?> dataFetcher, List<SchemaReport.NullnessMismatch> nullnessMismatches) {
-			this.argumentsNullnessMismatches.put(dataFetcher, nullnessMismatches);
+		void argumentsNullnessErrors(DataFetcher<?> dataFetcher, List<SchemaReport.NullnessError> nullnessErrors) {
+			this.argumentsNullnessErrors.put(dataFetcher, nullnessErrors);
 		}
 
 		void skippedType(
@@ -973,7 +973,7 @@ SchemaReport build() {
 			});
 
 			return new DefaultSchemaReport(this.unmappedFields, this.unmappedRegistrations,
-					this.unmappedArguments, this.fieldNullnessMismatches, this.argumentsNullnessMismatches, this.skippedTypes);
+					this.unmappedArguments, this.fieldNullnessErrors, this.argumentsNullnessErrors, this.skippedTypes);
 		}
 	}
 
@@ -989,24 +989,24 @@ private final class DefaultSchemaReport implements SchemaReport {
 
 		private final MultiValueMap<DataFetcher<?>, String> unmappedArguments;
 
-		private final Map<FieldCoordinates, NullnessMismatch> fieldsNullnessMismatches;
+		private final Map<FieldCoordinates, NullnessError> fieldNullnessErrors;
 
-		private final MultiValueMap<DataFetcher<?>, NullnessMismatch> argumentsNullnessMismatches;
+		private final MultiValueMap<DataFetcher<?>, NullnessError> argumentNullnessErrors;
 
 		private final List<SchemaReport.SkippedType> skippedTypes;
 
 		DefaultSchemaReport(
 				List<FieldCoordinates> unmappedFields, Map<FieldCoordinates, DataFetcher<?>> unmappedRegistrations,
 				MultiValueMap<DataFetcher<?>, String> unmappedArguments,
-				Map<FieldCoordinates, NullnessMismatch> fieldsNullnessMismatches,
-				MultiValueMap<DataFetcher<?>, NullnessMismatch> argumentsNullnessMismatches,
+				Map<FieldCoordinates, NullnessError> fieldNullnessErrors,
+				MultiValueMap<DataFetcher<?>, NullnessError> argumentNullnessErrors,
 				List<DefaultSkippedType> skippedTypes) {
 
 			this.unmappedFields = Collections.unmodifiableList(unmappedFields);
 			this.unmappedRegistrations = Collections.unmodifiableMap(unmappedRegistrations);
 			this.unmappedArguments = CollectionUtils.unmodifiableMultiValueMap(unmappedArguments);
-			this.fieldsNullnessMismatches = Collections.unmodifiableMap(fieldsNullnessMismatches);
-			this.argumentsNullnessMismatches = CollectionUtils.unmodifiableMultiValueMap(argumentsNullnessMismatches);
+			this.fieldNullnessErrors = Collections.unmodifiableMap(fieldNullnessErrors);
+			this.argumentNullnessErrors = CollectionUtils.unmodifiableMultiValueMap(argumentNullnessErrors);
 			this.skippedTypes = Collections.unmodifiableList(skippedTypes);
 		}
 
@@ -1026,13 +1026,13 @@ public MultiValueMap<DataFetcher<?>, String> unmappedArguments() {
 		}
 
 		@Override
-		public Map<FieldCoordinates, NullnessMismatch> fieldsNullnessMismatches() {
-			return this.fieldsNullnessMismatches;
+		public Map<FieldCoordinates, NullnessError> fieldNullnessErrors() {
+			return this.fieldNullnessErrors;
 		}
 
 		@Override
-		public MultiValueMap<DataFetcher<?>, NullnessMismatch> argumentsNullnessMismatches() {
-			return this.argumentsNullnessMismatches;
+		public MultiValueMap<DataFetcher<?>, NullnessError> argumentNullnessErrors() {
+			return this.argumentNullnessErrors;
 		}
 
 		@Override
@@ -1058,8 +1058,8 @@ public String toString() {
 					"\tUnmapped fields: " + formatUnmappedFields() + "\n" +
 					"\tUnmapped registrations: " + this.unmappedRegistrations + "\n" +
 					"\tUnmapped arguments: " + this.unmappedArguments + "\n" +
-					"\tFields nullness mismatches: " + formatFieldsNullnessMismatches() + "\n" +
-					"\tArguments nullness mismatches: " + formatArgumentsNullnessMismatches() + "\n" +
+					"\tField nullness errors: " + formatFieldNullnessErrors() + "\n" +
+					"\tArgument nullness errors: " + formatArgumentNullnessErrors() + "\n" +
 					"\tSkipped types: " + this.skippedTypes;
 		}
 
@@ -1072,21 +1072,21 @@ private String formatUnmappedFields() {
 			return map.toString();
 		}
 
-		private String formatFieldsNullnessMismatches() {
+		private String formatFieldNullnessErrors() {
 			MultiValueMap<String, String> map = new LinkedMultiValueMap<>();
-			this.fieldsNullnessMismatches.forEach((coordinates, mismatch) -> {
+			this.fieldNullnessErrors.forEach((coordinates, nullnessError) -> {
 				List<String> fields = map.computeIfAbsent(coordinates.getTypeName(), (s) -> new ArrayList<>());
-				fields.add(String.format("%s is %s -> '%s' is %s", coordinates.getFieldName(), mismatch.schemaNullness(),
-						mismatch.annotatedElement(), mismatch.applicationNullness()));
+				fields.add(String.format("%s is %s -> '%s' is %s", coordinates.getFieldName(), nullnessError.schemaNullness(),
+						nullnessError.annotatedElement(), nullnessError.applicationNullness()));
 			});
 			return map.toString();
 		}
 
-		private String formatArgumentsNullnessMismatches() {
+		private String formatArgumentNullnessErrors() {
 			MultiValueMap<String, String> map = new LinkedMultiValueMap<>();
-			this.argumentsNullnessMismatches.forEach((dataFetcher, mismatches) -> {
-				List<String> arguments = mismatches.stream()
-						.map((mismatch) -> String.format("%s should be %s", mismatch.annotatedElement(), mismatch.schemaNullness()))
+			this.argumentNullnessErrors.forEach((dataFetcher, nullnessErrors) -> {
+				List<String> arguments = nullnessErrors.stream()
+						.map((nullnessError) -> String.format("%s should be %s", nullnessError.annotatedElement(), nullnessError.schemaNullness()))
 						.toList();
 				map.put(dataFetcher.toString(), arguments);
 			});
@@ -1116,11 +1116,11 @@ public static DefaultSkippedType create(
 	}
 
 	/**
-	 * Default implementation of a {@link SchemaReport.NullnessMismatch}.
+	 * Default implementation of a {@link SchemaReport.NullnessError}.
 	 */
-	private record DefaultNullnessMismatch(
+	private record DefaultNullnessError(
 			Nullness schemaNullness, Nullness applicationNullness, AnnotatedElement annotatedElement)
-			implements SchemaReport.NullnessMismatch {
+			implements SchemaReport.NullnessError {
 
 	}
 

spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaReport.java

@@ -72,18 +72,19 @@ public interface SchemaReport {
 	MultiValueMap<DataFetcher<?>, String> unmappedArguments();
 
 	/**
-	 * Return the coordinates for mismatches when a schema field type
-	 * nullness information does not match the nullness of the
-	 * corresponding {@link AnnotatedElement} in the application.
+	 * Return the coordinates for nullness errors between
+	 * schema fields and the corresponding {@link AnnotatedElement} in the application.
+	 * @see NullnessError
 	 */
-	Map<FieldCoordinates, NullnessMismatch> fieldsNullnessMismatches();
+	Map<FieldCoordinates, NullnessError> fieldNullnessErrors();
 
 	/**
 	 * Return a map with {@link DataFetcher}s and information for its arguments
-	 * if the schema nullness does not match the nullness of the
+	 * if there is a nullness error between the schema arguments and the
 	 * corresponding {@link AnnotatedElement} in the application.
+	 * @see NullnessError
 	 */
-	MultiValueMap<DataFetcher<?>, NullnessMismatch> argumentsNullnessMismatches();
+	MultiValueMap<DataFetcher<?>, NullnessError> argumentNullnessErrors();
 
 	/**
 	 * Return types skipped during the inspection, either because the schema type
@@ -121,9 +122,18 @@ interface SkippedType {
 	}
 
 	/**
-	 * Information about a Nullness mismatch between the GraphQL schema and the application code.
+	 * Information about a Nullness error between the GraphQL schema and the application code.
+	 *
+	 * <p>Errors are raised when the application nullness information breaks requirements
+	 * from the schema: a schema field is marked as "non-null" (like {@code "id: ID!"}),
+	 * but the application element is "nullable" (like {@code @Nullable String getId();}).
+	 *
+	 * <p>The opposite ({@code "author: Author"} and {@code @NonNull Author getAuthor();}
+	 * is not considered as an error, because if an exception is raised while fetching a
+	 * field, the GraphQL engine will null out fields in the hierarchy up until {@code null}
+	 * is allowed. A schema consistently using "non-null" types will not return partial responses.
 	 */
-	interface NullnessMismatch {
+	interface NullnessError {
 
 		/**
 		 * Nullness expected in the schema.

spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorNullnessTests.java

@@ -112,7 +112,7 @@ void reportHasEntriesWhenSchemaFieldNonNullAndFieldMethodsNullable() {
 						}
 					""";
 			SchemaReport report = inspectSchema(schema, NullableRecordFieldsBookController.class);
-			assertThatReport(report).containsFieldsNullnessMismatches("NullableRecordFieldsBook", "id", "title");
+			assertThatReport(report).containsFieldsNullnessErrors("NullableRecordFieldsBook", "id", "title");
 		}
 
 		@Test
@@ -135,7 +135,7 @@ void reportFormatWhenSchemaFieldNonNullAndFieldMethodsNullable() {
 		}
 
 		@Test
-		void reportHasEntriesWhenSchemaFieldNullableAndFieldTypeNonNull() {
+		void reportIsEmptyWhenSchemaFieldNullableAndFieldTypeNonNull() {
 			String schema = """
 						type Query {
 							bookById(id: ID): NonNullFieldBook
@@ -146,7 +146,7 @@ void reportHasEntriesWhenSchemaFieldNullableAndFieldTypeNonNull() {
 						}
 					""";
 			SchemaReport report = inspectSchema(schema, NonNullFieldBookController.class);
-			assertThatReport(report).containsFieldsNullnessMismatches("NonNullFieldBook", "id", "title");
+			assertThatReport(report).isEmpty();
 		}
 
 
@@ -162,7 +162,7 @@ void reportHasEntriesWhenSchemaFieldNonNullAndFieldNullable() {
 						}
 					""";
 			SchemaReport report = inspectSchema(schema, NullableFieldBookController.class);
-			assertThatReport(report).containsFieldsNullnessMismatches("NullableFieldBook", "title");
+			assertThatReport(report).containsFieldsNullnessErrors("NullableFieldBook", "title");
 		}
 
 		@Test
@@ -316,6 +316,21 @@ void reportIsEmptyWhenSchemaFieldNonNullAndReturnTypeNonNull() {
 			assertThatReport(report).isEmpty();
 		}
 
+		@Test
+		void reportIsEmptyWhenSchemaFieldNullableAndReturnTypeNonNull() {
+			String schema = """
+						type Query {
+							bookById(id: ID): Book
+						}
+						type Book {
+							id: ID!
+							title: String!
+						}
+					""";
+			SchemaReport report = inspectSchema(schema, NonNullBookController.class);
+			assertThatReport(report).isEmpty();
+		}
+
 
 		@Test
 		void reportHasEntryWhenSchemaFieldNonNullAndReturnTypeNullable() {
@@ -329,7 +344,7 @@ void reportHasEntryWhenSchemaFieldNonNullAndReturnTypeNullable() {
 						}
 					""";
 			SchemaReport report = inspectSchema(schema, NullableBookController.class);
-			assertThatReport(report).containsFieldsNullnessMismatches("Query", "bookById");
+			assertThatReport(report).containsFieldsNullnessErrors("Query", "bookById");
 		}
 
 		@Test
@@ -506,7 +521,7 @@ void reportHasEntryWhenSchemaFieldNonNullAndReturnTypeNullable() {
 						}
 					""";
 			SchemaReport report = inspectSchema(schema, NullableArgBookController.class);
-			assertThatReport(report).containsArgumentsNullnessMismatches("java.lang.String id");
+			assertThatReport(report).containsArgumentsNullnessErrors("java.lang.String id");
 		}
 
 		@Test