MarshalJSONPB is not called for custom type if it's an alias to a primitive

[yurishkuro]

I am using a custom type

type SpanID uint64

// MarshalJSONPB renders span id as a single hex string.
func (s SpanID) MarshalJSONPB(*jsonpb.Marshaler) ([]byte, error) {
	// something
}

When serializing to JSON using github.com/gogo/protobuf/jsonpb I realized that its MarshalJSONPB method is never called, even though it works for other custom types that are defined as structs, instead of a type alias. Instead, the stdlib's json package is used:

> github.com/jaegertracing/jaeger/vendor/github.com/gogo/protobuf/jsonpb.(*Marshaler).marshalValue() ./vendor/github.com/gogo/protobuf/jsonpb/jsonpb.go:682 (PC: 0x16927a9)
   677:				return out.err
   678:			}
   679:		}
   680:
   681:		// Default handling defers to the encoding/json library.
=> 682:		b, err := json.Marshal(v.Interface())
   683:		if err != nil {

which happens to call MarshalText().

[awalterschulze]

Have you tried https://github.com/gogo/protobuf/blob/master/custom_types.md
I think you need to func (t T) MarshalJSON() ([]byte, error) {} and not MarshalJSONPB if that makes sense?

[yurishkuro]

Hm, I had those before but ran into some other issue. Notably, if the custom type implements jsonpb.JSONPBUnmarshaler, then UnmarshalJSONPB is called on it even if it defines UnmarshalJSON.

Thanks.

[yurishkuro]

Actually, I think there's still something wrong. I changed the code to only provide MarshalJSON / UnmarshalJSON functions, and my parsing tests started breaking. When I restore UnmarshalJSONPB function, I get successful run:

$ go test -v -run TestSpanIDUnmarshalJSON ./model
=== RUN   TestSpanIDUnmarshalJSON
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1"}
SpanID.UnmarshalJSONPB("1") called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"f"}
SpanID.UnmarshalJSONPB("f") called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1f"}
SpanID.UnmarshalJSONPB("1f") called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"101"}
SpanID.UnmarshalJSONPB("101") called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":""}
SpanID.UnmarshalJSONPB("") called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"x"}
SpanID.UnmarshalJSONPB("x") called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"x123"}
SpanID.UnmarshalJSONPB("x123") called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"10123456789abcdef"}
SpanID.UnmarshalJSONPB("10123456789abcdef") called
--- PASS: TestSpanIDUnmarshalJSON (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"f"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1f"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"101"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":""} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"x"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"x123"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"10123456789abcdef"} (0.00s)
PASS

Note that UnmarshalJSON is never called. If I remove UnmarshalJSONPB function, then UnmarshalJSON is called only two times in the whole test, for numeric values (the custom type is an alias to uint64), but for hex values it's not even called, something else is throwing an error.

$ go test -v -run TestSpanIDUnmarshalJSON ./model
=== RUN   TestSpanIDUnmarshalJSON
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1"}
SpanID.UnmarshalJSON(1) called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"f"}
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1f"}
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"101"}
SpanID.UnmarshalJSON(101) called
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":""}
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"x"}
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"x123"}
=== RUN   TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"10123456789abcdef"}
--- FAIL: TestSpanIDUnmarshalJSON (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1"} (0.00s)
    --- FAIL: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"f"} (0.00s)
        Error Trace:    span_test.go:140
    	Error:      	Received unexpected error:
    	            	invalid character ' ' in literal false (expecting 'a')
    	Test:       	TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"f"}
    --- FAIL: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1f"} (0.00s)
        Error Trace:    span_test.go:140
    	Error:      	Received unexpected error:
    	            	invalid character 'f' after top-level value
    	Test:       	TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"1f"}
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"101"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":""} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"x"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"x123"} (0.00s)
    --- PASS: TestSpanIDUnmarshalJSON/{"traceID":"0","spanID":"10123456789abcdef"} (0.00s)
FAIL

[awalterschulze]

Is it possible to provide this as a test that we could merge into gogoprotobuf? Then it might also be easier to me to debug.

[yurishkuro]

I created a simple test here https://github.com/yurishkuro/gogoproto-custom-type.

If you rename UnmarshalJSONPB_disabled to UnmarshalJSONPB (in [./jsontest/alias.go]), then the test passes. As can be seen below, when the value of val is not only digits, the custom unmarshal function Uint64Alias.UnmarshalJSON is not even called.

# 18:46:53 ...github.com/yurishkuro/gogoproto-custom-type
$ go test -v .
=== RUN   TestMarshalSimple
=== RUN   TestMarshalSimple/{"first":"00000000000000000000000000000000","val":"42"}
Uint64Alias.UnmarshalJSON called
=== RUN   TestMarshalSimple/{"first":"00000000000000000000000000000000","val":"42f"}
--- FAIL: TestMarshalSimple (0.00s)
    --- PASS: TestMarshalSimple/{"first":"00000000000000000000000000000000","val":"42"} (0.00s)
    --- FAIL: TestMarshalSimple/{"first":"00000000000000000000000000000000","val":"42f"} (0.00s)
        Error Trace:    alias_test.go:27
    	Error:      	Received unexpected error:
    	            	invalid character 'f' after top-level value
    	Test:       	TestMarshalSimple/{"first":"00000000000000000000000000000000","val":"42f"}

[yurishkuro]

Kind of related - should I be using casttype instead of customtype extension here? The distinction between the two is not very clear from the docs. I need a custom type because of both (a) custom methods on it and (b) custom JSON serialization.

[awalterschulze]

Ah yes I think you nailed it.
customtype is only for bytes and message field types.
For uint64 casttype works better, but I don't know if modifying the json marshaling and unmarshaling is supported.
Could you try using casttype and see if you have any luck.

PS
Sorry for the late reply, things have been truly busy on my side.

[yurishkuro]

I actually decided to use bytes for both TraceID and SpanID fields (jaegertracing/jaeger#856), one mapped to a custom type that's a struct, another mapped to a custom type that's an alias to uint64. I had to implement binary and JSON serialization for both, and again the uint64 alias has the issue that it was only calling UnmarshalJSONPB.

[awalterschulze]

Ok so both TraceID and SpanID are now bytes fields which use customtype.
I don't think you should implement UnmarshalJSONPB only UnmarshalJSON
Also if a customtype is a uint64 underneath, if might need to be a pointer

func (t *T) UnmarshalJSON(data []byte) error {}

But I am just shooting in the dark right now.
I would be nice if you could update your https://github.com/yurishkuro/gogoproto-custom-type repo again, so we can make sure we are on the same page.

[yurishkuro]

I updated https://github.com/yurishkuro/gogoproto-custom-type to reflect the exact use case I have in Jaeger PR. Added README.

[awalterschulze]

Thank you so much.
I think I finally get it now.
That helped me a lot.

So when the customtype's Go type is a struct or []byte (previous experience) then you only need to implement UnmarshalJSON, but when the cusomtype's Go type is a native type like uint64, then it calls UnmarshalJSONPB over UnmarshalJSON.
Exactly your issue title, sorry it took me so long to understand it. I am context switching too much.

I think we can merge your test into gogoprotobuf and attempt a fix.
Would you like to try and fix this problem for yourself and all the other users?

[yurishkuro]

Sadly, I originally tried to write the example & tests as a branch of gogoprotobuf repo, but had issues with unrelated test failures before I could do anything. Right now my workaround is to call UnmarshalJSON from UnmarshalJSONPB function.

[awalterschulze]

If you have the unrelated test failures handy, I can help take a look.

But if you prefer that we take a look, given your great work to produce a test case (thanks again), then we can also do that.

Also thanks for the work around.

[donaldgraham]

@yurishkuro, we've merged in your test, and our fix. Your test now passes.

I'm going to close this issue now. Please re-open it if our fix doesn't resolve your problem.