Merge pull request #71 from seznam/rudo-fix-stringmap-merge

fix: Make StringMap.Merge, StringMap.Without always return a new map, as documented.

CHANGELOG.md

@@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+### Fixed
+
+- [#71](https://github.com/seznam/slo-exporter/pull/71) Fix corner cases in StringMap.Merge(), StringMap.Without()
+
 ## [v6.9.0] 2021-07-14
 ### Added
 - [#60](https://github.com/seznam/slo-exporter/pull/60) New module eventMetadataRenamer

pkg/stringmap/stringmap.go

@@ -26,7 +26,7 @@ func NewFromLabels(labels labels.Labels) StringMap {
 
 type StringMap map[string]string
 
-// Copy returns new StringMap as a copy of the original.
+// Copy returns a new non-nil StringMap with a copy of the original.
 func (m StringMap) Copy() StringMap {
 	copied := StringMap{}
 	for k, v := range m {
@@ -35,11 +35,8 @@ func (m StringMap) Copy() StringMap {
 	return copied
 }
 
-// Merge returns new StringMap from the original one with all values from the other merged in. The other StringMap overrides original StringMap keys.
+// Merge returns a new non-nil StringMap from the original one with all values from the other merged in. The other StringMap overrides original StringMap keys.
 func (m StringMap) Merge(other StringMap) StringMap {
-	if m == nil {
-		return other
-	}
 	merged := m.Copy()
 	for k, v := range other {
 		merged[k] = v
@@ -152,21 +149,16 @@ func (m StringMap) Select(keys []string) StringMap {
 	return selected
 }
 
-// Without returns new StringMap with without specified keys from the original StringMap.
+// Without returns a new non-nil StringMap with without specified keys from the original StringMap.
 func (m StringMap) Without(keys []string) StringMap {
-	if m == nil {
-		return nil
-	}
-	if len(keys) == 0 {
-		return m
+	result := m.Copy()
+	if len(result) == 0 {
+		return result
 	}
-	other := m.Copy()
 	for _, key := range keys {
-		if _, ok := other[key]; ok {
-			delete(other, key)
-		}
+		delete(result, key)
 	}
-	return other
+	return result
 }
 
 // AsPrometheusLabels converts the stringmap to prometheus labels as is.

pkg/stringmap/stringmap_test.go

@@ -1,9 +1,11 @@
 package stringmap
 
 import (
+	"fmt"
 	"github.com/prometheus/common/model"
 	"github.com/prometheus/prometheus/pkg/labels"
 	"github.com/stretchr/testify/assert"
+	"reflect"
 	"testing"
 )
 
@@ -42,10 +44,21 @@ func TestStringMap_Merge(t *testing.T) {
 		{a: StringMap{}, b: StringMap{}, result: StringMap{}},
 		{a: nil, b: StringMap{"a": "1"}, result: StringMap{"a": "1"}},
 		{a: StringMap{"a": "1"}, b: nil, result: StringMap{"a": "1"}},
+		{a: nil, b: nil, result: StringMap{}},
 	}
 
-	for _, tc := range testCases {
-		assert.Equal(t, tc.result, tc.a.Merge(tc.b))
+	for i, tc := range testCases {
+		t.Run(fmt.Sprintf("testCases[%d]", i), func(t *testing.T) {
+			merged := tc.a.Merge(tc.b)
+			assert.Equal(t, tc.result, merged)
+			assert.NotNil(t, merged)
+			for name, v := range map[string]StringMap{"a": tc.a, "b": tc.b} {
+				if v != nil {
+					assert.NotEqualf(t, reflect.ValueOf(merged).Pointer(), reflect.ValueOf(v).Pointer(),
+						`"merged" and "tc.%s" are the same object`, name)
+				}
+			}
+		})
 	}
 }
 
@@ -145,13 +158,22 @@ func TestStringMap_Without(t *testing.T) {
 		{a: StringMap{"a": "1", "b": "2"}, b: []string{"b"}, result: StringMap{"a": "1"}},
 		{a: StringMap{"a": "2"}, b: []string{"a"}, result: StringMap{}},
 		{a: StringMap{"a": "1"}, b: []string{}, result: StringMap{"a": "1"}},
+		{a: StringMap{"a": "1"}, b: []string{"b"}, result: StringMap{"a": "1"}},
 		{a: StringMap{}, b: []string{}, result: StringMap{}},
-		{a: nil, b: []string{"A"}, result: nil},
+		{a: nil, b: []string{"A"}, result: StringMap{}},
+		{a: nil, b: nil, result: StringMap{}},
 		{a: StringMap{"a": "1"}, b: nil, result: StringMap{"a": "1"}},
 	}
 
-	for _, tc := range testCases {
-		assert.Equal(t, tc.result, tc.a.Without(tc.b), tc)
+	for i, tc := range testCases {
+		t.Run(fmt.Sprintf("testCases[%d]", i), func(t *testing.T) {
+			got := tc.a.Without(tc.b)
+			assert.Equal(t, tc.result, got)
+			assert.NotNil(t, got)
+			if tc.a != nil {
+				assert.NotEqual(t, reflect.ValueOf(got).Pointer(), reflect.ValueOf(tc.a).Pointer(), "got the same map")
+			}
+		})
 	}
 }