testing/protocmp: add IgnoreUnsetFields about protobuf HOT 8 OPEN

Implementing an IgnoreUnsetFields function in protocmp seems straight-forward:

func IgnoreUnsetFields(want proto.Message) cmp.Option {
	msg := want.ProtoReflect()
	md := msg.Descriptor()
	fields := md.Fields()
	var names []protoreflect.Name
	for i := 0; i < fields.Len(); i++ {
		fd := fields.Get(i)
		if msg.Has(fd) {
			continue // field is set, do not ignore
		}
		names = append(names, fd.Name())
	}
	return protocmp.IgnoreFields(want, names...)
}

…but I’m curious to hear what others think about this addition.

cc @neild @dsnet

from protobuf.

What's the expected behavior with field extensions?

from protobuf.

What's the expected behavior with field extensions?

Good question. Can you spell out what the concerns and options are?

It sounds like maybe we’ll need to use protoreflect.Range instead of walking only the msg.Descriptor().Fields(). Or were you thinking of something else/more?

Thanks

from protobuf.

I don't have any specific concerns, but just trying to remember things that would often bite us later on.

Overall this feature sounds fine. It reminds me of @neild's "messagediff" package, which IIRC had an asymmetrical comparison that did something similar.

from protobuf.

For extensions, I'd expect IgnoreUnsetFields(want) to ignore any extensions not set in want. I think that might require some additional handling beyond @stapelberg's implementation above.

The messagediff package (Google-internal) was based on a C++ library (not sure if that one is internal or open source) that contained a "partial compare" which compares two messages, considering only fields set on the right-hand message. This is quite convenient in tests.

I wonder if the IgnoreUnsetFields feature could work as a general-purpose cmp or cmpopt option, not a protocmp proto-specific one.

got := map[string]int{"a": 1, "b": 2}
want := map[string]int{"a": 1}
diff := cmp.Diff(got, want, cmpopts.IgnoreZero(want)) // ignores "b"

And perhaps a PartialDiff helper:

func PartialDiff(got, want any, opts ...Option) string {
  return cmp.Diff(x, y, IgnoreZero(y), cmp.Options(opts))
}

from protobuf.

I was curious about the messagediff package, thanks for mentioning it. It seems like the idea there is similar to the cmpopts.IgnoreFields. Can you link the partial compare you're referring to @neild? I thought that wouldn't be possible because default field values and unset values would be treated equally.

The way I see it, the biggest value of having an IgnoreUnsetFields (or maybe a better name would be IgnoreFieldsFrom) function over other approaches that can tackle the same problem is that the proposed approach here is compatible with parametrized tests that are so popular in Go.

In the table below all 'yes' values are positive, all 'no' values are negative.

I couldn't find a downside with the proposed approach, but there might be something I'm not considering and I should. I share the concern with @dsnet that there might be some unforeseen complications that will come to bite us later on.

The way I'll proceed is by creating this function in my team/org and popularizing it there.

from protobuf.

The messagediff package (Google-internal) was based on a C++ library (not sure if that one is internal or open source) that contained a "partial compare" which compares two messages, considering only fields set on the right-hand message. This is quite convenient in tests.

Are you referring to https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/util/message_differencer.h perhaps? It is a C++ message diffing library with partial comparison support.

[comparison table] Can the comparison library avoid using reflection?

I don’t think your “yes”es in this row are correct. The cmp library certainly uses reflection, and so does protoreflect.

I wonder if the IgnoreUnsetFields feature could work as a general-purpose cmp or cmpopt option, not a protocmp proto-specific one.

Perhaps! Though it might be prudent to start with a protocmp option that knows about protobuf messages, and later replace it with a call to the general-purpose IgnoreUnsetFields() if that is indeed a drop-in replacement.

@MasterMedo filed google/go-cmp#345 and retracted it with this comment:

I'm retracting this feature request. Structs are significantly different from Protobuf messages in the sense that the primitive values of structs don't have corresponding HasField functions. That would introduce a challenge in the current proposal because one couldn't distinguish between an unset primitive field and a field set to the default value of the primitive.

…but that’s not inherent to structs. proto3 does not have presence either (except for fields explicitly marked as “optional”), so it does not distinguish between an unset field and a default-valued field either. So, we’ll face the same issue with a protocmp option.

Returning to my original implementation:

Implementing an IgnoreUnsetFields function in protocmp seems straight-forward:

func IgnoreUnsetFields(want proto.Message) cmp.Option { … }

This function signature is different from the other protocmp signatures: the currently existing ones take a parameter to work with the type, whereas mine takes a parameter to work with the specific message. My signature is problematic because it requires an odd structure for table-driven tests: instead of preparing opts := []cmp.Option{protocmp.Transform(), …} once and then calling cmp.Diff(tt.want, got, opts) for each table entry, my signature requires re-constructing the opts based on tt.want for each table entry.

I think we should try to come up with an implementation that matches the semantics of the existing options better. I had a quick look at it, but diving into cmp’s bowels requires more time than I can spend on this right now. Suggestions and contributions welcome.

from protobuf.

Are you referring to https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/util/message_differencer.h perhaps? It is a C++ message diffing library with partial comparison support.

Yes, that's the one. The Google-internal messagediff package was my attempt at providing a Go alternative to it. (This predated official protobuf reflection, so the messagediff internals were quite a mess. Joe rewrote messagediff to use protobuf reflection at some point, and designed cmp/protocmp as a more general-purpose replacement.)

from protobuf.