Why the usage of `int` to hold values that overflow it (when it can only 32 bits wide) about graphql HOT 11 CLOSED

Hi @bbuck

Thanks for sharing your time to take a look into this project, appreciate it a lot! 👍🏻

I contributed part of that chunk of code that was ported from graphql-js. At that point of time, I found it strange that integer value was restricted that way and wondered whether we should just support int64 since golang does not the same limitations as Javascript.

But looking deeper into it, I found out that the GraphQL specs specify support only for signed 32-bit integers. But to add to the confusion, it seems that graphql-js implementation supports 52-bits integer.

There have been discussions in the graphql-js camp about 64-bit support and the direction for graphql-js whether to update the spec or implementation regarding this 52-bits inconsistency.

At this moment, it seems that a part of the GraphQL community is voting for a strict specs of signed 32-bits integer on all platforms, both client and server. (I encourage everyone else to make your voice be heard as well).

Personally, I would love to adopt support for 64-bit since golang provides it for free (yay!) but we also have to consider GraphQL clients that consume the results won't necessarily support 64-bit. So it makes sense to support the lowest common denominator.

What do you guys think?

/cc @chris-ramon

from graphql.

I guess what I was more getting it is that internally I feel that int64 should be used, but externally (as per the spec) 32 bit integers (or the 52 bit values as you mention) could be returned. Basically what I did modifying the coerceInt (and the corresponding one for float) function in my pull request.

Mostly because the application itself might not use int, like in my use case where gorm defaults it's ID field to uint. It seems logical to just return a uint and let the GraphQL system convert that uint to whatever value best relates to it's GraphQLInt type.

The best comparison I have is that, during this session of this one particular course here (which may or may not be accessible to you, I hope so) they discuss returning a JavaScript object when graphql-js was told to expect a GraphQLString so the final result returned is "[object Object]" (the closest to a string for a standard object GraphQL could get).

tl;dr Internally I think graphql-go should handle "maximum" values (so to speak) and just convert them to the final result that GraphQL has standardized.

Also, you're welcome. I've just started (like less than a week ago) diving into GraphQL and I'm super excited about it and want something to do in Go so I hope to try and start contributing some useful things going forward.

from graphql.

Ah, I see what you mean right now lol 👍🏻

Yeah you are right, those MaxInt and MinInt would be a problem on 32-bit platforms, (shown here on a 32-bit golang playground: http://play.golang.org/p/BZlMvi86aW)

prog.go:9: constant 9007199254740991 overflows int
prog.go:10: constant -9007199254740991 overflows int

Specifying them as int64 as you've suggested would address that issue: http://play.golang.org/p/BtL6YFAFeg

amd64p32 nacl go1.5.1
MaxInt 9007199254740991
MinInt -9007199254740991
int(MaxInt) -1
int(MinInt) 1

Thanks for that catch 😉

I see you already have a PR open; you think you want to include these changes in them as well?

And I believe those MaxInt and MinInt could be specified as const as well, what do you think?

Again, appreciate your contribution and looking forward for more!

from graphql.

I think both great points. I'll throw that in the PR!
On Thu, Oct 15, 2015 at 20:32 Hafiz Ismail [email protected] wrote:

Ah, I see what you mean right now lol 👍🏻

Yeah you are right, those MaxInt and MinInt would be a problem on 32-bit
platforms, (shown here on a 32-bit golang playground:
http://play.golang.org/p/BZlMvi86aW)

prog.go:9: constant 9007199254740991 overflows int
prog.go:10: constant -9007199254740991 overflows int

Specifying them as int64 as you've suggested would address that issue:
http://play.golang.org/p/BtL6YFAFeg

amd64p32 nacl go1.5.1
MaxInt 9007199254740991
MinInt -9007199254740991
int(MaxInt) -1
int(MinInt) 1

Thanks for that catch 😉

I see you already have a PR open; you think you want to include these
changes in them as well?

And I believe those MaxInt and MinInt could be specified as const as
well, what do you think?

Again, appreciate your contribution and looking forward for more!

—
Reply to this email directly or view it on GitHub
https://github.com/chris-ramon/graphql-go/issues/24#issuecomment-148570851
.

from graphql.

@sogko So I guess I have a dilemma here. Should we be returning int32 values? If so we can't handle the same numeric ranges. Yet if we use 64 bit we can constrain it ourselves but we violate the spec. What do you think?

from graphql.

Hey @bbuck

I think for integer values (int, int32, int64, uint, et al) should return the same type as its input when passed into coerceInt().

intOrNil() then exists to check if integer value falls within allowed range (currently signed 52-bit range, might change if graphql-js updates its implementation)

This is what I imagine the implementation it could be.

// intOrNil checks if value is within allowed range. 
// Returns the original value if ok, else nil.
func intOrNil(value interface{}) interface{} {
    val := int64(value)
    if val <= MaxInt && val >= MinInt {
        return value
    }
    return nil
}

// For non-integers (strings, bool, floats), coerce to `int`
// For integers (including its variations), value should be within 
// signed 52-bit range for its original input type. Else returns nil
// TODO: The below code can be written more elegantly I guess lol
func coerceInt(value interface{}) interface{} {
    switch value := value.(type) {
    case bool:
        if value == true {
            return int(1)
        }
        return int(0)
    case int:
        return intOrNil(value) // returns as int or nil
    case int32:
        return intOrNil(value) // returns as int32 or nil
    case int64:
        return intOrNil(value) // returns as int64 or nil
    case uint:
        // TODO: check if uint value is larger than signed int64
        return intOrNil(value) // returns as uint or nil
    // TODO: case uint8, uint16 etc..
    case float32:
        if intOrNil(value) == nil {
          return nil
        }
        return int(value) // casts float32 as int
    case float64:
        return intOrNil(value) // returns as int64 or nil
    case string:
        val, err := strconv.ParseFloat(value, 0)
        if err != nil {
            return nil
        }
        return coerceInt(val)
    }
    return int(0)
}

What do you think about this?

from graphql.

Okay I've updated my pull request with some of the ideas you posted. I modified the switch piece so that it returns the "best choice" of int* type based on it's input type. So long as the value sits within [MinInt, MaxInt] and is an integer then that integer type is returned (i.e. int32(1) and input returns int32(1) as output). For uint* types they're cast to the size up int* value (if they're less than MaxInt) so uint8 ([0, 255]) becomes an int16. Any value (uint/uint64, int64, float32, float64) that can deviate from the range [MinInt, MaxInt] are passed through intOrNil if they are small enough to fit within an int64 value (which intOrNil now takes as input).

I've updated the tests so that corresponding types are validated.

Personally, I'm not sure how comfortable I feel with that varied return values. I think I would be more comfortable if only int64 or nil was returned. But that could just be me.

from graphql.

After pushing and commenting I suppose there is little reason specifically with forcing uint* to int* at this point as long they're within the min/max range.

from graphql.

Just to keep this issue up-to-date:

I think this issue is still valid, and now that most of the codebase has settle down a little bit from the API changes, we can re-look into this.

from graphql.

Just to not here as well that #83 is an updated proposed fix. Also fixes the invalid bit size (according to spec) numbers used which was also updated in the graphql-js library.

from graphql.

This should be closed now.

from graphql.