mailru / easyjson

Fast JSON serializer for golang.
MIT License
4.48k stars 421 forks source link

code generator pretends to allow use of embedded structs with custom marshaler/unmarshaler #296

Open eli-darkly opened 4 years ago

eli-darkly commented 4 years ago

By "pretends to allow", I don't mean that easyjson is doing anything sneaky— just that I think the following situation should be treated as an error, and instead it is treated as acceptable but implemented in an undesirable way.

Say that I have two structs like this:

//easyjson:json
type A struct {
    AField string
}

//easyjson:json
type B struct {
    A
    BField string
}

In this example, marshaling A{AField: "aaa"} will produce {AField:"aaa"}, and marshaling B{A: A{AField: "aaa"}, BField: "bbb"} will produce {"AField":"aaa","BField":"bbb"}. That's consistent with how Go treats embedded anonymous structs and how they're normally rendered in JSON— so far, so good.

However, suppose I try to do this instead:

type A struct {
    AField string
}

//easyjson:json
type B struct {
    A
    BField string
}

func (a A) MarshalEasyJSON(out *jwriter.Writer) {
    out.RawByte('{')
    out.RawString(`"AField":`)
    out.String(a.AField + "-modified")
    out.RawByte('}')
}

In this case, marshaling A{AField: "aaa"} will produce {AField:"aaa-modified"}. But marshaling B{A: A{AField: "aaa"}, BField: "bbb"} will produce... {"AField":"aaa","BField":"bbb"}, just the same as before. The custom marshaler for A is ignored when A is embedded in B, and instead it falls back to inspecting the fields and generating code for both AField and BField in the usual way.

Now, I know there's no way this could possibly work as intended. A marshaler in easyjson has to serialize the entire value— A.MarshalEasyJSON has no way to know that it shouldn't add the {} delimiters in this context (and of course there's no guarantee that the serialization of A is even a JSON object, rather than something like 3). So there's a basic incompatibility between the Go language concept of embedded anonymous structs and the idea of a type being able to define its own serialization.

But that's why I think this should be treated as an error condition; easyjson should refuse to generate code for B in this scenario (that is, specifically when the type of an embedded anonymous struct is known to have a custom marshal/unmarshal method). Trying to make it into something that "works", by simply acting like A did not have a custom marshaler, is guaranteed to produce wrong results from the developer's point of view— since, if the usual field-generating code could produce the desired JSON schema, they wouldn't have bothered to write a custom marshaler.

And this kind of error can be somewhat annoying to track down (as I recently found out) because, if you added the custom marshaler for A without knowing that it's been used elsewhere in the code in this way, and you've got tests to prove that the serialization of A is correct, you wouldn't necessarily expect that a bunch of other stuff that uses B is going to break.

GoWebProd commented 4 years ago

Hello. Thank you for reporting