The Wayback Machine - https://web.archive.org/web/20201105192406/https://github.com/mailru/easyjson/issues/285
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null UnknownField should not be skipped #285

Open
egdeliya opened this issue Apr 24, 2020 · 2 comments
Open

Null UnknownField should not be skipped #285

egdeliya opened this issue Apr 24, 2020 · 2 comments

Comments

@egdeliya
Copy link

@egdeliya egdeliya commented Apr 24, 2020

Hi! Kind of really impressed with your library ❤️

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 🙈

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
Copy link
Collaborator

@GoWebProd GoWebProd commented Apr 24, 2020

Hello.

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

@egdeliya
Copy link
Author

@egdeliya egdeliya commented Apr 27, 2020

Thanks for the quick response,

I moved this check inside switch cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.