avoid reflection-based Transaction cloning

[tamird]

This allows reverting the ugly hacks in util/uuid. See gogo/protobuf#147.

[petermattis]

LGTM

---

Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[tbg]

LGTM (whether or not you want to wrap proto.Clone as we discussed one-to-one).

[tamird]

Wrapper is ready, you guys are gonna wanna take another look at this. 💣🔍

[petermattis]

Review status: 8 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

util/clone_proto.go, line 50 [r2] (raw file):
I'm not clear on why you need both isOrContainsUUID and findUUIDValue. I'm also not clear on why UUID is special. Can't you recursively look for any embedded array? Or are there embedded arrays that are ok>

---

util/clone_proto.go, line 57 [r2] (raw file):
You don't need to hold the types lock while cloning.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 8 of 24 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

util/clone_proto.go, line 50 [r2] (raw file):
isOrContainsUUID is meant to rule out types that are definitely never going to contain "forbidden" types. It's an optimization.

You're right, the problem is any array, not just uuid.UUID - I've made that change.

---

util/clone_proto.go, line 57 [r2] (raw file):
Ah, you're right, and in an earlier change, I wasn't. Done.

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 8 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

util/clone_proto.go, line 50 [r2] (raw file):
The optimization is that instead of iterating over every value of a slice/map you just use the type? I'm not sure that is worthwhile given this is going to happen once per type. I'm also not clear on why you need to find the specific array value. I think it would be as or more useful to output that path from the top-level struct down to the first array element found and that can be done in a single pass.

---

util/clone_proto.go, line 54 [r3] (raw file):
When can the type contain an array but you can't find it using findArrayValue?

---

Comments from the review on Reviewable.io

[tamird]

Review status: 8 of 24 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

util/clone_proto.go, line 50 [r2] (raw file):
The optimization is that for types which never contain arrays, they will be iterated over just once. To determine that, I have to iterate over the type.

For types which might contain arrays, I have to iterate over the value and see if it actually contains an array.

---

util/clone_proto.go, line 54 [r3] (raw file):

type foo struct {
  arrayPtr *[16]byte
}

var doesNotContainArray = foo{}

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 8 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

util/clone_proto.go, line 50 [r2] (raw file):
See my other comment below. I would determination of whether the proto contains an array (and is thus not clone-able) should be based purely on the type.

---

util/clone_proto.go, line 54 [r3] (raw file):
Won't that fail to clone? And iterating over the value to find the arrays means that the contents of the knownGood map are dependent on the contents of the pb passed to ProtoClone. That doesn't seem right. What am I missing?

---

Comments from the review on Reviewable.io

[tbg]

Reviewed 12 of 12 files at r1, 16 of 16 files at r2, 1 of 1 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from the review on Reviewable.io

[tamird]

I've optimized this further so that each type (including inner types) only gets traversed once (values may still be traversed many times).

---

Review status: 23 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

util/clone_proto.go, line 50 [r2] (raw file):
Yes, that would be nice, but would break existing usage.

---

util/clone_proto.go, line 54 [r3] (raw file):
Why would it fail to clone? What does failing to clone mean?

The contents of the known map depend only on the type of pb, not its value.

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 23 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

util/clone_proto.go, line 50 [r2] (raw file):
Ah, you're calling CloneProto on types which contain a nil Transaction. Ok, I think I understand this now. And I think this code needs a healthy dose of explanatory comments. The relation between isOrContainsArray and findArrayValue is not obvious. That you want to check that the proto has an actually reachable array vs one that is potentially reachable is also not obvious.

---

util/clone_proto.go, line 54 [r3] (raw file):
Yes knownGood is solely dependent on the type. I misread that.

Failing to clone would be the error (or is it a panic) you've seen when trying to clone a proto containing an array. But now that we've established that you're examining the value and not just the type I'm wondering what the utility is. If you call proto.Clone won't you also discover the proto contains an array/UUID?

---

Comments from the review on Reviewable.io

[tamird]

Review status: 23 of 24 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

util/clone_proto.go, line 54 [r3] (raw file):
There's no panic and no error. Behold the glory of https://github.com/golang/protobuf/blob/master/proto/clone.go#L204 (this code is the same in gogo/protobuf).

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 23 of 24 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

util/clone_proto.go, line 54 [r3] (raw file):
Oh, lovely. Ok, the rationale for this is good, the implementation looks good, just sprinkle some comments about what is going on. Oh, and the CloneProto comment is out of date (referring only to UUID). I think the CloneProto function comment would be a good place for some explanation about why this is necessary and the short comings of proto.Clone.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 23 of 24 files reviewed at latest revision, 1 unresolved discussion.

---

util/clone_proto.go, line 54 [r3] (raw file):
Done and also generalized this in case we find more kinds that need this treatment in the future.

---

Comments from the review on Reviewable.io

[petermattis]

LGTM

---

Review status: 23 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

util/clone_proto.go, line 67 [r7] (raw file):
Verboten. Is Tobias infecting you?

---

Comments from the review on Reviewable.io

[tbg]

Reviewed 1 of 1 files at r5.
Review status: 23 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

util/clone_proto.go, line 67 [r7] (raw file):
ssht.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 23 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

util/clone_proto.go, line 67 [r7] (raw file):
No way, man. I've been saying verboten for years.

---

Comments from the review on Reviewable.io