easyjson icon indicating copy to clipboard operation
easyjson copied to clipboard

Null UnknownField should not be skipped

Open egdeliya opened this issue 5 years ago • 2 comments

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

egdeliya avatar Apr 24 '20 13:04 egdeliya

Hello.

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

GoWebProd avatar Apr 24 '20 17:04 GoWebProd

Thanks for the quick response,

I moved this check inside switch cases

egdeliya avatar Apr 27 '20 07:04 egdeliya