Skip to content

Revert stricter scalar parseValue coercion #3186

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 9 commits into from
Apr 17, 2023
Merged

Revert stricter scalar parseValue coercion #3186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
94dffa4
Add test to revert parseValue behaviour for String
dondonz Apr 14, 2023
90bf567
Revert String parseValue coercion to accept non-String input
dondonz Apr 14, 2023
ca4836b
Add tests to revert stricter Boolean parseValue coercion
dondonz Apr 14, 2023
753c2ec
Revert stricter parseValue coercion for Boolean
dondonz Apr 14, 2023
64a9204
Add tests to revert stricter parseValue coercion for Float
dondonz Apr 14, 2023
24c7eb7
Revert stricter parseValue coercion for Float
dondonz Apr 14, 2023
971923c
Add tests for reverting stricter parseValue for Int
dondonz Apr 14, 2023
82e0712
Revert stricter parseValue coercion for Int
dondonz Apr 14, 2023
951e342
Fix changed strings in ValuesResolver
dondonz Apr 14, 2023
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: 4 additions & 3 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,12 +68,13 @@ private Boolean serializeImpl(@NotNull Object input, @NotNull Locale locale) {

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

private static boolean parseLiteralImpl(@NotNull Object input, @NotNull Locale locale) {
Expand Down
6 changes: 0 additions & 6 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,12 +65,6 @@ 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
34 changes: 2 additions & 32 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,45 +64,15 @@ private Integer serialiseImpl(Object input, @NotNull Locale locale) {

@NotNull
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;
}
Integer result = convertImpl(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();
}

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;
}
return result;
}

private static int parseLiteralImpl(Object input, @NotNull Locale locale) {
Expand Down
13 changes: 2 additions & 11 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,15 +28,6 @@ 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 @@ -64,12 +55,12 @@ public String serialize(@NotNull Object dataFetcherResult) {
@Override
@Deprecated
public String parseValue(@NotNull Object input) {
return parseValueImpl(input, Locale.getDefault());
return toStringImpl(input);
}

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

@Override
Expand Down
4 changes: 0 additions & 4 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,10 +24,6 @@ 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: 0 additions & 4 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,10 +24,6 @@ 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}''
32 changes: 21 additions & 11 deletions src/test/groovy/graphql/ScalarsBooleanTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,27 @@ 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 @@ -141,17 +162,6 @@ class ScalarsBooleanTest extends Specification {
where:
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 | _
}

}
9 changes: 6 additions & 3 deletions src/test/groovy/graphql/ScalarsFloatTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ class ScalarsFloatTest extends Specification {
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 @@ -171,6 +174,9 @@ class ScalarsFloatTest extends Specification {
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 @@ -197,9 +203,6 @@ class ScalarsFloatTest extends Specification {
Float.POSITIVE_INFINITY.toString() | _
Float.NEGATIVE_INFINITY | _
Float.NEGATIVE_INFINITY.toString() | _
"42" | _
"42.123" | _
"-1" | _
}

}
21 changes: 12 additions & 9 deletions src/test/groovy/graphql/ScalarsIntTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,15 @@ class ScalarsIntTest extends Specification {
new Short("42") | 42
1234567l | 1234567
new AtomicInteger(42) | 42
Integer.MAX_VALUE | Integer.MAX_VALUE
Integer.MIN_VALUE | Integer.MIN_VALUE
Copy link
Member Author

@dondonz dondonz Apr 14, 2023

Choose a reason for hiding this comment

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

Some reorganising so these two adjacent tests are ordered in the same way

42.0000d | 42
new BigDecimal("42") | 42
42.0f | 42
42.0d | 42
Integer.MAX_VALUE | Integer.MAX_VALUE
Integer.MIN_VALUE | Integer.MIN_VALUE
"42" | 42
"42.0000" | 42
"-1" | -1
}

@Unroll
Expand All @@ -152,18 +155,21 @@ class ScalarsIntTest extends Specification {

where:
value | result
42.0000d | 42
new Integer(42) | 42
new BigInteger("42") | 42
new BigDecimal("42") | 42
42.0f | 42
42.0d | 42
Copy link
Member Author

@dondonz dondonz Apr 14, 2023

Choose a reason for hiding this comment

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

Reorganising

new Byte("42") | 42
new Short("42") | 42
1234567l | 1234567
new AtomicInteger(42) | 42
42.0000d | 42
new BigDecimal("42") | 42
42.0f | 42
42.0d | 42
Integer.MAX_VALUE | Integer.MAX_VALUE
Integer.MIN_VALUE | Integer.MIN_VALUE
"42" | 42
"42.0000" | 42
"-1" | -1
}

@Unroll
Expand All @@ -184,9 +190,6 @@ class ScalarsIntTest extends Specification {
Integer.MAX_VALUE + 1l | _
Integer.MIN_VALUE - 1l | _
new Object() | _
"42" | _
"42.0000" | _
"-1" | _
}

}
24 changes: 7 additions & 17 deletions src/test/groovy/graphql/ScalarsStringTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import graphql.execution.CoercedVariables
import graphql.language.BooleanValue
import graphql.language.StringValue
import graphql.schema.CoercingParseLiteralException
import graphql.schema.CoercingParseValueException
import spock.lang.Shared
import spock.lang.Specification
import spock.lang.Unroll
Expand Down Expand Up @@ -86,24 +85,15 @@ class ScalarsStringTest extends Specification {
}

@Unroll
def "String parseValue throws exception for non-String values"() {
when:
Scalars.GraphQLString.getCoercing().parseValue(literal, GraphQLContext.default, Locale.default)
then:
def ex = thrown(CoercingParseValueException)
def "String parseValue can parse non-String values"() {
expect:
Scalars.GraphQLString.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result

where:
literal | _
123 | _
true | _
customObject | _
value | result
123 | "123"
true | "true"
customObject | "foo"
Copy link
Member Author

@dondonz dondonz Apr 14, 2023

Choose a reason for hiding this comment

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

As all Java objects must have a toString method, there will always be a result for parseValue with the looser coercion.

}

def "String parseValue English exception message"() {
when:
Scalars.GraphQLString.getCoercing().parseValue(9001, GraphQLContext.default, Locale.ENGLISH)
then:
def ex = thrown(CoercingParseValueException)
ex.message == "Expected a String input, but it was a 'Integer'"
}
}
4 changes: 2 additions & 2 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 @@ -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 Boolean input, but it was a 'String'"
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].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 Number input, but it was a 'String'"
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].locations == [new SourceLocation(2, 35)]
}
}