json.Unmarshal converts json literal integer to float64 instead of int64 when taget type is of type interface{}

[narg95]

Issue

json.Unmarshal unmarshals integers values into float64 when target type is of type interface{}.

Expected behavior

If a json literal is assignable to an integer without losing precision the json.Unmarshal must convert to int64 instead of float64

The problem is located in json/decode.go in convertNumber function:

func (d *decodeState) convertNumber(s string) (interface{}, error) {
    if d.useNumber {
        return Number(s), nil
    }
    f, err := strconv.ParseFloat(s, 64)  // See here! By default it converts to Float
    if err != nil {
        return nil, &UnmarshalTypeError{"number " + s, reflect.TypeOf(0.0), int64(d.off)}
    }
    return f, nil
}

by default it does strconv.ParseFloat

Steps to Reproduce

func TestUnmarshalNumber(t *testing.T) {
    var v interface{}
    exp := int64(1621867181)
    err := json.Unmarshal([]byte(`1621867181`), &v)
    require.NoError(t, err)
    require.IsType(t, exp, v)
}

Error:

=== RUN   TestUnmarshalNumber
    token_test.go:502:
            Error Trace:    token_test.go:502
            Error:          Object expected to be of type int64, but was float64
            Test:           TestUnmarshalNumber
--- FAIL: TestUnmarshalNumber (0.00s)
FAIL
FAIL    token/jwt   0.025s

Proposed Solution

convertNumber should try first strconv.ParseInt and in case of err it does strconv.ParseFloat

If you agree with the proposal I can submit a PR

[csstaub]

Sounds reasonable. What does the latest encoding/json in Go's standard library do in this case?

[narg95]

Also float64 https://play.golang.org/p/8JKQ3G8nr1M but would'nt it make sense that the custom go-jose's json marshalling library is tailored to jwt specific needs like a correct data type for claims with timestamps e.g. iat, exp, nbf?

[narg95]

I was doing a quick review on GO's standard library and there was an issue about same topic: https://forum.golangbridge.org/t/type-problem-in-json-conversion/19420 Nevertheless it was closed mentioning:

JSON does only know floats

That is partially right, JSON and JavaScript do not have distinct types for integers and floating-point values, but it does not mean that when parsed, programming languages can not use their internal representations for integers and floats. This conclusion is also supported by:

json-schema.org

The JSON Schema specification recommends, but does not require, that validators use the mathematical value to determine whether a number is an integer, and not the type alone.

ietf draft-bhutton-json-schema-00

Some programming languages and parsers use different internal representations for floating point numbers than they do for integers.

Therefore using the proper golang integer representation seems to be right.

[csstaub]

Yeah totally reasonable to change the behavior to do something specific for this library. (That's part of the reason we forked encoding/json, so we be stricter around parsing fields into structs).

[narg95]

great, I was thinking to make it an opt-in feature so it is backwards compatible. I was thinking to refactor the decodestate.useNumber type to an enum and rename it as numberType:

json/decode.go

type NumberDecodeType int

const (
    // Use float64 as json unmarshal type
    NumberDecodeTypeDefault NumberDecodeType = iota
    // Use `json.Number`as json unmarshal type
    NumberDecodeTypeJSON
    // Use `int64` as json unmarshal type in case of an integer JSON numbers
    // otherwise float64
    NumberDecodeTypeInt64
)

type decodeState struct {
    // useNumber gets renamed
    numberType  NumberDecodeType 
}

and in Decoder:

stream.go

// Deprecated: use SetNumberType
func (dec *Decoder) UseNumber() { dec.d.numberType= NumberDecodeTypeJSON } // this to be backwards compatible

func (dec *Decoder) SetNumberType(t NumberDecodeType) { dec.d.numberType = t} 

and of course the implementation logic in convertNumber function

func (d *decodeState) convertNumber(s string) (interface{}, error) {
    swtich d.numberType {
        case NumberDecodeTypeJSON:
        return Number(s), nil
        case NumberDecodeTypeInt64:
            ....
        }

    f, err := strconv.ParseFloat(s, 64)
    if err != nil {
        return nil, &UnmarshalTypeError{"number " + s, reflect.TypeOf(0.0), int64(d.off)}
    }
    return f, nil
}

does it sound good to you for a PR?

[csstaub]

That sounds reasonable to me, though as far as I'm aware only this library uses the forked version internally. We should perhaps move it to be in an internal folder and then we don't have to worry about backwards compat so much.

[narg95]

I see, nevertheless existing logic that type casts interface{} to float64 will break. Having the json library public might support those implementing custom UnmarshalJSON logic .

An internal folder but with public types is a good compromise. Changes are not guaranteed to be backwards compatible