Skip to content

Commit

Permalink
do not initilize empty labelset
Browse files Browse the repository at this point in the history
  • Loading branch information
eutopian committed Feb 5, 2025
1 parent aa7b906 commit 5cefdd1
Show file tree
Hide file tree
Showing 4 changed files with 656 additions and 60 deletions.
110 changes: 90 additions & 20 deletions deployment/address_book.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,23 @@ var (
)

type TypeAndVersion struct {
Type ContractType `json:"type"`
Version semver.Version `json:"version"`
Labels LabelSet `json:"labels,omitempty"`
Type ContractType `json:"Type"`
Version semver.Version `json:"Version"`
Labels LabelSet `json:"Labels,omitempty"`
}

// String returns ONLY the type and version of the struct. It does not include labels. To get the string including labels, use FullString().
func (tv TypeAndVersion) String() string {
return fmt.Sprintf("%s %s", tv.Type, tv.Version)
}

// FullString returns the type, version, and labels of the struct.
func (tv TypeAndVersion) FullString() string {
if len(tv.Labels) == 0 {
return fmt.Sprintf("%s %s", tv.Type, tv.Version.String())
return fmt.Sprintf("%s %s", tv.Type, tv.Version)
}

// Use the LabelSet's String method for sorted labels
sortedLabels := tv.Labels.String()
return fmt.Sprintf("%s %s %s",
tv.Type,
tv.Version.String(),
sortedLabels,
)
return fmt.Sprintf("%s %s %s", tv.Type, tv.Version, tv.Labels)
}

