-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support editions, required fields, and groups #45
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,10 @@ type UnmarshalOptions struct { | |
protoregistry.ExtensionTypeResolver | ||
} | ||
|
||
// If AllowPartial is set, input for messages that will result in missing | ||
// required fields will not return an error. | ||
AllowPartial bool | ||
Comment on lines
+58
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
// DiscardUnknown specifies whether to discard unknown fields instead of | ||
// returning an error. | ||
DiscardUnknown bool | ||
|
@@ -71,7 +75,15 @@ func (o UnmarshalOptions) Unmarshal(data []byte, message proto.Message) error { | |
if err := yaml.Unmarshal(data, &yamlFile); err != nil { | ||
return err | ||
} | ||
return o.unmarshalNode(&yamlFile, message, data) | ||
if err := o.unmarshalNode(&yamlFile, message, data); err != nil { | ||
return err | ||
} | ||
if !o.AllowPartial { | ||
if err := proto.CheckInitialized(message); err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
// ParseDuration parses a duration string into a durationpb.Duration. | ||
|
@@ -243,32 +255,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The caller previously had to check 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. |
||
switch field.Kind() { | ||
case protoreflect.BoolKind: | ||
return protoreflect.ValueOfBool(u.unmarshalBool(node, forKey)) | ||
return protoreflect.ValueOfBool(u.unmarshalBool(node, forKey)), true | ||
case protoreflect.Int32Kind, protoreflect.Sint32Kind, protoreflect.Sfixed32Kind: | ||
return protoreflect.ValueOfInt32(int32(u.unmarshalInteger(node, 32))) | ||
return protoreflect.ValueOfInt32(int32(u.unmarshalInteger(node, 32))), true | ||
case protoreflect.Int64Kind, protoreflect.Sint64Kind, protoreflect.Sfixed64Kind: | ||
return protoreflect.ValueOfInt64(u.unmarshalInteger(node, 64)) | ||
return protoreflect.ValueOfInt64(u.unmarshalInteger(node, 64)), true | ||
case protoreflect.Uint32Kind, protoreflect.Fixed32Kind: | ||
return protoreflect.ValueOfUint32(uint32(u.unmarshalUnsigned(node, 32))) | ||
return protoreflect.ValueOfUint32(uint32(u.unmarshalUnsigned(node, 32))), true | ||
case protoreflect.Uint64Kind, protoreflect.Fixed64Kind: | ||
return protoreflect.ValueOfUint64(u.unmarshalUnsigned(node, 64)) | ||
return protoreflect.ValueOfUint64(u.unmarshalUnsigned(node, 64)), true | ||
case protoreflect.FloatKind: | ||
return protoreflect.ValueOfFloat32(float32(u.unmarshalFloat(node, 32))) | ||
return protoreflect.ValueOfFloat32(float32(u.unmarshalFloat(node, 32))), true | ||
case protoreflect.DoubleKind: | ||
return protoreflect.ValueOfFloat64(u.unmarshalFloat(node, 64)) | ||
return protoreflect.ValueOfFloat64(u.unmarshalFloat(node, 64)), true | ||
case protoreflect.StringKind: | ||
u.checkKind(node, yaml.ScalarNode) | ||
return protoreflect.ValueOfString(node.Value) | ||
return protoreflect.ValueOfString(node.Value), true | ||
case protoreflect.BytesKind: | ||
return protoreflect.ValueOfBytes(u.unmarshalBytes(node)) | ||
return protoreflect.ValueOfBytes(u.unmarshalBytes(node)), true | ||
case protoreflect.EnumKind: | ||
return protoreflect.ValueOfEnum(u.unmarshalEnum(node, field)) | ||
return protoreflect.ValueOfEnum(u.unmarshalEnum(node, field)), true | ||
default: | ||
u.addErrorf(node, "unimplemented scalar type %v", field.Kind()) | ||
return protoreflect.Value{} | ||
return protoreflect.Value{}, false | ||
} | ||
} | ||
|
||
|
@@ -550,10 +562,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shorthand way of checking if kind is either message or group |
||
u.unmarshalMessage(node, message.ProtoReflect().Mutable(field).Message().Interface(), false) | ||
default: | ||
message.ProtoReflect().Set(field, u.unmarshalScalar(node, field, false)) | ||
if val, ok := u.unmarshalScalar(node, field, false); ok { | ||
message.ProtoReflect().Set(field, val) | ||
} | ||
} | ||
} | ||
|
||
|
@@ -569,29 +583,41 @@ func (u *unmarshaler) unmarshalList(node *yaml.Node, field protoreflect.FieldDes | |
} | ||
default: | ||
for _, itemNode := range node.Content { | ||
list.Append(u.unmarshalScalar(itemNode, field, false)) | ||
val, ok := u.unmarshalScalar(itemNode, field, false) | ||
if !ok { | ||
continue | ||
} | ||
list.Append(val) | ||
} | ||
} | ||
} | ||
} | ||
|
||
// Unmarshal the map, with explicit handling for maps to messages. | ||
func (u *unmarshaler) unmarshalMap(node *yaml.Node, field protoreflect.FieldDescriptor, mapVal protoreflect.Map) { | ||
if u.checkKind(node, yaml.MappingNode) { | ||
mapKeyField := field.MapKey() | ||
mapValueField := field.MapValue() | ||
for i := 1; i < len(node.Content); i += 2 { | ||
keyNode := node.Content[i-1] | ||
valueNode := node.Content[i] | ||
mapKey := u.unmarshalScalar(keyNode, mapKeyField, true) | ||
switch mapValueField.Kind() { | ||
case protoreflect.MessageKind, protoreflect.GroupKind: | ||
mapValue := mapVal.NewValue() | ||
u.unmarshalMessage(valueNode, mapValue.Message().Interface(), false) | ||
mapVal.Set(mapKey.MapKey(), mapValue) | ||
default: | ||
mapVal.Set(mapKey.MapKey(), u.unmarshalScalar(valueNode, mapValueField, false)) | ||
if !u.checkKind(node, yaml.MappingNode) { | ||
return | ||
} | ||
Comment on lines
+598
to
+600
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
mapKeyField := field.MapKey() | ||
mapValueField := field.MapValue() | ||
for i := 1; i < len(node.Content); i += 2 { | ||
keyNode := node.Content[i-1] | ||
valueNode := node.Content[i] | ||
mapKey, ok := u.unmarshalScalar(keyNode, mapKeyField, true) | ||
if !ok { | ||
continue | ||
} | ||
switch mapValueField.Kind() { | ||
case protoreflect.MessageKind, protoreflect.GroupKind: | ||
mapValue := mapVal.NewValue() | ||
u.unmarshalMessage(valueNode, mapValue.Message().Interface(), false) | ||
mapVal.Set(mapKey.MapKey(), mapValue) | ||
default: | ||
val, ok := u.unmarshalScalar(valueNode, mapValueField, false) | ||
if !ok { | ||
continue | ||
} | ||
mapVal.Set(mapKey.MapKey(), val) | ||
} | ||
} | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)