ugorji / go

idiomatic codec and rpc lib for msgpack, cbor, json, etc. msgpack.org[Go]
MIT License
1.86k stars 295 forks source link

Msgpack time encoding does not match between code-generated and reflected #225

Closed bboreham closed 6 years ago

bboreham commented 6 years ago

The code generator is using Go's MarshalBinary whereas the non-code-generated version is using an EncodeTime function introduced in debb8e2d2e8bd8cf1e9b6d806cf5e58df86b970c.

(The EncodeTime version has advantages but I need to be able to handle data encoded with the MarshalBinary version.)

Slightly stupid repro but I couldn't figure out how to encode with one scheme and decode with the other so I just hard-coded the encoding of one into the test:

timecheck.go:

package codec

import "time"

type TestTime struct {
    T time.Time
}

timecheck_test.go:

package codec

import (
    "bytes"
    "testing"
)

func TestMsgpackEncodeTime(t *testing.T) {
    v := TestTime{T: time.Unix(1500000000, 0)}
    msgpackH := &MsgpackHandle{}
    var bs []byte
    NewEncoderBytes(&bs, msgpackH).MustEncode(&v)

    goldenResult := []byte{0x81, 0xa1, 0x54, 0xa4, 0x59, 0x68, 0x2f, 0x00}

    if !bytes.Equal(bs, goldenResult) {
        logT(t, "encoded != expected: \nexpected: %x\nencoded: %x", goldenResult, bs)
        failT(t)
    }
}

Test without codecgen:

$ go test -v -run MsgpackEncodeTime
=== RUN   TestMsgpackEncodeTime
--- PASS: TestMsgpackEncodeTime (0.00s)
PASS
ok      github.com/ugorji/go/codec  0.011s

Now with codecgen:

$ ./codecgen/codecgen -o timecheck_generated_test.go timecheck.go
vagrant@vagrant-ubuntu-wily-64:~/.../ugorji/go/codec$ go test -v -run MsgpackEncodeTime
=== RUN   TestMsgpackEncodeTime
--- FAIL: TestMsgpackEncodeTime (0.00s)
    shared_test.go:277: encoded != expected: 
        expected: 81a154a459682f00
        encoded: 81a154af010000000ed0fa2600000000000000
FAIL
exit status 1
FAIL    github.com/ugorji/go/codec  0.007s
ugorji commented 6 years ago

Good catch. Missed a push to include (En|De)codeTime in codecgen. Will include that.

Beyond that, msgpack spec now specifies how to encode/decode time. So we MUST support it. If you want to do something else, I suggest that you create a custom type (even if it just wraps time.Time) and define MarshalBinary on that, and that will be used.

I will push a fix for (En|De)codeTime support in codecgen tomorrow.

ugorji commented 6 years ago

FYI - https://github.com/msgpack/msgpack/blob/master/spec.md#timestamp-extension-type