func (tv TypeAndVersion) Equal(other TypeAndVersion) bool {
Expand Down Expand Up @@ -83,7 +83,24 @@ func TypeAndVersionFromString(s string) (TypeAndVersion, error) {
if err != nil {
return TypeAndVersion{}, err
}
labels := make(LabelSet)

return TypeAndVersion{
Type: ContractType(parts[0]),
Version: *v,
}, nil
}

// TypeAndVersionFromFullString parses a string containing a type, version, and optional labels.
func TypeAndVersionFromFullString(s string) (TypeAndVersion, error) {
parts := strings.Fields(s) // Ignores consecutive spaces
if len(parts) < 2 {
return TypeAndVersion{}, fmt.Errorf("invalid type and version string: %s", s)
}
v, err := semver.NewVersion(parts[1])
if err != nil {
return TypeAndVersion{}, err
}
var labels LabelSet
if len(parts) > 2 {
labels = NewLabelSet(parts[2:]...)
}
Expand All @@ -98,8 +115,21 @@ func NewTypeAndVersion(t ContractType, v semver.Version) TypeAndVersion {
return TypeAndVersion{
Type: t,
Version: v,
Labels: make(LabelSet), // empty set,
Labels: nil,
}
}

// DeepClone returns a copy of the TypeAndVersion struct with its Labels cloned.
func (tv TypeAndVersion) DeepClone() TypeAndVersion {
// Make a shallow copy first
out := tv

// Now deep-copy the Labels map
if tv.Labels != nil {
out.Labels = tv.Labels.DeepClone()
}

return out
}

// AddressBook is a simple interface for storing and retrieving contract addresses across
Expand Down Expand Up @@ -172,7 +202,7 @@ func (m *AddressBookMap) Addresses() (map[uint64]map[string]TypeAndVersion, erro
// maps are mutable and pass via a pointer
// creating a copy of the map to prevent concurrency
// read and changes outside object-bound
return m.cloneAddresses(m.addressesByChain), nil
return m.deepCloneAddresses(m.addressesByChain), nil
}

func (m *AddressBookMap) AddressesForChain(chainSelector uint64) (map[string]TypeAndVersion, error) {
Expand Down Expand Up @@ -245,7 +275,7 @@ func (m *AddressBookMap) Remove(ab AddressBook) error {
return nil
}

// cloneAddresses creates a deep copy of map[uint64]map[string]TypeAndVersion object
// cloneAddresses creates a shallow copy of map[uint64]map[string]TypeAndVersion object
func (m *AddressBookMap) cloneAddresses(input map[uint64]map[string]TypeAndVersion) map[uint64]map[string]TypeAndVersion {
result := make(map[uint64]map[string]TypeAndVersion)
for chainSelector, chainAddresses := range input {
Expand All @@ -254,6 +284,23 @@ func (m *AddressBookMap) cloneAddresses(input map[uint64]map[string]TypeAndVersi
return result
}

// deepCloneAddresses creates a deep copy of map[uint64]map[string]TypeAndVersion object
func (m *AddressBookMap) deepCloneAddresses(
input map[uint64]map[string]TypeAndVersion,
) map[uint64]map[string]TypeAndVersion {
result := make(map[uint64]map[string]TypeAndVersion, len(input))
for chainSelector, chainAddresses := range input {
// Make a new map for the nested addresses
newChainMap := make(map[string]TypeAndVersion, len(chainAddresses))
for addr, tv := range chainAddresses {
// Use the DeepClone method on the TypeAndVersion
newChainMap[addr] = tv.DeepClone()
}
result[chainSelector] = newChainMap
}
return result
}

// TODO: Maybe could add an environment argument
// which would ensure only mainnet/testnet chain selectors are used
// for further safety?
Expand Down Expand Up @@ -307,11 +354,15 @@ type typeVersionKey struct {
}

func tvKey(tv TypeAndVersion) typeVersionKey {
sortedLabels := tv.Labels.String()
var labels string
if tv.Labels != nil {
labels = tv.Labels.String()
}

return typeVersionKey{
Type: tv.Type,
Version: tv.Version.String(),
Labels: sortedLabels,
Labels: labels,
}
}

Expand All @@ -328,8 +379,12 @@ func AddressesContainBundle(addrs map[string]TypeAndVersion, wantTypes []TypeAnd
// They match exactly (Type, Version, Labels)
counts[wantKey]++
if counts[wantKey] > 1 {
var labels string
if wantTV.Labels != nil {
labels = wantTV.Labels.String()
}
return false, fmt.Errorf("found more than one instance of contract %s %s (labels=%s)",
wantTV.Type, wantTV.Version.String(), wantTV.Labels.String())
wantTV.Type, wantTV.Version, labels)
}
}
}
Expand All @@ -340,9 +395,24 @@ func AddressesContainBundle(addrs map[string]TypeAndVersion, wantTypes []TypeAnd
}

// AddLabel adds a string to the LabelSet in the TypeAndVersion.
func (tv *TypeAndVersion) AddLabel(label string) {
func (tv *TypeAndVersion) AddLabel(label ...string) {
if tv.Labels == nil {
tv.Labels = make(LabelSet)
}
tv.Labels.Add(label)
tv.Labels.Add(label...)
}

func (tv *TypeAndVersion) RemoveLabel(label ...string) *TypeAndVersion {
if tv.Labels == nil {
return tv
}
tv.Labels.Remove(label...)
return tv
}

func (tv *TypeAndVersion) LabelsString() string {
if tv.Labels == nil {
return ""
}
return tv.Labels.String()
}
24 changes: 20 additions & 4 deletions deployment/address_book_labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,17 @@ func NewLabelSet(labels ...string) LabelSet {
}

// Add inserts a labels into the set.
func (ls LabelSet) Add(labels string) {
ls[labels] = struct{}{}
func (ls LabelSet) Add(labels ...string) {
for _, label := range labels {
ls[label] = struct{}{}
}
}

// Remove deletes a labels from the set, if it exists.
func (ls LabelSet) Remove(labels string) {
delete(ls, labels)
func (ls LabelSet) Remove(labels ...string) {
for _, label := range labels {
delete(ls, label)
}
}

// Contains checks if the set contains the given labels.
Expand Down Expand Up @@ -65,3 +69,15 @@ func (ls LabelSet) Equal(other LabelSet) bool {
}
return true
}

// DeepClone returns a copy of the LabelSet.
func (ls LabelSet) DeepClone() LabelSet {
if ls == nil {
return nil
}
out := make(LabelSet, len(ls))
for label := range ls {
out[label] = struct{}{}
}
return out
}
142 changes: 117 additions & 25 deletions deployment/address_book_labels_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,51 +8,71 @@ import (

func TestNewLabelSet(t *testing.T) {
t.Run("no labels", func(t *testing.T) {
ms := NewLabelSet()
assert.Empty(t, ms, "expected empty set")
ls := NewLabelSet()
assert.Empty(t, ls, "expected empty set")
})

t.Run("some labels", func(t *testing.T) {
ms := NewLabelSet("foo", "bar")
assert.Len(t, ms, 2)
assert.True(t, ms.Contains("foo"))
assert.True(t, ms.Contains("bar"))
assert.False(t, ms.Contains("baz"))
ls := NewLabelSet("foo", "bar")
assert.Len(t, ls, 2)
assert.True(t, ls.Contains("foo"))
assert.True(t, ls.Contains("bar"))
assert.False(t, ls.Contains("baz"))
})
}

func TestLabelSet_Add(t *testing.T) {
ms := NewLabelSet("initial")
ms.Add("new")
ls := NewLabelSet("initial")
ls.Add("new")

assert.True(t, ms.Contains("initial"), "expected 'initial' in set")
assert.True(t, ms.Contains("new"), "expected 'new' in set")
assert.Len(t, ms, 2, "expected 2 distinct labels in set")
assert.True(t, ls.Contains("initial"), "expected 'initial' in set")
assert.True(t, ls.Contains("new"), "expected 'new' in set")
assert.Len(t, ls, 2, "expected 2 distinct labels in set")

// Add duplicate "new" again; size should remain 2
ms.Add("new")
assert.Len(t, ms, 2, "expected size to remain 2 after adding a duplicate")
ls.Add("new")
assert.Len(t, ls, 2, "expected size to remain 2 after adding a duplicate")

// Add multiple labels at once
ls.Add("label1", "label2", "label3")
assert.Len(t, ls, 5, "expected 5 distinct labels in set") // 2 previous + 3 new
assert.True(t, ls.Contains("label1"))
assert.True(t, ls.Contains("label2"))
assert.True(t, ls.Contains("label3"))
}

func TestLabelSet_Remove(t *testing.T) {
ms := NewLabelSet("remove_me", "keep")
ms.Remove("remove_me")
ls := NewLabelSet("remove_me", "keep", "label1", "label2", "label3")
ls.Remove("remove_me")

assert.False(t, ms.Contains("remove_me"), "expected 'remove_me' to be removed")
assert.True(t, ms.Contains("keep"), "expected 'keep' to remain")
assert.Len(t, ms, 1, "expected set size to be 1 after removal")
assert.False(t, ls.Contains("remove_me"), "expected 'remove_me' to be removed")
assert.True(t, ls.Contains("keep"), "expected 'keep' to remain")
assert.True(t, ls.Contains("label1"), "expected 'label1' to remain")
assert.True(t, ls.Contains("label2"), "expected 'label2' to remain")
assert.True(t, ls.Contains("label3"), "expected 'label3' to remain")
assert.Len(t, ls, 4, "expected set size to be 4 after removal")

// Removing a non-existent item shouldn't change the size
ms.Remove("non_existent")
assert.Len(t, ms, 1, "expected size to remain 1 after removing a non-existent item")
ls.Remove("non_existent")
assert.Len(t, ls, 4, "expected size to remain 4 after removing a non-existent item")

// Remove multiple labels at once
ls.Remove("label2", "label4")

assert.Len(t, ls, 3, "expected 3 distinct labels in set after removal") // keep, label1, label3
assert.True(t, ls.Contains("keep"))
assert.True(t, ls.Contains("label1"))
assert.False(t, ls.Contains("label2"))
assert.True(t, ls.Contains("label3"))
assert.False(t, ls.Contains("label4"))
}

func TestLabelSet_Contains(t *testing.T) {
ms := NewLabelSet("foo", "bar")
ls := NewLabelSet("foo", "bar")

assert.True(t, ms.Contains("foo"))
assert.True(t, ms.Contains("bar"))
assert.False(t, ms.Contains("baz"))
assert.True(t, ls.Contains("foo"))
assert.True(t, ls.Contains("bar"))
assert.False(t, ls.Contains("baz"))
}

// TestLabelSet_String tests the String() method of the LabelSet type.
Expand Down Expand Up @@ -181,3 +201,75 @@ func TestLabelSet_Equal(t *testing.T) {
})
}
}

func TestLabelSet_DeepClone(t *testing.T) {
tests := []struct {
name string
input LabelSet
mutate func(ls LabelSet)
wantEqual bool
}{
{
name: "Empty label set",
// An empty-but-initialized map
input: NewLabelSet(),
mutate: func(ls LabelSet) {
ls.Add("test-label")
},
wantEqual: false,
},
{
name: "Non-empty label set",
input: NewLabelSet("fast", "secure"),
mutate: func(ls LabelSet) {
ls.Remove("secure") // remove an existing label
ls.Add("extra") // add a new label
},
wantEqual: false,
},
{
name: "Clone but no mutation",
input: NewLabelSet("unchanged"),
mutate: func(ls LabelSet) {
// no change
},
// If we do no mutation, they should remain equal
wantEqual: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
original := tt.input
clone := original.DeepClone()

// Clones should initially match the original (especially for non-nil).
// For nil, clone should also be nil, so they're logically "equal" if you treat nil sets as empty.
assert.True(t, original.Equal(clone),
"DeepClone result should match input before mutation")

// Mutate the clone
tt.mutate(clone)

// After mutation, check if they should remain equal or differ.
if tt.wantEqual {
assert.True(t, original.Equal(clone),
"Unexpected difference between original and clone")
} else {
assert.False(t, original.Equal(clone),
"Expected original and clone to differ after mutation, but they are equal")
}

// Optionally, also check that mutating the original won't affect the clone.
if original != nil {
original.Add("original-mutated")
// They should differ if we just mutated original (unless they were already different).
// We'll just do a quick check that the new label isn't in the clone.
if original.Contains("original-mutated") && clone != nil {
assert.False(t, clone.Contains("original-mutated"),
"Clone should not contain a label added to the original after deep clone")
}
}
})
}
}
Loading

0 comments on commit 5cefdd1

Please sign in to comment.