Add schema inspection support for nullness checks

Prior to this commit, the schema inspection feature would detect missing
fields and arguments when looking at class properties and registered
data fetchers.

The GraphQL schema also scpecifies the nullability of fields and
arguments, using the "!" marker type.

This commit enhances the schema inspector to read the nullability
information from the schema and compare it to the nullability
information present in the code. This is only active if the application
code is written in Kotlin or is using JSpecify annotations.
If a nullability mismatch is detected (NULL vs NON_NULL), the
information is displayed in the inspection report.

Note that by default, Java types have the "UNSPECIFIED" nullness
information, because we cannot infer the nullability from typical Java
classes.

This feature is currently only supported if the `DataFetcher` is
actually backed by a Java method, a Kotlin function or a class property.
In other cases, the type use information will not be available.

Closes gh-1220

Author: bclozel

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

@@ -259,6 +259,16 @@ to inspect schema mappings on startup to ensure the following:
 - `DataFetcher` registrations refer to a schema field that exists.
 - `DataFetcher` arguments have matching schema field arguments.
 
+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
+
+
 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/data/method/annotation/support/AnnotatedControllerConfigurer.java

@@ -500,6 +500,11 @@ public Map<String, ResolvableType> getArguments() {
 							ResolvableType::forMethodParameter));
 		}
 
