Added fix for the incostistent error responses while using @NonNull annotations

server/implementation-cdi/src/test/java/io/smallrye/graphql/execution/CdiExecutionTest.java

@@ -294,7 +294,24 @@ public void testMutationWithInvalidNumberInput() {
         assertFalse(error.isNull("message"), "message should not be null");
 
         assertEquals(
-                "argument 'powerLevel' with value 'StringValue{value='Unlimited'}' is not a valid 'Int'",
+                "Validation error (WrongType@[updateItemPowerLevel]) : argument 'powerLevel' with value 'StringValue{value='Unlimited'}' is not a valid 'Int' - SRGQL000022: Can not parse a number from [StringValue{value='Unlimited'}]",
+                error.getString("message"),
+                "Wrong error message while updateItemPowerLevel with wrong number");
+    }
+
+    @Test
+    public void testParsingInvalidNumberScalar() {
+        JsonArray errors = executeAndGetError(MUTATION_INVALID_INTEGER_SCALAR);
+
+        assertEquals(1, errors.size(),
+                "Wrong size for errors while updateItemPowerLevel with wrong number");
+
+        JsonObject error = errors.getJsonObject(0);
+
+        assertFalse(error.isNull("message"), "message should not be null");
+
+        assertEquals(
+                "Validation error (WrongType@[updateItemPowerLevel]) : argument 'powerLevel' with value 'StringValue{value='3.14'}' is not a valid 'Int' - SRGQL000021: Can not parse a integer from [StringValue{value='3.14'}]",
                 error.getString("message"),
                 "Wrong error message while updateItemPowerLevel with wrong number");
     }
@@ -308,7 +325,6 @@ public void testDefaultTimeScalarFormat() {
         assertFalse(data.getJsonObject("testScalarsInPojo").isNull("timeObject"), "timeObject should not be null");
         assertEquals("11:46:34.263", data.getJsonObject("testScalarsInPojo").getString("timeObject"),
                 "Wrong wrong time format");
-
     }
 
     @Test
@@ -391,6 +407,15 @@ private JsonObject toJsonObject(String graphQL) {
             "  }\n" +
             "}";
 
+    // This test invalid integer (as a float) scalar as input (expecting an error)
+    private static final String MUTATION_INVALID_INTEGER_SCALAR = "mutation increaseIronManSuitPowerTooHigh {\n" +
+            "  updateItemPowerLevel(itemID:1001, powerLevel:\"3.14\") {\n" +
+            "    id\n" +
+            "    name\n" +
+            "    powerLevel\n" +
+            "  }\n" +
+            "}";
+
     // This test invalid date scalars as input (expecting an error)
     private static final String MUTATION_INVALID_TIME_SCALAR = "mutation invalidPatrollingDate {\n" +
             "  startPatrolling(name:\"Starlord\", time:\"Today\") {\n" +

server/implementation/src/main/java/io/smallrye/graphql/SmallRyeGraphQLServerMessages.java

@@ -75,4 +75,10 @@ public interface SmallRyeGraphQLServerMessages {
 
     @Message(id = 20, value = "Can not inject an instance of class [%s]. Please make sure it is a CDI bean, also possibly the beans.xml file is needed")
     RuntimeException canNotInjectClass(String className, @Cause Exception cause);
+
+    @Message(id = 21, value = "Can not parse a integer from [%s]")
+    CoercingParseLiteralException integerCoercingParseException(String input);
+
+    @Message(id = 22, value = "Can not parse a number from [%s]")
+    CoercingParseLiteralException numberCoercingParseException(String input);
 }

server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/AbstractDataFetcher.java

@@ -81,7 +81,6 @@ public T get(final DataFetchingEnvironment dfe) throws Exception {
             eventEmitter.fireOnDataFetchError(smallRyeContext, ex);
             throw ex;
         }
-
         return invokeFailure(resultBuilder);
     }
 

server/implementation/src/main/java/io/smallrye/graphql/scalar/number/NumberCoercing.java

@@ -74,10 +74,10 @@ public Object parseLiteral(Object input) {
                 BigDecimal value = new BigDecimal(((StringValue) input).getValue());
                 return converter.fromBigDecimal(value);
             } catch (NumberFormatException e) {
-                // TODO: Do we still need this ? Here we allow strings through becauce of Numberformatting.
-                return ((StringValue) input).getValue();
+                throw msg.numberCoercingParseException(input.toString());
+            } catch (ArithmeticException e) {
+                throw msg.integerCoercingParseException(input.toString());
             }
