Skip to content

Stricter parseValue coercion for String, Int, Float, Boolean #3553

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
dondonz merged 7 commits into from
Apr 2, 2024
Merged

Stricter parseValue coercion for String, Int, Float, Boolean #3553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
da2bce1
Add error strings
dondonz Apr 1, 2024
b7a648c
Add stricter string parseValue coercion
dondonz Apr 1, 2024
0336f9b
Add stricter Boolean parseValue coercion
dondonz Apr 1, 2024
2aecb85
Add stricter float parseValue coercion
dondonz Apr 1, 2024
ea7661f
Add back stricter parseValue coercion for Int
dondonz Apr 1, 2024
8dfdcc4
Fix tests
dondonz Apr 1, 2024
b7dc516
Tidy up
dondonz Apr 2, 2024
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
9 changes: 4 additions & 5 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 @@ -67,13 +67,12 @@ private Boolean serializeImpl(@NotNull Object input, @NotNull Locale locale) {

Copy link
Member Author

@dondonz dondonz Apr 1, 2024

Choose a reason for hiding this comment

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

This PR is a reversion of PR #3186, plus little bits of tidying up

@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 Expand Up @@ -129,7 +128,7 @@ public Boolean parseLiteral(@NotNull Object input) {

@Override
@Deprecated
public Value valueToLiteral(@NotNull Object input) {
public @NotNull Value<?> valueToLiteral(@NotNull Object input) {
Copy link
Member Author

@dondonz dondonz Apr 1, 2024

Choose a reason for hiding this comment

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

IDEA complained so I'm fixing these up while I'm here

return valueToLiteralImpl(input, Locale.getDefault());
}

Expand Down
6 changes: 6 additions & 0 deletions src/main/java/graphql/scalar/GraphqlFloatCoercing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ private Double serialiseImpl(Object input, @NotNull Locale locale) {

@NotNull
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(
Expand Down
35 changes: 32 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 @@ -64,15 +64,44 @@ private Integer serialiseImpl(Object input, @NotNull Locale locale) {

@NotNull
private Integer parseValueImpl(@NotNull Object input, @NotNull Locale locale) {
Integer result = convertImpl(input);
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))
);
}
if (result.compareTo(INT_MIN) < 0 || result.compareTo(INT_MAX) > 0) {
throw new CoercingParseValueException(
i18nMsg(locale, "Int.outsideRange", result.toString())
);
}
return result.intValueExact();
}

return result;
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 Expand Up @@ -134,7 +163,7 @@ public Integer parseLiteral(@NotNull Object input) {

@Override
@Deprecated
public Value valueToLiteral(@NotNull Object input) {
public @NotNull Value<?> valueToLiteral(@NotNull Object input) {
return valueToLiteralImpl(input, Locale.getDefault());
}

Expand Down
15 changes: 12 additions & 3 deletions src/main/java/graphql/scalar/GraphqlStringCoercing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,15 @@ private String toStringImpl(Object input) {
return String.valueOf(input);
}

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

private String parseLiteralImpl(@NotNull Object input, Locale locale) {
if (!(input instanceof StringValue)) {
throw new CoercingParseLiteralException(
Expand Down Expand Up @@ -55,12 +64,12 @@ public String serialize(@NotNull Object dataFetcherResult) {
@Override
@Deprecated
public String parseValue(@NotNull Object input) {
return toStringImpl(input);
return parseValueImpl(input, Locale.getDefault());
}

@Override
public String parseValue(@NotNull Object input, @NotNull GraphQLContext graphQLContext, @NotNull Locale locale) throws CoercingParseValueException {
return toStringImpl(input);
return parseValueImpl(input, locale);
}

@Override
Expand All @@ -76,7 +85,7 @@ public String parseLiteral(@NotNull Object input) {

@Override
@Deprecated
public Value valueToLiteral(@NotNull Object input) {
public @NotNull Value<?> valueToLiteral(@NotNull Object input) {
return valueToLiteralImpl(input);
}

Expand Down
4 changes: 4 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,6 +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}''
4 changes: 4 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 @@ -24,6 +24,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}''
7 changes: 7 additions & 0 deletions src/main/resources/i18n/Scalars_nl.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ ID.unexpectedAstType=Verwacht werd een AST-type ''IntValue'' of ''StringValue'',
#
Float.notFloat=Verwacht werd een waarde die in ''Float'' veranderd kon worden, maar het was een ''{0}''
Float.unexpectedAstType=Verwacht werd een AST-type ''IntValue'' of ''FloatValue'', maar het was een ''{0}''
# TODO: To be translated into Dutch
Float.unexpectedRawValueType=Expected a Number input, but it was a ''{0}''
#
Boolean.notBoolean=Verwacht werd een waarde die in ''Boolean'' veranderd kon worden, maar het was een ''{0}''
Boolean.unexpectedAstType=Verwacht werd een AST-type ''BooleanValue'', maar het was een ''{0}''
# TODO: To be translated into Dutch
Boolean.unexpectedRawValueType=Expected a Boolean input, but it was a ''{0}''
#
# TODO: To be translated into Dutch
String.unexpectedRawValueType=Expected a String input, but it was a ''{0}''
88 changes: 39 additions & 49 deletions src/test/groovy/graphql/ScalarsBooleanTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,19 @@ class ScalarsBooleanTest extends Specification {
Scalars.GraphQLBoolean.getCoercing().serialize(value, GraphQLContext.default, Locale.default) == result

where:
value | result
true | true
"false" | false
"true" | true
"True" | true
0 | false
1 | true
-1 | true
new Long(42345784398534785l) | true
Copy link
Member Author

@dondonz dondonz Apr 1, 2024

Choose a reason for hiding this comment

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

A bunch of constructors were deprecated in Java 9. Now we're on Java 11 let's update these

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

@Unroll
Expand All @@ -76,19 +76,19 @@ class ScalarsBooleanTest extends Specification {
Scalars.GraphQLBoolean.getCoercing().serialize(value) == result // Retain deprecated method for test coverage

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

@Unroll
Expand Down Expand Up @@ -131,27 +131,6 @@ class ScalarsBooleanTest extends Specification {
false | false
}

@Unroll
def "parseValue parses non-Boolean input #value"() {
expect:
Scalars.GraphQLBoolean.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result

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

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

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

}
33 changes: 15 additions & 18 deletions src/test/groovy/graphql/ScalarsFloatTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,15 @@ class ScalarsFloatTest extends Specification {
"42" | 42d
"42.123" | 42.123d
42.0000d | 42
new Integer(42) | 42
Integer.valueOf(42) | 42
"-1" | -1
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
Byte.valueOf("42") | 42
Short.valueOf("42") | 42
1234567l | 1234567d
new AtomicInteger(42) | 42
Double.MAX_VALUE | Double.MAX_VALUE
Expand All @@ -89,15 +89,15 @@ class ScalarsFloatTest extends Specification {
"42" | 42d
"42.123" | 42.123d
42.0000d | 42
new Integer(42) | 42
Integer.valueOf(42) | 42
"-1" | -1
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
Byte.valueOf("42") | 42
Short.valueOf("42") | 42
1234567l | 1234567d
new AtomicInteger(42) | 42
Double.MAX_VALUE | Double.MAX_VALUE
Expand Down Expand Up @@ -137,21 +137,18 @@ class ScalarsFloatTest extends Specification {
where:
value | result
42.0000d | 42
new Integer(42) | 42
Integer.valueOf(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
Byte.valueOf("42") | 42
Short.valueOf("42") | 42
1234567l | 1234567d
new AtomicInteger(42) | 42
Double.MAX_VALUE | Double.MAX_VALUE
Double.MIN_VALUE | Double.MIN_VALUE
"42" | 42d
"42.123" | 42.123d
"-1" | -1
}

@Unroll
Expand All @@ -162,21 +159,18 @@ class ScalarsFloatTest extends Specification {
where:
value | result
42.0000d | 42
new Integer(42) | 42
Integer.valueOf(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
Byte.valueOf("42") | 42
Short.valueOf("42") | 42
1234567l | 1234567d
new AtomicInteger(42) | 42
Double.MAX_VALUE | Double.MAX_VALUE
Double.MIN_VALUE | Double.MIN_VALUE
"42" | 42d
"42.123" | 42.123d
"-1" | -1
}


Expand All @@ -203,6 +197,9 @@ class ScalarsFloatTest extends Specification {
Float.POSITIVE_INFINITY.toString() | _
Float.NEGATIVE_INFINITY | _
Float.NEGATIVE_INFINITY.toString() | _
"42" | _
"42.123" | _
"-1" | _
}

}
Loading