graphql: Coerce value for scalar types correctly. #5487

pawanrawal · 2020-05-20T14:29:50Z

Fixes GRAPHQL-382

This change is

…teger

abhimanyusinghgaur

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @harshil-goel, @MichaelJCompton, and @pawanrawal)

graphql/resolve/resolver.go, line 1341 at r1 (raw file):

Quoted 4 lines of code…

			case float64:
				val = v > 0
			case int64:
				val = v > 0

It will give false for negative numbers too.
For Boolean, the example GraphQL spec proposes is: returning true for non‐zero numbers, although its not mandatory to follow.
So, should we change the condition to: v != 0?

graphql/resolve/resolver.go, line 1362 at r1 (raw file):

				}
			case bool:
				if !v {

A simplification:

if v {
   val = 1
} else {
   val = 0
}

graphql/resolve/resolver.go, line 1398 at r1 (raw file):

			switch v := val.(type) {
			case bool:
				if !v {

Simplification possible.

graphql/resolve/resolver.go, line 1415 at r1 (raw file):

				return nil, valueCoercionError(v)
			}
		case "DateTime":

can we consider int64 as time since Epoch and parse it as DateTime instead of giving coercion error?

graphql/resolve/resolver.go, line 1449 at r1 (raw file):

		}

		// val is a scalar

We should move this comment at the top of this newly inserted code block.

graphql/resolve/resolver.go, line 1453 at r1 (raw file):

		// Can this ever error?  We can't have an unsupported type or value because
		// we just unmarshaled this val.
		json, err := json.Marshal(val)

Unrelated, but my IDE gives me this warning:
Variable 'json' collides with imported package name

We should rename it to something like b.

graphql/resolve/resolver_test.go, line 143 at r1 (raw file):

			}},
			Expected: `{"getPost": {"numLikes": null}}`,
		},

Also, a test case for when we can't coerce an int value which is out of range of int32 to Int.

graphql/resolve/resolver_test.go, line 175 at r1 (raw file):

		// test bool/int/string/datetime can be coerced to Datetime
		{Name: "int value should raise an error when tried to be coerced to datetime",

this will change if we interpret int as time since Epoch.

graphql/resolve/resolver_test.go, line 204 at r1 (raw file):

		{Name: "invalid string value should raise an error when tried to be coerced to datetime",
			GQLQuery: `query { getAuthor(id: "0x1") { dob } }`,
			Response: `{ "getAuthor": { "dob": "123" }}`,

If we are interpreting int as time since Epoch, then I guess this should also change, and should have a different case to show the error.

abhimanyusinghgaur

Another thing is, while validating remote schema for @custom graphql, currently we mandate exact type match. We should create a task for that too, to respect coercion once this is merged.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @harshil-goel, @MichaelJCompton, and @pawanrawal)

pawanrawal

Sure, will do once I understand it better.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @abhimanyusinghgaur, @harshil-goel, and @MichaelJCompton)

graphql/resolve/resolver.go, line 1341 at r1 (raw file):