Float coercion tidy up by dondonz · Pull Request #2982 · graphql-java/graphql-java

dondonz · 2022-10-10T06:15:29Z

The GraphQL Float spec was updated a year ago to specifically not allow NaN and positive/negative infinity in both result coercion and input coercion

https://spec.graphql.org/draft/#sec-Float
graphql/graphql-spec#778

The float coercing was mostly compliant with this spec update, as NaN and positive/negative infinity are not allowed in the BigDecimal String constructor used in GraphqlFloatCoercing, specifically the line value = new BigDecimal(input.toString());.

I say mostly, there was one uncaught case. It's possible for the old convertImpl method to return a Double value that is later converted to positive infinity with BigDecimal.valueOf, such as in the valueToLiteralImpl

private FloatValue valueToLiteralImpl(Object input, @NotNull Locale locale) {
    Double result = assertNotNull(convertImpl(input), () -> i18nMsg(locale, "Float.notFloat", typeName(input)));
    return FloatValue.newFloatValue(BigDecimal.valueOf(result)).build();
}

This PR introduces an explicit check for positive/negative infinity and NaN to address this problem. This PR cleans up NaN and infinity tests.

dondonz · 2022-10-10T06:18:55Z

src/main/java/graphql/scalar/GraphqlFloatCoercing.java