+		@Override
+		public Method asMethod() {
+			return this.mappingInfo.getHandlerMethod().getMethod();
+		}
+
 		@Override
 		public boolean usesDataLoader() {
 			return this.usesDataLoader;
@@ -595,6 +600,11 @@ public ResolvableType getReturnType() {
 			return this.returnType;
 		}
 
+		@Override
+		public Method asMethod() {
+			return this.mappingInfo.getHandlerMethod().getMethod();
+		}
+
 		@Override
 		public Object get(DataFetchingEnvironment env) {
 			DataLoader<?, ?> dataLoader = env.getDataLoader(this.dataLoaderKey);

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

@@ -16,6 +16,7 @@
 
 package org.springframework.graphql.execution;
 
+import java.lang.reflect.Method;
 import java.util.List;
 import java.util.Map;
 
@@ -217,6 +218,11 @@ public ResolvableType getReturnType() {
 			return this.delegate.getReturnType();
 		}
 
+		@Override
+		public @Nullable Method asMethod() {
+			return this.delegate.asMethod();
+		}
+
 		@Override
 		public Map<String, ResolvableType> getArguments() {
 			return this.delegate.getArguments();

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

@@ -17,9 +17,12 @@
 package org.springframework.graphql.execution;
 
 import java.beans.PropertyDescriptor;
+import java.lang.annotation.Annotation;
+import java.lang.reflect.AnnotatedElement;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.lang.reflect.Parameter;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -31,8 +34,11 @@
 import java.util.function.Function;
 import java.util.function.Predicate;
 
+import graphql.language.NonNullType;
+import graphql.language.Type;
 import graphql.schema.DataFetcher;
 import graphql.schema.FieldCoordinates;
+import graphql.schema.GraphQLArgument;
 import graphql.schema.GraphQLEnumType;
 import graphql.schema.GraphQLFieldDefinition;
 import graphql.schema.GraphQLFieldsContainer;
@@ -54,6 +60,7 @@
 import org.springframework.beans.BeanUtils;
 import org.springframework.beans.BeansException;
 import org.springframework.core.MethodParameter;
+import org.springframework.core.Nullness;
 import org.springframework.core.ReactiveAdapter;
 import org.springframework.core.ReactiveAdapterRegistry;
 import org.springframework.core.ResolvableType;
@@ -70,6 +77,9 @@
  * 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.
  * </ul>
  *
  * <p>Use methods of {@link GraphQlSource.SchemaResourceBuilder} to enable schema
@@ -166,10 +176,14 @@ private void checkFieldsContainer(
 		for (GraphQLFieldDefinition field : fieldContainer.getFieldDefinitions()) {
 			String fieldName = field.getName();
 			DataFetcher<?> dataFetcher = dataFetcherMap.get(fieldName);
+			FieldCoordinates fieldCoordinates = FieldCoordinates.coordinates(typeName, fieldName);
+			Nullness schemaNullness = resolveNullness(field);
 
 			if (dataFetcher != null) {
 				if (dataFetcher instanceof SelfDescribingDataFetcher<?> selfDescribing) {
+					checkDataFetcherNullness(fieldCoordinates, selfDescribing, schemaNullness);
 					checkFieldArguments(field, selfDescribing);
+					checkFieldArgumentsNullness(field, selfDescribing);
 					checkField(fieldContainer, field, selfDescribing.getReturnType());
 				}
 				else {
@@ -182,11 +196,13 @@ private void checkFieldsContainer(
 				PropertyDescriptor descriptor = getProperty(resolvableType, fieldName);
 				if (descriptor != null) {
 					MethodParameter returnType = new MethodParameter(descriptor.getReadMethod(), -1);
+					checkReadMethodNullness(fieldCoordinates, resolvableType, descriptor, schemaNullness);
 					checkField(fieldContainer, field, ResolvableType.forMethodParameter(returnType, resolvableType));
 					continue;
 				}
 				Field javaField = getField(resolvableType, fieldName);
 				if (javaField != null) {
+					checkFieldNullNess(fieldCoordinates, javaField, schemaNullness);
 					checkField(fieldContainer, field, ResolvableType.forField(javaField));
 					continue;
 				}
@@ -199,7 +215,34 @@ private void checkFieldsContainer(
 				}
 			}
 
-			this.reportBuilder.unmappedField(FieldCoordinates.coordinates(typeName, fieldName));
+			this.reportBuilder.unmappedField(fieldCoordinates);
+		}
+	}
+
+	private void checkFieldNullNess(FieldCoordinates fieldCoordinates, Field javaField, Nullness schemaNullness) {
+		Nullness applicationNullness = Nullness.forField(javaField);
+		if (isMismatch(schemaNullness, applicationNullness)) {
+			DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(javaField,
+					javaField.getDeclaringClass().getSimpleName() + "#" + javaField.getName());
+			this.reportBuilder.fieldNullnessMismatch(fieldCoordinates,
+					new DefaultNullnessMismatch(schemaNullness, applicationNullness, annotatedElement));
+
+		}
+	}
+
+	private void checkDataFetcherNullness(FieldCoordinates fieldCoordinates, SelfDescribingDataFetcher dataFetcher, Nullness schemaNullness) {
+		Method dataFetcherMethod = dataFetcher.asMethod();
+		if (dataFetcherMethod != null) {
+			Nullness applicationNullness = Nullness.forMethodReturnType(dataFetcherMethod);
+			// we cannot infer nullness if wrapped by reactive types
+			ReactiveAdapter reactiveAdapter = ReactiveAdapterRegistry.getSharedInstance()
+					.getAdapter(dataFetcherMethod.getReturnType());
+			if (reactiveAdapter == null && isMismatch(schemaNullness, applicationNullness)) {
+				DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(dataFetcherMethod, dataFetcher.getDescription());
+				this.reportBuilder.fieldNullnessMismatch(fieldCoordinates,
+						new DefaultNullnessMismatch(schemaNullness, applicationNullness, annotatedElement));
+
+			}
 		}
 	}
 
@@ -215,6 +258,37 @@ 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<>();
+			for (Parameter parameter : dataFetcherMethod.getParameters()) {
+				GraphQLArgument argument = field.getArgument(parameter.getName());
+				if (argument != 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 (!mismatches.isEmpty()) {
+				this.reportBuilder.argumentsNullnessMismatches(dataFetcher, mismatches);
+			}
+		}
+	}
+
+	private void checkReadMethodNullness(FieldCoordinates fieldCoordinates, ResolvableType resolvableType, PropertyDescriptor descriptor, Nullness schemaNullness) {
+		Nullness applicationNullness = Nullness.forMethodReturnType(descriptor.getReadMethod());
+		if (isMismatch(schemaNullness, applicationNullness)) {
+			DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(descriptor.getReadMethod(),
+					resolvableType.toClass().getSimpleName() + "#" + descriptor.getName());
+			this.reportBuilder.fieldNullnessMismatch(fieldCoordinates,
+					new DefaultNullnessMismatch(schemaNullness, applicationNullness, annotatedElement));
+		}
+	}
+
+
 	/**
 	 * Resolve field wrapper types (connection, list, non-null), nest into generic types,
 	 * and recurse with {@link #checkFieldsContainer} if there is enough type information.
@@ -308,6 +382,22 @@ private void checkDataFetcherRegistrations() {
 				}));
 	}
 
+	private Nullness resolveNullness(GraphQLFieldDefinition fieldDefinition) {
+		return resolveNullness(fieldDefinition.getDefinition().getType());
+	}
+
+	private Nullness resolveNullness(Type type) {
+		if (type instanceof NonNullType) {
+			return Nullness.NON_NULL;
+		}
+		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);
+	}
+
 
 	/**
 	 * Check the schema against {@code DataFetcher} registrations, and produce a report.
@@ -801,6 +891,10 @@ private final class ReportBuilder {
 
 		private final MultiValueMap<DataFetcher<?>, String> unmappedArguments = new LinkedMultiValueMap<>();
 
+		private final Map<FieldCoordinates, SchemaReport.NullnessMismatch> fieldNullnessMismatches = new LinkedHashMap<>();
+
+		private final MultiValueMap<DataFetcher<?>, SchemaReport.NullnessMismatch> argumentsNullnessMismatches = new LinkedMultiValueMap<>();
+
 		private final List<DefaultSkippedType> skippedTypes = new ArrayList<>();
 
 		private final List<DefaultSkippedType> candidateSkippedTypes = new ArrayList<>();
@@ -817,6 +911,14 @@ 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 argumentsNullnessMismatches(DataFetcher<?> dataFetcher, List<SchemaReport.NullnessMismatch> nullnessMismatches) {
+			this.argumentsNullnessMismatches.put(dataFetcher, nullnessMismatches);
+		}
+
 		void skippedType(
 				GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field,
 				String reason, boolean isDerivedType) {
@@ -852,8 +954,8 @@ SchemaReport build() {
 				skippedType(skippedType);
 			});
 
-			return new DefaultSchemaReport(
-					this.unmappedFields, this.unmappedRegistrations, this.unmappedArguments, this.skippedTypes);
+			return new DefaultSchemaReport(this.unmappedFields, this.unmappedRegistrations,
+					this.unmappedArguments, this.fieldNullnessMismatches, this.argumentsNullnessMismatches, this.skippedTypes);
 		}
 	}
 
@@ -869,15 +971,24 @@ private final class DefaultSchemaReport implements SchemaReport {
 
 		private final MultiValueMap<DataFetcher<?>, String> unmappedArguments;
 
+		private final Map<FieldCoordinates, NullnessMismatch> fieldsNullnessMismatches;
+
+		private final MultiValueMap<DataFetcher<?>, NullnessMismatch> argumentsNullnessMismatches;
+
 		private final List<SchemaReport.SkippedType> skippedTypes;
 
 		DefaultSchemaReport(
 				List<FieldCoordinates> unmappedFields, Map<FieldCoordinates, DataFetcher<?>> unmappedRegistrations,
-				MultiValueMap<DataFetcher<?>, String> unmappedArguments, List<DefaultSkippedType> skippedTypes) {
+				MultiValueMap<DataFetcher<?>, String> unmappedArguments,
+				Map<FieldCoordinates, NullnessMismatch> fieldsNullnessMismatches,
+				MultiValueMap<DataFetcher<?>, NullnessMismatch> argumentsNullnessMismatches,
+				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.skippedTypes = Collections.unmodifiableList(skippedTypes);
 		}
 
@@ -896,6 +1007,16 @@ public MultiValueMap<DataFetcher<?>, String> unmappedArguments() {
 			return this.unmappedArguments;
 		}
 
+		@Override
+		public Map<FieldCoordinates, NullnessMismatch> fieldsNullnessMismatches() {
+			return this.fieldsNullnessMismatches;
+		}
+
+		@Override
+		public MultiValueMap<DataFetcher<?>, NullnessMismatch> argumentsNullnessMismatches() {
+			return this.argumentsNullnessMismatches;
+		}
+
 		@Override
 		public List<SkippedType> skippedTypes() {
 			return this.skippedTypes;
@@ -919,6 +1040,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" +
 					"\tSkipped types: " + this.skippedTypes;
 		}
 
@@ -931,6 +1054,27 @@ private String formatUnmappedFields() {
 			return map.toString();
 		}
 
+		private String formatFieldsNullnessMismatches() {
+			MultiValueMap<String, String> map = new LinkedMultiValueMap<>();
+			this.fieldsNullnessMismatches.forEach((coordinates, mismatch) -> {
+				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()));
+			});
+			return map.toString();
+		}
+
+		private String formatArgumentsNullnessMismatches() {
+			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()))
+						.toList();
+				map.put(dataFetcher.toString(), arguments);
+			});
+			return map.toString();
+		}
+
 	}
 
 
@@ -953,4 +1097,40 @@ public static DefaultSkippedType create(
 		}
 	}
 
+	/**
+	 * Default implementation of a {@link SchemaReport.NullnessMismatch}.
+	 */
+	private record DefaultNullnessMismatch(
+			Nullness schemaNullness, Nullness applicationNullness, AnnotatedElement annotatedElement)
+			implements SchemaReport.NullnessMismatch {
+
+	}
+
+	/**
+	 * {@link AnnotatedElement} that overrides the {@code toString} method for displaying in the report.
+	 */
+	private record DescribedAnnotatedElement(AnnotatedElement delegate,
+			String description) implements AnnotatedElement {
+
+		@Override
+			public String toString() {
+				return this.description;
+			}
+
+			@Override
+			public <T extends Annotation> T getAnnotation(Class<T> annotationClass) {
+				return this.delegate.getAnnotation(annotationClass);
+			}
+
+			@Override
+			public Annotation[] getAnnotations() {
+				return this.delegate.getAnnotations();
+			}
+
+			@Override
+			public Annotation[] getDeclaredAnnotations() {
+				return this.delegate.getDeclaredAnnotations();
+			}
+	}
+
 }