From 29f215756383cf58cd1de326dd1c2961b06ea201 Mon Sep 17 00:00:00 2001 From: Evgenii Stratonikov Date: Fri, 11 Oct 2024 10:17:42 +0300 Subject: [PATCH] [#123] protogen: Treat bytes field as non-nullable In protobuf 3.12 they have added an support for `optional` keyword, which has made it into the main branch in 3.15. https://github.com/protocolbuffers/protobuf/blob/main/docs/implementing_proto3_presence.md https://github.com/protocolbuffers/protobuf/blob/v3.12.0/docs/field_presence.md#presence-in-proto3-apis This means that without an explicit `optional` keyword field presence for scalars is not tracked, thus empty string in JSON should be unmarshaled to a nil byte slice. Relevant decoding code and tests from protojson: https://github.com/protocolbuffers/protobuf-go/blob/fb995f184a1719ec42b247a3771d1036d92adf67/internal/impl/message_reflect_field.go#L327 https://github.com/protocolbuffers/protobuf-go/blob/fb995f184a1719ec42b247a3771d1036d92adf67/encoding/protojson/decode_test.go#L134 https://github.com/protocolbuffers/protobuf-go/blob/fb995f184a1719ec42b247a3771d1036d92adf67/encoding/protojson/decode_test.go#L156 We do not support `optional` keyword and the generator will fail if it sees on. So only implement the default behaviour. Refs #122 Signed-off-by: Evgenii Stratonikov --- acl/grpc/types_frostfs.pb.go | 8 ++- ape/grpc/types_frostfs.pb.go | 8 ++- apemanager/grpc/service_frostfs.pb.go | 16 +++++- container/grpc/types_frostfs.pb.go | 8 ++- netmap/grpc/types_frostfs.pb.go | 24 +++++++-- object/grpc/service_frostfs.pb.go | 48 ++++++++++++++--- object/grpc/types_frostfs.pb.go | 40 ++++++++++++-- refs/grpc/types_frostfs.pb.go | 64 ++++++++++++++++++++--- session/grpc/service_frostfs.pb.go | 16 +++++- session/grpc/types_frostfs.pb.go | 16 +++++- status/grpc/types_frostfs.pb.go | 8 ++- tombstone/grpc/types_frostfs.pb.go | 8 ++- util/proto/marshal_test.go | 10 +--- util/proto/test/custom/test_frostfs.pb.go | 24 +++++++-- util/protogen/internalgengo/json.go | 12 ++++- 15 files changed, 264 insertions(+), 46 deletions(-) diff --git a/acl/grpc/types_frostfs.pb.go b/acl/grpc/types_frostfs.pb.go index e5a6f32..bc90cb7 100644 --- a/acl/grpc/types_frostfs.pb.go +++ b/acl/grpc/types_frostfs.pb.go @@ -691,7 +691,13 @@ func (x *EACLRecord_Target) UnmarshalEasyJSON(in *jlexer.Lexer) { var list [][]byte in.Delim('[') for !in.IsDelim(']') { - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } list = append(list, f) in.WantComma() } diff --git a/ape/grpc/types_frostfs.pb.go b/ape/grpc/types_frostfs.pb.go index cf9499f..1f2a1a5 100644 --- a/ape/grpc/types_frostfs.pb.go +++ b/ape/grpc/types_frostfs.pb.go @@ -401,7 +401,13 @@ func (x *Chain) UnmarshalEasyJSON(in *jlexer.Lexer) { x.Kind = xx { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } xx.Raw = f } } diff --git a/apemanager/grpc/service_frostfs.pb.go b/apemanager/grpc/service_frostfs.pb.go index 7f3ed10..b2633e1 100644 --- a/apemanager/grpc/service_frostfs.pb.go +++ b/apemanager/grpc/service_frostfs.pb.go @@ -558,7 +558,13 @@ func (x *AddChainResponse_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { case "chainId": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.ChainId = f } } @@ -974,7 +980,13 @@ func (x *RemoveChainRequest_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { case "chainId": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.ChainId = f } } diff --git a/container/grpc/types_frostfs.pb.go b/container/grpc/types_frostfs.pb.go index 4ca23fa..a4f0882 100644 --- a/container/grpc/types_frostfs.pb.go +++ b/container/grpc/types_frostfs.pb.go @@ -500,7 +500,13 @@ func (x *Container) UnmarshalEasyJSON(in *jlexer.Lexer) { case "nonce": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Nonce = f } case "basicACL": diff --git a/netmap/grpc/types_frostfs.pb.go b/netmap/grpc/types_frostfs.pb.go index 0328531..24003c6 100644 --- a/netmap/grpc/types_frostfs.pb.go +++ b/netmap/grpc/types_frostfs.pb.go @@ -1855,7 +1855,13 @@ func (x *NodeInfo) UnmarshalEasyJSON(in *jlexer.Lexer) { case "publicKey": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.PublicKey = f } case "addresses": @@ -2279,13 +2285,25 @@ func (x *NetworkConfig_Parameter) UnmarshalEasyJSON(in *jlexer.Lexer) { case "key": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Key = f } case "value": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Value = f } } diff --git a/object/grpc/service_frostfs.pb.go b/object/grpc/service_frostfs.pb.go index 9ee5a3d..a55a41b 100644 --- a/object/grpc/service_frostfs.pb.go +++ b/object/grpc/service_frostfs.pb.go @@ -924,7 +924,13 @@ func (x *GetResponse_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { x.ObjectPart = xx { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } xx.Chunk = f } case "splitInfo": @@ -1699,7 +1705,13 @@ func (x *PutRequest_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { x.ObjectPart = xx { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } xx.Chunk = f } } @@ -6254,7 +6266,13 @@ func (x *GetRangeResponse_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { x.RangePart = xx { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } xx.Chunk = f } case "splitInfo": @@ -6802,7 +6820,13 @@ func (x *GetRangeHashRequest_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { case "salt": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Salt = f } case "type": @@ -7267,7 +7291,13 @@ func (x *GetRangeHashResponse_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { var list [][]byte in.Delim('[') for !in.IsDelim(']') { - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } list = append(list, f) in.WantComma() } @@ -8455,7 +8485,13 @@ func (x *PatchRequest_Body_Patch) UnmarshalEasyJSON(in *jlexer.Lexer) { case "chunk": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Chunk = f } } diff --git a/object/grpc/types_frostfs.pb.go b/object/grpc/types_frostfs.pb.go index e4a916f..a6d4f01 100644 --- a/object/grpc/types_frostfs.pb.go +++ b/object/grpc/types_frostfs.pb.go @@ -1015,7 +1015,13 @@ func (x *Header_Split) UnmarshalEasyJSON(in *jlexer.Lexer) { case "splitID": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.SplitId = f } } @@ -1436,13 +1442,25 @@ func (x *Header_EC) UnmarshalEasyJSON(in *jlexer.Lexer) { case "header": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Header = f } case "parentSplitID": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.ParentSplitId = f } case "parentSplitParentID": @@ -2347,7 +2365,13 @@ func (x *Object) UnmarshalEasyJSON(in *jlexer.Lexer) { case "payload": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Payload = f } } @@ -2552,7 +2576,13 @@ func (x *SplitInfo) UnmarshalEasyJSON(in *jlexer.Lexer) { case "splitId": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.SplitId = f } case "lastPart": diff --git a/refs/grpc/types_frostfs.pb.go b/refs/grpc/types_frostfs.pb.go index 5a16f8e..f2a2663 100644 --- a/refs/grpc/types_frostfs.pb.go +++ b/refs/grpc/types_frostfs.pb.go @@ -390,7 +390,13 @@ func (x *ObjectID) UnmarshalEasyJSON(in *jlexer.Lexer) { case "value": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Value = f } } @@ -529,7 +535,13 @@ func (x *ContainerID) UnmarshalEasyJSON(in *jlexer.Lexer) { case "value": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Value = f } } @@ -668,7 +680,13 @@ func (x *OwnerID) UnmarshalEasyJSON(in *jlexer.Lexer) { case "value": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Value = f } } @@ -1063,13 +1081,25 @@ func (x *Signature) UnmarshalEasyJSON(in *jlexer.Lexer) { case "key": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Key = f } case "signature": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Sign = f } case "scheme": @@ -1264,13 +1294,25 @@ func (x *SignatureRFC6979) UnmarshalEasyJSON(in *jlexer.Lexer) { case "key": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Key = f } case "signature": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Sign = f } } @@ -1466,7 +1508,13 @@ func (x *Checksum) UnmarshalEasyJSON(in *jlexer.Lexer) { case "sum": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Sum = f } } diff --git a/session/grpc/service_frostfs.pb.go b/session/grpc/service_frostfs.pb.go index 6d3ea9f..26213b9 100644 --- a/session/grpc/service_frostfs.pb.go +++ b/session/grpc/service_frostfs.pb.go @@ -598,13 +598,25 @@ func (x *CreateResponse_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { case "id": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Id = f } case "sessionKey": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.SessionKey = f } } diff --git a/session/grpc/types_frostfs.pb.go b/session/grpc/types_frostfs.pb.go index e9f3b42..542a02b 100644 --- a/session/grpc/types_frostfs.pb.go +++ b/session/grpc/types_frostfs.pb.go @@ -1284,7 +1284,13 @@ func (x *SessionToken_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { case "id": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Id = f } case "ownerID": @@ -1304,7 +1310,13 @@ func (x *SessionToken_Body) UnmarshalEasyJSON(in *jlexer.Lexer) { case "sessionKey": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.SessionKey = f } case "object": diff --git a/status/grpc/types_frostfs.pb.go b/status/grpc/types_frostfs.pb.go index 1d8b477..6c62a0f 100644 --- a/status/grpc/types_frostfs.pb.go +++ b/status/grpc/types_frostfs.pb.go @@ -439,7 +439,13 @@ func (x *Status_Detail) UnmarshalEasyJSON(in *jlexer.Lexer) { case "value": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.Value = f } } diff --git a/tombstone/grpc/types_frostfs.pb.go b/tombstone/grpc/types_frostfs.pb.go index acb1836..e56832a 100644 --- a/tombstone/grpc/types_frostfs.pb.go +++ b/tombstone/grpc/types_frostfs.pb.go @@ -231,7 +231,13 @@ func (x *Tombstone) UnmarshalEasyJSON(in *jlexer.Lexer) { case "splitID": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.SplitId = f } case "members": diff --git a/util/proto/marshal_test.go b/util/proto/marshal_test.go index 5498cf9..29af711 100644 --- a/util/proto/marshal_test.go +++ b/util/proto/marshal_test.go @@ -50,9 +50,6 @@ func TestStableMarshalSingle(t *testing.T) { var actualFrostfs generated.Primitives require.NoError(t, actualFrostfs.UnmarshalJSON(r)) - if len(actualFrostfs.FieldA) == 0 { - actualFrostfs.FieldA = nil - } require.Equal(t, input, &actualFrostfs) primitivesEqual(t, input, &actual) @@ -110,9 +107,6 @@ func TestStableMarshalSingle(t *testing.T) { var actualFrostfs generated.Primitives require.NoError(t, actualFrostfs.UnmarshalJSON(r)) - if len(actualFrostfs.FieldA) == 0 { - actualFrostfs.FieldA = nil - } require.Equal(t, tc.input, &actualFrostfs) primitivesEqual(t, tc.input, &actual) @@ -124,9 +118,7 @@ func TestStableMarshalSingle(t *testing.T) { func primitivesEqual(t *testing.T, a *generated.Primitives, b *test.Primitives) { // Compare each field directly, because proto-generated code has private fields. require.Equal(t, len(a.FieldA), len(b.FieldA)) - if len(a.FieldA) != 0 { - require.Equal(t, a.FieldA, b.FieldA) - } + require.Equal(t, a.FieldA, b.FieldA) require.Equal(t, a.FieldB, b.FieldB) require.Equal(t, a.FieldC, b.FieldC) require.Equal(t, a.FieldD, b.FieldD) diff --git a/util/proto/test/custom/test_frostfs.pb.go b/util/proto/test/custom/test_frostfs.pb.go index f979038..5ef95a6 100644 --- a/util/proto/test/custom/test_frostfs.pb.go +++ b/util/proto/test/custom/test_frostfs.pb.go @@ -771,7 +771,13 @@ func (x *Primitives) UnmarshalEasyJSON(in *jlexer.Lexer) { case "fieldA": { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } x.FieldA = f } case "fieldB": @@ -903,7 +909,13 @@ func (x *Primitives) UnmarshalEasyJSON(in *jlexer.Lexer) { x.FieldM = xx { var f []byte - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } xx.FieldMa = f } case "fieldMe": @@ -1520,7 +1532,13 @@ func (x *RepPrimitives) UnmarshalEasyJSON(in *jlexer.Lexer) { var list [][]byte in.Delim('[') for !in.IsDelim(']') { - f = in.Bytes() + { + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + f = tmp + } list = append(list, f) in.WantComma() } diff --git a/util/protogen/internalgengo/json.go b/util/protogen/internalgengo/json.go index 15facfd..55c9cd3 100644 --- a/util/protogen/internalgengo/json.go +++ b/util/protogen/internalgengo/json.go @@ -129,7 +129,17 @@ func emitJSONFieldRead(g *protogen.GeneratedFile, f *protogen.Field, name string case protoreflect.StringKind: template = "%s = in.String()" case protoreflect.BytesKind: - template = "%s = in.Bytes()" + // Since some time ago proto3 support optional keyword, thus the presence is not tracked by default: + // https://github.com/protocolbuffers/protobuf-go/blob/fb995f184a1719ec42b247a3771d1036d92adf67/internal/impl/message_reflect_field.go#L327 + // We do not explicitly support `optional` keyword, protoc will fail on such fileds. + // Thus, treat empty string as `[]byte(nil)`. + template = `{ + tmp := in.Bytes() + if len(tmp) == 0 { + tmp = nil + } + %s = tmp + }` case protoreflect.MessageKind: if f.Desc.IsList() { g.P("f = ", fieldType(g, f)[2:], "{}")