+                return null;
+            }
+            return doubleInput;
+        } else if (isNumberIsh(input)) {


I did not add a separate else if (input instanceof Float) branch because it causes some tests to fail.

Getting the doubleValue() from a BigDecimal produces an ever-so-slightly-different result to getting the doubleValue() out of a Float.

Current this is

private Double convertImpl(Object input) { // From the GraphQL Float spec, non-finite floating-point internal values (NaN and Infinity) // must raise a field error on both result and input coercion if (input instanceof Double) { Double doubleInput = (Double) input; if (Double.isNaN(doubleInput) || Double.isInfinite(doubleInput)) { return null; } return doubleInput; } else if (isNumberIsh(input)) { BigDecimal value; try { value = new BigDecimal(input.toString()); } catch (NumberFormatException e) { return null; } return value.doubleValue(); } else { return null; } }

So it does not apply the checks to the BigDecimal derived string

Why not do

private Double convertImpl(Object input) { // From the GraphQL Float spec, non-finite floating-point internal values (NaN and Infinity) // must raise a field error on both result and input coercion Double doubleInput; if (input instanceof Double) { doubleInput = (Double) input; } else if (isNumberIsh(input)) { BigDecimal value; try { value = new BigDecimal(input.toString()); } catch (NumberFormatException e) { return null; } doubleInput = value.doubleValue(); } else { return null; } if (Double.isNaN(doubleInput) || Double.isInfinite(doubleInput)) { return null; } return doubleInput }

Forgive me if this doesnt compile

This seems more sensible - eg first get it into a double and then check it - regardless

Even if BigDecimal never produces Nan or infinity, the logic reads better

Oh no, it can produce infinity! This was a great idea

dondonz · 2022-10-10T06:19:43Z

src/test/groovy/graphql/ScalarsQuerySchema.java


    public static final GraphQLObjectType queryType = newObject()
            .name("QueryType")
-            // Static scalars


This was added a long time ago, before the GraphQL Float spec was updated to specifically not allow NaN and positive/negative infinity values

bbakerman

Can we tweak it as suggested or justify why not :)

bbakerman · 2022-10-10T06:34:41Z

src/main/java/graphql/scalar/GraphqlFloatCoercing.java

+                return null;
+            }
+            return doubleInput;
+        } else if (isNumberIsh(input)) {


Current this is

private Double convertImpl(Object input) { // From the GraphQL Float spec, non-finite floating-point internal values (NaN and Infinity) // must raise a field error on both result and input coercion if (input instanceof Double) { Double doubleInput = (Double) input; if (Double.isNaN(doubleInput) || Double.isInfinite(doubleInput)) { return null; } return doubleInput; } else if (isNumberIsh(input)) { BigDecimal value; try { value = new BigDecimal(input.toString()); } catch (NumberFormatException e) { return null; } return value.doubleValue(); } else { return null; } }

So it does not apply the checks to the BigDecimal derived string

Why not do

private Double convertImpl(Object input) { // From the GraphQL Float spec, non-finite floating-point internal values (NaN and Infinity) // must raise a field error on both result and input coercion Double doubleInput; if (input instanceof Double) { doubleInput = (Double) input; } else if (isNumberIsh(input)) { BigDecimal value; try { value = new BigDecimal(input.toString()); } catch (NumberFormatException e) { return null; } doubleInput = value.doubleValue(); } else { return null; } if (Double.isNaN(doubleInput) || Double.isInfinite(doubleInput)) { return null; } return doubleInput }

Forgive me if this doesnt compile

bbakerman · 2022-10-10T06:35:48Z

src/main/java/graphql/scalar/GraphqlFloatCoercing.java

+                return null;
+            }
+            return doubleInput;
+        } else if (isNumberIsh(input)) {


This seems more sensible - eg first get it into a double and then check it - regardless

Even if BigDecimal never produces Nan or infinity, the logic reads better

bbakerman · 2022-10-10T06:36:44Z

src/test/groovy/graphql/ScalarsFloatTest.groovy

+        Double.NEGATIVE_INFINITY | _
+        Float.NaN                | _
+        Float.POSITIVE_INFINITY  | _
+        Float.NEGATIVE_INFINITY  | _


Can we have a string rep here? Float.NEGATIVE_INFINITY.toString() ?

public static void main(String[] args) { String negInf = String.valueOf(Float.NEGATIVE_INFINITY); System.out.println(negInf); double dv = new BigDecimal(negInf).doubleValue(); System.out.println(dv); } -Infinity Exception in thread "main" java.lang.NumberFormatException at java.math.BigDecimal.<init>(BigDecimal.java:494) at java.math.BigDecimal.<init>(BigDecimal.java:383) at java.math.BigDecimal.<init>(BigDecimal.java:806)

So its not acceptable but lets test anyway to show that we handle it

dondonz · 2022-10-16T21:07:20Z

src/main/java/graphql/execution/ValuesResolverLegacy.java

        final Object serialized = serializeLegacy(type, value, graphqlContext, locale);
-        if (isNullishLegacy(serialized)) {
-            return null;
-        }


This NaN check was unnecessary. The NaN check already happened inside the serialize method.

dondonz · 2022-10-16T21:07:55Z

src/main/java/graphql/scalar/GraphqlFloatCoercing.java

+        if (Double.isNaN(doubleInput) || Double.isInfinite(doubleInput)) {
+            return null;
+        }
+        return doubleInput;


Move NaN/Infinity check lower down to make this easier to read

Edit: not only easier to read, it catches the edge case where value.doubleValue() returns positive/negative infinity

dondonz · 2022-10-16T21:08:14Z

src/test/groovy/graphql/ScalarsFloatTest.groovy

+        Float.POSITIVE_INFINITY             | _
+        Float.POSITIVE_INFINITY.toString()  | _
+        Float.NEGATIVE_INFINITY             | _
+        Float.NEGATIVE_INFINITY.toString()  | _


Add String representations to test

dondonz · 2022-10-16T21:09:33Z

src/test/groovy/graphql/execution/ValuesResolverTestLegacy.groovy

@@ -1,6 +1,13 @@
-package graphql.language
+package graphql.execution


Tidy up package name and constructor usage

bbakerman

Merge away

dondonz added 3 commits October 10, 2022 16:32

Add all Float and Double coercion exception cases

cf7da3c

Clean up tests, remove no longer allowed NaN value

39d5cb7

Add early return for Doubles

1c63f44

dondonz commented Oct 10, 2022

View reviewed changes

dondonz added this to the 20.0 milestone Oct 10, 2022

bbakerman requested changes Oct 10, 2022

View reviewed changes

dondonz added 4 commits October 17, 2022 07:50

Reorder NaN/Infinity check and add string representation to test

216380c

Remove unnecessary NaN check

b74d960

Tidy package name

978bfd3

Tidy up

3b032cd

dondonz commented Oct 16, 2022

View reviewed changes

dondonz requested a review from bbakerman October 16, 2022 21:11

bbakerman approved these changes Oct 27, 2022

View reviewed changes

bbakerman merged commit 6335cf0 into master Oct 27, 2022

dondonz deleted the float-coercion branch October 27, 2022 06:15

dondonz mentioned this pull request Oct 30, 2022

Bump graphql-java version to include float coercion fix atlassian-labs/nadel#444

Merged

		@@ -1,6 +1,13 @@
		package graphql.language
		package graphql.execution

Comments

Conversation

dondonz commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dondonz Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbakerman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dondonz Oct 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbakerman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dondonz commented Oct 10, 2022 •

edited

Loading

dondonz Oct 27, 2022 •

edited

Loading

dondonz Oct 16, 2022 •

edited

Loading