[protobuf] encoding: Add EmitUnpopulatedSkipNull option
Gopher Robot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Joseph Tsai, Shmuel Eiderman.
4 comments:
File encoding/protojson/encode.go:
Patch Set #1, Line 87:
// nil-value messages."...does not emit unpopulated messages fields."
We tried to avoid the terminology "nil-value message" in documentation. The protobuf data model does not have a concept of a "nil message"; the case being described here is an unpopulated field of message type, where "unpopulated" is an attribute of the *field* not the *field value* (the message).
nil messages are an aspect of one particular implementation of messages: the generated open struct API. The `dynamicpb` package is an example of an alternate implementation that doesn't have nil messages, but does have unpopulated message fields.
Patch Set #1, Line 89:
// of the cpp protobuf library.No need to give the history of the option.
You could say "Equivalent to the C++ always_print_primitive_fields option", but using a consistent name like `EmitUnpopulatedPrimitiveFields` should convey that without the need to explicitly document it.
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull boolPerhaps `EmitUnpopulatedPrimitiveFields`, which is a compromise between the existing Go EmitUnpopulated and the C++ `always_print_primitive_fields`?
"SkipNull" isn't consistent with any terminology I can think of: There's no "null" in Go, and I don't think there's one in the protobuf data model.
Patch Set #1, Line 208:
continueThis is skipping proto2 scalar fields, which isn't what the documentation of the option says happens. Either the documentation or the implementation is wrong; which is it?
This is not skipping repeated fields. Is this intentional? What does C++ emit for an unpopulated repeated field when always_print_primitive_fields is set?
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Shmuel Eiderman has uploaded this change for review.
encoding: Add EmitUnpopulatedSkipNull option
Introduce the EmitUnpopulatedSkipNull in addition to the existing
EmitUnpopulated option.
Both options can not be used simultaneously.
EmitUnpopulatedSkipNull is added to emit json messages compatible with
the `always_print_primitive_fields` option of the cpp protobuf library.
See descussion:
https://github.com/golang/protobuf/issues/1536
Change-Id: Ib29b69d630fa3e8d8fdeb0de43b5683f30152151
---
M encoding/protojson/encode.go
M encoding/protojson/encode_test.go
2 files changed, 222 insertions(+), 3 deletions(-)
diff --git a/encoding/protojson/encode.go b/encoding/protojson/encode.go
index 66b9587..98806ae 100644
--- a/encoding/protojson/encode.go
+++ b/encoding/protojson/encode.go
@@ -81,6 +81,14 @@
// ╚═══════╧════════════════════════════╝
EmitUnpopulated bool
+ // EmitUnpopulatedSkipNull specifies whether to emit unpopulated
+ // primitive fields.
+ // Behaves similarly as EmitUnpopulated, except that it does not emit
+ // nil-value messages.
+ // Added for compatibility with the `always_print_primitive_fields` option
+ // of the cpp protobuf library.
+ EmitUnpopulatedSkipNull bool
+
// Resolver is used for looking up types when expanding google.protobuf.Any
// messages. If nil, this defaults to using protoregistry.GlobalTypes.
Resolver interface {
@@ -178,7 +186,11 @@
// unpopulatedFieldRanger wraps a protoreflect.Message and modifies its Range
// method to additionally iterate over unpopulated fields.
-type unpopulatedFieldRanger struct{ protoreflect.Message }
+type unpopulatedFieldRanger struct {
+ protoreflect.Message
+
+ skipNull bool
+}
func (m unpopulatedFieldRanger) Range(f func(protoreflect.FieldDescriptor, protoreflect.Value) bool) {
fds := m.Descriptor().Fields()
@@ -192,6 +204,9 @@
isProto2Scalar := fd.Syntax() == protoreflect.Proto2 && fd.Default().IsValid()
isSingularMessage := fd.Cardinality() != protoreflect.Repeated && fd.Message() != nil
if isProto2Scalar || isSingularMessage {
+ if m.skipNull {
+ continue
+ }
v = protoreflect.Value{} // use invalid value to emit null
}
if !f(fd, v) {
@@ -217,8 +232,12 @@
defer e.EndObject()
var fields order.FieldRanger = m
- if e.opts.EmitUnpopulated {
- fields = unpopulatedFieldRanger{m}
+ if e.opts.EmitUnpopulated && e.opts.EmitUnpopulatedSkipNull {
+ return errors.New("cannot use EmitUnpopulated and EmitUnpopulatedSkipNull together")
+ } else if e.opts.EmitUnpopulated {
+ fields = unpopulatedFieldRanger{m, false}
+ } else if e.opts.EmitUnpopulatedSkipNull {
+ fields = unpopulatedFieldRanger{m, true}
}
if typeURL != "" {
fields = typeURLFieldRanger{fields, typeURL}
diff --git a/encoding/protojson/encode_test.go b/encoding/protojson/encode_test.go
index adda076..01c816b 100644
--- a/encoding/protojson/encode_test.go
+++ b/encoding/protojson/encode_test.go
@@ -2194,6 +2194,206 @@
"optString": null
}`,
}, {
+ desc: "EmitUnpopulated & EmitUnpopulatedSkipNull",
+ mo: protojson.MarshalOptions{EmitUnpopulated: true, EmitUnpopulatedSkipNull: true},
+ input: &pb2.Scalars{},
+ wantErr: true,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto2 optional scalars",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb2.Scalars{},
+ want: `{}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto3 scalars",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb3.Scalars{},
+ want: `{
+ "sBool": false,
+ "sInt32": 0,
+ "sInt64": "0",
+ "sUint32": 0,
+ "sUint64": "0",
+ "sSint32": 0,
+ "sSint64": "0",
+ "sFixed32": 0,
+ "sFixed64": "0",
+ "sSfixed32": 0,
+ "sSfixed64": "0",
+ "sFloat": 0,
+ "sDouble": 0,
+ "sBytes": "",
+ "sString": ""
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto2 enum",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb2.Enums{},
+ want: `{
+ "rptEnum": [],
+ "rptNestedEnum": []
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto3 enum",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb3.Enums{},
+ want: `{
+ "sEnum": "ZERO",
+ "sNestedEnum": "CERO"
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto2 message and group fields",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb2.Nests{},
+ want: `{
+ "rptNested": [],
+ "rptgroup": []
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto3 message field",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb3.Nests{},
+ want: `{}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto2 empty message and group fields",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb2.Nests{
+ OptNested: &pb2.Nested{},
+ Optgroup: &pb2.Nests_OptGroup{},
+ },
+ want: `{
+ "optNested": {},
+ "optgroup": {},
+ "rptNested": [],
+ "rptgroup": []
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto3 empty message field",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb3.Nests{
+ SNested: &pb3.Nested{},
+ },
+ want: `{
+ "sNested": {
+ "sString": ""
+ }
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: proto2 required fields",
+ mo: protojson.MarshalOptions{
+ AllowPartial: true,
+ EmitUnpopulatedSkipNull: true,
+ },
+ input: &pb2.Requireds{},
+ want: `{}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: repeated fields",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb2.Repeats{},
+ want: `{
+ "rptBool": [],
+ "rptInt32": [],
+ "rptInt64": [],
+ "rptUint32": [],
+ "rptUint64": [],
+ "rptFloat": [],
+ "rptDouble": [],
+ "rptString": [],
+ "rptBytes": []
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: repeated containing empty message",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb2.Nests{
+ RptNested: []*pb2.Nested{nil, {}},
+ },
+ want: `{
+ "rptNested": [
+ {},
+ {}
+ ],
+ "rptgroup": []
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: map fields",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb3.Maps{},
+ want: `{
+ "int32ToStr": {},
+ "boolToUint32": {},
+ "uint64ToEnum": {},
+ "strToNested": {},
+ "strToOneofs": {}
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: map containing empty message",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb3.Maps{
+ StrToNested: map[string]*pb3.Nested{
+ "nested": &pb3.Nested{},
+ },
+ StrToOneofs: map[string]*pb3.Oneofs{
+ "nested": &pb3.Oneofs{},
+ },
+ },
+ want: `{
+ "int32ToStr": {},
+ "boolToUint32": {},
+ "uint64ToEnum": {},
+ "strToNested": {
+ "nested": {
+ "sString": ""
+ }
+ },
+ "strToOneofs": {
+ "nested": {}
+ }
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: oneof fields",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb3.Oneofs{},
+ want: `{}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: extensions",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: func() proto.Message {
+ m := &pb2.Extensions{}
+ proto.SetExtension(m, pb2.E_OptExtNested, &pb2.Nested{})
+ proto.SetExtension(m, pb2.E_RptExtNested, []*pb2.Nested{
+ nil,
+ {},
+ })
+ return m
+ }(),
+ want: `{
+ "[pb2.opt_ext_nested]": {},
+ "[pb2.rpt_ext_nested]": [
+ {},
+ {}
+ ]
+}`,
+ }, {
+ desc: "EmitUnpopulatedSkipNull: with populated fields",
+ mo: protojson.MarshalOptions{EmitUnpopulatedSkipNull: true},
+ input: &pb2.Scalars{
+ OptInt32: proto.Int32(0xff),
+ OptUint32: proto.Uint32(47),
+ OptSint32: proto.Int32(-1001),
+ OptFixed32: proto.Uint32(32),
+ OptSfixed32: proto.Int32(-32),
+ OptFloat: proto.Float32(1.02),
+ OptBytes: []byte("谷歌"),
+ },
+ want: `{
+ "optInt32": 255,
+ "optUint32": 47,
+ "optSint32": -1001,
+ "optFixed32": 32,
+ "optSfixed32": -32,
+ "optFloat": 1.02,
+ "optBytes": "6LC35q2M"
+}`,
+ }, {
desc: "UseEnumNumbers in singular field",
mo: protojson.MarshalOptions{UseEnumNumbers: true},
input: &pb2.Enums{
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull boolPerhaps `EmitUnpopulatedPrimitiveFields`, which is a compromise between the existing Go EmitUnpopula […]
“null” does appear a few times in the JSON-mapping part of the proto3 standard, which means it is potentially relevant. In fact, ignoring the documentation, as you note below it will “skip[] proto2 scalar fields” which—per the documentation for `EmitUnpopulated`—be emitted as `null`.
I think we need to settle on some consistency in terminology and code behavior.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Cassondra Foesch, Joseph Tsai, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
“null” does appear a few times in the JSON-mapping part of the proto3 standard, which means it is po […]
Ah, that's true; `null` does appear in JSON, so this does make sense in this context.
I still think this should be named `EmitUnpopulatedPrimitiveFields` for consistency with C++, and it should behave identically to the C++ option for proto3 fields. (IIRC, the C++ JSON serializer doesn't handle proto2 at all.)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
Ah, that's true; `null` does appear in JSON, so this does make sense in this context. […]
I would agree with this.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai.
4 comments:
File encoding/protojson/encode.go:
Patch Set #1, Line 87:
// nil-value messages."...does not emit unpopulated messages fields." […]
This is less about Go's Nil values and more about the `"null"` values that are actually being printed in the encoded json. That's why it made sense to me to explicitly say that.
I could have changed it to
```
// Behaves similarly as EmitUnpopulated, except that it does not emit
// "null" json fields.
```
Patch Set #1, Line 89:
// of the cpp protobuf library.
No need to give the history of the option. […]
More about the name later on
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
I would agree with this.
I started out with `EmitUnpopulatedPrimitiveFields` (see GitHub issue) - but then I thought to myself: Most Go users don't care about the CPP library of protobuf and never even heard of envoy or grpc transcoding.
They just called the "to-json" serialization with `EmitUnpopulated` and now they got empty fields with `"null"`s which they don't like - so naturally they will just call `EmitUnpopulatedSkipNull`.
Maybe the fact that I got here from C++ compatibility is biased, and for power users such as myself I would just find the compatibility note in the documentation.Thoughts?
Patch Set #1, Line 208:
continue
This is skipping proto2 scalar fields, which isn't what the documentation of the option says happens […]
Yes, at the end I just understood that this is more about not printing `"null"` values in the output json. This is the line that prints it.
C++ does emit empty maps and repeated fields.
It makes sense since if I get an empty response from a `ListResourcesResponse` message I want it to be:
```
{
"resources": []
}
```
and not
```
{}
```
Anyway, this what c++ does as well.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull boolI started out with `EmitUnpopulatedPrimitiveFields` (see GitHub issue) - but then I thought to mysel […]
While we can always understand that many Go protobuf users are probably only ever interacting with the standard through Golang, it is still always the case that protobuf is a language-agnostic standard, where we map the semantics of protobuf as best as possible to our particular languages.
However, the original protobuf semantic is largely an “overriding” interest. That is, the interoperability and compatibility of the languages takes precedence over single-language interests.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai.
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
While we can always understand that many Go protobuf users are probably only ever interacting with t […]
I don't mind renaming and fixing comments, EmitUnpopulatedPrimitiveFields it is then
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Cassondra Foesch, Joseph Tsai, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
I don't mind renaming and fixing comments, EmitUnpopulatedPrimitiveFields it is then
I went and dug through the C++ implementation some more, and this is a bit of a mess.
I can't find a definitive source for the definition of a "primitive field", but it appears to be anything other than a message. go/proto-encoding#packed says "any scalar type that is not string or bytes", but "non-message" seems to be the more common definition.
Setting `always_print_primitive_fields` will:
- Print default-valued proto3 non-message fields. (`"int32_value: 0"`) The documentation implies that this is the purpose of the option.
- Print unset optional proto2 non-message fields. (`"optional_int32_value: 0"`) This is...well, I suppose it's technically what the option says, but it doesn't seem right to me at all.
- Print empty repeated primitive fields. (`"repeated_int32_value: []"`) The documentation doesn't mention this, but okay, it's a primitive field.
- Print empty repeated message fields. (`"repeated_message: []"`) This isn't mentioned in the documentation, and it doesn't follow from the name of the option because these fields are not primitive, and I have no idea whether this is intentional or not.
- I have no idea what it does with maps, and it looks like there are no tests covering this case so whatever it is is probably accidental.
So I no longer feel we should be consistent with C++, because the C++ option makes no sense and I really don't think we should match the behavior of printing optional fields as if they were set.
How about `EmitEmptyFields`?
```
// EmitEmptyFields specifies whether to emit proto3 fields with zero values,
// empty lists, and empty maps.
// The JSON value emitted is as follows:
// ╔═══════╤════════════════════════════╗
// ║ JSON │ Protobuf field ║
// ╠═══════╪════════════════════════════╣
// ║ false │ proto3 boolean fields ║
// ║ 0 │ proto3 numeric fields ║
// ║ "" │ proto3 string/bytes fields ║
// ║ [] │ list fields ║
// ║ {} │ map fields ║
// ╚═══════╧════════════════════════════╝
```Alternatively, perhaps we should have one knob that controls non-optional proto3 fields, and a second knob that controls repeated fields. It's more knobs, but it reduces the possibility that someone will want some other configuration of settings here.
(Note: I am an emeritus maintainer of the Go protobuf implementation. Lasse Folger should make the decision on whether to accept this feature, what the exact semantics are, and what the option name or names should be.)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
I went and dug through the C++ implementation some more, and this is a bit of a mess. […]
All makes sense to me, at this point I will blindly just paste anything 😊
(As long as as there are no "null" messages and as long as empty repeated fields are still "[]")
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Shmuel Eiderman.
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
All makes sense to me, at this point I will blindly just paste anything 😊 […]
Hm… I’m unsure about the use of the word “empty”, as this doesn’t seem to really fit for me in two of the cases: false, and 0.
I’m wondering if “default values” is possible, because the standard says the default value of a message is “the field is not set”, which would suggest even when emitting default values, a message field should not appear...? Plus, this would also allow us to align more closely onto the same terminology used by the standard itself?
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Joseph Tsai, Lasse Folger, Shmuel Eiderman.
Shmuel Eiderman uploaded patch set #2 to this change.
encoding: Add EmitDefaultValues option
Introduce the EmitDefaultValues in addition to the existing
EmitUnpopulated option.
Both options can not be used simultaneously.
EmitDefaultValues is added to emit json messages more compatible with
the `always_print_primitive_fields` option of the cpp protobuf library.
See descussion:
https://github.com/golang/protobuf/issues/1536
Change-Id: Ib29b69d630fa3e8d8fdeb0de43b5683f30152151
---
M encoding/protojson/encode.go
M encoding/protojson/encode_test.go
2 files changed, 218 insertions(+), 3 deletions(-)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger.
3 comments:
File encoding/protojson/encode.go:
Patch Set #1, Line 87:
// nil-value messages.This is less about Go's Nil values and more about the `"null"` values that are actually being printe […]
Changed
Patch Set #1, Line 89:
// of the cpp protobuf library.
More about the name later on
Changed, is it good now?
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
Hm… I’m unsure about the use of the word “empty”, as this doesn’t seem to really fit for me in two o […]
Changed
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger.
2 comments:
Patchset:
Now considering the code instead of high-level discussion.
File encoding/protojson/encode.go:
if e.opts.EmitUnpopulated && e.opts.EmitDefaultValues {
return errors.New("cannot use EmitUnpopulated and EmitDefaultValues together")
} else if e.opts.EmitUnpopulated {
fields = unpopulatedFieldRanger{m, false}
} else if e.opts.EmitDefaultValues {
fields = unpopulatedFieldRanger{m, true}
Normally in Go, it’s preferred to use `switch { case cond1: … case cond2: … }` rather than `if cond1 { … } else if cond2 { … }`
Also, I think we might want to pick an overriding behavior (and document it) rather than unexpectedly returning a new and undocumented error. I’m thinking `EmitUnpopulated` since it generates a strict superset of what `EmitDefaultValues` generates.
And a final note, it might be a good idea to switch to using field names (`unpopulatedFieldRanger{ Message: m, skipNull: true}`) so the code self-documents, now that we’re passing in an otherwise unqualified `true` and `false`.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger.
Shmuel Eiderman uploaded patch set #3 to this change.
encoding: Add EmitDefaultValues option
Introduce the EmitDefaultValues in addition to the existing
EmitUnpopulated option.
EmitDefaultValues is added to emit json messages more compatible with
the `always_print_primitive_fields` option of the cpp protobuf library.
EmitUnpopulated overrides EmitDefaultValues since the former generates
a strict superset of the latter.
See descussion:
https://github.com/golang/protobuf/issues/1536
Change-Id: Ib29b69d630fa3e8d8fdeb0de43b5683f30152151
---
M encoding/protojson/encode.go
M encoding/protojson/encode_test.go
2 files changed, 233 insertions(+), 3 deletions(-)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger.
if e.opts.EmitUnpopulated && e.opts.EmitDefaultValues {
return errors.New("cannot use EmitUnpopulated and EmitDefaultValues together")
} else if e.opts.EmitUnpopulated {
fields = unpopulatedFieldRanger{m, false}
} else if e.opts.EmitDefaultValues {
fields = unpopulatedFieldRanger{m, true}
Normally in Go, it’s preferred to use `switch { case cond1: … case cond2: … }` rather than `if cond1 […]
Done
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger.
Shmuel Eiderman uploaded patch set #4 to this change.
encoding: Add EmitDefaultValues option
Introduce the EmitDefaultValues in addition to the existing
EmitUnpopulated option.
EmitDefaultValues is added to emit json messages more compatible with
the `always_print_primitive_fields` option of the cpp protobuf library.
EmitUnpopulated overrides EmitDefaultValues since the former generates
a strict superset of the latter.
See descussion:
https://github.com/golang/protobuf/issues/1536
Change-Id: Ib29b69d630fa3e8d8fdeb0de43b5683f30152151
---
M encoding/protojson/encode.go
M encoding/protojson/encode_test.go
2 files changed, 233 insertions(+), 3 deletions(-)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Lasse Folger, Shmuel Eiderman.
Patch set 4:Code-Review +1
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Lasse Folger, Shmuel Eiderman.
1 comment:
Patchset:
Probably still want feedback on Damien, and then we’ll need obviously someone who is a maintainer.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger.
1 comment:
Patchset:
Probably still want feedback on Damien, and then we’ll need obviously someone who is a maintainer.
Thanks!
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull boolChanged
I followed the discussion but couldn't make up my mind so far.
My understanding is the currently suggested semantic is:// EmitDefaultValues specifies whether to emit proto3 fields with zero values,
// empty lists, and empty maps.
// The JSON value emitted is as follows:
// ╔═══════╤════════════════════════════╗
// ║ JSON │ Protobuf field ║
// ╠═══════╪════════════════════════════╣
// ║ false │ proto3 boolean fields ║
// ║ 0 │ proto3 numeric fields ║
// ║ "" │ proto3 string/bytes fields ║
// ║ [] │ list fields ║
// ║ {} │ map fields ║
// ╚═══════╧════════════════════════════╝
What I dislike about this is that it does not match the definition of default values in the proto2 scalar field case.
The spec says:
```
If the default value is not specified for an optional element, a type-specific default value is used instead: for strings, the default value is the empty string. For bytes, the default value is the empty byte string. For bools, the default value is false. For numeric types, the default value is zero. For enums, the default value is the first value listed in the enum’s type definition.
```This seems to contradict the option in that it does not emit default values for proto2 but for proto3 which seems inconsistent. It is unfortunate that `EmitUnpopulated` documents what it does with unpopulated proto3 fields given that proto3 does not have a concept of populated/unpopulated without explicit optional label and that it different from proto2 for fields with the optional label.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Lasse Folger, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
I followed the discussion but couldn't make up my mind so far. […]
Default values in proto2 have always been very complicated. While you do definitely quote some relevant code about what proto2 does when there is not an explicit default value specified, proto2 does have explicit non-zero default values, which would complicate any form of emitting those values for any optional field that is not present.
Also note that proto2 does not have any JSON mapping standardization, but in the case of proto2 optional values, there is inherent presence detection in optional fields, and—just like proto3 optional fields—if the field is not set, then the field should remain not set. This is why `EmitUnpopulatedFields` prints `null` not a concrete default value.
Perhaps it is best to explain that proto3 and proto2 optional fields that are unset will be omitted under `EmitDefaultValues` in order to reflect their presence-sensing semantics.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
Default values in proto2 have always been very complicated. […]
I don't disagree with what you are saying.
It feels like this API is something like `EmitDefaultFieldsWithoutPresenceTracking` which is a horrible name but I want to make sure that I understand the desired solution.
I don't suggest to use that specific name but maybe there is something more concise that encapsulated this meaning.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Lasse Folger, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
I don't disagree with what you are saying. […]
`EmitNonoptionalDefaultValues` 😬
I think this is a case where we’re going to face issues with any more detailed name. But much like linguists can often omit the quality of the rhotic in phonetic transcription when it is non-phonemic, (that is using /r/ instead of /ɹ/ in English) I think getting more detailed/precise isn’t going to have diminishing returns.
I think `EmitDefaultValues` with a documentary note that “presence-sensing fields that are omitted will remain omitted to preserve presence-sensing.”
Or `EmitProto3DefaultValues` but again… being more precise might not actually confer any benefit to offset the potential confusion.
🤔 Actually, are we testing the behavior of proto3 `optional` tagged fields already?
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
`EmitNonoptionalDefaultValues` 😬 […]
>I think EmitDefaultValues with a documentary note that “presence-sensing fields that are omitted will remain omitted to preserve presence-sensing.”
I like this idea. I think this removed most the ambiguity.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
>I think EmitDefaultValues with a documentary note that “presence-sensing fields that are omitted wi […]
And this is why originally I uploaded this CL as `EmitUnpopulatedSkipNull`.
I just imagine some other simpletons (read cave-men) such as myself using `EmitUnpopulated`.
Then seeing all those `null` values, and trying to check for a NOT_NULL flag and there it is, even the name "says what it does": `EmitUnpopulatedSkipNull`.
Even the code itself just checks that it's not null before emitting - it is that self documenting.
`EmitDefaultValues`, `EmitProto3DefaultValues`, `EmitDefaultFieldsWithoutPresenceTracking` and documentation that includes "presence-sensing fields" might as well be `EmitUnpopulated2` - the user will just see what each one prints and decide that way - hoping for the behavior not to break.
As opposed to `EmitUnpopulatedSkipNull` which will never print `null`s since this is the name of the option.Just my two cents
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Michael Stapelberg (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
And this is why originally I uploaded this CL as `EmitUnpopulatedSkipNull`. […]
I like EmitDefaultValues, along with a description.
I want to caution against relying too much on terms such as “presence-sensing”, because while they are descriptive to us protobuf maintainers, I fear that users won’t understand what the term means.
I would encourage adding examples of the relevant cases to the explanation, perhaps in a “this is what happens by default:” vs. “this happens with EmitDefaultValues enabled” framing.
(Nit: I would also like it if we could avoid using the word “omit” when describing the “emit” feature, because it’s very easy to misread and get confused.)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
I like EmitDefaultValues, along with a description. […]
Thank you all for the input.
I suggest to move forward with this, please name the option `EmitDefaultValues`, add an explanation according to Cassondra's suggestion (presence-sensing fields that are omitted will remain omitted to preserve presence-sensing) and call out the differences to `EmitUnpopulated` e.g. by adding a table (as Michael suggested).
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Shmuel Eiderman.
Shmuel Eiderman uploaded patch set #5 to this change.
encoding: Add EmitDefaultValues option
Introduce the EmitDefaultValues in addition to the existing
EmitUnpopulated option.
EmitDefaultValues is added to emit json messages more compatible with
the `always_print_primitive_fields` option of the cpp protobuf library.
EmitUnpopulated overrides EmitDefaultValues since the former generates
a strict superset of the latter.
See descussion:
https://github.com/golang/protobuf/issues/1536
Change-Id: Ib29b69d630fa3e8d8fdeb0de43b5683f30152151
---
M encoding/protojson/encode.go
M encoding/protojson/encode_test.go
2 files changed, 249 insertions(+), 3 deletions(-)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger, Michael Stapelberg.
1 comment:
File encoding/protojson/encode.go:
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
Thank you all for the input. […]
Already named `EmitDefaultValues`, so all good on that front.
Modified the documentation, although I do agree with Michael that having emit and omit in the same sentence can be very confusing in addition to the "presence sensing" instructions.
P.S. if anyone wants to sudo-override my phrasing I would welcome that, might save us a few round trips :-)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
Patch set 5:Code-Review +2
2 comments:
File encoding/protojson/encode.go:
Patch Set #1, Line 87:
// nil-value messages.Changed
Thanks for adjusting this. The current version looks good to me.
Patch Set #1, Line 90:
EmitUnpopulatedSkipNull bool
Already named `EmitDefaultValues`, so all good on that front. […]
Thanks for adjusting this and thanks for your patience. API additions are always difficult because you can't realy change them later. The current version looks good to me.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
1 comment:
Patchset:
I'll give others 1 more day to chime in. If they don't have any objections, I'll merge this merge.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Michael Stapelberg (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Shmuel Eiderman.
2 comments:
Patchset:
Could you also go through the comment threads that are resolved by now and mark them as resolved? It’s hard to keep track of where this review stands otherwise.
File encoding/protojson/encode.go:
// ╔═══════╤════════════════════════════╗
// ║ JSON │ Protobuf field ║
// ╠═══════╪════════════════════════════╣
// ║ false │ proto3 boolean fields ║
// ║ 0 │ proto3 numeric fields ║
// ║ "" │ proto3 string/bytes fields ║
// ║ N/A │ proto2 scalar fields ║
// ║ N/A │ message fields ║
// ║ [] │ list fields ║
// ║ {} │ map fields ║
// ╚═══════╧════════════════════════════╝
This table is just copied from EmitUnpopulated above.
It should be adjusted to convey what EmitDefaultValues does. I was thinking something along these lines:
```
|----------------------+---------+-------------------|
| Protobuf field | default | EmitDefaultValues |
|----------------------+---------+-------------------|
| proto3 bool | N/A | false |
| proto3 optional bool | N/A | N/A |
…
```
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
Patch set 5:Code-Review +1
1 comment:
File encoding/protojson/encode.go:
// ╔═══════╤════════════════════════════╗
// ║ JSON │ Protobuf field ║
// ╠═══════╪════════════════════════════╣
// ║ false │ proto3 boolean fields ║
// ║ 0 │ proto3 numeric fields ║
// ║ "" │ proto3 string/bytes fields ║
// ║ N/A │ proto2 scalar fields ║
// ║ N/A │ message fields ║
// ║ [] │ list fields ║
// ║ {} │ map fields ║
// ╚═══════╧════════════════════════════╝
This table is just copied from EmitUnpopulated above. […]
I’m not sure that N/A is particularly clear. Also the conflation of “default” in such a table is not going to do as much as including “omit” in text about “emission”.
I think including “unset proto3 optional field” is a good choice, but just having it black would be more clear. With a mark “anywhere the JSON field is empty, the field is not emitted at all.” Or something similar.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Michael Stapelberg (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Shmuel Eiderman.
// ╔═══════╤════════════════════════════╗
// ║ JSON │ Protobuf field ║
// ╠═══════╪════════════════════════════╣
// ║ false │ proto3 boolean fields ║
// ║ 0 │ proto3 numeric fields ║
// ║ "" │ proto3 string/bytes fields ║
// ║ N/A │ proto2 scalar fields ║
// ║ N/A │ message fields ║
// ║ [] │ list fields ║
// ║ {} │ map fields ║
// ╚═══════╧════════════════════════════╝
I’m not sure that N/A is particularly clear.
Just going by what’s already in our tables. But sure, I don’t have strong opinions on this.
Also the conflation of “default” in such a table is not going to do as much as including “omit” in text about “emission”.
Agreed. I considered naming it “usual behavior” or something, but couldn’t really convince myself of one option over the others. I left it at “default”, but agree that it can be made clearer.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai.
3 comments:
File encoding/protojson/encode.go:
Patch Set #1, Line 87:
// nil-value messages.
Thanks for adjusting this. The current version looks good to me.
Done
Patch Set #1, Line 89:
// of the cpp protobuf library.
Changed, is it good now?
Acknowledged
Patch Set #1, Line 208:
continueYes, at the end I just understood that this is more about not printing `"null"` values in the output […]
Done
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai.
Shmuel Eiderman uploaded patch set #6 to this change.
encoding: Add EmitDefaultValues option
Introduce the EmitDefaultValues in addition to the existing
EmitUnpopulated option.
EmitDefaultValues is added to emit json messages more compatible with
the `always_print_primitive_fields` option of the cpp protobuf library.
EmitUnpopulated overrides EmitDefaultValues since the former generates
a strict superset of the latter.
See descussion:
https://github.com/golang/protobuf/issues/1536
Change-Id: Ib29b69d630fa3e8d8fdeb0de43b5683f30152151
---
M encoding/protojson/encode.go
M encoding/protojson/encode_test.go
2 files changed, 250 insertions(+), 3 deletions(-)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger, Michael Stapelberg.
// ╔═══════╤════════════════════════════╗
// ║ JSON │ Protobuf field ║
// ╠═══════╪════════════════════════════╣
// ║ false │ proto3 boolean fields ║
// ║ 0 │ proto3 numeric fields ║
// ║ "" │ proto3 string/bytes fields ║
// ║ N/A │ proto2 scalar fields ║
// ║ N/A │ message fields ║
// ║ [] │ list fields ║
// ║ {} │ map fields ║
// ╚═══════╧════════════════════════════╝
> I’m not sure that N/A is particularly clear. […]
Should be good now
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Lasse Folger, Michael Stapelberg.
1 comment:
File encoding/protojson/encode.go:
// ╔═══════╤════════════════════════════╗
// ║ JSON │ Protobuf field ║
// ╠═══════╪════════════════════════════╣
// ║ false │ proto3 boolean fields ║
// ║ 0 │ proto3 numeric fields ║
// ║ "" │ proto3 string/bytes fields ║
// ║ N/A │ proto2 scalar fields ║
// ║ N/A │ message fields ║
// ║ [] │ list fields ║
// ║ {} │ map fields ║
// ╚═══════╧════════════════════════════╝
Should be good now
Done
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Cassondra Foesch, Joseph Tsai, Lasse Folger, Michael Stapelberg, Shmuel Eiderman.
2 comments:
File encoding/protojson/encode.go:
Patch Set #5, Line 84:
// EmitDefaultValues specifies whether to emit default primitive fields.I have to say, I find `EmitDefaultValues` very confusing. You say "default", I think:
```
optional int32 port = 1 [default = 80];
```I think I'd call this `EmitZeroValues`. For a non-optional field with the zero value, it emits the zero value (0, "", false). For an unpopulated optional field, it emits nothing, because the field is unpopulated not zero. List/map fields are a bit less principled, but it's not unreasonable to say the zero value of a list is `[]`.
I'll defer to Michael on naming, however.
Patch Set #5, Line 88:
// ╔═══════╤════════════════════════════╗
There should be a sentence explaining what this table is. In `EmitUnpopulated` above, this is "The JSON value emitted for unpopulated fields are as follows:".
Here, I might say something like:
```
// EmitDefaultValues specifies whether to emit zero-valued primitive fields,
// empty lists, and empty maps. The fields affected are as follows:
// ╔═══════╤════════════════════════════════════════╗
// ║ JSON │ Protobuf field ║
// ╠═══════╪════════════════════════════════════════╣
// ║ false │ non-optional scalar boolean fields ║
// ║ 0 │ non-optional scalar numeric fields ║
// ║ "" │ non-optional scalar string/byte fields ║
// ║ [] │ empty repeated fields ║
// ║ {} │ empty map fields ║
// ╚═══════╧════════════════════════════════════════╝
```
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
Patch set 6:Code-Review +2
2 comments:
Patchset:
I'm unavailable for the next days and not able to to provide more input but I have trust in other reviewers to find an appropriate name.
From my point of view, the behavior is fine and I don't have a strong opinion on the best name (although I agree that all suggested names have some limitations).
File encoding/protojson/encode.go:
Patch Set #5, Line 84:
// EmitDefaultValues specifies whether to emit default primitive fields.
I have to say, I find `EmitDefaultValues` very confusing. You say "default", I think: […]
This is a good point.
Correct me if I'm wrong but there is no concept of ZeroValues in proto, is there?
I see a problems that most of the API is defined via proto terms.
Using EmitZeroValues seems like we are using a Go term to explain the behavior which seems inconsistent with other terms in the API.Is there a better term from the proto vocabulary that could be used here?
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Cassondra Foesch, Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
1 comment:
File encoding/protojson/encode.go:
Patch Set #5, Line 84:
// EmitDefaultValues specifies whether to emit default primitive fields.
This is a good point. […]
Apparently, in proto terms, it's the "default value":
https://protobuf.dev/programming-guides/proto3/#default:(
So I guess `EmitDefaultValues` is correct, confusing though it may be.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
Shmuel Eiderman uploaded patch set #7 to this change.
encoding: Add EmitDefaultValues option
Introduce the EmitDefaultValues in addition to the existing
EmitUnpopulated option.
EmitDefaultValues is added to emit json messages more compatible with
the `always_print_primitive_fields` option of the cpp protobuf library.
EmitUnpopulated overrides EmitDefaultValues since the former generates
a strict superset of the latter.
See descussion:
https://github.com/golang/protobuf/issues/1536
Change-Id: Ib29b69d630fa3e8d8fdeb0de43b5683f30152151
---
M encoding/protojson/encode.go
M encoding/protojson/encode_test.go
2 files changed, 248 insertions(+), 3 deletions(-)
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Cassondra Foesch, Damien Neil, Joseph Tsai, Michael Stapelberg.
1 comment:
File encoding/protojson/encode.go:
Patch Set #5, Line 88:
// ╔═══════╤════════════════════════════╗There should be a sentence explaining what this table is. […]
How about now?
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Cassondra Foesch, Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
Patch Set #5, Line 88:
// ╔═══════╤════════════════════════════╗
How about now?
LGTM
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Cassondra Foesch (Gerrit)
Attention is currently required from: Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
Patchset:
LGTM
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Attention is currently required from: Joseph Tsai, Michael Stapelberg.
1 comment:
Patchset:
Thanks all for reviewing.
Just calling out the fact that I can not merge this due to permissions.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
王轩 (Gerrit)
Attention is currently required from: Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
王轩 removed a vote from this change.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
王轩 (Gerrit)
Attention is currently required from: Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
Patch set 7:Code-Review +1
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Shmuel Eiderman (Gerrit)
Patchset:
Gentle ping on merging this
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Joseph Tsai, Michael Stapelberg, Shmuel Eiderman.
Gentle ping on merging this
I thought this had already happened. Sorry for the delay.
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Lasse Folger submitted this change.
encoding: Add EmitDefaultValues option
Introduce the EmitDefaultValues in addition to the existing
EmitUnpopulated option.
EmitDefaultValues is added to emit json messages more compatible with
the `always_print_primitive_fields` option of the cpp protobuf library.
EmitUnpopulated overrides EmitDefaultValues since the former generates
a strict superset of the latter.
See descussion:
https://github.com/golang/protobuf/issues/1536
Change-Id: Ib29b69d630fa3e8d8fdeb0de43b5683f30152151
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/521215
Reviewed-by: Damien Neil <dn...@google.com>
Reviewed-by: Cassondra Foesch <cfo...@gmail.com>
Reviewed-by: Lasse Folger <lasse...@google.com>
---
M encoding/protojson/encode.go
M encoding/protojson/encode_test.go
2 files changed, 248 insertions(+), 3 deletions(-)
diff --git a/encoding/protojson/encode.go b/encoding/protojson/encode.go
index 97f1f7f..3f75098 100644
--- a/encoding/protojson/encode.go
+++ b/encoding/protojson/encode.go
@@ -81,6 +81,25 @@
// ╚═══════╧════════════════════════════╝
EmitUnpopulated bool
+ // EmitDefaultValues specifies whether to emit default-valued primitive fields,
+ // empty lists, and empty maps. The fields affected are as follows:
+ // ╔═══════╤════════════════════════════════════════╗
+ // ║ JSON │ Protobuf field ║
+ // ╠═══════╪════════════════════════════════════════╣
+ // ║ false │ non-optional scalar boolean fields ║
+ // ║ 0 │ non-optional scalar numeric fields ║
+ // ║ "" │ non-optional scalar string/byte fields ║
+ // ║ [] │ empty repeated fields ║
+ // ║ {} │ empty map fields ║
+ // ╚═══════╧════════════════════════════════════════╝
+ //
+ // Behaves similarly to EmitUnpopulated, but does not emit "null"-value fields,
+ // i.e. presence-sensing fields that are omitted will remain omitted to preserve
+ // presence-sensing.
+ // EmitUnpopulated takes precedence over EmitDefaultValues since the former generates
+ // a strict superset of the latter.
+ EmitDefaultValues bool
+
// Resolver is used for looking up types when expanding google.protobuf.Any
// messages. If nil, this defaults to using protoregistry.GlobalTypes.
Resolver interface {
@@ -178,7 +197,11 @@
// unpopulatedFieldRanger wraps a protoreflect.Message and modifies its Range
// method to additionally iterate over unpopulated fields.
-type unpopulatedFieldRanger struct{ protoreflect.Message }
+type unpopulatedFieldRanger struct {
+ protoreflect.Message
+
+ skipNull bool
+}
func (m unpopulatedFieldRanger) Range(f func(protoreflect.FieldDescriptor, protoreflect.Value) bool) {
fds := m.Descriptor().Fields()
@@ -192,6 +215,9 @@
isProto2Scalar := fd.Syntax() == protoreflect.Proto2 && fd.Default().IsValid()
isSingularMessage := fd.Cardinality() != protoreflect.Repeated && fd.Message() != nil
if isProto2Scalar || isSingularMessage {
+ if m.skipNull {
+ continue
+ }
v = protoreflect.Value{} // use invalid value to emit null
}
if !f(fd, v) {
@@ -217,8 +243,11 @@
defer e.EndObject()
var fields order.FieldRanger = m
- if e.opts.EmitUnpopulated {
- fields = unpopulatedFieldRanger{m}
+ switch {
+ case e.opts.EmitUnpopulated:
+ fields = unpopulatedFieldRanger{Message: m, skipNull: false}
+ case e.opts.EmitDefaultValues:
+ fields = unpopulatedFieldRanger{Message: m, skipNull: true}
}
if typeURL != "" {
fields = typeURLFieldRanger{fields, typeURL}
diff --git a/encoding/protojson/encode_test.go b/encoding/protojson/encode_test.go
index adda076..63ddb78 100644
--- a/encoding/protojson/encode_test.go
+++ b/encoding/protojson/encode_test.go
@@ -2194,6 +2194,222 @@
"optString": null
}`,
}, {
+ desc: "EmitUnpopulated overrides EmitDefaultValues",
+ mo: protojson.MarshalOptions{EmitUnpopulated: true, EmitDefaultValues: true},
+ input: &pb2.Nests{
+ RptNested: []*pb2.Nested{nil, {}},
+ },
+ want: `{
+ "optNested": null,
+ "optgroup": null,
+ "rptNested": [
+ {
+ "optString": null,
+ "optNested": null
+ },
+ {
+ "optString": null,
+ "optNested": null
+ }
+ ],
+ "rptgroup": []
+}`,
+ }, {
+ desc: "EmitDefaultValues: proto2 optional scalars",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb2.Scalars{},
+ want: `{}`,
+ }, {
+ desc: "EmitDefaultValues: proto3 scalars",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb3.Scalars{},
+ want: `{
+ "sBool": false,
+ "sInt32": 0,
+ "sInt64": "0",
+ "sUint32": 0,
+ "sUint64": "0",
+ "sSint32": 0,
+ "sSint64": "0",
+ "sFixed32": 0,
+ "sFixed64": "0",
+ "sSfixed32": 0,
+ "sSfixed64": "0",
+ "sFloat": 0,
+ "sDouble": 0,
+ "sBytes": "",
+ "sString": ""
+}`,
+ }, {
+ desc: "EmitDefaultValues: proto2 enum",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb2.Enums{},
+ want: `{
+ "rptEnum": [],
+ "rptNestedEnum": []
+}`,
+ }, {
+ desc: "EmitDefaultValues: proto3 enum",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb3.Enums{},
+ want: `{
+ "sEnum": "ZERO",
+ "sNestedEnum": "CERO"
+}`,
+ }, {
+ desc: "EmitDefaultValues: proto2 message and group fields",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb2.Nests{},
+ want: `{
+ "rptNested": [],
+ "rptgroup": []
+}`,
+ }, {
+ desc: "EmitDefaultValues: proto3 message field",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb3.Nests{},
+ want: `{}`,
+ }, {
+ desc: "EmitDefaultValues: proto2 empty message and group fields",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb2.Nests{
+ OptNested: &pb2.Nested{},
+ Optgroup: &pb2.Nests_OptGroup{},
+ },
+ want: `{
+ "optNested": {},
+ "optgroup": {},
+ "rptNested": [],
+ "rptgroup": []
+}`,
+ }, {
+ desc: "EmitDefaultValues: proto3 empty message field",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb3.Nests{
+ SNested: &pb3.Nested{},
+ },
+ want: `{
+ "sNested": {
+ "sString": ""
+ }
+}`,
+ }, {
+ desc: "EmitDefaultValues: proto2 required fields",
+ mo: protojson.MarshalOptions{
+ AllowPartial: true,
+ EmitDefaultValues: true,
+ },
+ input: &pb2.Requireds{},
+ want: `{}`,
+ }, {
+ desc: "EmitDefaultValues: repeated fields",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb2.Repeats{},
+ want: `{
+ "rptBool": [],
+ "rptInt32": [],
+ "rptInt64": [],
+ "rptUint32": [],
+ "rptUint64": [],
+ "rptFloat": [],
+ "rptDouble": [],
+ "rptString": [],
+ "rptBytes": []
+}`,
+ }, {
+ desc: "EmitDefaultValues: repeated containing empty message",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb2.Nests{
+ RptNested: []*pb2.Nested{nil, {}},
+ },
+ want: `{
+ "rptNested": [
+ {},
+ {}
+ ],
+ "rptgroup": []
+}`,
+ }, {
+ desc: "EmitDefaultValues: map fields",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb3.Maps{},
+ want: `{
+ "int32ToStr": {},
+ "boolToUint32": {},
+ "uint64ToEnum": {},
+ "strToNested": {},
+ "strToOneofs": {}
+}`,
+ }, {
+ desc: "EmitDefaultValues: map containing empty message",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb3.Maps{
+ StrToNested: map[string]*pb3.Nested{
+ "nested": &pb3.Nested{},
+ },
+ StrToOneofs: map[string]*pb3.Oneofs{
+ "nested": &pb3.Oneofs{},
+ },
+ },
+ want: `{
+ "int32ToStr": {},
+ "boolToUint32": {},
+ "uint64ToEnum": {},
+ "strToNested": {
+ "nested": {
+ "sString": ""
+ }
+ },
+ "strToOneofs": {
+ "nested": {}
+ }
+}`,
+ }, {
+ desc: "EmitDefaultValues: oneof fields",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb3.Oneofs{},
+ want: `{}`,
+ }, {
+ desc: "EmitDefaultValues: extensions",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: func() proto.Message {
+ m := &pb2.Extensions{}
+ proto.SetExtension(m, pb2.E_OptExtNested, &pb2.Nested{})
+ proto.SetExtension(m, pb2.E_RptExtNested, []*pb2.Nested{
+ nil,
+ {},
+ })
+ return m
+ }(),
+ want: `{
+ "[pb2.opt_ext_nested]": {},
+ "[pb2.rpt_ext_nested]": [
+ {},
+ {}
+ ]
+}`,
+ }, {
+ desc: "EmitDefaultValues: with populated fields",
+ mo: protojson.MarshalOptions{EmitDefaultValues: true},
+ input: &pb2.Scalars{
+ OptInt32: proto.Int32(0xff),
+ OptUint32: proto.Uint32(47),
+ OptSint32: proto.Int32(-1001),
+ OptFixed32: proto.Uint32(32),
+ OptSfixed32: proto.Int32(-32),
+ OptFloat: proto.Float32(1.02),
+ OptBytes: []byte("谷歌"),
+ },
+ want: `{
+ "optInt32": 255,
+ "optUint32": 47,
+ "optSint32": -1001,
+ "optFixed32": 32,
+ "optSfixed32": -32,
+ "optFloat": 1.02,
+ "optBytes": "6LC35q2M"
+}`,
+ }, {
desc: "UseEnumNumbers in singular field",
mo: protojson.MarshalOptions{UseEnumNumbers: true},
input: &pb2.Enums{
To view, visit change 521215. To unsubscribe, or for help writing mail filters, visit settings.