From c86de166ec4e613c3d366512b2d8e2302967d9a2 Mon Sep 17 00:00:00 2001
From: Carolyn Van Slyck <me@carolynvanslyck.com>
Date: Wed, 26 Apr 2023 16:35:07 -0500
Subject: [PATCH] Add ArrayEncodedMap

It's a common pattern in Porter to represent a map of items, with a unique key, as a list in the porter.yaml or a file representation of the resource because the UX for autocomplete is better. We can't change that now, parameters and credentials for example will always be a list, but we can use a map in Porter's code so that it's easier to work with.

I've added ArrayEncodedMap, a data structure that reads in a list and stores it in-memory as a map:

- name: a
  value: stuff
- name: b
  value: things

We can use this as a base data structure initially for Parameter and Credential Sets, storing the mapping of the parameter/credential name to the secret resolution strategy. Over time, it may make sense to use it for the other data structures in porter.yaml as well.

It has support for iterating as a map, or as a sorted list of values. The sorted list is good for unit tests, and anytime we need the output to be stable. The map isn't exposed directly and I have a funky "best you can do with Go" iterator to help keep our for loops looking pretty normal.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
---
 pkg/encoding/array_encoded_map.go             | 226 ++++++++++++++++
 pkg/encoding/array_encoded_map_test.go        | 243 ++++++++++++++++++
 pkg/encoding/testdata/array-empty.json        |   1 +
 .../testdata/array-with-duplicates.yaml       |   6 +
 pkg/encoding/testdata/array.json              |  10 +
 pkg/encoding/testdata/array.yaml              |   4 +
 pkg/encoding/testdata/nested-array.json       |  12 +
 pkg/encoding/testdata/nested-array.yaml       |   5 +
 8 files changed, 507 insertions(+)
 create mode 100644 pkg/encoding/array_encoded_map.go
 create mode 100644 pkg/encoding/array_encoded_map_test.go
 create mode 100644 pkg/encoding/testdata/array-empty.json
 create mode 100644 pkg/encoding/testdata/array-with-duplicates.yaml
 create mode 100644 pkg/encoding/testdata/array.json
 create mode 100644 pkg/encoding/testdata/array.yaml
 create mode 100644 pkg/encoding/testdata/nested-array.json
 create mode 100644 pkg/encoding/testdata/nested-array.yaml

