Skip to content

parseValue coercion alignment for Boolean, Float, and Int #3042

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 6, 2022
Merged

parseValue coercion alignment for Boolean, Float, and Int #3042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
03e10fb
Align Boolean parseValue with JS implementation
dondonz Dec 4, 2022
e042b8e
Align Float parseValue with JS implementation
dondonz Dec 4, 2022
2c514ca
Align integer parseValue coercion with JS implementation
dondonz Dec 5, 2022
64934d6
Add separate exception message for values outside integer range
dondonz Dec 5, 2022
954ec14
Tidy
dondonz Dec 5, 2022
fd23fc8
Fix up tests
dondonz Dec 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/main/java/graphql/scalar/GraphqlBooleanCoercing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,12 @@ private Boolean serializeImpl(@NotNull Object input, @NotNull Locale locale) {

@NotNull
private Boolean parseValueImpl(@NotNull Object input, @NotNull Locale locale) {
Boolean result = convertImpl(input);
if (result == null) {
if (!(input instanceof Boolean)) {
throw new CoercingParseValueException(
i18nMsg(locale, "Boolean.notBoolean", typeName(input))
i18nMsg(locale, "Boolean.unexpectedRawValueType", typeName(input))
);
}
return result;
return (Boolean) input;
}

private static boolean parseLiteralImpl(@NotNull Object input, @NotNull Locale locale) {
Expand Down
9 changes: 8 additions & 1 deletion src/main/java/graphql/scalar/GraphqlFloatCoercing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,20 @@ private Double serialiseImpl(Object input, @NotNull Locale locale) {
}

@NotNull
private Double parseValueImpl(Object input, @NotNull Locale locale) {
private Double parseValueImpl(@NotNull Object input, @NotNull Locale locale) {
if (!(input instanceof Number)) {
throw new CoercingParseValueException(
i18nMsg(locale, "Float.unexpectedRawValueType", typeName(input))
);
}

Double result = convertImpl(input);
if (result == null) {
throw new CoercingParseValueException(
i18nMsg(locale, "Float.notFloat", typeName(input))
);
}

return result;
}

Expand Down
38 changes: 35 additions & 3 deletions src/main/java/graphql/scalar/GraphqlIntCoercing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,46 @@ private Integer serialiseImpl(Object input, @NotNull Locale locale) {
}

@NotNull
private Integer parseValueImpl(Object input, @NotNull Locale locale) {
Integer result = convertImpl(input);
private Integer parseValueImpl(@NotNull Object input, @NotNull Locale locale) {
if (!(input instanceof Number)) {
throw new CoercingParseValueException(
i18nMsg(locale, "Int.notInt", typeName(input))
);
}

if (input instanceof Integer) {
return (Integer) input;
}

BigInteger result = convertParseValueImpl(input);
if (result == null) {
throw new CoercingParseValueException(
i18nMsg(locale, "Int.notInt", typeName(input))
);
}
return result;

if (result.compareTo(INT_MIN) < 0 || result.compareTo(INT_MAX) > 0) {
throw new CoercingParseValueException(
i18nMsg(locale, "Int.outsideRange", result.toString())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For values outside of the integer range, we previously threw an exception with the same Int.notInt message. We ought to use Int.outsideRange for this case, as we do in parseLiteral

);
}
return result.intValueExact();
}

private BigInteger convertParseValueImpl(Object input) {
BigDecimal value;
try {
value = new BigDecimal(input.toString());
} catch (NumberFormatException e) {
return null;
}

try {
return value.toBigIntegerExact();
} catch (ArithmeticException e) {
// Exception if number has non-zero fractional part
return null;
}
}

private static int parseLiteralImpl(Object input, @NotNull Locale locale) {
Expand Down
2 changes: 2 additions & 0 deletions src/main/resources/i18n/Scalars.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ ID.unexpectedAstType=Expected an AST type of ''IntValue'' or ''StringValue'' but
#
Float.notFloat=Expected a value that can be converted to type ''Float'' but it was a ''{0}''
Float.unexpectedAstType=Expected an AST type of ''IntValue'' or ''FloatValue'' but it was a ''{0}''
Float.unexpectedRawValueType=Expected a Number input, but it was a ''{0}''
#
Boolean.notBoolean=Expected a value that can be converted to type ''Boolean'' but it was a ''{0}''
Boolean.unexpectedAstType=Expected an AST type of ''BooleanValue'' but it was a ''{0}''
Boolean.unexpectedRawValueType=Expected a Boolean input, but it was a ''{0}''
#
String.unexpectedRawValueType=Expected a String input, but it was a ''{0}''
2 changes: 2 additions & 0 deletions src/main/resources/i18n/Scalars_de.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ ID.unexpectedAstType=Erwartet wurde ein AST type von ''IntValue'' oder ''StringV
#
Float.notFloat=Erwartet wurde ein Wert, der in den Typ ''Float'' konvertiert werden kann, aber es war ein ''{0}''
Float.unexpectedAstType=Erwartet wurde ein AST type von ''IntValue'' oder ''FloatValue'', aber es war ein ''{0}''
Float.unexpectedRawValueType=Erwartet wurde eine Number-Eingabe, aber es war ein ''{0}''
#
Boolean.notBoolean=Erwartet wurde ein Wert, der in den Typ ''Boolean'' konvertiert werden kann, aber es war ein ''{0}''
Boolean.unexpectedAstType=Erwartet wurde ein AST type ''BooleanValue'', aber es war ein ''{0}''
Boolean.unexpectedRawValueType=Erwartet wurde eine Boolean-Eingabe, aber es war ein ''{0}''
#
String.unexpectedRawValueType=Erwartet wurde eine String-Eingabe, aber es war ein ''{0}''
39 changes: 35 additions & 4 deletions src/test/groovy/graphql/ScalarsBooleanTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ class ScalarsBooleanTest extends Specification {
def "Boolean serialize #value into #result (#result.class)"() {
expect:
Scalars.GraphQLBoolean.getCoercing().serialize(value, GraphQLContext.default, Locale.default) == result
Scalars.GraphQLBoolean.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into separate test below


where:
value | result
Expand All @@ -75,7 +74,6 @@ class ScalarsBooleanTest extends Specification {
def "Boolean serialize #value into #result (#result.class) with deprecated methods"() {
expect:
Scalars.GraphQLBoolean.getCoercing().serialize(value) == result // Retain deprecated method for test coverage
Scalars.GraphQLBoolean.getCoercing().parseValue(value) == result // Retain deprecated method for test coverage

where:
value | result
Expand Down Expand Up @@ -111,6 +109,28 @@ class ScalarsBooleanTest extends Specification {
"f" | _
}

@Unroll
def "Boolean parseValue #value into #result (#result.class)"() {
expect:
Scalars.GraphQLBoolean.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result

where:
value | result
true | true
false | false
}

@Unroll
def "Boolean parseValue #value into #result (#result.class) with deprecated methods"() {
expect:
Scalars.GraphQLBoolean.getCoercing().parseValue(value) == result // Retain deprecated method for test coverage

where:
value | result
true | true
false | false
}

@Unroll
def "parseValue throws exception for invalid input #value"() {
when:
Expand All @@ -119,8 +139,19 @@ class ScalarsBooleanTest extends Specification {
thrown(CoercingParseValueException)

where:
value | _
new Object() | _
value | _
new Object() | _
"false" | _
"true" | _
"True" | _
0 | _
1 | _
-1 | _
new Long(42345784398534785l) | _
new Double(42.3) | _
new Float(42.3) | _
Integer.MAX_VALUE + 1l | _
Integer.MIN_VALUE - 1l | _
}

}
50 changes: 48 additions & 2 deletions src/test/groovy/graphql/ScalarsFloatTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ class ScalarsFloatTest extends Specification {
def "Float serialize #value into #result (#result.class)"() {
expect:
Scalars.GraphQLFloat.getCoercing().serialize(value, GraphQLContext.default, Locale.default) == result
Scalars.GraphQLFloat.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into separate test below


where:
value | result
Expand All @@ -84,7 +83,6 @@ class ScalarsFloatTest extends Specification {
def "Float serialize #value into #result (#result.class) with deprecated methods"() {
expect:
Scalars.GraphQLFloat.getCoercing().serialize(value) == result // Retain deprecated method for coverage
Scalars.GraphQLFloat.getCoercing().parseValue(value) == result // Retain deprecated method for coverage

where:
value | result
Expand Down Expand Up @@ -131,6 +129,51 @@ class ScalarsFloatTest extends Specification {
Float.NEGATIVE_INFINITY.toString() | _
}

@Unroll
def "Float parseValue #value into #result (#result.class)"() {
expect:
Scalars.GraphQLFloat.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result

where:
value | result
42.0000d | 42
new Integer(42) | 42
new BigInteger("42") | 42
new BigDecimal("42") | 42
new BigDecimal("4.2") | 4.2d
42.3f | 42.3d
42.0d | 42d
new Byte("42") | 42
new Short("42") | 42
1234567l | 1234567d
new AtomicInteger(42) | 42
Double.MAX_VALUE | Double.MAX_VALUE
Double.MIN_VALUE | Double.MIN_VALUE
}

@Unroll
def "Float parseValue #value into #result (#result.class) with deprecated methods"() {
expect:
Scalars.GraphQLFloat.getCoercing().parseValue(value) == result // Retain deprecated method for coverage

where:
value | result
42.0000d | 42
new Integer(42) | 42
new BigInteger("42") | 42
new BigDecimal("42") | 42
new BigDecimal("4.2") | 4.2d
42.3f | 42.3d
42.0d | 42d
new Byte("42") | 42
new Short("42") | 42
1234567l | 1234567d
new AtomicInteger(42) | 42
Double.MAX_VALUE | Double.MAX_VALUE
Double.MIN_VALUE | Double.MIN_VALUE
}


@Unroll
def "parseValue throws exception for invalid input #value"() {
when:
Expand All @@ -154,6 +197,9 @@ class ScalarsFloatTest extends Specification {
Float.POSITIVE_INFINITY.toString() | _
Float.NEGATIVE_INFINITY | _
Float.NEGATIVE_INFINITY.toString() | _
"42" | _
"42.123" | _
"-1" | _
}

}
47 changes: 45 additions & 2 deletions src/test/groovy/graphql/ScalarsIntTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ class ScalarsIntTest extends Specification {
def "Int serialize #value into #result (#result.class)"() {
expect:
Scalars.GraphQLInt.getCoercing().serialize(value, GraphQLContext.default, Locale.default) == result
Scalars.GraphQLInt.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result

where:
value | result
Expand All @@ -85,7 +84,6 @@ class ScalarsIntTest extends Specification {
def "Int serialize #value into #result (#result.class) with deprecated methods"() {
expect:
Scalars.GraphQLInt.getCoercing().serialize(value) == result // Retain deprecated for test coverage
Scalars.GraphQLInt.getCoercing().parseValue(value) == result // Retain deprecated for test coverage

where:
value | result
Expand Down Expand Up @@ -126,6 +124,48 @@ class ScalarsIntTest extends Specification {
new Object() | _
}

@Unroll
def "Int parseValue #value into #result (#result.class)"() {
expect:
Scalars.GraphQLInt.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result

where:
value | result
new Integer(42) | 42
new BigInteger("42") | 42
new Byte("42") | 42
new Short("42") | 42
1234567l | 1234567
new AtomicInteger(42) | 42
Integer.MAX_VALUE | Integer.MAX_VALUE
Integer.MIN_VALUE | Integer.MIN_VALUE
42.0000d | 42
new BigDecimal("42") | 42
42.0f | 42
42.0d | 42
}

@Unroll
def "Int parseValue #value into #result (#result.class) with deprecated methods"() {
expect:
Scalars.GraphQLInt.getCoercing().parseValue(value) == result // Retain deprecated for test coverage

where:
value | result
42.0000d | 42
new Integer(42) | 42
new BigInteger("42") | 42
new BigDecimal("42") | 42
42.0f | 42
42.0d | 42
new Byte("42") | 42
new Short("42") | 42
1234567l | 1234567
new AtomicInteger(42) | 42
Integer.MAX_VALUE | Integer.MAX_VALUE
Integer.MIN_VALUE | Integer.MIN_VALUE
}

@Unroll
def "parseValue throws exception for invalid input #value"() {
when:
Expand All @@ -144,6 +184,9 @@ class ScalarsIntTest extends Specification {
Integer.MAX_VALUE + 1l | _
Integer.MIN_VALUE - 1l | _
new Object() | _
"42" | _
"42.0000" | _
"-1" | _
}

}
8 changes: 4 additions & 4 deletions src/test/groovy/graphql/execution/ValuesResolverTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ class ValuesResolverTest extends Specification {
inputType | variableType | inputValue || outputValue
GraphQLInt | new TypeName("Int") | 100 || 100
GraphQLString | new TypeName("String") | 'someString' || 'someString'
GraphQLBoolean | new TypeName("Boolean") | 'true' || true
GraphQLFloat | new TypeName("Float") | '42.43' || 42.43d
GraphQLBoolean | new TypeName("Boolean") | true || true
GraphQLFloat | new TypeName("Float") | 42.43d || 42.43d
}

def "getVariableValues: map object as variable input"() {
Expand Down Expand Up @@ -641,7 +641,7 @@ class ValuesResolverTest extends Specification {
executionResult.data == null
executionResult.errors.size() == 1
executionResult.errors[0].errorType == ErrorType.ValidationError
executionResult.errors[0].message == "Variable 'input' has an invalid value: Expected a value that can be converted to type 'Boolean' but it was a 'String'"
executionResult.errors[0].message == "Variable 'input' has an invalid value: Expected a Boolean input, but it was a 'String'"
executionResult.errors[0].locations == [new SourceLocation(2, 35)]
}

Expand Down Expand Up @@ -679,7 +679,7 @@ class ValuesResolverTest extends Specification {
executionResult.data == null
executionResult.errors.size() == 1
executionResult.errors[0].errorType == ErrorType.ValidationError
executionResult.errors[0].message == "Variable 'input' has an invalid value: Expected a value that can be converted to type 'Float' but it was a 'String'"
executionResult.errors[0].message == "Variable 'input' has an invalid value: Expected a Number input, but it was a 'String'"
executionResult.errors[0].locations == [new SourceLocation(2, 35)]
}
}