-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
fix(GraphQL): This PR extend int64 range to 64-bit numeric values and adds input coercing and validation for integers. #6275
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)
graphql/schema/validation_rules.go, line 69 at r1 (raw file):
} func intRangeCheck(observers *validator.Events, addError validator.AddErrFunc) {
Do we need another check for the Int64 type that we introduced recently?
graphql/schema/validation_rules.go, line 79 at r1 (raw file):
} intVal, err := strconv.ParseInt(value.Raw, 10, 64)
Shouldn't this be parsed with bit size 32? That already gives an out of bound error.
From https://golang.org/pkg/strconv/#ParseInt
if the value corresponding to s cannot be represented by a signed integer of the given size, err.Err = ErrRange
So maybe you can check for the error if its ErrRange, then you can throw the Out of range error otherwise whatever error is returned.
graphql/schema/validation_rules.go, line 88 at r1 (raw file):
} addError(validator.Message("Out of range value '%s', for Variable type `%s`",
The value.Definition.Name is always going to be Int
so no need to substitute it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @jatindevdg and @MichaelJCompton)
graphql/e2e/common/error_test.yaml, line 120 at r1 (raw file):
"Out of range value '2147483648', for Variable type `Int`"
Is it possible to change it to:
"Out of range value '2147483648', for field `numLikes` of type `Int`"
or, if it is not possible to get numLikes
, then:
"Out of range value '2147483648', for type `Int`"
Variable
doesn't feel right in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/error_test.yaml, line 120 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
"Out of range value '2147483648', for Variable type `Int`"
Is it possible to change it to:
"Out of range value '2147483648', for field `numLikes` of type `Int`"
or, if it is not possible to get
numLikes
, then:"Out of range value '2147483648', for type `Int`"
Variable
doesn't feel right in this context.
field name is not there , changed it to Out of range value '2147483648', for type Int
or 'int64'.
graphql/schema/validation_rules.go, line 69 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Do we need another check for the Int64 type that we introduced recently?
yes, done.
graphql/schema/validation_rules.go, line 79 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Shouldn't this be parsed with bit size 32? That already gives an out of bound error.
From https://golang.org/pkg/strconv/#ParseInt
if the value corresponding to s cannot be represented by a signed integer of the given size, err.Err = ErrRange
So maybe you can check for the error if its ErrRange, then you can throw the Out of range error otherwise whatever error is returned.
changed.
graphql/schema/validation_rules.go, line 88 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
The value.Definition.Name is always going to be
Int
so no need to substitute it
didn't changed it because,Now we also have int64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 9 files reviewed, 7 unresolved discussions (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/error_test.yaml, line 176 at r2 (raw file):
9007199300000000
It was supposed to be negative, but there is no negative sign here.
And 2^53
is 9007199254740992
not 9007199300000000
. I suspect the calculator being used is itself not precise.
graphql/e2e/common/mutation.go, line 3654 at r2 (raw file):
9223372036854775000
It looks something close to 2^63
but is not 2^63
, because powers of 2 don't end with 0
s.
2^63 = 9,223,372,036,854,775,808
Also, if we are giving errors for it being greater than (2^53)-1
than how can this test pass?
graphql/schema/validation_rules.go, line 99 at r2 (raw file):
func intRangeCheck(observers *validator.Events, addError validator.AddErrFunc) { observers.OnValue(func(walker *validator.Walker, value *ast.Value) { if value.Definition == nil || value.ExpectedType == nil || value.Kind != ast.IntValue{
what if we receive value in a GraphQL variable instead of directly in the query?
graphql/schema/validation_rules.go, line 116 at r2 (raw file):
float64(val) == (-1)*math.Pow(2, 53)
we shouldn't be converting to float and then compare, because that is exactly where we lose the precision. We should just keep the comparison between int64 values. Also, since we know the RHS in this equation, we can keep it as a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 9 files at r2, 6 of 6 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jatindevdg and @MichaelJCompton)
graphql/e2e/common/error_test.yaml, line 171 at r5 (raw file):
{ } errors: [ { "message": "Given input type mismatch, Expected Int64",
Can we have the field name or the value as part of the error message.
graphql/e2e/directives/schema.graphql, line 178 at r5 (raw file):
type post1{ id:ID title:String! @id @search(by: [regexp])
Should be formatted properly.
graphql/e2e/directives/schema.graphql, line 181 at r5 (raw file):
numLikes:Int64 numViews:Int64 numComments:Int64
can be removed as its not being used. You only probably need one value here and you can create two posts.
graphql/schema/validation_rules.go, line 123 at r5 (raw file):
} func intRangeCheck1(observers *validator.Events, addError validator.AddErrFunc) {
should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 47 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/error_test.yaml, line 176 at r2 (raw file):
modified.
graphql/e2e/common/error_test.yaml, line 171 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Can we have the field name or the value as part of the error message.
added value.
graphql/e2e/common/mutation.go, line 3654 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
9223372036854775000
It looks something close to
2^63
but is not2^63
, because powers of 2 don't end with0
s.
2^63 = 9,223,372,036,854,775,808
Also, if we are giving errors for it being greater than(2^53)-1
than how can this test pass?
modified the test to check rangefor int64.
graphql/e2e/directives/schema.graphql, line 178 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Should be formatted properly.
done.
graphql/e2e/directives/schema.graphql, line 181 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
can be removed as its not being used. You only probably need one value here and you can create two posts.
done.
graphql/schema/validation_rules.go, line 99 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
what if we receive value in a GraphQL variable instead of directly in the query?
Right now , we get output coercing error for variables as we were getting previously. Input coercing for variables will be added in separate PR.
graphql/schema/validation_rules.go, line 116 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
float64(val) == (-1)*math.Pow(2, 53)
we shouldn't be converting to float and then compare, because that is exactly where we lose the precision. We should just keep the comparison between int64 values. Also, since we know the RHS in this equation, we can keep it as a constant.
done.
graphql/schema/validation_rules.go, line 123 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
should be removed
removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 48 files reviewed, 18 unresolved discussions (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/error_test.yaml, line 119 at r6 (raw file):
- name: "Out of range error for int32 type"
also an out of range error case with 2^63 for Int64
graphql/e2e/common/mutation.go, line 3656 at r6 (raw file):
-9223372036854775807
should this be -9223372036854775808
, i.e. the actual negative bound?
graphql/e2e/common/mutation.go, line 3672 at r6 (raw file):
9223372036854775808
shouldn't this be 9223372036854775807
?
graphql/resolve/resolver.go, line 1509 at r6 (raw file):
// numUids are added as int, so we need special handling for that.
a new line before this like comments in other cases. Also, we can just keep this case int: ...
part after case int64
part to minimize the git diff.
graphql/resolve/resolver.go, line 1532 at r6 (raw file):
Quoted 13 lines of code…
case float64: // The spec says that we can coerce a Float value to Int, if we don't lose information. // See: https: //spec.graphql.org/June2018/#sec-Float // Lets try to see if this number could be converted to int64 without losing // information, otherwise return error. // See: https://github.com/golang/go/issues/19405 to understand why the comparison // should be done after double conversion. i64Val := int64(v) if v == float64(i64Val) { val = i64Val } else { return nil, valueCoercionError(v) }
I guess , this part could also be removed now that we are using json Number to represent Int64 internally, so it will no longer be parsed to float.
graphql/resolve/resolver.go, line 1466 at r7 (raw file):
valFloat, _ := v.Float64() val = strconv.FormatFloat(valFloat, 'f', -1, 64)
for string, we can just do:
val = v.String()
no need to parse it to float and then parse it back to a string.
graphql/resolve/resolver.go, line 1474 at r7 (raw file):
Quoted 4 lines of code…
case float64: val = v != 0 case int64: val = v != 0
now maybe we can remove these cases.
graphql/resolve/resolver.go, line 1492 at r7 (raw file):
Quoted 13 lines of code…
case float64: // The spec says that we can coerce a Float value to Int, if we don't lose information. // See: https: //spec.graphql.org/June2018/#sec-Float // Lets try to see if this number could be converted to int32 without losing // information, otherwise return error. // See: https://github.com/golang/go/issues/19405 to understand why the comparison // should be done after double conversion. i32Val := int32(v) if v == float64(i32Val) { val = i32Val } else { return nil, valueCoercionError(v) }
I guess this case could be removed now.
graphql/resolve/resolver.go, line 1508 at r7 (raw file):
32
the reason for this being 64 was that a 32-bit float won't be able to represent the full range of 32-bit ints. So, we should change it back to 64.
graphql/resolve/resolver.go, line 1520 at r7 (raw file):
val = i
execution flow won't reach this line, can be removed.
graphql/resolve/resolver.go, line 1532 at r7 (raw file):
32
64
graphql/resolve/resolver.go, line 1542 at r7 (raw file):
val = i
stale line, should be removed
graphql/resolve/resolver.go, line 1620 at r7 (raw file):
Quoted 12 lines of code…
case float64: truncated := math.Trunc(v) if truncated == v { // Lets interpret int values as unix timestamp. t := time.Unix(int64(truncated), 0).UTC() val = t.Format(time.RFC3339) } else { return nil, valueCoercionError(v) } case int64: t := time.Unix(v, 0).UTC() val = t.Format(time.RFC3339)
so, both these cases could be removed now.
graphql/schema/gqlschema.go, line 66 at r6 (raw file):
schemaExtras = ` """ The Int64 scalar type represents a signed 64‐bit numeric non‐fractional value.
The docs for Int
type say:
The Int scalar type represents non-fractional signed whole numeric values.
Int can represent values between -(2^31) and 2^31 - 1.
So, similarly, for Int64
we can now say:
The Int64 scalar type represents a signed 64‐bit numeric non‐fractional value.
Int64 can represent values between -(2^63) and 2^63 - 1.
graphql/schema/validation_rules.go, line 115 at r6 (raw file):
Quoted 6 lines of code…
else if value.Kind == ast.StringValue { addError(validator.Message("Type mismatched for Value `\"%s\"`, Expected type Int64", value.Raw), validator.At(value.Position)) } else if value.Kind == ast.FloatValue { addError(validator.Message("Type mismatched for Value `%s`, Expected type Int64", value.Raw), validator.At(value.Position)) }
we should not use a different if condition for every kind of value, instead we should just mark all other kinds of values in a single else block as invalid. Because otherwise there will be a separate case for boolean, enum, null, object, list, ... all these kinds of values.
Then the error message could be like:
Type mismatch for Value `%s`; expected: Int64, got: String
The got
can change based on the value kind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 48 files reviewed, 18 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/error_test.yaml, line 119 at r6 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
also an out of range error case with 2^63 for Int64
done.
graphql/e2e/common/mutation.go, line 3656 at r6 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
-9223372036854775807
should this be
-9223372036854775808
, i.e. the actual negative bound?
changed, swapped values by mistake.
graphql/e2e/common/mutation.go, line 3672 at r6 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
9223372036854775808
shouldn't this be
9223372036854775807
?
changed.
graphql/resolve/resolver.go, line 1509 at r6 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
// numUids are added as int, so we need special handling for that.
a new line before this like comments in other cases. Also, we can just keep this
case int: ...
part aftercase int64
part to minimize the git diff.
done.
graphql/resolve/resolver.go, line 1532 at r6 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
case float64: // The spec says that we can coerce a Float value to Int, if we don't lose information. // See: https: //spec.graphql.org/June2018/#sec-Float // Lets try to see if this number could be converted to int64 without losing // information, otherwise return error. // See: https://github.com/golang/go/issues/19405 to understand why the comparison // should be done after double conversion. i64Val := int64(v) if v == float64(i64Val) { val = i64Val } else { return nil, valueCoercionError(v) }
I guess , this part could also be removed now that we are using json Number to represent Int64 internally, so it will no longer be parsed to float.
removed.
graphql/resolve/resolver.go, line 1466 at r7 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
valFloat, _ := v.Float64() val = strconv.FormatFloat(valFloat, 'f', -1, 64)
for string, we can just do:
val = v.String()
no need to parse it to float and then parse it back to a string.
done.
graphql/resolve/resolver.go, line 1474 at r7 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
case float64: val = v != 0 case int64: val = v != 0
now maybe we can remove these cases.
removed.
graphql/resolve/resolver.go, line 1492 at r7 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
case float64: // The spec says that we can coerce a Float value to Int, if we don't lose information. // See: https: //spec.graphql.org/June2018/#sec-Float // Lets try to see if this number could be converted to int32 without losing // information, otherwise return error. // See: https://github.com/golang/go/issues/19405 to understand why the comparison // should be done after double conversion. i32Val := int32(v) if v == float64(i32Val) { val = i32Val } else { return nil, valueCoercionError(v) }
I guess this case could be removed now.
There are some admin e2e tests that require this case.
graphql/resolve/resolver.go, line 1508 at r7 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
32
the reason for this being 64 was that a 32-bit float won't be able to represent the full range of 32-bit ints. So, we should change it back to 64.
ok, changed.
graphql/resolve/resolver.go, line 1520 at r7 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
val = i
execution flow won't reach this line, can be removed.
removed.
graphql/resolve/resolver.go, line 1532 at r7 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
32
64
changed.
graphql/resolve/resolver.go, line 1542 at r7 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
val = i
stale line, should be removed
removed.
graphql/resolve/resolver.go, line 1620 at r7 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
case float64: truncated := math.Trunc(v) if truncated == v { // Lets interpret int values as unix timestamp. t := time.Unix(int64(truncated), 0).UTC() val = t.Format(time.RFC3339) } else { return nil, valueCoercionError(v) } case int64: t := time.Unix(v, 0).UTC() val = t.Format(time.RFC3339)
so, both these cases could be removed now.
removed.
graphql/schema/gqlschema.go, line 66 at r6 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
The docs for
Int
type say:The Int scalar type represents non-fractional signed whole numeric values. Int can represent values between -(2^31) and 2^31 - 1.
So, similarly, for
Int64
we can now say:The Int64 scalar type represents a signed 64‐bit numeric non‐fractional value. Int64 can represent values between -(2^63) and 2^63 - 1.
changed.
graphql/schema/validation_rules.go, line 115 at r6 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
else if value.Kind == ast.StringValue { addError(validator.Message("Type mismatched for Value `\"%s\"`, Expected type Int64", value.Raw), validator.At(value.Position)) } else if value.Kind == ast.FloatValue { addError(validator.Message("Type mismatched for Value `%s`, Expected type Int64", value.Raw), validator.At(value.Position)) }
we should not use a different if condition for every kind of value, instead we should just mark all other kinds of values in a single else block as invalid. Because otherwise there will be a separate case for boolean, enum, null, object, list, ... all these kinds of values.
Then the error message could be like:Type mismatch for Value `%s`; expected: Int64, got: String
The
got
can change based on the value kind.
changed. I also added an out of range error case for int64, because the parsing library is giving error for values greater than 2^64 or less than -2^64 only and if we have a value greater than 2^63 and less than 2^64, dgraph was throwing error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 48 files reviewed, 8 unresolved discussions (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)
graphql/resolve/resolver.go, line 1505 at r8 (raw file):
32 bit int.
64 bit floating point number.
graphql/resolve/resolver.go, line 1506 at r8 (raw file):
// Lets try to see if this number could be converted to int32 without losing // information, otherwise return error.
this comment can be put back now
graphql/schema/rules.go, line 52 at r8 (raw file):
ValueKindToString
no need to export it:
valueKindToString
graphql/schema/rules.go, line 53 at r8 (raw file):
var valueKind string
instead of keeping a var and assigning to it, we can just return
from each case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 48 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)
graphql/resolve/resolver.go, line 1505 at r8 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
32 bit int.
64 bit floating point number.
changed.
graphql/resolve/resolver.go, line 1506 at r8 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
// Lets try to see if this number could be converted to int32 without losing // information, otherwise return error.
this comment can be put back now
added.
graphql/schema/rules.go, line 52 at r8 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
ValueKindToString
no need to export it:
valueKindToString
put in validation_rules.go file.
graphql/schema/rules.go, line 53 at r8 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
var valueKind string
instead of keeping a var and assigning to it, we can just
return
from each case.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 45 files at r6, 1 of 2 files at r7, 38 of 42 files at r8, 6 of 6 files at r9.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, and @MichaelJCompton)
graphql/e2e/schema/generatedSchema.graphql, line 16 at r9 (raw file):
""" The Int64 scalar type represents a signed 64‐bit numeric non‐fractional value. Int64 can represent values between -(2^63) and 2^63 - 1.
...values in the range [-(2^63), 2^63 - 1]. Values out of this range would give an out of range error.
To make it clear that it's inclusive of these values.
Fixes GRAPHQL-630
This PR extends the int64 range to 64-bit numeric values and adds input coercing and validation for integers.
This change is
Docs Preview: