marshaling byte slice ignores json.Marshaler/encoding.TextMarshaler interfaces

[wI2L]

When a codec is created for a byte-slice type at

encoding/json/codec.go

Line 80 in bc545e9

case bytesType:

it ignores the fact that the type could be implementing the json.Marshaler or encoding.TextMarshaler interfaces. This should be considered in priority, to be compliant with the standard library.

The following playground demonstrates the issue:
https://play.golang.org/p/Bm0d3jsWkZf

The relevant code can be found in the standard library at https://github.com/golang/go/blob/da4d58587e0e4028ea384580053c3c455127e446/src/encoding/json/encode.go#L415
The function newTypeEncoder checks in priority if the reflect.Value implements one of the interfaces.

[achille-roussel]

Thanks for raising the issue!

I could be wrong, but I don't think the issue comes from the comparison to bytesType, because this is just a code path to match the []byte type exactly (for example https://play.golang.org/p/JDkXu1XkRgE).

I'm going to look into it, it should be an easy fix.

[wI2L]

@achille-roussel What I meant was the comparison to bytesType is made before checking if the type implement either of json.Marshaler or encoding.TextMarshaler. The newTypeEncode function of the standard library check for (JSON|Text)Marshaler first.

[achille-roussel]

Yes but in this case we wouldn't enter this code path at all, which means we'll fallback to checking whether the type implements the interfaces below.

[wI2L]

@achille-roussel You are right, I confused named byte slice type and byte slice type regarding the code part I mentionned as the source of the error.

[achille-roussel]

No worries. It was an edge case because we have tests that verified this behavior already, but it turned out the zero-value of slice types implementing json.Marshaler was being misinterpreted, and we were missing a test for this specific case.

[wI2L]

Perhaps constructSliceCodec should be responsible for returning a codec that uses encodeBytes and decodeBytes if the kind of the slice element is uint8, and it doesn't implements either the json.Marshaler/encoding.TextMarshaler interfaces, as in https://github.com/golang/go/blob/da4d58587e0e4028ea384580053c3c455127e446/src/encoding/json/encode.go#L864.

[achille-roussel]

The fix was a bit more subtle than I anticipated actually: the issue you reported was just the fact that we were mistakenly encoding zero-value slices as null when they implemented json.Marshaler, but I added a couple more tests and noticed a there were differences in encoding based on whether a value will be addressable or not (here's an example https://play.golang.org/p/qRwJTqQodUi).

This is kind of crazy, and I hope no code out there relies on this behavior... anyways, it should be fixed in #17

[wI2L]

@achille-roussel Ahaha, I know right. This was a pain to implement for me as well.
Edit: This reminds me that you should double check if the current implementation is subject to the bug I reported at golang/go#34235. But from what I see, the check at

encoding/json/encode.go

Line 656 in bc545e9

switch v.Kind() {

should be enough.

[achille-roussel]

I added a test for that case as well, it seems like we're handling it fine 👍