From 35bb7dca97d8085aeb710bd436e7f751c0b3dd41 Mon Sep 17 00:00:00 2001 From: Hubertbits <170125456+hubertbits@users.noreply.github.com> Date: Wed, 16 Apr 2025 15:45:20 +0200 Subject: [PATCH] pkg/common: Update types Signed-off-by: yxxhero --- pkg/common/README.md | 108 +++++++++++++++ pkg/common/map_flag.go | 69 ++++++++++ pkg/common/map_flag_test.go | 159 ++++++++++++++++++++++ pkg/common/string_array_flag.go | 71 +++++----- pkg/common/string_array_flag_test.go | 195 ++++++++++----------------- 5 files changed, 449 insertions(+), 153 deletions(-) create mode 100644 pkg/common/README.md create mode 100644 pkg/common/map_flag.go create mode 100644 pkg/common/map_flag_test.go diff --git a/pkg/common/README.md b/pkg/common/README.md new file mode 100644 index 00000000..4a8b8fc4 --- /dev/null +++ b/pkg/common/README.md @@ -0,0 +1,108 @@ +# Common Package + +## Overview + +The `pkg/common` package provides common utilities, types, and interfaces used throughout the Helmfile application. This package contains shared functionality that doesn't belong to any specific domain but is used by multiple components. + +## Current File Structure + +``` +pkg/common/ +├── README.md # This documentation file +├── bool_flag.go # Boolean flag implementation +├── bool_flag_test.go # Tests for boolean flag +├── string_flag.go # String flag implementation +├── string_flag_test.go # Tests for string flag +├── string_array_flag.go # String array flag implementation +├── string_array_flag_test.go # Tests for array flag +├── map_flag.go # Map flag implementation +├── map_flag_test.go # Tests for map flag +``` + +## Planned Future Implementations + +The following files are planned for future development: +``` +├── constants.go # Common constants used throughout the application +├── errors.go # Common error types and error handling utilities +├── errors_test.go # Tests for error utilities +├── logging.go # Logging utilities and interfaces +├── logging_test.go # Tests for logging utilities +├── types.go # Common type definitions +├── types_test.go # Tests for common types +├── utils.go # General utility functions +└── utils_test.go # Tests for utility functions +``` + +## Components + +### Currently Implemented + +#### Flag Types +- **BoolFlag**: A boolean flag implementation with value tracking +- **StringFlag**: A string flag implementation with value tracking +- **StringArrayFlag**: A string array flag implementation with value tracking +- **MapFlag**: A map flag implementation with value tracking + +### Planned for Future Implementation + +#### Utilities +- **Constants**: Common constants used throughout the application +- **Error Handling**: Standardized error types and handling +- **Logging**: Consistent logging interfaces and utilities +- **Types**: Common type definitions +- **Utilities**: General utility functions + +## Key Features + +### Currently Available +- **Flag Implementations**: Reusable flag implementations with value tracking + +### Planned Features +- **Error Handling**: Standardized error types and handling +- **Logging**: Consistent logging interfaces and utilities +- **Type Safety**: Common type definitions for consistent usage + +## Usage + +### Flag Usage + +```go +// Create a boolean flag +includeFlag := common.NewBoolFlag(false) + +// Set the flag value +includeFlag.Set(true) + +// Get the flag value +value := includeFlag.Value() +``` + +### Future Error Handling (Planned) + +```go +// Create a common error +err := common.NewError("operation failed") + +// Check if an error is of a specific type +if common.IsNotFoundError(err) { + // Handle not found error +} +``` + +### Future Logging (Planned) + +```go +// Create a logger +logger := common.NewLogger() + +// Log messages +logger.Info("Operation started") +logger.Error("Operation failed: %v", err) +``` + +## Related Packages + +- `pkg/config`: Uses common flag types for option configuration +- `pkg/flags`: Integrates with common flag types for flag handling +- `pkg/app`: Uses common utilities for application functionality diff --git a/pkg/common/map_flag.go b/pkg/common/map_flag.go new file mode 100644 index 00000000..1a2954f1 --- /dev/null +++ b/pkg/common/map_flag.go @@ -0,0 +1,69 @@ +package common + +// MapFlag represents a string map flag that tracks whether it was explicitly set +type MapFlag interface { + // Value returns a copy of the current map value + Value() map[string]string + + // WasExplicitlySet returns whether the flag was explicitly set + WasExplicitlySet() bool + + // Set sets the value and marks the flag as explicitly set + Set(value map[string]string) + + // SetKey sets a specific key-value pair and marks the flag as explicitly set + SetKey(key, value string) +} + +// mapFlag is the implementation of MapFlag +type mapFlag struct { + value map[string]string + wasExplicitlySet bool +} + +// NewMapFlag creates a new MapFlag with default values +func NewMapFlag(defaultValue map[string]string) MapFlag { + // Create a copy of the default value to avoid modifying the original + valueCopy := make(map[string]string, len(defaultValue)) + for k, v := range defaultValue { + valueCopy[k] = v + } + + return &mapFlag{ + value: valueCopy, + wasExplicitlySet: false, + } +} + +// Value returns a copy of the current map value +func (mf *mapFlag) Value() map[string]string { + // Create a copy to prevent external modification of internal state + result := make(map[string]string, len(mf.value)) + for k, v := range mf.value { + result[k] = v + } + return result +} + +// WasExplicitlySet returns whether the flag was explicitly set +func (mf *mapFlag) WasExplicitlySet() bool { + return mf.wasExplicitlySet +} + +// Set sets the value and marks the flag as explicitly set +func (mf *mapFlag) Set(value map[string]string) { + // Create a copy of the value to avoid modifying the original + valueCopy := make(map[string]string, len(value)) + for k, v := range value { + valueCopy[k] = v + } + + mf.value = valueCopy + mf.wasExplicitlySet = true +} + +// SetKey sets a specific key-value pair and marks the flag as explicitly set +func (mf *mapFlag) SetKey(key, value string) { + mf.value[key] = value + mf.wasExplicitlySet = true +} diff --git a/pkg/common/map_flag_test.go b/pkg/common/map_flag_test.go new file mode 100644 index 00000000..8f8be19e --- /dev/null +++ b/pkg/common/map_flag_test.go @@ -0,0 +1,159 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNewMapFlag(t *testing.T) { + tests := []struct { + name string + defaultValue map[string]string + expected map[string]string + }{ + { + name: "default empty", + defaultValue: map[string]string{}, + expected: map[string]string{}, + }, + { + name: "default with values", + defaultValue: map[string]string{"key1": "value1", "key2": "value2"}, + expected: map[string]string{"key1": "value1", "key2": "value2"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewMapFlag(tt.defaultValue) + + // Check initial state + assert.Equal(t, tt.expected, flag.Value(), "Value should match default") + assert.False(t, flag.WasExplicitlySet(), "New flag should not be marked as explicitly set") + + // Ensure the default value is copied, not referenced + if len(tt.defaultValue) > 0 { + original := make(map[string]string) + for k, v := range tt.defaultValue { + original[k] = v + } + + // Modify the original + for k := range tt.defaultValue { + tt.defaultValue[k] = "modified" + break + } + + // Flag value should remain unchanged + assert.Equal(t, original, flag.Value(), "Flag value should be a copy, not a reference") + } + }) + } +} + +func TestMapFlag_ValueImmutability(t *testing.T) { + // Test that modifying the returned value doesn't affect the internal state + flag := NewMapFlag(map[string]string{"key1": "value1", "key2": "value2"}) + + // Get the value and modify it + value := flag.Value() + value["key1"] = "modified" + + // Check that the flag's internal state is unchanged + expected := map[string]string{"key1": "value1", "key2": "value2"} + assert.Equal(t, expected, flag.Value(), "Modifying the returned value should not affect the flag's internal state") +} + +func TestMapFlag_Set(t *testing.T) { + tests := []struct { + name string + defaultValue map[string]string + setValue map[string]string + expected map[string]string + }{ + { + name: "default empty, set values", + defaultValue: map[string]string{}, + setValue: map[string]string{"key1": "value1", "key2": "value2"}, + expected: map[string]string{"key1": "value1", "key2": "value2"}, + }, + { + name: "default with values, set new values", + defaultValue: map[string]string{"key1": "value1", "key2": "value2"}, + setValue: map[string]string{"key3": "value3", "key4": "value4"}, + expected: map[string]string{"key3": "value3", "key4": "value4"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewMapFlag(tt.defaultValue) + + // Set the value + flag.Set(tt.setValue) + + // Check state after setting + assert.Equal(t, tt.expected, flag.Value(), "Value should match set value") + assert.True(t, flag.WasExplicitlySet(), "Flag should be marked as explicitly set") + + // Ensure the set value is copied, not referenced + if len(tt.setValue) > 0 { + original := make(map[string]string) + for k, v := range tt.setValue { + original[k] = v + } + + // Modify the original + for k := range tt.setValue { + tt.setValue[k] = "modified" + break + } + + // Flag value should remain unchanged + assert.Equal(t, original, flag.Value(), "Flag value should be a copy, not a reference") + } + }) + } +} + +func TestMapFlag_SetKey(t *testing.T) { + flag := NewMapFlag(map[string]string{"existing": "value"}) + + // Initial state + assert.Equal(t, map[string]string{"existing": "value"}, flag.Value()) + assert.False(t, flag.WasExplicitlySet()) + + // Set a new key + flag.SetKey("new", "value") + assert.Equal(t, map[string]string{"existing": "value", "new": "value"}, flag.Value()) + assert.True(t, flag.WasExplicitlySet()) + + // Override an existing key + flag.SetKey("existing", "updated") + assert.Equal(t, map[string]string{"existing": "updated", "new": "value"}, flag.Value()) + assert.True(t, flag.WasExplicitlySet()) +} + +func TestMapFlag_MultipleSet(t *testing.T) { + flag := NewMapFlag(map[string]string{"initial": "value"}) + + // Initial state + assert.Equal(t, map[string]string{"initial": "value"}, flag.Value()) + assert.False(t, flag.WasExplicitlySet()) + + // First set + flag.Set(map[string]string{"first": "set"}) + assert.Equal(t, map[string]string{"first": "set"}, flag.Value()) + assert.True(t, flag.WasExplicitlySet()) + + // Second set + flag.Set(map[string]string{"second": "set"}) + assert.Equal(t, map[string]string{"second": "set"}, flag.Value()) + assert.True(t, flag.WasExplicitlySet(), "Flag should remain explicitly set") +} + +func TestMapFlag_Implementation(t *testing.T) { + // Test that mapFlag properly implements MapFlag interface + var _ MapFlag = &mapFlag{} +} diff --git a/pkg/common/string_array_flag.go b/pkg/common/string_array_flag.go index eaa5350a..35d72078 100644 --- a/pkg/common/string_array_flag.go +++ b/pkg/common/string_array_flag.go @@ -1,56 +1,63 @@ package common -// For array/slice flags +// StringArrayFlag represents a string array flag that tracks whether it was explicitly set type StringArrayFlag interface { - Values() []string + // Value returns a copy of the current string array value + Value() []string + + // WasExplicitlySet returns whether the flag was explicitly set WasExplicitlySet() bool - Add(value string) - Set(values []string) + + // Set sets the value and marks the flag as explicitly set + Set(value []string) + + // Append adds a value to the array and marks the flag as explicitly set + Append(value string) } +// stringArrayFlag is the implementation of ArrayFlag type stringArrayFlag struct { - values []string + value []string wasExplicitlySet bool } -// NewStringArrayFlag creates a new StringArrayFlag with the given default values -// Create a defensive copy of the default values to prevent external modifications -// and to ensure that the original values remain unchanged. -func NewStringArrayFlag(defaultValues []string) StringArrayFlag { - valuesCopy := make([]string, len(defaultValues)) - copy(valuesCopy, defaultValues) +// NewStringArrayFlag creates a new ArrayFlag with default values +func NewStringArrayFlag(defaultValue []string) StringArrayFlag { + // Create a copy of the default value to avoid modifying the original + valueCopy := make([]string, len(defaultValue)) + copy(valueCopy, defaultValue) return &stringArrayFlag{ - values: valuesCopy, + value: valueCopy, wasExplicitlySet: false, } } -// Values returns the values of the flag -// It returns a copy of the values to prevent external modifications -// and to ensure that the original values remain unchanged. -// This is important for flags that may be modified by the user -// or other parts of the program. -func (f *stringArrayFlag) Values() []string { - // Return a copy to prevent external modifications - valuesCopy := make([]string, len(f.values)) - copy(valuesCopy, f.values) - return valuesCopy +// Value returns a copy of the current string array value +func (af *stringArrayFlag) Value() []string { + // Create a copy to prevent external modification of internal state + result := make([]string, len(af.value)) + copy(result, af.value) + return result } // WasExplicitlySet returns whether the flag was explicitly set -func (f *stringArrayFlag) WasExplicitlySet() bool { - return f.wasExplicitlySet +func (af *stringArrayFlag) WasExplicitlySet() bool { + return af.wasExplicitlySet } -// Set sets the values and marks the flag as explicitly set -func (f *stringArrayFlag) Set(values []string) { - f.values = values - f.wasExplicitlySet = true +// Set sets the value and marks the flag as explicitly set +func (af *stringArrayFlag) Set(value []string) { + // Create a copy of the value to avoid modifying the original + valueCopy := make([]string, len(value)) + copy(valueCopy, value) + + af.value = valueCopy + af.wasExplicitlySet = true } -// Add sets the value and marks the flag as explicitly set -func (f *stringArrayFlag) Add(value string) { - f.values = append(f.values, value) - f.wasExplicitlySet = true +// Append adds a value to the array and marks the flag as explicitly set +func (af *stringArrayFlag) Append(value string) { + af.value = append(af.value, value) + af.wasExplicitlySet = true } diff --git a/pkg/common/string_array_flag_test.go b/pkg/common/string_array_flag_test.go index 5bd45aba..6d9c9e20 100644 --- a/pkg/common/string_array_flag_test.go +++ b/pkg/common/string_array_flag_test.go @@ -13,19 +13,14 @@ func TestNewStringArrayFlag(t *testing.T) { expected []string }{ { - name: "nil default", - defaultValue: nil, - expected: []string{}, - }, - { - name: "empty default", + name: "default empty", defaultValue: []string{}, expected: []string{}, }, { - name: "non-empty default", - defaultValue: []string{"value1", "value2"}, - expected: []string{"value1", "value2"}, + name: "default with values", + defaultValue: []string{"one", "two"}, + expected: []string{"one", "two"}, }, } @@ -34,8 +29,20 @@ func TestNewStringArrayFlag(t *testing.T) { flag := NewStringArrayFlag(tt.defaultValue) // Check initial state - assert.Equal(t, tt.expected, flag.Values(), "Values should match default") + assert.Equal(t, tt.expected, flag.Value(), "Value should match default") assert.False(t, flag.WasExplicitlySet(), "New flag should not be marked as explicitly set") + + // Ensure the default value is copied, not referenced + if len(tt.defaultValue) > 0 { + original := make([]string, len(tt.defaultValue)) + copy(original, tt.defaultValue) + + // Modify the original + tt.defaultValue[0] = "modified" + + // Flag value should remain unchanged + assert.Equal(t, original, flag.Value(), "Flag value should be a copy, not a reference") + } }) } } @@ -48,22 +55,16 @@ func TestStringArrayFlag_Set(t *testing.T) { expected []string }{ { - name: "nil default, set values", - defaultValue: nil, - setValue: []string{"new1", "new2"}, - expected: []string{"new1", "new2"}, + name: "default empty, set values", + defaultValue: []string{}, + setValue: []string{"one", "two"}, + expected: []string{"one", "two"}, }, { - name: "non-empty default, set empty", - defaultValue: []string{"default1", "default2"}, - setValue: []string{}, - expected: []string{}, - }, - { - name: "non-empty default, set new values", - defaultValue: []string{"default1", "default2"}, - setValue: []string{"new1", "new2"}, - expected: []string{"new1", "new2"}, + name: "default with values, set new values", + defaultValue: []string{"one", "two"}, + setValue: []string{"three", "four"}, + expected: []string{"three", "four"}, }, } @@ -71,125 +72,77 @@ func TestStringArrayFlag_Set(t *testing.T) { t.Run(tt.name, func(t *testing.T) { flag := NewStringArrayFlag(tt.defaultValue) - // Set the values + // Set the value flag.Set(tt.setValue) // Check state after setting - assert.Equal(t, tt.expected, flag.Values(), "Values should match set values") + assert.Equal(t, tt.expected, flag.Value(), "Value should match set value") assert.True(t, flag.WasExplicitlySet(), "Flag should be marked as explicitly set") + + // Ensure the set value is copied, not referenced + if len(tt.setValue) > 0 { + original := make([]string, len(tt.setValue)) + copy(original, tt.setValue) + + // Modify the original + tt.setValue[0] = "modified" + + // Flag value should remain unchanged + assert.Equal(t, original, flag.Value(), "Flag value should be a copy, not a reference") + } }) } } -func TestStringArrayFlag_Add(t *testing.T) { - tests := []struct { - name string - defaultValue []string - addValue string - expected []string - }{ - { - name: "nil default, add value", - defaultValue: nil, - addValue: "new", - expected: []string{"new"}, - }, - { - name: "empty default, add value", - defaultValue: []string{}, - addValue: "new", - expected: []string{"new"}, - }, - { - name: "non-empty default, add value", - defaultValue: []string{"default1", "default2"}, - addValue: "new", - expected: []string{"default1", "default2", "new"}, - }, - } +func TestStringArrayFlag_ValueImmutability(t *testing.T) { + // Test that modifying the returned value doesn't affect the internal state + flag := NewStringArrayFlag([]string{"one", "two"}) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - flag := NewStringArrayFlag(tt.defaultValue) + // Get the value and modify it + value := flag.Value() + value[0] = "modified" - // Add the value - flag.Add(tt.addValue) - - // Check state after adding - assert.Equal(t, tt.expected, flag.Values(), "Values should include added value") - assert.True(t, flag.WasExplicitlySet(), "Flag should be marked as explicitly set") - }) - } + // Check that the flag's internal state is unchanged + assert.Equal(t, []string{"one", "two"}, flag.Value(), "Modifying the returned value should not affect the flag's internal state") } -func TestStringArrayFlag_MultipleOperations(t *testing.T) { +func TestStringArrayFlag_Append(t *testing.T) { + flag := NewStringArrayFlag([]string{"one"}) + + // Initial state + assert.Equal(t, []string{"one"}, flag.Value()) + assert.False(t, flag.WasExplicitlySet()) + + // Append a value + flag.Append("two") + assert.Equal(t, []string{"one", "two"}, flag.Value()) + assert.True(t, flag.WasExplicitlySet()) + + // Append another value + flag.Append("three") + assert.Equal(t, []string{"one", "two", "three"}, flag.Value()) + assert.True(t, flag.WasExplicitlySet()) +} + +func TestStringArrayFlag_MultipleSet(t *testing.T) { flag := NewStringArrayFlag([]string{"initial"}) // Initial state - assert.Equal(t, []string{"initial"}, flag.Values()) + assert.Equal(t, []string{"initial"}, flag.Value()) assert.False(t, flag.WasExplicitlySet()) - // Add a value - flag.Add("added") - assert.Equal(t, []string{"initial", "added"}, flag.Values()) + // First set + flag.Set([]string{"first", "set"}) + assert.Equal(t, []string{"first", "set"}, flag.Value()) assert.True(t, flag.WasExplicitlySet()) - // Set completely new values - flag.Set([]string{"new1", "new2"}) - assert.Equal(t, []string{"new1", "new2"}, flag.Values()) + // Second set + flag.Set([]string{"second", "set"}) + assert.Equal(t, []string{"second", "set"}, flag.Value()) assert.True(t, flag.WasExplicitlySet(), "Flag should remain explicitly set") - - // Add another value after Set - flag.Add("added2") - assert.Equal(t, []string{"new1", "new2", "added2"}, flag.Values()) - assert.True(t, flag.WasExplicitlySet()) } -func TestStringArrayFlag_Implementation(t *testing.T) { +func TestArrayStringFlag_Implementation(t *testing.T) { // Test that stringArrayFlag properly implements StringArrayFlag interface var _ StringArrayFlag = &stringArrayFlag{} } - -func TestStringArrayFlag_DefensiveCopy(t *testing.T) { - // Test that modifying the original slice doesn't affect the flag - original := []string{"value1", "value2"} - flag := NewStringArrayFlag(original) - - // Verify initial state - assert.Equal(t, []string{"value1", "value2"}, flag.Values()) - - // Modify the original slice - should NOT affect the flag's internal state - // because NewStringArrayFlag creates a defensive copy - original[0] = "modified" - original = append(original, "added") - - // Flag values should remain unchanged - assert.Equal(t, []string{"value1", "value2"}, flag.Values()) - - // Test that modifying the returned slice doesn't affect the flag - values := flag.Values() - values[0] = "modified" - values = append(values, "added") - - // Flag values should remain unchanged because Values() returns a copy - assert.Equal(t, []string{"value1", "value2"}, flag.Values()) -} - -func TestStringArrayFlag_SetDefensiveCopy(t *testing.T) { - // Test that Set doesn't create a defensive copy (current implementation) - flag := NewStringArrayFlag([]string{}) - - // Create a slice to set - setValues := []string{"value1", "value2"} - flag.Set(setValues) - - // Verify state after setting - assert.Equal(t, []string{"value1", "value2"}, flag.Values()) - - // Modify the original slice - this WILL affect the flag's internal state - // because Set doesn't create a defensive copy in the current implementation - setValues[0] = "modified" - - // Flag values will reflect the modification - assert.Equal(t, []string{"modified", "value2"}, flag.Values()) -}