Further refine nullness checks in schema inspection

This commit further refines nullness checks in the schema mapping
inspection to process argument errors differently: unlike fields, where
the application should not return `null` when the schema does not allow
it, arguments take user input. This means we should only treat as errors
if the schema accept nullable fields but the applicaiton code does not.

Closes gh-1344

Author: bclozel

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

@@ -264,16 +264,17 @@ annotations, further inspections can be performed. The GraphQL schema can declar
 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`
+When schema fields are non-null, we ensure that the relevant `Class` properties and `DataFetcher` return types
+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.
 
+When field arguments are nullable, we ensure that `DataFetcher` parameters are also nullable.
+In this case, user input should not be fed to the application if it breaks the nullability contract as this will lead
+to runtime failures.
+
 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:
 
@@ -294,13 +295,17 @@ GraphQL schema inspection:
     Unmapped fields: {Book=[title], Author[firstName, lastName]} // <1>
     Unmapped registrations: {Book.reviews=BookController#reviews[1 args]} <2>
     Unmapped arguments: {BookController#bookSearch[1 args]=[myAuthor]} // <3>
-    Skipped types: [BookOrAuthor] // <4>
+    Field nullness errors: {Book=[title is NON_NULL -> 'Book#title' is NULLABLE]} <4>
+    Argument nullness errors: {BookController#bookById[1 args]=[java.lang.String id should be NULLABLE]} <5>
+    Skipped types: [BookOrAuthor] // <6>
 ----
 
 <1> Schema fields that are not covered in any way
 <2> `DataFetcher` registrations to fields that don't exist
 <3> `DataFetcher` expected arguments that don't exist
-<4> Schema types that have been skipped (explained next)
+<4> "title" schema field is non null, but `Book.getTitle()` is `@Nullable`
+<5> `bookById(id: ID)` has a nullable "id" argument, but `Book bookById(@NonNull String id)` is non null.
+<6> Schema types that have been skipped (explained next)
 
 In some cases, the `Class` type for a schema type is unknown. Maybe the `DataFetcher` does not
 implement `SelfDescribingDataFetcher`, or the declared return type is too general

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

@@ -224,7 +224,7 @@ private void checkFieldsContainer(
 
 	private void checkFieldNullNess(FieldCoordinates fieldCoordinates, Field javaField, Nullness schemaNullness) {
 		Nullness applicationNullness = Nullness.forField(javaField);
-		if (isNullnessError(schemaNullness, applicationNullness)) {
+		if (isFieldNullnessError(schemaNullness, applicationNullness)) {
 			DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(javaField,
 					javaField.getDeclaringClass().getSimpleName() + "#" + javaField.getName());
 			this.reportBuilder.fieldNullnessError(fieldCoordinates,
@@ -247,7 +247,7 @@ 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 (isNullnessError(schemaNullness, applicationNullness)) {
+			else if (isFieldNullnessError(schemaNullness, applicationNullness)) {
 				DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(dataFetcherMethod, dataFetcher.getDescription());
 				this.reportBuilder.fieldNullnessError(fieldCoordinates,
 						new DefaultNullnessError(schemaNullness, applicationNullness, annotatedElement));
@@ -277,7 +277,7 @@ private void checkFieldArgumentsNullness(GraphQLFieldDefinition field, SelfDescr
 				if (argument != null && argument.getDefinition() != null) {
 					Nullness schemaNullness = resolveNullness(argument.getDefinition().getType());
 					Nullness applicationNullness = Nullness.forMethodParameter(MethodParameter.forParameter(parameter));
-					if (isNullnessError(schemaNullness, applicationNullness)) {
+					if (isArgumentNullnessError(schemaNullness, applicationNullness)) {
 						nullnessErrors.add(new DefaultNullnessError(schemaNullness, applicationNullness, parameter));
 					}
 				}
@@ -290,7 +290,7 @@ private void checkFieldArgumentsNullness(GraphQLFieldDefinition field, SelfDescr
 
 	private void checkReadMethodNullness(FieldCoordinates fieldCoordinates, ResolvableType resolvableType, PropertyDescriptor descriptor, Nullness schemaNullness) {
 		Nullness applicationNullness = Nullness.forMethodReturnType(descriptor.getReadMethod());
-		if (isNullnessError(schemaNullness, applicationNullness)) {
+		if (isFieldNullnessError(schemaNullness, applicationNullness)) {
 			DescribedAnnotatedElement annotatedElement = new DescribedAnnotatedElement(descriptor.getReadMethod(),
 					resolvableType.toClass().getSimpleName() + "#" + descriptor.getName());
 			this.reportBuilder.fieldNullnessError(fieldCoordinates,
@@ -412,10 +412,14 @@ private Nullness resolveNullness(Type type) {
 		return Nullness.NULLABLE;
 	}
 
-	private boolean isNullnessError(Nullness schemaNullness, Nullness applicationNullness) {
+	private boolean isFieldNullnessError(Nullness schemaNullness, Nullness applicationNullness) {
 		return (schemaNullness == Nullness.NON_NULL && applicationNullness == Nullness.NULLABLE);
 	}
 
+	private boolean isArgumentNullnessError(Nullness schemaNullness, Nullness applicationNullness) {
+		return (schemaNullness == Nullness.NULLABLE && applicationNullness == Nullness.NON_NULL);
+	}
+
 
 	/**
 	 * Check the schema against {@code DataFetcher} registrations, and produce a report.

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

@@ -138,7 +138,7 @@ void reportFormatWhenSchemaFieldNonNullAndFieldMethodsNullable() {
 		void reportIsEmptyWhenSchemaFieldNullableAndFieldTypeNonNull() {
 			String schema = """
 						type Query {
-							bookById(id: ID): NonNullFieldBook
+							bookById(id: ID!): NonNullFieldBook
 						}
 						type NonNullFieldBook {
 							id: ID
@@ -510,7 +510,7 @@ public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node,
 	class ArgumentsNullnessTests {
 
 		@Test
-		void reportHasEntryWhenSchemaFieldNonNullAndReturnTypeNullable() {
+		void reportIsEmptyWhenSchemaArgumentIsNonNullAndMethodArgumentNullable() {
 			String schema = """
 						type Query {
 							bookById(id: ID!): Book!
@@ -521,23 +521,38 @@ void reportHasEntryWhenSchemaFieldNonNullAndReturnTypeNullable() {
 						}
 					""";
 			SchemaReport report = inspectSchema(schema, NullableArgBookController.class);
+			assertThatReport(report).isEmpty();
+		}
+
+		@Test
+		void reportHasEntryWhenSchemaArgumentNullableAndMethodArgumentNonNull() {
+			String schema = """
+						type Query {
+							bookById(id: ID): Book!
+						}
+						type Book {
+							id: ID!
+							title: String!
+						}
+					""";
+			SchemaReport report = inspectSchema(schema, NonNullArgBookController.class);
 			assertThatReport(report).containsArgumentsNullnessErrors("java.lang.String id");
 		}
 
 		@Test
-		void reportFormatWhenSchemaFieldNonNullAndReturnTypeNullable() {
+		void reportFormatWhenSchemaArgumentNullableAndMethodArgumentNonNull() {
 			String schema = """
 						type Query {
-							bookById(id: ID!): Book!
+							bookById(id: ID): Book!
 						}
 						type Book {
 							id: ID!
 							title: String!
 						}
 					""";
-			SchemaReport report = inspectSchema(schema, NullableArgBookController.class);
+			SchemaReport report = inspectSchema(schema, NonNullArgBookController.class);
 			assertThat(report.toString())
-					.contains("{NullableArgBookController#bookById[1 args]=[java.lang.String id should be NON_NULL]}");
+					.contains("{NonNullArgBookController#bookById[1 args]=[java.lang.String id should be NULLABLE]}");
 		}
 
 
@@ -552,6 +567,18 @@ static class NullableArgBookController {
 
 		}
 
+
+		@Controller
+		@NullMarked
+		static class NonNullArgBookController {
+
+			@QueryMapping
+			public Book bookById(String id) {
+				return new Book("42", "Spring for GraphQL Book");
+			}
+
+		}
+
 		record Book(String id, String title) {
 
 		}

spring-graphql/src/test/kotlin/org/springframework/graphql/execution/SchemaMappingInspectorNullnessKotlinTests.kt

@@ -16,7 +16,6 @@
 
 package org.springframework.graphql.execution
 
-import org.junit.jupiter.api.Disabled
 import org.junit.jupiter.api.Test
 import org.springframework.graphql.data.method.annotation.QueryMapping
 import org.springframework.stereotype.Controller
@@ -26,7 +25,6 @@ class SchemaMappingInspectorNullnessKotlinTests : SchemaMappingInspectorTestSupp
 
 
     @Test
-    @Disabled("until https://github.com/spring-projects/spring-framework/issues/35419 is fixed")
     fun reportHasEntriesWhenSchemaFieldNonNullAndTypeFieldNullable() {
         val schema = """
 						type Query {
@@ -59,7 +57,7 @@ class SchemaMappingInspectorNullnessKotlinTests : SchemaMappingInspectorTestSupp
     }
 
     @Test
-    fun reportHasEntriesWhenSchemaFieldNonNullAndDataFetcherArgumentNullable() {
+    fun reportIsEmptuWhenSchemaFieldNonNullAndDataFetcherArgumentNullable() {
         val schema = """
 						type Query {
 							bookById(id: ID!): Book
@@ -71,7 +69,7 @@ class SchemaMappingInspectorNullnessKotlinTests : SchemaMappingInspectorTestSupp
 					
 					""".trimIndent()
         val report: SchemaReport = inspectSchema(schema, NullableFetcherArgumentBookController::class.java)
-        assertThatReport(report).containsArgumentsNullnessErrors("java.lang.String id");
+        assertThatReport(report).isEmpty();
     }