Skip to content

Commit

Permalink
Tests and comments for MakeDefaultConfig perf work
Browse files Browse the repository at this point in the history
Document updated code in MakeDefaultConfig.
Add unit tests to ensure DeepCopy works.
Add hints to other code to ensure DeepCopy is kept up to date.
  • Loading branch information
chlunde committed Oct 30, 2023
1 parent 17f7b2b commit 7eb0704
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 2 deletions.
7 changes: 7 additions & 0 deletions api/internal/plugins/builtinconfig/namebackreferences.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ type NameBackReferences struct {
// TODO: rename json 'fieldSpecs' to 'referrers' for clarity.
// This will, however, break anyone using a custom config.
Referrers types.FsSlice `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"`

// Note: If any new pointer based members are added, DeepCopy needs to be updated
}

func (n NameBackReferences) String() string {
Expand All @@ -66,9 +68,14 @@ func (s nbrSlice) Less(i, j int) bool {
return s[i].Gvk.IsLessThan(s[j].Gvk)
}

// DeepCopy returns a new copy of nbrSlice
func (s nbrSlice) DeepCopy() nbrSlice {
ret := make(nbrSlice, len(s))
copy(ret, s)
for i, slice := range ret {
ret[i].Referrers = slice.Referrers.DeepCopy()
}

return ret
}

Expand Down
26 changes: 26 additions & 0 deletions api/internal/plugins/builtinconfig/namebackreferences_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,29 @@ func TestMergeAll(t *testing.T) {
t.Fatalf("expected\n %v\n but got\n %v\n", expected, actual)
}
}

func TestNbrSlice_DeepCopy(t *testing.T) {
original := make(nbrSlice, 2, 4)
original[0] = NameBackReferences{Gvk: resid.FromKind("A"), Referrers: types.FsSlice{{Path: "a"}}}
original[1] = NameBackReferences{Gvk: resid.FromKind("B"), Referrers: types.FsSlice{{Path: "b"}}}

copied := original.DeepCopy()

original, _ = original.mergeOne(NameBackReferences{Gvk: resid.FromKind("C"), Referrers: types.FsSlice{{Path: "c"}}})

// perform mutations which should not affect original
copied.Swap(0, 1)
copied[0].Referrers[0].Path = "very b" // ensure Referrers are not shared
_, _ = copied.mergeOne(NameBackReferences{Gvk: resid.FromKind("D"), Referrers: types.FsSlice{{Path: "d"}}})

// if DeepCopy does not work, original would be {very b,a,d} instead of {a,b,c}
expected := nbrSlice{
{Gvk: resid.FromKind("A"), Referrers: types.FsSlice{{Path: "a"}}},
{Gvk: resid.FromKind("B"), Referrers: types.FsSlice{{Path: "b"}}},
{Gvk: resid.FromKind("C"), Referrers: types.FsSlice{{Path: "c"}}},
}

if !reflect.DeepEqual(original, expected) {
t.Fatalf("original affected by mutations to copied object:\ngot\t%+v,\nexpected: %+v", original, expected)
}
}
10 changes: 8 additions & 2 deletions api/internal/plugins/builtinconfig/transformerconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

// TransformerConfig holds the data needed to perform transformations.
type TransformerConfig struct {
// if any fields are added, update the DeepCopy implementation
NamePrefix types.FsSlice `json:"namePrefix,omitempty" yaml:"namePrefix,omitempty"`
NameSuffix types.FsSlice `json:"nameSuffix,omitempty" yaml:"nameSuffix,omitempty"`
NameSpace types.FsSlice `json:"namespace,omitempty" yaml:"namespace,omitempty"`
Expand All @@ -33,6 +34,7 @@ func MakeEmptyConfig() *TransformerConfig {
return &TransformerConfig{}
}

// DeepCopy returns a new copy of TransformerConfig
func (t *TransformerConfig) DeepCopy() *TransformerConfig {
return &TransformerConfig{
NamePrefix: t.NamePrefix.DeepCopy(),
Expand All @@ -48,13 +50,16 @@ func (t *TransformerConfig) DeepCopy() *TransformerConfig {
}
}

// the default transformer config is initialized by MakeDefaultConfig,
// and must only be accessed via that function.
var (
initDefaultConfig sync.Once
defaultConfig *TransformerConfig
initDefaultConfig sync.Once //nolint:gochecknoglobals
defaultConfig *TransformerConfig //nolint:gochecknoglobals
)

// MakeDefaultConfig returns a default TransformerConfig.
func MakeDefaultConfig() *TransformerConfig {
// parsing is expensive when having a large tree with many kustomization modules, so only do it once
initDefaultConfig.Do(func() {
var err error
defaultConfig, err = makeTransformerConfigFromBytes(
Expand All @@ -64,6 +69,7 @@ func MakeDefaultConfig() *TransformerConfig {
}
})

// return a copy to avoid any mutations to protect the reference copy
return defaultConfig.DeepCopy()
}

Expand Down
19 changes: 19 additions & 0 deletions api/internal/plugins/builtinconfig/transformerconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,22 @@ func TestMerge(t *testing.T) {
t.Fatalf("expected: %v\n but got: %v\n", cfga, actual)
}
}

func TestMakeDefaultConfig_mutation(t *testing.T) {
a := MakeDefaultConfig()

// mutate
a.NameReference[0].Kind = "mutated"
a.NameReference = a.NameReference[:1]

clean := MakeDefaultConfig()
if clean.NameReference[0].Kind == "mutated" {
t.Errorf("MakeDefaultConfig() did not return a clean copy: %+v", clean.NameReference)
}
}

func BenchmarkMakeDefaultConfig(b *testing.B) {
for i := 0; i < b.N; i++ {
_ = MakeDefaultConfig()
}
}
3 changes: 3 additions & 0 deletions api/types/fieldspec.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ type FieldSpec struct {
resid.Gvk `json:",inline,omitempty" yaml:",inline,omitempty"`
Path string `json:"path,omitempty" yaml:"path,omitempty"`
CreateIfNotPresent bool `json:"create,omitempty" yaml:"create,omitempty"`

// Note: If any new pointer based members are added, FsSlice.DeepCopy needs to be updated
}

func (fs FieldSpec) String() string {
Expand All @@ -50,6 +52,7 @@ func (s FsSlice) Less(i, j int) bool {
return s[i].Gvk.IsLessThan(s[j].Gvk)
}

// DeepCopy returns a new copy of FsSlice
func (s FsSlice) DeepCopy() FsSlice {
ret := make(FsSlice, len(s))
copy(ret, s)
Expand Down
24 changes: 24 additions & 0 deletions api/types/fieldspec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,27 @@ func TestFsSlice_MergeAll(t *testing.T) {
}
}
}

func TestFsSlice_DeepCopy(t *testing.T) {
original := make(FsSlice, 2, 4)
original[0] = FieldSpec{Path: "a"}
original[1] = FieldSpec{Path: "b"}

copied := original.DeepCopy()

original, _ = original.MergeOne(FieldSpec{Path: "c"})

// perform mutations which should not affect original
copied.Swap(0, 1)
_, _ = copied.MergeOne(FieldSpec{Path: "d"})

// if DeepCopy does not work, original would be {b,a,d} instead of {a,b,c}
expected := FsSlice{
{Path: "a"},
{Path: "b"},
{Path: "c"},
}
if !reflect.DeepEqual(original, expected) {
t.Fatalf("original affected by mutations to copied object:\ngot\t%+v,\nexpected: %+v", original, expected)
}
}

0 comments on commit 7eb0704

Please sign in to comment.