From 30b6a3b88a4a71a4473486965089c2fc11f39ef7 Mon Sep 17 00:00:00 2001 From: Marek Skacelik Date: Thu, 5 Oct 2023 09:50:27 +0200 Subject: [PATCH] Added fix for the incostistent error responses while using @NonNull annotations --- .../graphql/execution/CdiExecutionTest.java | 29 +++++++- .../SmallRyeGraphQLServerMessages.java | 6 ++ .../datafetcher/AbstractDataFetcher.java | 1 - .../graphql/scalar/number/NumberCoercing.java | 19 ++++-- .../nonnull/NonNullErrorResponseTest.java | 67 +++++++++++++++++++ .../graphql/tests/nonnull/SomeApi.java | 32 +++++++++ 6 files changed, 146 insertions(+), 8 deletions(-) create mode 100644 server/integration-tests/src/test/java/io/smallrye/graphql/tests/nonnull/NonNullErrorResponseTest.java create mode 100644 server/integration-tests/src/test/java/io/smallrye/graphql/tests/nonnull/SomeApi.java diff --git a/server/implementation-cdi/src/test/java/io/smallrye/graphql/execution/CdiExecutionTest.java b/server/implementation-cdi/src/test/java/io/smallrye/graphql/execution/CdiExecutionTest.java index f18486973..0666af761 100644 --- a/server/implementation-cdi/src/test/java/io/smallrye/graphql/execution/CdiExecutionTest.java +++ b/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" + diff --git a/server/implementation/src/main/java/io/smallrye/graphql/SmallRyeGraphQLServerMessages.java b/server/implementation/src/main/java/io/smallrye/graphql/SmallRyeGraphQLServerMessages.java index 981e4532d..4993bc389 100644 --- a/server/implementation/src/main/java/io/smallrye/graphql/SmallRyeGraphQLServerMessages.java +++ b/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); } diff --git a/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/AbstractDataFetcher.java b/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/AbstractDataFetcher.java index 20200df2e..2b95c8471 100644 --- a/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/AbstractDataFetcher.java +++ b/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); } diff --git a/server/implementation/src/main/java/io/smallrye/graphql/scalar/number/NumberCoercing.java b/server/implementation/src/main/java/io/smallrye/graphql/scalar/number/NumberCoercing.java index ed20793ba..431d413e1 100644 --- a/server/implementation/src/main/java/io/smallrye/graphql/scalar/number/NumberCoercing.java +++ b/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)) { diff --git a/server/integration-tests/src/test/java/io/smallrye/graphql/tests/nonnull/NonNullErrorResponseTest.java b/server/integration-tests/src/test/java/io/smallrye/graphql/tests/nonnull/NonNullErrorResponseTest.java new file mode 100644 index 000000000..d17b05e28 --- /dev/null +++ b/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"); + } + +} diff --git a/server/integration-tests/src/test/java/io/smallrye/graphql/tests/nonnull/SomeApi.java b/server/integration-tests/src/test/java/io/smallrye/graphql/tests/nonnull/SomeApi.java new file mode 100644 index 000000000..0f71eab7c --- /dev/null +++ b/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; + } +}