Graphql-go: Int implemented only by int32

[jakubdal]

Hey, I've realised this library accepts only int32 as int. This brings not only confusion, but also doesn't allow user to return certain common values (ex. timestamp in milliseconds). I've found the problem in graphql-go/internal/exec/resolvable/resolvable.go:195 in the following function:

func makeScalarExec(t *schema.Scalar, resolverType reflect.Type) (Resolvable, error) {
    implementsType := false
    switch r := reflect.New(resolverType).Interface().(type) {
    case *int32:
        implementsType = (t.Name == "Int")
    case *float64:
        implementsType = (t.Name == "Float")
    case *string:
        implementsType = (t.Name == "String")
    case *bool:
        implementsType = (t.Name == "Boolean")
    case packer.Unmarshaler:
        implementsType = r.ImplementsGraphQLType(t.Name)
    }
    if !implementsType {
        return nil, fmt.Errorf("can not use %s as %s", resolverType, t.Name)
    }
    return &Scalar{}, nil
}

I suggest changing it to (PoC here: https://play.golang.org/p/-5b10j3dS9):

func makeScalarExec(t *schema.Scalar, resolverType reflect.Type) (Resolvable, error) {
    implementsType := false
    switch r := reflect.New(resolverType).Interface().(type) {
    case *uint8, *uint16, *uint32, *uint64, *uint, *int8, *int16, *int32, *int64, *int:
        implementsType = (t.Name == "Int")
    case *float64, *float32:
        implementsType = (t.Name == "Float")
    case *string:
        implementsType = (t.Name == "String")
    case *bool:
        implementsType = (t.Name == "Boolean")
    case packer.Unmarshaler:
        implementsType = r.ImplementsGraphQLType(t.Name)
    }
    if !implementsType {
        return nil, fmt.Errorf("can not use %s as %s", resolverType, t.Name)
    }
    return &Scalar{}, nil
}

I don't know the library's internals at all, so I don't know if there aren't any more pitfalls and as such didn't prepare a MR on the go, but it feels like a core functionality that just isn't there.

[sfriedel]

This change would actually violate the GraphQL spec.

The Int scalar type represents a signed 32‐bit numeric non‐fractional value. Response formats that support a 32‐bit integer or a number type should use that type to represent this scalar.

Source

[dmitshur]

I'm not sure if this is a valid option in this scenario, but I suspect it is, so let me point it out.

This library could decide to accept types other than int32 and implicitly convert them/truncate them to int32. Whether that should be supported is a design decision.

Of course, it might be better not to do that and instead require the user to explicitly convert themselves.

[tonyghita]

@jakubdal have you considered using RFC3339 timestamps instead of integer milliseconds?

[jakubdal]

@sfriedel I wasn't aware of that part of specification. In those circumstances, my request to use int64 is absurd.

@shurcooL In regards to that specification, I think truncating int64 to int32 silently is worse than loud fail on start.

@tonyghita I've hit this problem while trying to return timestamp in milliseconds. For the project I'm working on now, we will use RFC3339, so that's an on-point suggestion :)

In the light of evidence shown here, I'd say that you are right and my idea is too far-fetched. However, I'm thinking about changing error messages - int does not implement Int or error of such sort was really off-putting on first try (the same goes for something along the lines of string does not implement String). I'm pretty sure word implement wasn't used there, but you get the gist. I will thereby close this request, and make a MR with 'cleaner' error messages, whenever I'll find some spare time for it :)

Thanks for your feedback!

[zentrope]

BTW, isn't at least one reason for this due to the fact that JavaScript (the main receiver of GraphQL results) doesn't support 64bit ints?

[tonyghita]

Yeah, Javascript uses the maximum integer float value.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#Integer_range_for_Number

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

Number.MAX_SAFE_INTEGER // 9007199254740991
Math.pow(2, 53) - 1     // 9007199254740991