Protobuf api V2: add protoMessage, merge and autoboxing wrapperspb

[seena-stripe]

Summary

This PR adds the remaining type implementation

• protoMessage matches https://github.com/stripe/skycfg/blob/trunk/internal/go/skycfg/proto_message.go
• maybeConvertToWrapper copies

  skycfg/internal/go/skycfg/proto_message.go

  Line 368 in 3410450

  func maybeConvertToWrapper(t reflect.Type, sky starlark.Value) (*reflect.Value, error) {

• merge is an implementation of proto.Merge

Implementation differences

The main implementation differences here is the new implementation does not store values on the underlying proto.Message, which is only used as a type to later instantiate when using toProtoMessage. All the values are stored only on the fields map (which is similar to the attrCache) from before but the old implementation attempted to keep both msg and attrCache in sync, producing a lot of extra code and some weird edge case bugs.

This means that NewMessage will not mutate the incoming proto message. This is mostly an internal detail except when passing in global values into skycfg -- the evaluated starlark will no longer be able to mutate incoming global variables. IMO this is a good thing, as it's surprising behavior, and a user can always fetch the updated value with AsProtoMessage

Example:

msg := &pb.MessageV2{}
globals := starlark.StringDict{
	"msg": protomodule.NewMessage(msg),
}
_, err := starlark.Eval(&starlark.Thread{}, "", test.src, globals)

In the old implementation, msg is directly mutated, so starlark.Eval can cause a side effect. This means repeatedly running eval can produce different results.

In the new implementation, msg is unchanged, and the user can use the following to extract the changed global if they wish

wrapped := protomodule.NewMessage(msg)
globals := starlark.StringDict{
	"msg": wrapped,
}
_, err := starlark.Eval(&starlark.Thread{}, "", test.src, globals)
out, err := AsProtoMessage(wrapped)

Tests

This is all tested on https://github.com/stripe/skycfg/tree/seena/protobuf-api-v2 but am not adding the tests as part of this PR as they involve making skycfg use *protoMessage in place of the legacy *skyProtoMessage -- this is the switch from old to new implementation, and I thought it would be better to separate that into it's own PR, since again this pr is mostly boilerplate

[kathleen-stripe]

😎

[kathleen-stripe]

nit: Since msg.ProtoReflect() is also used on line 36, could we move this line to be before line 36 so that line 36 can use this variable?

[kathleen-stripe]

OOC - would we want to revert the fields for which we've already set defaults? Or is it okay to leave the proto message in a partially updated state where some fields have defaults and others don't (but could)?

Relatedly, is there value in collecting errors (to return at the end of the method) while continuing to set as many default values as possible?

[seena-stripe]

Are you asking if SetDefaults should behave like first clearing the message like proto.Reset and then setting the defaults for fields that have them? If so this is what I would expect and that's what the stated behavior in the docs seems to apply https://github.com/stripe/skycfg/blob/trunk/docs/modules.asciidoc#proto.set_defaults but let me look into how the current implementation behaves

With errors, I don't think it really matters as when an error is returned the starlark/skycfg evaluation stops and returns the error. Since Skycfg is not mutating any values outside of the evaluation, all these values will just get thrown away anyway even if in a partial state

[seena-stripe]

Alright I tested the existing implementation and it does not behave this way, only setting values with defaults not clearing those without. It calls out to https://github.com/golang/protobuf/blob/master/proto/defaults.go#L25 and here it's filtering out to only fields with HasDefault() which AFAICT is only true for fields with proto2 defaults

This deviates from my expectation/Skycfg's state docs but is also what protobuf is doing as well. That said I would prefer to match the existing behavior as closely as possible to de-risk the new implementation refactor, and then change any behavior separately/explicitly afterwards

[kathleen-stripe]

Sorry for the confusion!

Are you asking if SetDefaults should behave like first clearing the message like proto.Reset and then setting the defaults for fields that have them?

No, I was asking "when SetDefaults errors, should we clear the errors that were successfully set earlier in SetDefaults?" i.e. Should setting defaults be all-or-nothing or "best effort"?

But based on

Since Skycfg is not mutating any values outside of the evaluation, all these values will just get thrown away anyway even if in a partial state

it sounds like this is all-or-nothing?

That said I would prefer to match the existing behavior as closely as possible to de-risk the new implementation refactor, and then change any behavior separately/explicitly afterwards

👍 SGTM!

[kathleen-stripe]

same question here about whether it's safe/expected to leave the proto in a partially-merged state

[kathleen-stripe]

There's already a check for this case on line 253 (similarly for case UInt64ValueType on line 315 below).

EDIT: This is the only blocking comment.