Skip to content

Commit

Permalink
Remove inline typeconvert when logging some thrift typedefs
Browse files Browse the repository at this point in the history
#366 added fast zap logging
by generating extra `MarshalLog...` methods on generated types. For
typedefs in thrift, other types that log them will cast to the
underlying type in their `MarshalLog...` method, causing the generated
code to need to import multiple go packages generated from both direct
and indirect thrift dependencies.

This should not be necessary, as typedefs of complex types (anything
that cannot be mapped directly to a go stdlb type) get their own
`MarshalLog...` methods, which cast to underlying type when necessary.

This PR changes the logging for typedef'ed fields to:
- cast to the underlying type inline when the underlying thrift type
directly maps to a go type.
- defer to the typedef's generated `MarshalLog...` method otherwise.

This results in having more predictable importpaths (eg knowing that one
thrift import will lead to one go import in generated code) makes
implementing good bazel rules for thriftrw easier.

See internal issue GO-362 for a more elaborate example of how the
current behaviour complicates our Bazel setup.
  • Loading branch information
robbertvanginkel committed Feb 16, 2021
1 parent 2e8a093 commit 5115fe0
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 13 deletions.
2 changes: 2 additions & 0 deletions gen/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ func (g *generator) TextTemplate(s string, data interface{}, opts ...TemplateOpt
"isNotNil": func(val interface{}) bool {
return val != nil
},
"zapTypedefGenerateMarshaler": curryGenerator(g.z.zapTypedefGenerateMarshaler, g),
"zapTypedefHasGeneratedMarshaler": curryGenerator(g.z.zapTypedefHasGeneratedMarshaler, g),
}

tmpl := template.New("thriftrw").Delims("<", ">").Funcs(templateFuncs)
Expand Down
14 changes: 6 additions & 8 deletions gen/typedef.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,20 +114,18 @@ func typedef(g Generator, spec *compile.TypedefSpec) error {
</* We want the behavior of the underlying type for typedefs: in the case that
they are objects or arrays, we need to cast to the underlying object or array;
else, zapMarshaler in zap.go will take care of it. */>
<if (eq (zapEncoder .Target) "Object") ->
<if (zapTypedefHasGeneratedMarshaler .Target) ->
<$zapcore := import "go.uber.org/zap/zapcore">
<$enc := newVar "enc">
<if (eq (zapEncoder .Target) "Object") ->
func (<$v> <$typedefType>) MarshalLogObject(<$enc> <$zapcore>.ObjectEncoder) error {
return (<zapMarshaler . $v>).MarshalLogObject(<$enc>)
return (<zapTypedefGenerateMarshaler . $v>).MarshalLogObject(<$enc>)
}
<- end>
<if (eq (zapEncoder .Target) "Array") ->
<$zapcore := import "go.uber.org/zap/zapcore">
<$enc := newVar "enc">
<- else if (eq (zapEncoder .Target) "Array") ->
func (<$v> <$typedefType>) MarshalLogArray(<$enc> <$zapcore>.ArrayEncoder) error {
return (<zapMarshaler . $v>).MarshalLogArray(<$enc>)
return (<zapTypedefGenerateMarshaler . $v>).MarshalLogArray(<$enc>)
}
<- end>
<- end>
<- end>
`,
Expand Down
36 changes: 31 additions & 5 deletions gen/zap.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,28 @@ func (z *zapGenerator) zapEncoder(g Generator, spec compile.TypeSpec) string {
panic(root)
}

// zapTypedefHasGeneratedMarshaler defines if a typedef will have generated code
// zapcore.ObjectMarshaler or zapcore.ArrayMarshaler.
func (z *zapGenerator) zapTypedefHasGeneratedMarshaler(g Generator, spec compile.TypeSpec) bool {
switch z.zapEncoder(g, spec) {
case "Array", "Object":
return true
default:
return false
}
}

// zapTypedefGenerateMarshaler is similar to zapMarshaler but always converts a typedef
// to the next value. This should be used when generating the code for the Zap
// marshal implementation of the typedef.
func (z *zapGenerator) zapTypedefGenerateMarshaler(g Generator, spec *compile.TypedefSpec, fieldValue string) (string, error) {
rootName, err := typeReference(g, spec.Target)
if err != nil {
return "", err
}
return z.zapMarshalerGenerator(g, spec, fmt.Sprintf("(%v)(%v)", rootName, fieldValue))
}

// zapMarshaler takes a TypeSpec, evaluates whether there are underlying elements
// that require more Zap implementation to log everything, and returns a string
// that properly casts the fieldValue, if needed, for logging.
Expand All @@ -96,21 +118,25 @@ func (z *zapGenerator) zapEncoder(g Generator, spec compile.TypeSpec) string {
// enc.Add<zapEncoder .Type>("foo", <zapMarshaler .Type "v">)
//
func (z *zapGenerator) zapMarshaler(g Generator, spec compile.TypeSpec, fieldValue string) (string, error) {
root := compile.RootTypeSpec(spec)

if _, ok := spec.(*compile.TypedefSpec); ok {
// For typedefs, cast to the root type and rely on that functionality.
rootName, err := typeReference(g, root)
// For typedefs, cast to the root type and rely on that functionality if the
// typedef doesn't have generated Zap marshal methods.
if _, ok := spec.(*compile.TypedefSpec); ok && !z.zapTypedefHasGeneratedMarshaler(g, spec) {
rootName, err := typeReference(g, compile.RootTypeSpec(spec))
if err != nil {
return "", err
}
fieldValue = fmt.Sprintf("(%v)(%v)", rootName, fieldValue)
}

return z.zapMarshalerGenerator(g, spec, fieldValue)
}

func (z *zapGenerator) zapMarshalerGenerator(g Generator, spec compile.TypeSpec, fieldValue string) (string, error) {
if isPrimitiveType(spec) {
return fieldValue, nil
}

root := compile.RootTypeSpec(spec)
switch t := root.(type) {
case *compile.BinarySpec:
// There is no AppendBinary for ArrayEncoder, so we opt for encoding it ourselves and
Expand Down

0 comments on commit 5115fe0

Please # to comment.