From ce26bcf986209df3736ab693e30d9d9a972ddcf0 Mon Sep 17 00:00:00 2001 From: Chris Seto Date: Thu, 14 Mar 2024 15:50:51 -0400 Subject: [PATCH] Implement genpartial "partial" struct generator This commit implements the `genpartial` go runnable for generating "partial" variants of user defined structs. By default, all fields of a struct will be included in the json/yaml serialization. Developers may utilize the `omitempty` json tag to exclude fields when they are the zero value of their respective type. This however does not work with struct values unless the field is a pointer to a struct with a values of `nil`. This behavior can lead to various UX/DX issues, which you may have experienced if you've used the go amazon SDK. As we convert more of the redpanda chart into go, the exact shape of the `Values` struct will become more important to avoid constant double checking and second guessing. This struct should represent the union of the default values and the user provided values, assuming they match the JSON schema. This can be problematic when using the Values struct to define test values as they'll be fully serialized and overwrite many of the default values with go's zero values. To work around this, we'll leverage the "partial" structs that are generated by this new package. --- charts/redpanda/values_partial_test.go | 58 +++++ cmd/genpartial/main.go | 280 +++++++++++++++++++++++++ cmd/genpartial/main_test.go | 76 +++++++ cmd/genpartial/testdata/partial.go | 41 ++++ go.mod | 2 + go.sum | 2 + 6 files changed, 459 insertions(+) create mode 100644 charts/redpanda/values_partial_test.go create mode 100644 cmd/genpartial/main.go create mode 100644 cmd/genpartial/main_test.go create mode 100644 cmd/genpartial/testdata/partial.go diff --git a/charts/redpanda/values_partial_test.go b/charts/redpanda/values_partial_test.go new file mode 100644 index 0000000000..9e4f231019 --- /dev/null +++ b/charts/redpanda/values_partial_test.go @@ -0,0 +1,58 @@ +package redpanda_test + +import ( + "encoding/json" + "os" + "testing" + + "github.com/redpanda-data/helm-charts/charts/redpanda" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/yaml" +) + +// TestPartialValuesRoundTrip asserts that any .yaml file in ./ci/ can be round +// tripped through the redpanda.PartialValues structs (sans comments of +// course). +func TestPartialValuesRoundTrip(t *testing.T) { + values, err := os.ReadDir("./ci") + require.NoError(t, err) + + for _, v := range values { + v := v + t.Run(v.Name(), func(t *testing.T) { + yamlBytes, err := os.ReadFile("./ci/" + v.Name()) + require.NoError(t, err) + + var structuredValues *redpanda.PartialValues + var unstructuredValues map[string]any + require.NoError(t, yaml.Unmarshal(yamlBytes, &structuredValues)) + require.NoError(t, yaml.Unmarshal(yamlBytes, &unstructuredValues)) + + // Not yet typed field(s) + unstructured.RemoveNestedField(unstructuredValues, "console") + unstructured.RemoveNestedField(unstructuredValues, "storage", "persistentVolume", "nameOverwrite") + unstructured.RemoveNestedField(unstructuredValues, "resources", "memory", "redpanda") + + // listeners.kafka.external.*.tls slipped through the cracks. + kafkaExternal, ok, _ := unstructured.NestedMap(unstructuredValues, "listeners", "kafka", "external") + if ok { + for key := range kafkaExternal { + unstructured.RemoveNestedField(kafkaExternal, key, "tls") + } + unstructured.SetNestedMap(unstructuredValues, kafkaExternal, "listeners", "kafka", "external") + } + + // Potential bug in pre-existing test values. (listeners should be listener?) + unstructured.RemoveNestedField(unstructuredValues, "auditLogging", "listeners") + + structuredJSON, err := json.Marshal(structuredValues) + require.NoError(t, err) + + unstructuredJSON, err := json.Marshal(unstructuredValues) + require.NoError(t, err) + + require.JSONEq(t, string(unstructuredJSON), string(structuredJSON)) + }) + } +} diff --git a/cmd/genpartial/main.go b/cmd/genpartial/main.go new file mode 100644 index 0000000000..06cf0a1333 --- /dev/null +++ b/cmd/genpartial/main.go @@ -0,0 +1,280 @@ +// main executes the "genpartial" program which loads up a go package and +// recursively generates a "partial" variant of an specified struct. +// +// If you've ever worked with the AWS SDK, you've worked with "partial" structs +// before. They are any struct where every field is nullable and the json tag +// specifies "omitempty". +// +// genpartial allows us to write structs in ergonomic go where fields that must +// always exist are presented as values rather than pointers. In cases we were +// need to marshal a partial value back to json or only specify a subset of +// values (IE helm values), use the generated partial. +package main + +import ( + "bytes" + "flag" + "fmt" + "go/ast" + "go/format" + "go/token" + "go/types" + "io" + "os" + "regexp" + + "github.com/cockroachdb/errors" + "golang.org/x/tools/go/ast/astutil" + "golang.org/x/tools/go/packages" +) + +const ( + mode = packages.NeedTypes | packages.NeedName | packages.NeedSyntax | packages.NeedTypesInfo +) + +func main() { + cwd, _ := os.Getwd() + + outFlag := flag.String("out", "-", "The file to output to or `-` for stdout") + structFlag := flag.String("struct", "Values", "The struct name to generate a partial for") + + flag.Parse() + + if len(flag.Args()) != 1 { + fmt.Printf("Usage: genpartial \n") + fmt.Printf("Example: genpartial -struct Values ./charts/redpanda\n") + os.Exit(1) + } + + pkgs := Must(packages.Load(&packages.Config{ + Dir: cwd, + Mode: mode, + BuildFlags: []string{"-tags=generate"}, + }, flag.Arg(0))) + + var buf bytes.Buffer + if err := GeneratePartial(pkgs[0], *structFlag, &buf); err != nil { + panic(err) + } + + if *outFlag == "-" { + fmt.Println(buf.String()) + } else { + if err := os.WriteFile(*outFlag, buf.Bytes(), 0o644); err != nil { + panic(err) + } + } +} + +// PackageErrors returns any error reported by pkg during load or nil. +func PackageErrors(pkg *packages.Package) error { + for _, err := range pkg.Errors { + return err + } + + for _, err := range pkg.TypeErrors { + return err + } + + return nil +} + +func GeneratePartial(pkg *packages.Package, structName string, out io.Writer) error { + root := pkg.Types.Scope().Lookup(structName) + + if root == nil { + return errors.Newf("named struct not found in package %q: %q", pkg.Name, structName) + } + + if !IsType[*types.Named](root.Type()) || !IsType[*types.Struct](root.Type().Underlying()) { + return errors.Newf("named struct not found in package %q: %q", pkg.Name, structName) + } + + names := FindAllNames(root.Type()) + nameMap := map[string]*types.Named{} + + for _, name := range names { + nameMap[name.Obj().Name()] = name + } + + var partials []ast.Node + for _, f := range pkg.Syntax { + for _, decl := range f.Decls { + // nil decls indicate an empty line, skip over them. + if decl == nil { + continue + } + + // Skip over any non-type declaration. + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok != token.TYPE { + continue + } + + // We now know that genDecl contains a type declaration. Traverse + // the AST that comprises it and rewrite all fields to be nullable + // have an omitempty json tag. + partial := astutil.Apply(genDecl, func(c *astutil.Cursor) bool { + switch node := c.Node().(type) { + case *ast.Comment: + // Remove comments + c.Delete() + return false + + case *ast.TypeSpec: + // For any type spec, if it's in our list of named types, + // rename it to "Partial". + if _, ok := nameMap[node.Name.String()]; ok { + original := node.Name.Name + node.Name.Name = "Partial" + original + node.Name.Obj.Name = "Partial" + original + // TODO: Generate some nice comments. The trimming of + // original comments will remove generated comments as + // well. + // node.Doc = &ast.CommentGroup{ + // List: []*ast.Comment{ + // {Text: fmt.Sprintf(`// %s is a generated "Partial" variant of [%s]`, node.Name.Name, original)}, + // }, + // } + return true + } + // Or delete it if it's not in our list. + c.Delete() + return false + + case *ast.Ident: + // Rewrite all identifiers to be a nullable version. + switch parent := c.Parent().(type) { + case *ast.StarExpr, *ast.ArrayType, *ast.IndexExpr, *ast.MapType: + if _, ok := nameMap[node.Name]; ok { + node.Name = "Partial" + node.Name + } + return false + + case *ast.Field: + if parent.Type != node { + return true + } + + if _, ok := nameMap[node.Name]; ok { + node.Name = "Partial" + node.Name + c.Replace(&ast.StarExpr{X: node}) + return false + } + + // If Obj is nil, this is a builtin type like int, + // string, etc. We want these to become *int, *string. + // "any" however is already nullable, so skip that. + if node.Obj == nil && node.Name != "any" { + c.Replace(&ast.StarExpr{X: node}) + return false + } + return true + } + + return false + + case *ast.Field: + if node.Tag == nil { + node.Tag = &ast.BasicLit{Value: "``"} + } + node.Tag.Value = EnsureOmitEmpty(node.Tag.Value) + return true + + default: + return true + } + }, nil).(*ast.GenDecl) + + // If we've filtered out all the specs, skip over this declaration. + if len(partial.Specs) == 0 { + continue + } + + partials = append(partials, partial) + } + } + + // Printout the resultant set of structs to `out`. We could generate an + // ast.File and print that but it's a bit finicky. Printf and then + // formatting rewritten nodes is easier. + fmt.Fprintf(out, "// !DO NOT EDIT! Generated by genpartial\n") + fmt.Fprintf(out, "//\n") + fmt.Fprintf(out, "//go:build !generate\n") + fmt.Fprintf(out, "//+gotohelm:ignore=true\n") + fmt.Fprintf(out, "package %s\n\n", pkg.Name) + for i, d := range partials { + if i > 0 { + fmt.Fprintf(out, "\n\n") + } + format.Node(out, pkg.Fset, d) + } + fmt.Fprintf(out, "\n") + + return nil +} + +// FindAllNames traverses the given type and returns a slice of all named types +// that are referenced from the "root" type. +func FindAllNames(root types.Type) []*types.Named { + names := []*types.Named{} + + switch root := root.(type) { + case *types.Pointer: + names = append(names, FindAllNames(root.Elem())...) + + case *types.Slice: + names = append(names, FindAllNames(root.Elem())...) + + case *types.Named: + if _, ok := root.Underlying().(*types.Basic); ok { + break + } + + names = append(names, root) + names = append(names, FindAllNames(root.Underlying())...) + + for i := 0; i < root.TypeArgs().Len(); i++ { + arg := root.TypeArgs().At(i) + if named, ok := arg.(*types.Named); ok { + names = append(names, FindAllNames(named)...) + } + } + + case *types.Map: + names = append(names, FindAllNames(root.Key())...) + names = append(names, FindAllNames(root.Elem())...) + + case *types.Struct: + for i := 0; i < root.NumFields(); i++ { + field := root.Field(i) + // TODO how to handle Embeds? + names = append(names, FindAllNames(field.Type())...) + } + } + + return names +} + +var jsonTagRE = regexp.MustCompile(`json:"([^,"]+)"`) + +// EnsureOmitEmpty injects ,omitempty into existing json tags or adds one if +// not already present. +func EnsureOmitEmpty(tag string) string { + if !jsonTagRE.MatchString(tag) { + return tag[:len(tag)-1] + `json:",omitempty"` + "`" + } + return jsonTagRE.ReplaceAllString(tag, `json:"$1,omitempty"`) +} + +func Must[T any](value T, err error) T { + if err != nil { + panic(err) + } + return value +} + +func IsType[T types.Type](typ types.Type) bool { + _, ok := typ.(T) + return ok +} diff --git a/cmd/genpartial/main_test.go b/cmd/genpartial/main_test.go new file mode 100644 index 0000000000..9de195b483 --- /dev/null +++ b/cmd/genpartial/main_test.go @@ -0,0 +1,76 @@ +package main + +import ( + "bytes" + "testing" + + "github.com/redpanda-data/helm-charts/pkg/testutil" + "github.com/stretchr/testify/require" + "golang.org/x/tools/go/packages" +) + +type ( + IntAlias int + MapStructAlias map[string]int + MapGeneric[T any] map[string]T +) + +type ExampleStruct struct { + // Generics + A1 MapGeneric[int] + A2 MapGeneric[NestedStruct] + A3 MapGeneric[*NestedStruct] + A4 MapGeneric[IntAlias] + + // BasicTypes + B1 int + B2 *int + + // Inline structs + C1 struct { + Any any + Int int + } + C2 *struct{} + + // Structs + D1 NestedStruct + D2 *NestedStruct + + // Slices + E1 []any + E2 []int + E3 []*int + + // Tags + F1 []*int `json:"L"` + F2 string `yaml:"M"` + F3 IntAlias +} + +type NestedStruct struct { + Map map[string]string +} + +func TestGenerateParital(t *testing.T) { + pkgs, err := packages.Load(&packages.Config{ + Mode: mode, + BuildFlags: []string{"-tags=generate"}, + Tests: true, + }, ".") + require.NoError(t, err) + + // Loading with tests is weird but it let's us load up the example struct + // seen above. + require.Len(t, pkgs, 3) + pkg := pkgs[1] + require.Equal(t, "main", pkg.Name) + + require.NoError(t, PackageErrors(pkg)) + + require.EqualError(t, GeneratePartial(pkg, "Values", nil), `named struct not found in package "main": "Values"`) + + var buf bytes.Buffer + require.NoError(t, GeneratePartial(pkg, "ExampleStruct", &buf)) + testutil.AssertGolden(t, testutil.Text, "./testdata/partial.go", buf.Bytes()) +} diff --git a/cmd/genpartial/testdata/partial.go b/cmd/genpartial/testdata/partial.go new file mode 100644 index 0000000000..1a136553e8 --- /dev/null +++ b/cmd/genpartial/testdata/partial.go @@ -0,0 +1,41 @@ +// !DO NOT EDIT! Generated by genpartial +// +//go:build !generate +//+gotohelm:ignore=true +package main + +type ( + PartialMapGeneric[T any] map[string]T +) + +type PartialExampleStruct struct { + A1 PartialMapGeneric[int] `json:",omitempty"` + A2 PartialMapGeneric[PartialNestedStruct] `json:",omitempty"` + A3 PartialMapGeneric[*PartialNestedStruct] `json:",omitempty"` + A4 PartialMapGeneric[*PartialNestedStruct] `json:",omitempty"` + A5 PartialMapGeneric[IntAlias] `json:",omitempty"` + + B1 *int `json:",omitempty"` + B2 *int `json:",omitempty"` + + C1 struct { + Any any `json:",omitempty"` + Int *int `json:",omitempty"` + } `json:",omitempty"` + C2 *struct{} `json:",omitempty"` + + D1 *PartialNestedStruct `json:",omitempty"` + D2 *PartialNestedStruct `json:",omitempty"` + + E1 []any `json:",omitempty"` + E2 []int `json:",omitempty"` + E3 []*int `json:",omitempty"` + + F1 []*int `json:"L,omitempty"` + F2 *string `yaml:"M"json:",omitempty"` + F3 IntAlias `json:",omitempty"` +} + +type PartialNestedStruct struct { + Map map[string]string `json:",omitempty"` +} diff --git a/go.mod b/go.mod index 22f2fed748..d7f19c7712 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/gonvenience/ytbx v1.4.4 github.com/homeport/dyff v1.7.1 github.com/stretchr/testify v1.8.4 + golang.org/x/tools v0.17.0 k8s.io/api v0.29.2 k8s.io/apimachinery v0.29.2 k8s.io/client-go v0.29.2 @@ -61,6 +62,7 @@ require ( github.com/texttheater/golang-levenshtein v1.0.1 // indirect github.com/virtuald/go-ordered-json v0.0.0-20170621173500-b18e6e673d74 // indirect golang.org/x/exp v0.0.0-20231006140011-7918f672742d // indirect + golang.org/x/mod v0.14.0 // indirect golang.org/x/net v0.21.0 // indirect golang.org/x/oauth2 v0.12.0 // indirect golang.org/x/sync v0.6.0 // indirect diff --git a/go.sum b/go.sum index 6f385a701c..fe5d986c3c 100644 --- a/go.sum +++ b/go.sum @@ -163,6 +163,8 @@ golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= +golang.org/x/mod v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0= +golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190603091049-60506f45cf65/go.mod h1:HSz+uSET+XFnRR8LxR5pz3Of3rY3CfYBVs4xY44aLks= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=