Support editions, required fields, and groups #45
Conversation
jhump
force-pushed
the
jh/properly-support-group-encoding
branch
from
057a786 to
fc1ad83
Compare
| @@ -243,32 +247,32 @@ func (u *unmarshaler) unmarshalScalar( | |||
| node *yaml.Node, | |||
| field protoreflect.FieldDescriptor, | |||
| forKey bool, | |||
| ) protoreflect.Value { | |||
| ) (protoreflect.Value, bool) { | |||
There was a problem hiding this comment.
The caller previously had to check value.IsValid() before using the returned value. None of the callers actually did, so they all had potential panic bugs (which I encountered when trying to use a group field).
To make it more explicit and more easily require that the caller check the value, it now also returns a bool that is false when the value is invalid.
| @@ -550,10 +554,12 @@ func (u *unmarshaler) unmarshalField(node *yaml.Node, field protoreflect.FieldDe | |||
| u.unmarshalList(node, field, message.ProtoReflect().Mutable(field).List()) | |||
| case field.IsMap(): | |||
| u.unmarshalMap(node, field, message.ProtoReflect().Mutable(field).Map()) | |||
| case field.Kind() == protoreflect.MessageKind: | |||
| case field.Message() != nil: | |||
There was a problem hiding this comment.
shorthand way of checking if kind is either message or group
| if !u.checkKind(node, yaml.MappingNode) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Sorry for the noisy diff here. If you change diff settings in GitHub UI to ignore whitespace diff, it will look better. To be more idiomatic Go, I changed this to early return so I could un-indent the main logic.
| // If AllowPartial is set, input for messages that will result in missing | ||
| // required fields will not return an error. | ||
| AllowPartial bool |
There was a problem hiding this comment.
When I added an test editions file, and used features to toggle use of groups and required fields, I discovered that required fields weren't correctly enforced. This comment and field name come directly from corresponding fields in proto.UnmarshalOptions and protojson.UnmarshalOptions. So, by default, this will complain if the message has required fields that the YAML contents failed to set.
jhump
force-pushed
the
jh/properly-support-group-encoding
branch
from
4a12b2f to
60d4769
Compare
jhump
force-pushed
the
jh/properly-support-group-encoding
branch
from
60d4769 to
fb9769e
Compare
| @@ -79,11 +79,11 @@ $(BIN): | |||
| @mkdir -p $(BIN) | |||
|
|
|||
| $(BIN)/buf: $(BIN) Makefile | |||
| go install github.com/bufbuild/buf/cmd/buf@latest | |||
| go install github.com/bufbuild/buf/cmd/buf@v1.36.0 | |||
There was a problem hiding this comment.
I pinned all these so that CI could pass. In a separate PR, we can update this repo to require at least go 1.21 and then also update all of the new lint violations (latest golangci-lint has new checks). Then we can update these versions to the latest. (Though I'd still recommend keeping them pinned for more hermetic/reproducible CI results.)
|
@Alfus, I was about to merge this when it occurred to me that |
The issues I discovered with editions support were actually just the handling of required fields and groups (which are also proto2 features, but were absent in proto3). Group syntax is still disallowed in editions, but group encoding can be enabled via a feature.