From de29c62e72d35ded51f94b949ba0f9830cccac01 Mon Sep 17 00:00:00 2001 From: Rudolf Thomas Date: Thu, 26 Aug 2021 14:09:10 +0200 Subject: [PATCH 1/3] fix: Make StringMap.Merge always return a new map, as documented. Previously, StringMap.Merge(nil, x) would return x, not a new map. Improve the tests to catch this. While at it, clarify and fix some of the docstrings. --- pkg/stringmap/stringmap.go | 7 ++----- pkg/stringmap/stringmap_test.go | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/pkg/stringmap/stringmap.go b/pkg/stringmap/stringmap.go index d4c665f..81b655b 100644 --- a/pkg/stringmap/stringmap.go +++ b/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 diff --git a/pkg/stringmap/stringmap_test.go b/pkg/stringmap/stringmap_test.go index 1e7b12f..648acdf 100644 --- a/pkg/stringmap/stringmap_test.go +++ b/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) + } + } + }) } } From c26b0d75bf3f44413c76c1c3932dff60186f584e Mon Sep 17 00:00:00 2001 From: Rudolf Thomas Date: Thu, 26 Aug 2021 14:16:13 +0200 Subject: [PATCH 2/3] chore: Update CHANGELOG.md --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a850b99..afd9df5 100644 --- a/CHANGELOG.md +++ b/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 a corner case in StringMap.Merge() + ## [v6.9.0] 2021-07-14 ### Added - [#60](https://github.com/seznam/slo-exporter/pull/60) New module eventMetadataRenamer From efccb645db69cadc3f5746349d4f544069246797 Mon Sep 17 00:00:00 2001 From: Rudolf Thomas Date: Thu, 26 Aug 2021 15:13:02 +0200 Subject: [PATCH 3/3] fix: Make StringMap.Without() return a new map. --- CHANGELOG.md | 2 +- pkg/stringmap/stringmap.go | 17 ++++++----------- pkg/stringmap/stringmap_test.go | 15 ++++++++++++--- 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afd9df5..9f26690 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed -- [#71](https://github.com/seznam/slo-exporter/pull/71) Fix a corner case in StringMap.Merge() +- [#71](https://github.com/seznam/slo-exporter/pull/71) Fix corner cases in StringMap.Merge(), StringMap.Without() ## [v6.9.0] 2021-07-14 ### Added diff --git a/pkg/stringmap/stringmap.go b/pkg/stringmap/stringmap.go index 81b655b..48725c9 100644 --- a/pkg/stringmap/stringmap.go +++ b/pkg/stringmap/stringmap.go @@ -149,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 + result := m.Copy() + if len(result) == 0 { + return result } - if len(keys) == 0 { - return m - } - 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. diff --git a/pkg/stringmap/stringmap_test.go b/pkg/stringmap/stringmap_test.go index 648acdf..b9ecd33 100644 --- a/pkg/stringmap/stringmap_test.go +++ b/pkg/stringmap/stringmap_test.go @@ -158,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") + } + }) } }