delete shadow copy proto-struct

[pfi79]

The new fabric-protos-go-apiv2 has copy protection in the structures.
This code is adapted for the transition to the new protos-go-apiv2.

[denyeart]

There is an open issue discussing potential update to protos-go-apiv2. Due to some concerns that were raised in that issue, my latest thinking was to not proceed with the update. Let's continue the discussion in that issue prior to moving forward with the PRs to update to protos-go-apiv2.

[pfi79]

There is an open issue discussing potential update to protos-go-apiv2. Due to some concerns that were raised in that issue, my latest thinking was to not proceed with the update. Let's continue the discussion in that issue prior to moving forward with the PRs to update to protos-go-apiv2.

Yes, it's a move towards protos-go-apiv2. But independent of it. No new protos, no new protobuf.

I specifically highlighted this code, which is independent of protos-go-apiv2.

I would suggest that it should be considered independent.

[denyeart]

There is an open issue discussing potential update to protos-go-apiv2. Due to some concerns that were raised in that issue, my latest thinking was to not proceed with the update. Let's continue the discussion in that issue prior to moving forward with the PRs to update to protos-go-apiv2.

Yes, it's a move towards protos-go-apiv2. But independent of it. No new protos, no new protobuf.

I specifically highlighted this code, which is independent of protos-go-apiv2.

I would suggest that it should be considered independent.

Yes, it can be considered independent. But in that case the PR description should mention the rationale for the various changes. For example adding proto.Clone() in various places seems to add a dependency where none was required previously. Can you describe the benefits? And some of the other changes seem not related, ideally they would be done in a separate pull request or at least mention the rationale for the changes in the PR description.

[pfi79]

Yes, it can be considered independent. But in that case the PR description should mention the rationale for the various changes. For example adding proto.Clone() in various places seems to add a dependency where none was required previously. Can you describe the benefits? And some of the other changes seem not related, ideally they would be done in a separate pull request or at least mention the rationale for the changes in the PR description.

I think the developers of the protobuf package added shadow copy protection to the new structures for a reason.
It means that they had good reasons for it.
And if we can't switch to a new version of protobuf yet, we can at least remove shadow copying in them. And in those places where copying is required make it more explicit for further code maintenance.

But if you don't consider this an important reason, let's wait until I prepare a version with all the changes and present it in the corresponding issue.

[bestbeforetoday]

See this comment describing why it is not safe to make shallow copies of Go protobuf messages, which explains why they are referred to in the protobuf API only as pointers (proto.Message interface):

golang/protobuf#1225 (comment)

[bestbeforetoday]

I think the answer here is just to reference the protobuf message as a pointer rather than dereferencing it and causing a copy-by-value. I'm not sure there is any need to clone the message

[pfi79]

fix

[bestbeforetoday]

The standard Gomega Equal matcher is not safe for use on protobuf messages at all. It simply does a deep equal comparison, and this is not valid for protobuf messages since they can be logically equal while also having some implementation fields with different values.

Some other parts of the codebase use proto.Equal for protobuf message comparison, and then use Gomega to Expect a True return value. I think a better approach is to implement a matcher specifically for protobuf messages, for example:

func ProtoEqual(expected proto.Message) types.GomegaMatcher {
	return &protoEqualMatcher{
		expected: expected,
	}
}

type protoEqualMatcher struct {
	expected proto.Message
}

func (m *protoEqualMatcher) Match(actual interface{}) (bool, error) {
	actualMessage, ok := actual.(proto.Message)
	if !ok {
		return false, fmt.Errorf("ProtoEqual expects a proto.Message, got a %T", actual)
	}

	return proto.Equal(m.expected, actualMessage), nil
}

func (m *protoEqualMatcher) FailureMessage(actual interface{}) string {
	if message, ok := actual.(proto.Message); ok {
		return fmt.Sprintf("Expected\n\t%s\nto equal\n\t%s", message, m.expected)
	}

	return fmt.Sprintf("Expected\n\t%#v\nto equal\n\t%s", actual, m.expected)
}

func (m *protoEqualMatcher) NegatedFailureMessage(actual interface{}) string {
	if message, ok := actual.(proto.Message); ok {
		return fmt.Sprintf("Expected\n\t%s\nnot to equal\n\t%s", message, m.expected)
	}

	return fmt.Sprintf("Expected\n\t%#v\nnot to equal\n\t%s", actual, m.expected)
}

This is then used as follows:

Expect(actualMsg).To(ProtoEqual(expectedMsg))

[pfi79]

fix