From 2260f47bfee361836bfb50ae03af394c05f0ed33 Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Wed, 29 May 2024 11:05:58 -0400 Subject: [PATCH] chore(internal/protoveneer): support oneof fields (#10271) Proto oneofs translate into Go as an unexported type. Here is one example: type CachedContent struct { // Types that are assignable to Expiration: // *CachedContent_ExpireTime // *CachedContent_Ttl Expiration isCachedContent_Expiration `protobuf_oneof:"expiration"` ... } As the comment says, the `Expiration` field can be populated by one of two exported types, but the type of the field itself is not exported (it is an interface type that the two exported types satisfy). Oneof fields like this cause a problem for this code generator, which writes conversion code between veneers and protos that looks like return &pb.CachedContent{ Expiration: conversionFunction(...), ... } We cannot write conversion function because we cannot write its return type. This CL addresses the problem by adding two features. First, the user can configure population functions that are passed the two structs, proto and veneer. That makes it possible to set oneof fields: func popcc(p *pb.CachedContent, v *CachedContent) { if [something about v] { p.Expiration = &pb.CachedContent_ExpireTime{..} } else { p.Expiration = &pb.CachedkContent_Ttl{...} } } The second feature is the `noConvert` option, which prevents the field from being set on the struct literal. So by setting `noConvert` and specifying populate functions, A oneof field can be initialized from a corresponding veneer type. --- .../protoveneer/cmd/protoveneer/config.go | 5 ++ .../cmd/protoveneer/protoveneer.go | 59 +++++++++++++++---- .../protoveneer/testdata/basic/basic.pb.go | 10 ++++ .../protoveneer/testdata/basic/config.yaml | 6 ++ .../cmd/protoveneer/testdata/basic/golden | 30 ++++++++++ 5 files changed, 100 insertions(+), 10 deletions(-) diff --git a/internal/protoveneer/cmd/protoveneer/config.go b/internal/protoveneer/cmd/protoveneer/config.go index 08322ea3ae36..bfb866ce2810 100644 --- a/internal/protoveneer/cmd/protoveneer/config.go +++ b/internal/protoveneer/cmd/protoveneer/config.go @@ -54,6 +54,8 @@ type typeConfig struct { Fields map[string]fieldConfig // Custom conversion functions: "tofunc, fromfunc" ConvertToFrom string `yaml:"convertToFrom"` + // Custom population functions, that are called after field-by-field conversion: "tofunc, fromfunc" + PopulateToFrom string `yaml:"populateToFrom"` // Doc string for the type, omitting the initial type name. // This replaces the first line of the doc. Doc string @@ -69,6 +71,9 @@ type fieldConfig struct { Type string // veneer type // Omit from output. Omit bool + // Generate the type, but not conversions. + // The populate functions (see [typeConfg.PopulateToFrom]) should set the field. + NoConvert bool `yaml:"noConvert"` // Custom conversion functions: "tofunc, fromfunc" ConvertToFrom string `yaml:"convertToFrom"` } diff --git a/internal/protoveneer/cmd/protoveneer/protoveneer.go b/internal/protoveneer/cmd/protoveneer/protoveneer.go index 93b55ff519a7..79492fa8c3af 100644 --- a/internal/protoveneer/cmd/protoveneer/protoveneer.go +++ b/internal/protoveneer/cmd/protoveneer/protoveneer.go @@ -114,7 +114,7 @@ func run(ctx context.Context, configFile, pbDir, outDir string) error { if !*noFormat { src, err = format.Source(src) if err != nil { - return err + return fmt.Errorf("formatting: %v", err) } } @@ -284,15 +284,19 @@ func buildConverterMap(typeInfos []*typeInfo, conf *config) (map[string]converte } func parseCustomConverter(name, value string) (converter, error) { - toFunc, fromFunc, ok := strings.Cut(value, ",") - toFunc = strings.TrimSpace(toFunc) - fromFunc = strings.TrimSpace(fromFunc) - if !ok || toFunc == "" || fromFunc == "" { + toFunc, fromFunc := parseCommaPair(value) + if toFunc == "" || fromFunc == "" { return nil, fmt.Errorf(`%s: ConvertToFrom = %q, want "toFunc, fromFunc"`, name, value) } return customConverter{toFunc, fromFunc}, nil } +// parseCommaPair parses a string like "foo, bar" into "foo" and "bar". +func parseCommaPair(s string) (string, string) { + a, b, _ := strings.Cut(s, ",") + return strings.TrimSpace(a), strings.TrimSpace(b) +} + // makeConverter constructs a converter for the given type. Not every type is in the map: this // function puts together converters for types like pointers, slices and maps, as well as // named types. @@ -340,9 +344,11 @@ type typeInfo struct { values *ast.GenDecl // the list of values for an enum // These fields are added later. - veneerName string // may be provided by config; else same as protoName - fields []*fieldInfo // for structs - valueNames []string // to generate String functions + veneerName string // may be provided by config; else same as protoName + fields []*fieldInfo // for structs + valueNames []string // to generate String functions + populateFrom string // name of function doing additional work converting from proto + populateTo string // name of function doing additional work converting to proto } // A fieldInfo holds information about a struct field. @@ -351,6 +357,7 @@ type fieldInfo struct { af *ast.Field protoName, veneerName string converter converter + noConvert bool } // collectDecls collects declaration information from a package. @@ -445,6 +452,15 @@ func processType(ti *typeInfo, tconf *typeConfig, typeInfos map[string]*typeInfo ti.fields = append(ti.fields, fi) } } + // Other processing. + if tconf != nil && tconf.PopulateToFrom != "" { + toFunc, fromFunc := parseCommaPair(tconf.PopulateToFrom) + if toFunc == "" || fromFunc == "" { + return fmt.Errorf(`%s: PopulateToFrom = %q, want "toFunc, fromFunc"`, ti.protoName, tconf.PopulateToFrom) + } + ti.populateTo = toFunc + ti.populateFrom = fromFunc + } case *ast.Ident: // Enum type. Nothing else to do with the type itself; but see processEnumValues. default: @@ -492,6 +508,7 @@ func processField(af *ast.Field, tc *typeConfig, typeInfos map[string]*typeInfo) } fi.converter = c } + fi.noConvert = fc.NoConvert } } af.Type = veneerType(af.Type, typeInfos) @@ -756,22 +773,44 @@ func (ti *typeInfo) generateConversionMethods(pr func(string, ...any)) { func (ti *typeInfo) generateToProto(pr func(string, ...any)) { pr("func (v *%s) toProto() *pb.%s {\n", ti.veneerName, ti.protoName) pr(" if v == nil { return nil }\n") - pr(" return &pb.%s{\n", ti.protoName) + if ti.populateTo == "" { + pr(" return &pb.%s{\n", ti.protoName) + } else { + pr(" p := &pb.%s{\n", ti.protoName) + } for _, f := range ti.fields { + if f.noConvert { + continue + } pr(" %s: %s,\n", f.protoName, f.converter.genTo("v."+f.veneerName)) } pr(" }\n") + if ti.populateTo != "" { + pr(" %s(p, v)\n", ti.populateTo) + pr(" return p\n") + } pr("}\n") } func (ti *typeInfo) generateFromProto(pr func(string, ...any)) { pr("func (%s) fromProto(p *pb.%s) *%[1]s {\n", ti.veneerName, ti.protoName) pr(" if p == nil { return nil }\n") - pr(" return &%s{\n", ti.veneerName) + if ti.populateFrom == "" { + pr(" return &%s{\n", ti.veneerName) + } else { + pr(" v := &%s{\n", ti.veneerName) + } for _, f := range ti.fields { + if f.noConvert { + continue + } pr(" %s: %s,\n", f.veneerName, f.converter.genFrom("p."+f.protoName)) } pr(" }\n") + if ti.populateFrom != "" { + pr(" %s(v, p)\n", ti.populateFrom) + pr(" return v\n") + } pr("}\n") } diff --git a/internal/protoveneer/cmd/protoveneer/testdata/basic/basic.pb.go b/internal/protoveneer/cmd/protoveneer/testdata/basic/basic.pb.go index 2adb4ada76b7..0fff48a31ed0 100755 --- a/internal/protoveneer/cmd/protoveneer/testdata/basic/basic.pb.go +++ b/internal/protoveneer/cmd/protoveneer/testdata/basic/basic.pb.go @@ -123,3 +123,13 @@ type Citation struct { Struct *structpb.Struct CreateTime *timestamppb.Timestamp } + +type unexported interface{ u() } + +// This demonstrates using population functions to deal with +// proto oneof field, which has an unexported type. +// That can be a way to deal with proto oneofs. +type Pop struct { + X int + Y unexported +} diff --git a/internal/protoveneer/cmd/protoveneer/testdata/basic/config.yaml b/internal/protoveneer/cmd/protoveneer/testdata/basic/config.yaml index 1946480b065f..e9983990ffa4 100644 --- a/internal/protoveneer/cmd/protoveneer/testdata/basic/config.yaml +++ b/internal/protoveneer/cmd/protoveneer/testdata/basic/config.yaml @@ -46,3 +46,9 @@ types: fields: Uri: name: URI + + Pop: + fields: + Y: + noConvert: true + populateToFrom: popYTo, popYFrom diff --git a/internal/protoveneer/cmd/protoveneer/testdata/basic/golden b/internal/protoveneer/cmd/protoveneer/testdata/basic/golden index 2a997288d1e8..db7ec2e8a392 100644 --- a/internal/protoveneer/cmd/protoveneer/testdata/basic/golden +++ b/internal/protoveneer/cmd/protoveneer/testdata/basic/golden @@ -215,3 +215,33 @@ func (v HarmCategory) String() string { } return fmt.Sprintf("HarmCategory(%d)", v) } + +// Pop is this demonstrates using population functions to deal with +// proto oneof field, which has an unexported type. +// That can be a way to deal with proto oneofs. +type Pop struct { + X int + Y unexported +} + +func (v *Pop) toProto() *pb.Pop { + if v == nil { + return nil + } + p := &pb.Pop{ + X: v.X, + } + popYTo(p, v) + return p +} + +func (Pop) fromProto(p *pb.Pop) *Pop { + if p == nil { + return nil + } + v := &Pop{ + X: p.X, + } + popYFrom(v, p) + return v +}