Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion internal/schemas/generator/accessors.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,22 @@ func receiverTypeName(e ast.Expr) string {
return ""
}

// isNilable reports whether a zero value of the given type is spelled `nil`,
// letting the generated getter return nil directly instead of declaring a zero
// variable. Generated models use pointers throughout, so this is the common
// case; the zero-variable form covers everything else.
func isNilable(e ast.Expr) bool {
switch t := e.(type) {
case *ast.StarExpr, *ast.MapType, *ast.InterfaceType, *ast.ChanType, *ast.FuncType:
return true
case *ast.ArrayType:
// Slices are nilable; fixed-size arrays are not.
return t.Len == nil
default:
return false
}
}

// writeStructAccessors appends a getter and setter for each named field of the
// given struct to b. Any accessor whose name already exists in skip is omitted
// to avoid colliding with methods oapi-codegen already generated.
Expand Down Expand Up @@ -158,10 +174,20 @@ func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName stri

fieldName := name.Name

// Getter.
// Getter. It tolerates a nil receiver so that chained getters are
// safe on partially-populated resources.
if !skip["Get"+fieldName] {
b.WriteString("\n// Get" + fieldName + " returns the " + fieldName + " field.\n")
b.WriteString("// It returns the zero value if the receiver is nil.\n")
b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Get" + fieldName + "() " + fieldType + " {\n")
b.WriteString("\tif " + accessorReceiver + " == nil {\n")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the receivers guaranteed to be nil-able? I think they are because we mark every openapi field as optional, so they all become pointers - is that right?

if isNilable(field.Type) {
b.WriteString("\t\treturn nil\n")
} else {
b.WriteString("\t\tvar zero " + fieldType + "\n")
b.WriteString("\t\treturn zero\n")
}
b.WriteString("\t}\n")
b.WriteString("\treturn " + accessorReceiver + "." + fieldName + "\n")
b.WriteString("}\n")
}
Expand Down
110 changes: 110 additions & 0 deletions internal/schemas/generator/accessors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"os"
"os/exec"
"path/filepath"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -251,6 +252,12 @@ func useAccessors(a specAccessor) *v1alpha1.XAccountScaffoldSpec {
}

var _ = useAccessors

// ChainOnEmpty walks nested getters on a resource whose intermediate structs are
// all nil. It must return the zero value rather than panicking.
func ChainOnEmpty() *string {
return (&v1alpha1.XAccountScaffold{}).GetSpec().GetParameters().GetName()
}
`
consumerDir := filepath.Join(dir, "models", "consumer")
if err := os.MkdirAll(consumerDir, 0o755); err != nil {
Expand Down Expand Up @@ -280,6 +287,28 @@ var _ = useAccessors
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("generated models failed to compile: %v\n%s", err, out)
}

// Compiling proves the chain typechecks; running it proves the nil guards
// actually hold. Without them ChainOnEmpty panics on the first hop.
consumerTest := `package consumer

import "testing"

func TestChainOnEmptyDoesNotPanic(t *testing.T) {
if got := ChainOnEmpty(); got != nil {
t.Errorf("expected nil from a chain over an empty resource, got %v", *got)
}
}
`
if err := os.WriteFile(filepath.Join(consumerDir, "consumer_test.go"), []byte(consumerTest), 0o644); err != nil {
t.Fatal(err)
}

cmd = exec.CommandContext(t.Context(), "go", "test", "./consumer/...")
cmd.Dir = modelsDir
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("chained getters panicked on an empty resource: %v\n%s", err, out)
}
}

func TestAddAccessors(t *testing.T) {
Expand Down Expand Up @@ -327,6 +356,87 @@ type FooAlias = Foo
}
}

// guardsNilReceiver reports whether the body of method recv.name opens with an
// `if <receiver> == nil` guard.
func guardsNilReceiver(t *testing.T, src, recv, name string) bool {
t.Helper()
fset := token.NewFileSet()
f, err := parser.ParseFile(fset, "", src, parser.ParseComments)
if err != nil {
t.Fatalf("failed to parse source: %v\n%s", err, src)
}
for _, decl := range f.Decls {
fn, ok := decl.(*ast.FuncDecl)
if !ok || fn.Recv == nil || len(fn.Recv.List) != 1 {
continue
}
if receiverTypeName(fn.Recv.List[0].Type) != recv || fn.Name.Name != name {
continue
}
if fn.Body == nil || len(fn.Body.List) == 0 {
return false
}
ifs, ok := fn.Body.List[0].(*ast.IfStmt)
if !ok {
return false
}
bin, ok := ifs.Cond.(*ast.BinaryExpr)
if !ok || bin.Op != token.EQL {
return false
}
x, okX := bin.X.(*ast.Ident)
y, okY := bin.Y.(*ast.Ident)
return okX && okY && x.Name == accessorReceiver && y.Name == "nil"
}
t.Fatalf("method %s.%s not found", recv, name)
return false
}

// TestAddAccessorsGuardsNilReceiver verifies that getters tolerate a nil
// receiver, which is what makes chained getters safe on partially-populated
// resources. Setters are deliberately left unguarded: a set on a nil receiver
// has nowhere to store the value, so panicking is the honest behaviour.
func TestAddAccessorsGuardsNilReceiver(t *testing.T) {
input := `package v1alpha1

type Foo struct {
Bar *Bar ` + "`json:\"bar,omitempty\"`" + `
Name *string ` + "`json:\"name,omitempty\"`" + `
Count int64 ` + "`json:\"count\"`" + `
Fixed [2]byte ` + "`json:\"fixed\"`" + `
}

type Bar struct {
Count *int64 ` + "`json:\"count,omitempty\"`" + `
}
`

got, err := addAccessors(input)
if err != nil {
t.Fatalf("addAccessors returned error: %v", err)
}

for _, name := range []string{"GetBar", "GetName", "GetCount", "GetFixed"} {
if !guardsNilReceiver(t, got, "Foo", name) {
t.Errorf("Foo.%s must guard against a nil receiver", name)
}
}
if guardsNilReceiver(t, got, "Foo", "SetBar") {
t.Error("Foo.SetBar must not silently swallow a nil receiver")
}

// Nilable types return nil directly; non-nilable types need a zero value.
if !strings.Contains(got, "func (o *Foo) GetBar() *Bar {\n\tif o == nil {\n\t\treturn nil\n\t}") {
t.Errorf("GetBar should return nil for a nilable field, got:\n%s", got)
}
if !strings.Contains(got, "var zero int64") {
t.Errorf("GetCount should declare a zero value for a non-nilable field, got:\n%s", got)
}
if !strings.Contains(got, "var zero [2]byte") {
t.Errorf("GetFixed should treat a fixed-size array as non-nilable, got:\n%s", got)
}
}

// countMethods returns how many times recv.name is declared in src.
func countMethods(t *testing.T, src string, recv, name string) int {
t.Helper()
Expand Down
Loading