diff --git a/pkg/encoding/array_encoded_map.go b/pkg/encoding/array_encoded_map.go
new file mode 100644
index 000000000..b124b3da5
--- /dev/null
+++ b/pkg/encoding/array_encoded_map.go
@@ -0,0 +1,226 @@
+package encoding
+
+import (
+	"encoding/json"
+	"fmt"
+	"sort"
+
+	"gopkg.in/yaml.v3"
+)
+
+// MapElement is the in-memory representation of the item when stored in a map.
+type MapElement interface {
+	// ToArrayEntry converts to the representation of the item when stored in an
+	// array.
+	ToArrayEntry(key string) ArrayElement
+}
+
+// ArrayElement is the representation of the item when stored in an array, and
+// includes the key under which the element was stored in the original map.
+type ArrayElement interface {
+	// GetKey returns the unique item key.
+	GetKey() string
+
+	// ToMapEntry converts to the representation of the item when stored in a map.
+	ToMapEntry() MapElement
+}
+
+// ArrayEncodedMap is a map that is represented as an array when marshaled to json/yaml.
+// MapElement is the type of the elements when stored in a map and ArrayElement is the type of the elements when stored in an array.
+type ArrayEncodedMap[T MapElement, K ArrayElement] struct {
+	items map[string]T
+}
+
+// NewArrayEncodedMap initializes an empty ArrayEncodedMap.
+func NewArrayEncodedMap[T MapElement, K ArrayElement]() ArrayEncodedMap[T, K] {
+	return MakeArrayEncodedMap[T, K](0)
+}
+
+// MakeArrayEncodedMap allocates memory for the specified number of elements.
+func MakeArrayEncodedMap[T MapElement, K ArrayElement](len int) ArrayEncodedMap[T, K] {
+	return ArrayEncodedMap[T, K]{
+		items: make(map[string]T, len),
+	}
+}
+
+// Len returns the number of items.
+func (m *ArrayEncodedMap[T, K]) Len() int {
+	if m == nil {
+		return 0
+	}
+	return len(m.items)
+}
+
+// Items returns a copy of the items, and is intended for use with the range
+// operator.
+// Use ItemsUnsafe() to directly manipulate the backing items map.
+func (m *ArrayEncodedMap[T, K]) Items() map[string]T {
+	if m == nil {
+		return nil
+	}
+
+	result := make(map[string]T, len(m.items))
+	for k, v := range m.items {
+		result[k] = v
+	}
+	return result
+}
+
+// ItemsSorted returns a copy of the items, in a sorted array, and is intended
+// for using with serialization and consistently ranging over the items, in tests
+// or printing output to the console.
+func (m *ArrayEncodedMap[T, K]) ItemsSorted() []K {
+	if m == nil {
+		return nil
+	}
+
+	result := make([]K, len(m.items))
+	i := 0
+	for k, v := range m.items {
+		// I can't figure out how to constrain T such that ToArrayEntry returns K, so I'm doing a cast
+		result[i] = v.ToArrayEntry(k).(K)
+		i++
+	}
+	sort.SliceStable(result, func(i, j int) bool {
+		return result[i].GetKey() < result[j].GetKey()
+	})
+
+	return result
+}
+
+// ItemsUnsafe returns the backing items map. It is intended for optimizing
+// memory usage when iterating over the items to convert them into alternative
+// representations.
+func (m *ArrayEncodedMap[T, K]) ItemsUnsafe() *map[string]T {
+	if m == nil {
+		return nil
+	}
+
+	if m.items == nil {
+		m.items = make(map[string]T)
+	}
+
+	return &m.items
+}
+
+// Get returns the specified element by its key.
+func (m *ArrayEncodedMap[T, K]) Get(key string) (T, bool) {
+	if m == nil {
+		return *new(T), false
+	}
+
+	entry, ok := m.items[key]
+	return entry, ok
+}
+
+// Set the specified element by its key, overwriting previous values.
+func (m *ArrayEncodedMap[T, K]) Set(key string, entry T) {
+	if m.items == nil {
+		m.items = make(map[string]T, 1)
+	}
+
+	m.items[key] = entry
+}
+
+// Remove the specified element by its key.
+func (m *ArrayEncodedMap[T, K]) Remove(key string) {
+	if m == nil {
+		return
+	}
+	delete(m.items, key)
+}
+
+// MarshalRaw is the common Marshal implementation between YAML and JSON.
+func (m *ArrayEncodedMap[T, K]) MarshalRaw() interface{} {
+	if m == nil {
+		return nil
+	}
+
+	var raw []ArrayElement
+	if m.items == nil {
+		return raw
+	}
+
+	raw = make([]ArrayElement, 0, len(m.items))
+	for k, v := range m.items {
+		raw = append(raw, v.ToArrayEntry(k))
+	}
+	sort.SliceStable(raw, func(i, j int) bool {
+		return raw[i].GetKey() < raw[j].GetKey()
+	})
+	return raw
+}
+
+// UnmarshalRaw is the common Marshal implementation between YAML and JSON.
+func (m *ArrayEncodedMap[T, K]) UnmarshalRaw(raw []K) error {
+	if m == nil {
+		*m = ArrayEncodedMap[T, K]{}
+	}
+
+	m.items = make(map[string]T, len(raw))
+	for _, rawItem := range raw {
+		if _, hasKey := m.items[rawItem.GetKey()]; hasKey {
+			return fmt.Errorf("cannot unmarshal source map: duplicate key found '%s'", rawItem.GetKey())
+		}
+		item := rawItem.ToMapEntry()
+		typedItem, ok := item.(T)
+		if !ok {
+			return fmt.Errorf("invalid ArrayEncodedMap generic types, ArrayElement %T returned a %T from ToMapEntry(), when it should return %T", rawItem, item, *new(T))
+		}
+		m.items[rawItem.GetKey()] = typedItem
+	}
+	return nil
+}
+
+// MarshalJSON marshals the items to JSON.
+func (m *ArrayEncodedMap[T, K]) MarshalJSON() ([]byte, error) {
+	raw := m.MarshalRaw()
+	return json.Marshal(raw)
+}
+
+// UnmarshalJSON unmarshals the items in the specified JSON.
+func (m *ArrayEncodedMap[T, K]) UnmarshalJSON(data []byte) error {
+	var raw []K
+	err := json.Unmarshal(data, &raw)
+	if err != nil {
+		return err
+	}
+	return m.UnmarshalRaw(raw)
+}
+
+// MarshalYAML marshals the items to YAML.
+func (m *ArrayEncodedMap[T, K]) MarshalYAML() (interface{}, error) {
+	if m == nil {
+		return nil, nil
+	}
+	return m.MarshalRaw(), nil
+}
+
+// UnmarshalYAML unmarshals the items in the specified YAML.
+func (m *ArrayEncodedMap[T, K]) UnmarshalYAML(value *yaml.Node) error {
+	var raw []K
+	if err := value.Decode(&raw); err != nil {
+		return err
+	}
+	return m.UnmarshalRaw(raw)
+}
+
+// Merge applies the specified values on top of a base set of values. When a
+// key exists in both sets, use the value from the overrides.
+func (m *ArrayEncodedMap[T, K]) Merge(overrides *ArrayEncodedMap[T, K]) *ArrayEncodedMap[T, K] {
+	result := make(map[string]T, m.Len())
+	if m != nil {
+		for k, v := range m.items {
+			result[k] = v
+		}
+	}
+
+	if overrides != nil {
+		// If the name is in the base, overwrite its value with the override provided
+		for k, v := range overrides.items {
+			result[k] = v
+		}
+	}
+
+	return &ArrayEncodedMap[T, K]{items: result}
+}
diff --git a/pkg/encoding/array_encoded_map_test.go b/pkg/encoding/array_encoded_map_test.go
new file mode 100644
index 000000000..98e676d64
--- /dev/null
+++ b/pkg/encoding/array_encoded_map_test.go
@@ -0,0 +1,243 @@
+package encoding
+
+import (
+	"encoding/json"
+	"fmt"
+	"os"
+	"reflect"
+	"testing"
+
+	"get.porter.sh/porter/pkg/test"
+	"get.porter.sh/porter/pkg/yaml"
+	"github.com/carolynvs/aferox"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMakeArrayEncodedMap(t *testing.T) {
+	m := MakeArrayEncodedMap[TestMapEntry, TestArrayEntry](5)
+	require.NotNil(t, m.items, "MakeArrayEncodedMap should initialize the backing items")
+	// go doesn't let us read back out the capacity
+}
+
+func TestNewArrayEncodedMap(t *testing.T) {
+	m := NewArrayEncodedMap[TestMapEntry, TestArrayEntry]()
+	require.NotNil(t, m.items, "NewArrayEncodedMap should initialize the backing items")
+	require.Empty(t, m.items, "NewArrayEncodedMap should create an empty backing items map")
+}
+
+func TestArrayEncodedMap(t *testing.T) {
+	// Validate that we can work with the data like a map
+	wantItems := map[string]TestMapEntry{
+		"x": {Value: "foo"},
+		"y": {Value: "bar"},
+	}
+
+	// initialize the map
+	m := NewArrayEncodedMap[TestMapEntry, TestArrayEntry]()
+	for k, v := range wantItems {
+		m.Set(k, v)
+	}
+
+	// make sure the data was persisted and can be retrieved
+	assert.Equal(t, wantItems, m.items, "incorrect backing items persisted")
+	assert.Equal(t, m.items, m.Items(), "incorrect Items() returned")
+
+	// iterate the sorted items
+	wantSorted := []TestArrayEntry{
+		{Name: "x", Value: "foo"},
+		{Name: "y", Value: "bar"},
+	}
+	assert.Equal(t, wantSorted, m.ItemsSorted(), "incorrect ItemsSorted() returned")
+
+	// Get a specific item
+	gotX, ok := m.Get("x")
+	require.True(t, ok, "Get did not find x")
+	assert.Equal(t, wantItems["x"], gotX, "incorrect x item retrieved")
+
+	// Remove an item
+	m.Remove("y")
+
+	// Get the removed item
+	_, ok = m.Get("y")
+	require.False(t, ok, "Get should not have found 'y' because it was removed")
+}
+
+func TestArrayEncodedMap_ItemsUnsafe(t *testing.T) {
+	t.Run("initialized", func(t *testing.T) {
+		m := NewArrayEncodedMap[TestMapEntry, TestArrayEntry]()
+
+		// Check that they reference the same map
+		backingItems := m.items
+		gotItemsUnsafe := *m.ItemsUnsafe()
+		assert.Equal(t, reflect.ValueOf(backingItems).Pointer(), reflect.ValueOf(gotItemsUnsafe).Pointer(), "expected ItemsUnsafe to return the underlying map")
+	})
+
+	t.Run("uninitialized", func(t *testing.T) {
+		var m ArrayEncodedMap[TestMapEntry, TestArrayEntry]
+		assert.NotNil(t, m.ItemsUnsafe(), "expected ItemsUnsafe to not blow up when ArrayEncodedMap is uninitialized")
+		assert.NotNil(t, m.items, "expected the backing items to be initialized on access when possible")
+
+		// They should still reference the same map
+		backingItems := m.items
+		gotItemsUnsafe := *m.ItemsUnsafe()
+		assert.Equal(t, reflect.ValueOf(backingItems).Pointer(), reflect.ValueOf(gotItemsUnsafe).Pointer(), "expected ItemsUnsafe to return the underlying map")
+	})
+
+	t.Run("nil", func(t *testing.T) {
+		var m *ArrayEncodedMap[TestMapEntry, TestArrayEntry]
+		assert.Nil(t, m.ItemsUnsafe(), "expected ItemsUnsafe to not blow up when ArrayEncodedMap is nil")
+	})
+}
+
+func TestArrayEncodedMap_Merge(t *testing.T) {
+	m := &ArrayEncodedMap[TestMapEntry, TestArrayEntry]{}
+	m.Set("first", TestMapEntry{Value: "base first"})
+	m.Set("second", TestMapEntry{Value: "base second"})
+	m.Set("third", TestMapEntry{Value: "base third"})
+
+	result := m.Merge(&ArrayEncodedMap[TestMapEntry, TestArrayEntry]{})
+	require.Equal(t, 3, result.Len())
+	_, ok := result.Get("fourth")
+	assert.False(t, ok, "fourth should not be present in the base set")
+
+	wantFourth := TestMapEntry{Value: "new fourth"}
+	fourthMap := &ArrayEncodedMap[TestMapEntry, TestArrayEntry]{items: map[string]TestMapEntry{"fourth": wantFourth}}
+	result = m.Merge(fourthMap)
+	require.Equal(t, 4, result.Len())
+	gotFourth, ok := result.Get("fourth")
+	require.True(t, ok, "fourth should be present in the merged set")
+	assert.Equal(t, wantFourth, gotFourth, "incorrect merged value for fourth")
+
+	wantSecond := TestMapEntry{Value: "override second"}
+	secondMap := &ArrayEncodedMap[TestMapEntry, TestArrayEntry]{items: map[string]TestMapEntry{"second": wantSecond}}
+	result = m.Merge(secondMap)
+	require.Equal(t, 3, result.Len())
+	gotSecond, ok := result.Get("second")
+	require.True(t, ok, "second should be present in the merged set")
+	assert.Equal(t, wantSecond, gotSecond, "incorrect merged value for second")
+}
+
+func TestArrayEncodedMap_RoundTripMarshal(t *testing.T) {
+	testcases := []struct {
+		encoding string
+	}{
+		{encoding: "json"},
+		{encoding: "yaml"},
+	}
+
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.encoding, func(t *testing.T) {
+			testFile := fmt.Sprintf("testdata/array.%s", tc.encoding)
+
+			var m *ArrayEncodedMap[TestMapEntry, TestArrayEntry]
+
+			// Unmarshal
+			fx := aferox.NewAferox(".", afero.NewOsFs())
+			err := UnmarshalFile(fx, testFile, &m)
+			require.NoError(t, err, "UnmarshalFile failed")
+
+			// Validate the loaded data
+			require.Equal(t, 2, m.Len(), "unexpected number of items defined")
+			gotA, ok := m.Get("aname")
+			require.True(t, ok, "aname was not defined")
+			wantA := TestMapEntry{Value: "stuff"}
+			assert.Equal(t, wantA, gotA, "unexpected aname defined")
+			gotB, ok := m.Get("bname")
+			require.True(t, ok, "password was not defined")
+			wantB := TestMapEntry{Value: "things"}
+			assert.Equal(t, wantB, gotB, "unexpected bname defined")
+
+			// Marshal
+			data, err := Marshal(tc.encoding, m)
+			require.NoError(t, err, "Marshal failed")
+			test.CompareGoldenFile(t, testFile, string(data))
+		})
+	}
+}
+
+func TestArrayEncodedMap_UnmarshalIntoNil(t *testing.T) {
+	testcases := []struct {
+		encoding string
+	}{
+		{encoding: "json"},
+		{encoding: "yaml"},
+	}
+
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.encoding, func(t *testing.T) {
+			testFile := fmt.Sprintf("testdata/nested-array.%s", tc.encoding)
+
+			var dest testMap
+
+			// Unmarshal
+			fx := aferox.NewAferox(".", afero.NewOsFs())
+			err := UnmarshalFile(fx, testFile, &dest)
+			require.NoError(t, err, "UnmarshalFile failed")
+
+			assert.Equal(t, 2, dest.Items.Len())
+		})
+	}
+}
+
+func TestArrayEncodedMap_Unmarshal_DuplicateKeys(t *testing.T) {
+	data, err := os.ReadFile("testdata/array-with-duplicates.yaml")
+	require.NoError(t, err, "ReadFile failed")
+
+	var l ArrayEncodedMap[TestMapEntry, TestArrayEntry]
+	err = yaml.Unmarshal(data, &l)
+	require.ErrorContains(t, err, "cannot unmarshal source map: duplicate key found 'aname'")
+}
+
+type testMap struct {
+	Items *ArrayEncodedMap[TestMapEntry, TestArrayEntry] `json:"items"`
+}
+
+// check that when we marshal an empty or nil ArrayEncodedMap, it stays null and isn't initialized to an _empty_ ArrayEncodedMap
+// This impacts how it is marshaled later to yaml or json, because we often have fields tagged with omitempty
+// and so it must be null to not be written out.
+func TestArrayEncodedMap_MarshalEmptyToNull(t *testing.T) {
+	testcases := []struct {
+		name string
+		src  testMap
+	}{
+		{name: "nil", src: testMap{Items: nil}},
+		{name: "empty", src: testMap{Items: &ArrayEncodedMap[TestMapEntry, TestArrayEntry]{}}},
+	}
+
+	wantData, err := os.ReadFile("testdata/array-empty.json")
+	require.NoError(t, err, "ReadFile failed")
+
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			gotData, err := json.Marshal(tc.src)
+			require.NoError(t, err, "Marshal failed")
+			require.Equal(t, string(wantData), string(gotData), "empty ArrayEncodedMap should not marshal as empty, but nil so that it works with omitempty")
+		})
+	}
+}
+
+type TestMapEntry struct {
+	Value string `json:"value" yaml:"value"`
+}
+
+func (t TestMapEntry) ToArrayEntry(key string) ArrayElement {
+	return TestArrayEntry{Name: key, Value: t.Value}
+}
+
+type TestArrayEntry struct {
+	Name  string `json:"name" yaml:"name"`
+	Value string `json:"value" yaml:"value"`
+}
+
+func (t TestArrayEntry) ToMapEntry() MapElement {
+	return TestMapEntry{Value: t.Value}
+}
+
+func (t TestArrayEntry) GetKey() string {
+	return t.Name
+}
diff --git a/pkg/encoding/testdata/array-empty.json b/pkg/encoding/testdata/array-empty.json
new file mode 100644
index 000000000..6214a74cd
--- /dev/null
+++ b/pkg/encoding/testdata/array-empty.json
@@ -0,0 +1 @@
+{"items":null}
\ No newline at end of file
diff --git a/pkg/encoding/testdata/array-with-duplicates.yaml b/pkg/encoding/testdata/array-with-duplicates.yaml
new file mode 100644
index 000000000..12809b578
--- /dev/null
+++ b/pkg/encoding/testdata/array-with-duplicates.yaml
@@ -0,0 +1,6 @@
+- name: aname
+  value: stuff
+- name: bname
+  value: things
+- name: aname
+  value: duplicate stuff
diff --git a/pkg/encoding/testdata/array.json b/pkg/encoding/testdata/array.json
new file mode 100644
index 000000000..7a710c734
--- /dev/null
+++ b/pkg/encoding/testdata/array.json
@@ -0,0 +1,10 @@
+[
+  {
+    "name": "aname",
+    "value": "stuff"
+  },
+  {
+    "name": "bname",
+    "value": "things"
+  }
+]
\ No newline at end of file
diff --git a/pkg/encoding/testdata/array.yaml b/pkg/encoding/testdata/array.yaml
new file mode 100644
index 000000000..d3f02f5e3
--- /dev/null
+++ b/pkg/encoding/testdata/array.yaml
@@ -0,0 +1,4 @@
+- name: aname
+  value: stuff
+- name: bname
+  value: things
diff --git a/pkg/encoding/testdata/nested-array.json b/pkg/encoding/testdata/nested-array.json
new file mode 100644
index 000000000..544159d17
--- /dev/null
+++ b/pkg/encoding/testdata/nested-array.json
@@ -0,0 +1,12 @@
+{
+  "items": [
+    {
+      "name": "aname",
+      "value": "stuff"
+    },
+    {
+      "name": "bname",
+      "value": "things"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/pkg/encoding/testdata/nested-array.yaml b/pkg/encoding/testdata/nested-array.yaml
new file mode 100644
index 000000000..da896771e
--- /dev/null
+++ b/pkg/encoding/testdata/nested-array.yaml
@@ -0,0 +1,5 @@
+items:
+  - name: aname
+    value: stuff
+  - name: bname
+    value: things