Add other integer values to the coerceInt function

[bbuck]

This pull request will fix #22.

As a note, I added several more cases to the coerceInt function to support the sized int and uint types and well as unsized uint types. I unified the integer return value from the function to int64.

Reasoning for int64 returns

Past discussions that have been lost discussed returning the value that was sent but this breaks down quickly when parsing queries the values given are strings and a choice has to be made at that point how to parse them. I attempted to implement my best guess despite that fact and so if a value was passed in, I returned the closest, most appropriate, int sized value but this in itself is error prone. Just because the number fits in an int8 doesn't mean it was an int8 or an int8 is expected.

Given the spec for GraphQL (and the fuzzy number used by this library) for minimum/maximum values it's clear than an int64 is required. The previous use of int could functionally work so long as nobody ever built for a 32-bit architecture at which point the int value would be too small and errors would appear from every which way. Taking this into consideration and the fact that "guessing" at the appropriate type are both error prone in their own way I chose to forgo that and use int64 as the integer type when interfacing with GraphQL.

I'm open to discussion around this decision but I think it's the right way to go. Although several tests had to be adjusted to accommodate it I still feel it's the best direction we go to keep things working smoothly which such a varying type.

NOTE: I just want to mention a funky side effect that I encountered with tests. In order for enums associated to integer values to be fetched they have to be defined with int64 values, otherwise the lookup will fail. This is non-ideal as it restricts the users choice in types to use but I can't think of an immediate solution that would solve this. Returning the "best guess" type will also fail out here if int16 is used and an int8 is returned.

[benbjohnson]

@bbuck The graphql-js release today (0.4.14) limited the Int type to 32 bits:

https://github.com/graphql/graphql-js/releases/tag/v0.4.14

With that change, I would think that int would be the best Go type to translate to.

[bbuck]

Awesome. Then I will rework this tonight for int values and we can all rejoice.

[bbuck]

So now int is the default type returned by the library and parsing functions. This matches both the recent changed mentioned earlier in graphql-js as well as the actual GraphQL Specification (exciting!).

I also added in a really small bug that I found, where Enum.Description was returning "" instead of qt.PrivateDescription. This portion of the code still needs some tests tied to it, the Description method is never tested anywhere (probably how this wasn't corrected sooner).

[bbuck]

Oh, I should also note that I replaced intOrNil with if statements in the case where necessary. Reasoning here was casting and comparison of types, it was much easier to do these in controlled environments instead of trying to alter intOrNil to also type check and cast the math.MaxInt32/math.MinInt32 calls to the proper type to determine if nil is necessary.

[FugiTech]

Why int (which can be 32bit or 64bit) instead of int32, if the spec has chosen 32bit?

[bbuck]

Because it is at least 32 bits in size and is the default type assigned to literals. Instead of casting all of your numbers as int32 you can just work with integers.

[FugiTech]

Makes sense. Thanks for the explanation!

[bbuck]

@sogko @chris-ramon Any status on when this will get merged? This is a major feature I need to consume this library.

[chris-ramon]

Great work on this @bbuck, thanks a lot! 🌟

This def looks to me 👍

I think we can handle the Enum association issue in a new PR.

[bbuck]

The Enum problem isn't really an issue now that it's back to int which works fine for literal values.

[chris-ramon]

Hi @bbuck! - the Enum issue I've encountered is that when using int* instead of int for resolving Enum fields it won't work as expected, perhaps we are talking about diff issues?
Here's an example:

package main

import (
    "encoding/json"
    "fmt"
    "log"

    "github.com/graphql-go/graphql"
)

func main() {
    roleEnum := graphql.NewEnum(graphql.EnumConfig{
        Name: "roles",
        Values: graphql.EnumValueConfigMap{
            "ADMIN": &graphql.EnumValueConfig{
                Value: 1,
            },
            "USER": &graphql.EnumValueConfig{
                Value: 2,
            },
        },
    })
    fields := graphql.Fields{
        "roles": &graphql.Field{
            Type: graphql.NewList(roleEnum),
            Resolve: func(p graphql.ResolveParams) (interface{}, error) {
                return []int{1, 2}, nil // []int8{1, 2}, nil -> won't work
            },
        },
    }
    rootQuery := graphql.ObjectConfig{Name: "RootQuery", Fields: fields}
    schemaConfig := graphql.SchemaConfig{Query: graphql.NewObject(rootQuery)}
    schema, err := graphql.NewSchema(schemaConfig)
    if err != nil {
        log.Fatalf("failed to create new schema, error: %v", err)
    }
    query := `
        {
            roles
        }
    `
    params := graphql.Params{Schema: schema, RequestString: query}
    r := graphql.Do(params)
    if len(r.Errors) > 0 {
        log.Fatalf("failed to execute graphql operation, errors: %+v", r.Errors)
    }
    rJSON, _ := json.Marshal(r)
    fmt.Printf("%s \n", rJSON) // {"data":{"roles":["ADMIN","USER"]}} - {"data":{"roles":[null,null]}}
}

[bbuck]

That is definitely the issue I had when I originally made this pull request to support only int64 internally, but after switching back to int the tests passed normally.

In your example, I see two possible course of action that would (or should be taken) if employed in a live project. The first is, if you're resolving with int8 you should define with int8.

roleEnum := graphql.NewEnum(graphql.EnumConfig{
    Name: "roles",
    Values: graphql.EnumValueConfigMap{
        "ADMIN": &graphql.EnumValueConfig{
            Value: int8(1),
        },
        "USER": &graphql.EnumValueConfig{
            Value: int8(2),
        },
    },
})

While the other option is more idiomatic I would think. First you would create an enum type internally in the program:

type Role int8

const (
    RoleAdmin Role = iota + 1
    RoleUser
)

And then modify the resolution function:

// ...
Resolve: func(p graphql.ResolveParams) (interface{}, error) {
        return []Role{RoleAdmin, RoleUser}, nil
},
// ...

And the problem dissipates itself. I don't think there is an ideal method to match arbitrary types to other types in this situation (in the example, matching an int to an int8 unless you unify all types that enter into the graphql library. For example, any sized int passed in (or returned from a Resolve) is converted into an int value for use. Any sized int value passed as the Value of an Enum would be converted to an int. This solves the initial problem but could potentially introduce bugs in the future. For example, defining and Enum with int8 and then fetching the value by name you'd receive back an int which would cause a condition like if int8(1) == theEnumReturnValue to error on invalid types.

tl;dr I don't think it's an issue that would be worth resolving.

[chris-ramon]

That makes totally sense! - thanks @bbuck.

I agree this Enum issue isn't worth resolving.