-
         } else if (input instanceof IntValue) {
             BigInteger value = ((IntValue) input).getValue();
             if (!converter.isInRange(value)) {
@@ -86,10 +86,19 @@ public Object parseLiteral(Object input) {
             return converter.fromBigInteger(value);
         } else if (input instanceof FloatValue) {
             BigDecimal value = ((FloatValue) input).getValue();
-            return converter.fromBigDecimal(value);
+            try {
+                return converter.fromBigDecimal(value);
+            } catch (ArithmeticException e) {
+                throw msg.integerCoercingParseException(input.toString());
+            }
+
         } else if (input instanceof BigDecimal) {
             BigDecimal value = (BigDecimal) input;
-            return converter.fromBigDecimal(value);
+            try {
+                return converter.fromBigDecimal(value);
+            } catch (ArithmeticException e) {
+                throw msg.integerCoercingParseException(input.toString());
+            }
         } else if (input instanceof BigInteger) {
             BigInteger value = (BigInteger) input;
             if (!converter.isInRange(value)) {

server/integration-tests/src/test/java/io/smallrye/graphql/tests/nonnull/NonNullErrorResponseTest.java

@@ -0,0 +1,67 @@
+package io.smallrye.graphql.tests.nonnull;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.URL;
+
+import org.jboss.arquillian.container.test.api.Deployment;
+import org.jboss.arquillian.container.test.api.RunAsClient;
+import org.jboss.arquillian.junit.Arquillian;
+import org.jboss.arquillian.test.api.ArquillianResource;
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.spec.WebArchive;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import io.smallrye.graphql.tests.GraphQLAssured;
+
+@RunWith(Arquillian.class)
+@RunAsClient
+public class NonNullErrorResponseTest {
+
+    @Deployment
+    public static WebArchive deployment() {
+        return ShrinkWrap.create(WebArchive.class, "nonNull-test.war")
+                .addClasses(SomeApi.class);
+    }
+
+    @ArquillianResource
+    URL testingURL;
+
+    @Test
+    public void queryShouldNotReturnNonNullError() {
+        GraphQLAssured graphQLAssured = new GraphQLAssured(testingURL);
+
+        String response = graphQLAssured
+                .post("{ echoNumber(number: \"something\") }");
+        assertThat(response).contains("Can not parse a number from [StringValue{value='something'}]");
+        assertThat(response).doesNotContain("NullValueInNonNullableField");
+
+        response = graphQLAssured
+                .post("{ echoMessage(message: 314159) }");
+
+        assertThat(response)
+                .contains("argument 'message' with value 'IntValue{value=314159}' is not a valid 'String'")
+                .doesNotContain("NullValueInNonNullableField");
+    }
+
+    @Test
+    public void mutationShouldNotReturnNonNullError() {
+        GraphQLAssured graphQLAssured = new GraphQLAssured(testingURL);
+
+        String response = graphQLAssured
+                .post("mutation { add(a: \"one\", b: \"two\") }");
+        assertThat(response).contains("Can not parse a number from [StringValue{value='one'}]")
+                .contains("Can not parse a number from [StringValue{value='two'}]")
+                .doesNotContain("NullValueInNonNullableField");
+    }
+
+    @Test
+    public void queryShouldReturnNonNullError() {
+        GraphQLAssured graphQLAssured = new GraphQLAssured(testingURL);
+        String response = graphQLAssured
+                .post("{echoBigDecimal}");
+        assertThat(response).contains("NullValueInNonNullableField");
+    }
+
+}

server/integration-tests/src/test/java/io/smallrye/graphql/tests/nonnull/SomeApi.java

@@ -0,0 +1,32 @@
+package io.smallrye.graphql.tests.nonnull;
+
+import java.math.BigDecimal;
+
+import org.eclipse.microprofile.graphql.GraphQLApi;
+import org.eclipse.microprofile.graphql.Mutation;
+import org.eclipse.microprofile.graphql.NonNull;
+import org.eclipse.microprofile.graphql.Query;
+
+@GraphQLApi
+public class SomeApi {
+    @Query
+    public @NonNull Integer echoNumber(@NonNull Integer number) {
+        return number;
+    }
+
+    @Query
+    public @NonNull String echoMessage(@NonNull String message) {
+        return message;
+    }
+
+    // in this case the '@NonNull' annotations are redundant
+    @Mutation
+    public @NonNull int add(@NonNull int a, @NonNull int b) {
+        return a + b;
+    }
+
+    @Query
+    public @NonNull BigDecimal echoBigDecimal() {
+        return null;
+    }
+}