From 464bd6b43ab50fe1077e491ca670365fdd984bae Mon Sep 17 00:00:00 2001 From: Marek Skacelik Date: Fri, 24 Jan 2025 15:26:25 +0100 Subject: [PATCH] Added better exception messages for dynamic client name validation --- .../graphql/client/core/VariableType.java | 8 +-- .../core/utils/validation/NameValidation.java | 42 ++++++++++-- .../graphql/dynamic/core/OperationsTest.java | 31 ++++++--- .../dynamic/core/VariableTypesTest.java | 66 ++++++++++++++----- .../graphql/dynamic/core/VariablesTest.java | 26 ++++++-- 5 files changed, 129 insertions(+), 44 deletions(-) diff --git a/client/api/src/main/java/io/smallrye/graphql/client/core/VariableType.java b/client/api/src/main/java/io/smallrye/graphql/client/core/VariableType.java index 05d850287..abb835683 100644 --- a/client/api/src/main/java/io/smallrye/graphql/client/core/VariableType.java +++ b/client/api/src/main/java/io/smallrye/graphql/client/core/VariableType.java @@ -1,7 +1,7 @@ package io.smallrye.graphql.client.core; import static io.smallrye.graphql.client.core.utils.ServiceUtils.getNewInstanceFromFactory; -import static io.smallrye.graphql.client.core.utils.validation.NameValidation.validateName; +import static io.smallrye.graphql.client.core.utils.validation.NameValidation.validateTypeName; import io.smallrye.graphql.client.core.factory.VariableTypeFactory; @@ -14,7 +14,7 @@ public interface VariableType extends Buildable { static VariableType varType(String objectTypeName) { VariableType varType = getNewInstanceFromFactory(VariableTypeFactory.class); - varType.setName(validateName(objectTypeName)); + varType.setName(validateTypeName(objectTypeName)); varType.setNonNull(false); varType.setChild(null); @@ -47,7 +47,7 @@ static VariableType nonNull(ScalarType scalarType) { static VariableType nonNull(String objectTypeName) { VariableType varType = getNewInstanceFromFactory(VariableTypeFactory.class); - varType.setName(validateName(objectTypeName)); + varType.setName(validateTypeName(objectTypeName)); varType.setNonNull(true); varType.setChild(null); @@ -75,7 +75,7 @@ static VariableType list(ScalarType scalarType) { static VariableType list(String objectTypeName) { VariableType varType = getNewInstanceFromFactory(VariableTypeFactory.class); - varType.setName("list(" + validateName(objectTypeName) + ")"); + varType.setName("list(" + validateTypeName(objectTypeName) + ")"); varType.setNonNull(false); varType.setChild(varType(objectTypeName)); diff --git a/client/api/src/main/java/io/smallrye/graphql/client/core/utils/validation/NameValidation.java b/client/api/src/main/java/io/smallrye/graphql/client/core/utils/validation/NameValidation.java index aff32c1c7..bbc97be58 100644 --- a/client/api/src/main/java/io/smallrye/graphql/client/core/utils/validation/NameValidation.java +++ b/client/api/src/main/java/io/smallrye/graphql/client/core/utils/validation/NameValidation.java @@ -30,7 +30,9 @@ public static String validateNameAllowEmpty(String name) { if (name == null || name.isEmpty()) { return ""; } else if (!nameMatchesPattern(name, NAME_PATTERN)) { - throw new IllegalArgumentException("Invalid name: " + name); + throw new IllegalArgumentException( + "Invalid name: '%s'. Name does not match the regex pattern %s, please check the GraphQL specification %s" + .formatted(name, _NAME_REGEX, "https://spec.graphql.org/draft/#Name")); } return name; } @@ -48,9 +50,11 @@ public static String validateNameAllowEmpty(String name) { */ public static String validateFragmentName(String name) { if (name == null || !nameMatchesPattern(name, NAME_PATTERN)) { - throw new IllegalArgumentException("Invalid fragment name: " + name); + throw new IllegalArgumentException( + "Invalid fragment name '%s'. Fragment name does not match the regex pattern %s, please check the GraphQL specification %s" + .formatted(name, _NAME_REGEX, "https://spec.graphql.org/draft/#sec-Language.Fragments")); } else if (name.equals("on")) { - throw new IllegalArgumentException("Fragment name cannot be 'on'"); + throw new IllegalArgumentException("Invalid fragment name. Fragment name cannot be 'on'"); } return name; } @@ -65,7 +69,9 @@ public static String validateFragmentName(String name) { */ public static String validateName(String name) { if (name == null || !nameMatchesPattern(name, NAME_PATTERN)) { - throw new IllegalArgumentException("Invalid name: " + name); + throw new IllegalArgumentException( + "Invalid name: '%s'. Name does not match the regex pattern %s, please check the GraphQL specification %s" + .formatted(name, _NAME_REGEX, "https://spec.graphql.org/draft/#Name")); } return name; } @@ -83,13 +89,37 @@ public static String validateName(String name) { */ public static String validateFieldName(String fieldName) { if (fieldName == null || !nameMatchesPattern(fieldName, FIELD_NAME_PATTERN)) { - throw new IllegalArgumentException("Invalid field name: " + fieldName); + throw new IllegalArgumentException( + "Invalid field name: '%s'. Field name does not match the regex pattern %s, please check the GraphQL specification %s" + .formatted(fieldName, _FIELD_NAME_REGEX, "https://spec.graphql.org/draft/#sec-Language.Fields")); } return fieldName; } + public static String validateTypeName(String typeName) { + if (typeName == null) { + throw new IllegalArgumentException( + "Invalid type name. Type name cannot be null"); + } + if (typeName.contains("]") || typeName.contains("[")) { + throw new IllegalArgumentException( + "Invalid type name: '%s'. Type name cannot contain '[' or ']', instead use the io.smallrye.graphql.client.core.VariableType.list method to wrap your type into a list type" + .formatted(typeName)); + } + if (typeName.contains("!")) { + throw new IllegalArgumentException( + "Invalid type name: '%s'. Type name cannot contain '!', instead use the io.smallrye.graphql.client.core.VariableType.nonNull method to wrap your type into a non-null type" + .formatted(typeName)); + } + if (!nameMatchesPattern(typeName, NAME_PATTERN)) { + throw new IllegalArgumentException( + "Invalid type name: '%s'. Type name does not match the regex pattern %s, please check the GraphQL specification %s" + .formatted(typeName, _NAME_REGEX, "https://spec.graphql.org/draft/#sec-Type-References")); + } + return typeName; + } + private static boolean nameMatchesPattern(String name, Pattern pattern) { return pattern.matcher(name).matches(); } - } diff --git a/client/tck/src/main/java/tck/graphql/dynamic/core/OperationsTest.java b/client/tck/src/main/java/tck/graphql/dynamic/core/OperationsTest.java index f09f320b3..dd97c7263 100644 --- a/client/tck/src/main/java/tck/graphql/dynamic/core/OperationsTest.java +++ b/client/tck/src/main/java/tck/graphql/dynamic/core/OperationsTest.java @@ -4,12 +4,17 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; import io.smallrye.graphql.client.core.Operation; public class OperationsTest { + + private static final String NAME_URL = "https://spec.graphql.org/draft/#Name"; + @Test public void operationsShouldNotThrowExceptionForValidNameTest() { assertDoesNotThrow(() -> operation("myOperation")); @@ -29,14 +34,22 @@ public void operationsShouldNotThrowExceptionForValidNameTest() { @Test public void operationsShouldThrowExceptionForInvalidNameTest() { - assertThrows(IllegalArgumentException.class, () -> operation("Invalid Name")); - assertThrows(IllegalArgumentException.class, () -> operation(":InvalidName")); - assertThrows(IllegalArgumentException.class, () -> operation("InvalidName:")); - assertThrows(IllegalArgumentException.class, () -> operation("::InvalidName")); - assertThrows(IllegalArgumentException.class, () -> operation("InvalidName::")); - assertThrows(IllegalArgumentException.class, () -> operation("@InvalidName")); - assertThrows(IllegalArgumentException.class, () -> operation("my.Operation")); - assertThrows(IllegalArgumentException.class, () -> operation("my,Operation")); - assertThrows(IllegalArgumentException.class, () -> operation("my-Operation")); + assertThrowsWithUrlMessage(() -> operation("Invalid Name")); + assertThrowsWithUrlMessage(() -> operation(":InvalidName")); + assertThrowsWithUrlMessage(() -> operation("InvalidName:")); + assertThrowsWithUrlMessage(() -> operation("::InvalidName")); + assertThrowsWithUrlMessage(() -> operation("InvalidName::")); + assertThrowsWithUrlMessage(() -> operation("@InvalidName")); + assertThrowsWithUrlMessage(() -> operation("my.Operation")); + assertThrowsWithUrlMessage(() -> operation("my,Operation")); + assertThrowsWithUrlMessage(() -> operation("my-Operation")); + assertThrowsWithUrlMessage(() -> operation("[myOperation]")); + assertThrowsWithUrlMessage(() -> operation("myOperation!")); + } + + private void assertThrowsWithUrlMessage(Executable lambda) { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, lambda); + assertTrue(ex.getMessage().contains(NAME_URL), + "Expected the '%s' message to contain '%s'".formatted(ex.getMessage(), NAME_URL)); } } diff --git a/client/tck/src/main/java/tck/graphql/dynamic/core/VariableTypesTest.java b/client/tck/src/main/java/tck/graphql/dynamic/core/VariableTypesTest.java index 76b2f9310..260517843 100644 --- a/client/tck/src/main/java/tck/graphql/dynamic/core/VariableTypesTest.java +++ b/client/tck/src/main/java/tck/graphql/dynamic/core/VariableTypesTest.java @@ -5,10 +5,16 @@ import static io.smallrye.graphql.client.core.VariableType.varType; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; public class VariableTypesTest { + private static final String TYPE_NAME_URL = "https://spec.graphql.org/draft/#sec-Type-References"; + private static final String LIST = "io.smallrye.graphql.client.core.VariableType.list"; + private static final String NON_NULL = "io.smallrye.graphql.client.core.VariableType.nonNull"; + @Test public void varTypesShouldNotThrowExceptionForValidName() { assertDoesNotThrow(() -> varType("ValidName")); @@ -24,12 +30,14 @@ public void varTypesShouldNotThrowExceptionForValidName() { @Test public void varTypesShouldThrowExceptionForInvalidName() { - assertThrows(IllegalArgumentException.class, () -> varType("Invalid:Name")); - assertThrows(IllegalArgumentException.class, () -> varType("Invalid,Name")); - assertThrows(IllegalArgumentException.class, () -> varType("123InvalidName")); - assertThrows(IllegalArgumentException.class, () -> varType("invalid-Name")); - assertThrows(IllegalArgumentException.class, () -> varType("invalid name")); - assertThrows(IllegalArgumentException.class, () -> varType("@invalidName")); + assertThrowsWithUrlMessage(() -> varType("Invalid:Name")); + assertThrowsWithUrlMessage(() -> varType("Invalid,Name")); + assertThrowsWithUrlMessage(() -> varType("123InvalidName")); + assertThrowsWithUrlMessage(() -> varType("invalid-Name")); + assertThrowsWithUrlMessage(() -> varType("invalid name")); + assertThrowsWithUrlMessage(() -> varType("@invalidName")); + assertThrowsWithListMessage(() -> varType("[invalidName]")); + assertThrowsWithNonNullMessage(() -> varType("invalidName!")); } @Test @@ -47,12 +55,14 @@ public void nonNullsShouldNotThrowExceptionForValidName() { @Test public void nonNullsShouldThrowExceptionForInvalidName() { - assertThrows(IllegalArgumentException.class, () -> nonNull("Invalid:Name")); - assertThrows(IllegalArgumentException.class, () -> nonNull("Invalid,Name")); - assertThrows(IllegalArgumentException.class, () -> nonNull("123InvalidName")); - assertThrows(IllegalArgumentException.class, () -> nonNull("invalid-Name")); - assertThrows(IllegalArgumentException.class, () -> nonNull("invalid name")); - assertThrows(IllegalArgumentException.class, () -> nonNull("@invalidName")); + assertThrowsWithUrlMessage(() -> nonNull("Invalid:Name")); + assertThrowsWithUrlMessage(() -> nonNull("Invalid,Name")); + assertThrowsWithUrlMessage(() -> nonNull("123InvalidName")); + assertThrowsWithUrlMessage(() -> nonNull("invalid-Name")); + assertThrowsWithUrlMessage(() -> nonNull("invalid name")); + assertThrowsWithUrlMessage(() -> nonNull("@invalidName")); + assertThrowsWithListMessage(() -> nonNull("[invalidName]")); + assertThrowsWithNonNullMessage(() -> nonNull("invalidName!")); } @Test @@ -70,11 +80,31 @@ public void listsShouldNotThrowExceptionForValidName() { @Test public void listsShouldThrowExceptionForInvalidName() { - assertThrows(IllegalArgumentException.class, () -> list("Invalid:Name")); - assertThrows(IllegalArgumentException.class, () -> list("Invalid,Name")); - assertThrows(IllegalArgumentException.class, () -> list("123InvalidName")); - assertThrows(IllegalArgumentException.class, () -> list("invalidName-")); - assertThrows(IllegalArgumentException.class, () -> list("invalid name")); - assertThrows(IllegalArgumentException.class, () -> list("@invalidName")); + assertThrowsWithUrlMessage(() -> list("Invalid:Name")); + assertThrowsWithUrlMessage(() -> list("Invalid,Name")); + assertThrowsWithUrlMessage(() -> list("123InvalidName")); + assertThrowsWithUrlMessage(() -> list("invalid-Name")); + assertThrowsWithUrlMessage(() -> list("invalid name")); + assertThrowsWithUrlMessage(() -> list("@invalidName")); + assertThrowsWithListMessage(() -> list("[invalidName]")); + assertThrowsWithNonNullMessage(() -> list("invalidName!")); + } + + private void assertThrowsWithUrlMessage(Executable lambda) { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, lambda); + assertTrue(ex.getMessage().contains(TYPE_NAME_URL), + "Expected the '%s' message to contain '%s'".formatted(ex.getMessage(), TYPE_NAME_URL)); + } + + private void assertThrowsWithListMessage(Executable lambda) { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, lambda); + assertTrue(ex.getMessage().contains(LIST), + "Expected the '%s' message to contain '%s'".formatted(ex.getMessage(), LIST)); + } + + private void assertThrowsWithNonNullMessage(Executable lambda) { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, lambda); + assertTrue(ex.getMessage().contains(NON_NULL), + "Expected the '%s' message to contain '%s'".formatted(ex.getMessage(), NON_NULL)); } } diff --git a/client/tck/src/main/java/tck/graphql/dynamic/core/VariablesTest.java b/client/tck/src/main/java/tck/graphql/dynamic/core/VariablesTest.java index 48a271968..13e147640 100644 --- a/client/tck/src/main/java/tck/graphql/dynamic/core/VariablesTest.java +++ b/client/tck/src/main/java/tck/graphql/dynamic/core/VariablesTest.java @@ -20,11 +20,13 @@ import static io.smallrye.graphql.client.core.VariableType.nonNull; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; import java.net.URISyntaxException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; import io.smallrye.graphql.client.core.Document; import io.smallrye.graphql.client.core.ScalarType; @@ -34,6 +36,8 @@ public class VariablesTest { + private static final String NAME_URL = "https://spec.graphql.org/draft/#Name"; + @Test public void variablesDefaultValueTest() throws IOException, URISyntaxException { String expectedRequest = Utils.getResourceFileContent("core/variablesDefaultValue.graphql"); @@ -188,13 +192,21 @@ public void variablesShouldNotThrowExceptionForValidNameTest() { @Test public void variablesShouldThrowExceptionForInvalidNameTest() { ScalarType scalarType = GQL_INT; - assertThrows(IllegalArgumentException.class, () -> var("123", scalarType)); - assertThrows(IllegalArgumentException.class, () -> var("my_var$", scalarType)); - assertThrows(IllegalArgumentException.class, () -> var("va:r", scalarType)); - assertThrows(IllegalArgumentException.class, () -> var("", scalarType)); - assertThrows(IllegalArgumentException.class, () -> var(":var", scalarType)); - assertThrows(IllegalArgumentException.class, () -> var("va:r:", scalarType)); - assertThrows(IllegalArgumentException.class, () -> var(null, scalarType)); + assertThrowsWithUrlMessage(() -> var("123", scalarType)); + assertThrowsWithUrlMessage(() -> var("my_var$", scalarType)); + assertThrowsWithUrlMessage(() -> var("va:r", scalarType)); + assertThrowsWithUrlMessage(() -> var("", scalarType)); + assertThrowsWithUrlMessage(() -> var(":var", scalarType)); + assertThrowsWithUrlMessage(() -> var("va:r:", scalarType)); + assertThrowsWithUrlMessage(() -> var(null, scalarType)); + assertThrowsWithUrlMessage(() -> var("[var]", scalarType)); + assertThrowsWithUrlMessage(() -> var("var!", scalarType)); + } + + private void assertThrowsWithUrlMessage(Executable lambda) { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, lambda); + assertTrue(ex.getMessage().contains(NAME_URL), + "Expected the '%s' message to contain '%s'".formatted(ex.getMessage(), NAME_URL)); } }