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

equal: code doesn't distinguish between oneof fields when zero-valued #50

Closed
misberner opened this issue Aug 16, 2022 · 3 comments · Fixed by #51
Closed

equal: code doesn't distinguish between oneof fields when zero-valued #50

misberner opened this issue Aug 16, 2022 · 3 comments · Fixed by #51

Comments

@misberner
Copy link
Contributor

Because the comparison logic for oneof fields relies on the getters for the individual fields, it cannot differentiate between a field not being set, and a field being set to the zero value. While the nil checks allow distinguishing protos where one has a oneof field set to a zero value, while the other doesn't have any field in the oneof set, the code fails to distinguish protos where different fields in a oneof are set to the respective zero value.

Test case:

func TestEqualVT_Oneof_AbsenceVsZeroValue(t *testing.T) {
	a := &TestAllTypesProto3{
		OneofField: &TestAllTypesProto3_OneofUint32{
			OneofUint32: 0,
		},
	}
	b := &TestAllTypesProto3{
		OneofField: &TestAllTypesProto3_OneofString{
			OneofString: "",
		},
	}

	aJson, err := protojson.Marshal(a)
	require.NoError(t, err)
	bJson, err := protojson.Marshal(b)
	require.NoError(t, err)

	if a.EqualVT(b) {
		assert.JSONEq(t, string(aJson), string(bJson))
		err := fmt.Errorf("these %T should not be equal:\nmsg = %+v\noriginal = %+v", a, a, b)
		require.NoError(t, err)
	}
}

This is similar to #48 , but applies to oneofs and exercises different paths in the code generation.

@misberner
Copy link
Contributor Author

See misberner#2 for the fix (it depends on #49 , so I cannot create a PR against this repo yet, at least not without a blown-up diff)

@vmg vmg closed this as completed Aug 16, 2022
@vmg vmg reopened this Aug 16, 2022
@vmg
Copy link
Member

vmg commented Aug 16, 2022

Oops, closed too soon. Didn't notice this was a separate issue. Can you open a PR for misberner#2? :)

@misberner
Copy link
Contributor Author

Done, reopened as #51

@vmg vmg closed this as completed in #51 Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants