mailru / easyjson

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

Null UnknownField should not be skipped #285

Open egdeliya opened 4 years ago

egdeliya commented 4 years ago

Hi! Kind of really impressed with your library :heart:

But I have some issues with UnknownField null values. It is reproducible with the following test

func TestUnknownFieldsProxyNullField(t *testing.T) {
    baseJson := `{"Field1":"123","Field2":null}`

    s := StructWithUnknownsProxy{}

    err := s.UnmarshalJSON([]byte(baseJson))
    if err != nil {
        t.Errorf("UnmarshalJSON didn't expect error: %v", err)
    }

    if s.Field1 != "123" {
        t.Errorf("UnmarshalJSON expected to parse Field1 as \"123\". got: %v", s.Field1)
    }

    data, err := s.MarshalJSON()
    if err != nil {
        t.Errorf("MarshalJSON didn't expect error: %v", err)
    }

    if !reflect.DeepEqual(baseJson, string(data)) {
        t.Errorf("MarshalJSON expected to gen: %v. got: %v", baseJson, string(data))
    }
}

StructWithUnknownsProxy is copy-pasted from your unknown_fields.go :see_no_evil:

Field2 is skipped from resulting json. Error message : MarshalJSON expected to gen: {"Field1":"123","Field2":null}. got: {"Field1":"123"}

Looking through the generated UnmarshalJSON, it is found out, that null field is skipped from unknown fields map because of if in.IsNull() {...} check inside for loop

func easyjsonDd898260DecodeModels1(in *jlexer.Lexer, out *StructWithUnknownsProxy) {
    isTopLevel := in.IsStart()
    if in.IsNull() {
        if isTopLevel {
            in.Consumed()
        }
        in.Skip()
        return
    }
    in.Delim('{')
    for !in.IsDelim('}') {
        key := in.UnsafeString()
        in.WantColon()
        /*if in.IsNull() {
            in.Skip()
            in.WantComma()
            continue
        }*/
        switch key {
        case "Field1":
            out.Field1 = string(in.String())
        default:
            out.UnmarshalUnknown(in, key)
        }
        in.WantComma()
    }
    in.Delim('}')
    if isTopLevel {
        in.Consumed()
    }
}

And if I comment this check, then the test is passed.

Is it safe to delete this check? These null fields are really important for backward compatibility

I think this check should not be generated in case of easyjson.UnknownFieldsProxy is used

GoWebProd commented 4 years ago

Hello.

It's not safe to delete this check. If field1 is null, you receive error.

egdeliya commented 4 years ago

Thanks for the quick response,

I moved this check inside switch cases