ugorji / go

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

No tags emitted when serializing struct with multiple naked interfaces #301

Closed kostko closed 5 years ago

kostko commented 5 years ago

It seems that if you have a struct containing multiple naked interfaces, tags are not emitted at all when those interfaces are assigned different types that have extensions. If there is only a single naked interface or if both naked interfaces are assigned the same type, the test passes.

My feeling is that it seems like an issue with caching serializer functions.

Failing test case:

diff --git a/codec/codec_test.go b/codec/codec_test.go
index 596e779..9b8c56a 100644
--- a/codec/codec_test.go
+++ b/codec/codec_test.go
@@ -133,6 +133,7 @@ var (
 var wrapInt64Typ = reflect.TypeOf(wrapInt64(0))
 var wrapBytesTyp = reflect.TypeOf(wrapBytes(nil))
 var testSelfExtTyp = reflect.TypeOf((*TestSelfExtImpl)(nil)).Elem()
+var testSelfExt2Typ = reflect.TypeOf((*TestSelfExtImpl2)(nil)).Elem()

 func testByteBuf(in []byte) *bytes.Buffer {
    return bytes.NewBuffer(in)
@@ -431,6 +432,12 @@ func testInit() {
    chkErr(testJsonH.SetInterfaceExt(testSelfExtTyp, 78, SelfExt))
    chkErr(testCborH.SetInterfaceExt(testSelfExtTyp, 78, SelfExt))

+   chkErr(testSimpleH.SetBytesExt(testSelfExt2Typ, 79, SelfExt))
+   chkErr(testMsgpackH.SetBytesExt(testSelfExt2Typ, 79, SelfExt))
+   chkErr(testBincH.SetBytesExt(testSelfExt2Typ, 79, SelfExt))
+   chkErr(testJsonH.SetInterfaceExt(testSelfExt2Typ, 79, SelfExt))
+   chkErr(testCborH.SetInterfaceExt(testSelfExt2Typ, 79, SelfExt))
+
    // Now, add extensions for the type wrapInt64 and wrapBytes,
    // so we can execute the Encode/Decode Ext paths.

@@ -2491,6 +2498,20 @@ func doTestSelfExt(t *testing.T, name string, h Handle) {
    bs := testMarshalErr(&ts, h, t, name)
    testUnmarshalErr(&ts2, bs, h, t, name)
    testDeepEqualErr(&ts, &ts2, t, name)
+
+   var ts3 TestSelfExtImpl2
+   ts3.M = "moo"
+   ts3.O = true
+
+   s := TestTwoNakedInterfaces{
+       A: ts,
+       B: ts3,
+   }
+   var s2 TestTwoNakedInterfaces
+
+   bs = testMarshalErr(&s, h, t, name)
+   testUnmarshalErr(&s2, bs, h, t, name)
+   testDeepEqualErr(&s, &s2, t, name)
 }

 func doTestBytesEncodedAsArray(t *testing.T, name string, h Handle) {
diff --git a/codec/values_flex_test.go b/codec/values_flex_test.go
index 55a7f6e..0a5686f 100644
--- a/codec/values_flex_test.go
+++ b/codec/values_flex_test.go
@@ -145,6 +145,16 @@ type TestSelfExtImpl struct {
    testSelfExtHelper
 }

+type TestSelfExtImpl2 struct {
+   M string
+   O bool
+}
+
+type TestTwoNakedInterfaces struct {
+   A interface{}
+   B interface{}
+}
+
 var testWRepeated512 wrapBytes
 var testStrucTime = time.Date(2012, 2, 2, 2, 2, 2, 2000, time.UTC).UTC()
kostko commented 5 years ago

Ah found the issue, it was caching indeed, likely a leftover from before you refactored the API:

diff --git a/codec/helper.go b/codec/helper.go
index 40fa1ab..5dd3e41 100644
--- a/codec/helper.go
+++ b/codec/helper.go
@@ -717,7 +717,7 @@ func (x *BasicHandle) fnVia(rt reflect.Type, fs *atomicRtidFnSlice, checkExt boo
                        copy(sp2, sp[:idx])
                        copy(sp2[idx+1:], sp[idx:])
                        sp2[idx] = codecRtidFn{rtid, fn}
-                       x.rtidFns.store(sp2)
+                       fs.store(sp2)
                        // xdebugf(">>>> adding rt: %v to rtidfns of size: %v", rt, len(sp2))

                }
ugorji commented 5 years ago

Great catch. Phew. Thanks.

Can you send a PR with this fix to code and tests?

kostko commented 5 years ago

Done in #302.