From 46deeac832cddc44731e9a2bcf7993df968c30d6 Mon Sep 17 00:00:00 2001 From: Daniel Gront Date: Mon, 22 Feb 2021 09:46:00 +0200 Subject: [PATCH 1/6] avoid type loss with JSON unmarshal/marshal issue #63 --- go.mod | 1 + go.sum | 4 ++++ koanf.go | 8 ++------ koanf_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ maps/maps.go | 11 ++++++----- mock/mock.yml | 1 + 6 files changed, 60 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index 4fc729a7..c5c22faf 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/hashicorp/hcl v1.0.0 github.com/hashicorp/vault/api v1.0.4 github.com/joho/godotenv v1.3.0 + github.com/mitchellh/copystructure v1.1.1 github.com/mitchellh/mapstructure v1.2.2 github.com/pelletier/go-toml v1.7.0 github.com/rhnvrm/simples3 v0.6.1 diff --git a/go.sum b/go.sum index 7bf89dd8..155d3071 100644 --- a/go.sum +++ b/go.sum @@ -58,6 +58,8 @@ github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaO github.com/mattn/go-isatty v0.0.3/go.mod h1:M+lRXTBqGeGNdLjl/ufCoiOlB5xdOkqRJdNxMWT7Zi4= github.com/mitchellh/cli v1.0.0/go.mod h1:hNIlj7HEI86fIcpObd7a0FcrxTWetlwJDGcceTlRvqc= github.com/mitchellh/copystructure v1.0.0/go.mod h1:SNtv71yrdKgLRyLFxmLdkAbkKEFWgYaq1OVrnRcwhnw= +github.com/mitchellh/copystructure v1.1.1 h1:Bp6x9R1Wn16SIz3OfeDr0b7RnCG2OB66Y7PQyC/cvq4= +github.com/mitchellh/copystructure v1.1.1/go.mod h1:EBArHfARyrSWO/+Wyr9zwEkc6XMFB9XyNgFNmRkZZU4= github.com/mitchellh/go-homedir v1.1.0 h1:lukF9ziXFxDFPkA1vsr5zpc1XuPDn/wFntq5mG+4E0Y= github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0= github.com/mitchellh/go-testing-interface v0.0.0-20171004221916-a61a99592b77/go.mod h1:kRemZodwjscx+RGhAo8eIhFbs2+BFgRtFPeD/KE+zxI= @@ -67,6 +69,8 @@ github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh github.com/mitchellh/mapstructure v1.2.2 h1:dxe5oCinTXiTIcfgmZecdCzPmAJKd46KsCWc35r0TV4= github.com/mitchellh/mapstructure v1.2.2/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= github.com/mitchellh/reflectwalk v1.0.0/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= +github.com/mitchellh/reflectwalk v1.0.1 h1:FVzMWA5RllMAKIdUSC8mdWo3XtwoecrH79BY70sEEpE= +github.com/mitchellh/reflectwalk v1.0.1/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= github.com/oklog/run v1.0.0/go.mod h1:dlhp/R75TPv97u0XWUtDeV/lRKWPKSdTuV0TZvrmrQA= github.com/pascaldekloe/goe v0.1.0/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= github.com/pelletier/go-toml v1.7.0 h1:7utD74fnzVc/cpcyy8sjrlFr5vYpypUixARcHIMIGuI= diff --git a/koanf.go b/koanf.go index 75448cfb..b7e99d31 100644 --- a/koanf.go +++ b/koanf.go @@ -2,8 +2,8 @@ package koanf import ( "bytes" - "encoding/json" "fmt" + "github.com/mitchellh/copystructure" "sort" "strconv" "strings" @@ -300,11 +300,7 @@ func (ko *Koanf) Get(path string) interface{} { return maps.Copy(v) } - // Inefficient, but marshal and unmarshal to create a copy - // of reference types to not expose internal references to slices and maps. - var out interface{} - b, _ := json.Marshal(res) - json.Unmarshal(b, &out) + out, _ := copystructure.Copy(&res) return out } diff --git a/koanf_test.go b/koanf_test.go index 73257bb4..4b05e2fc 100644 --- a/koanf_test.go +++ b/koanf_test.go @@ -614,6 +614,52 @@ func TestMerge(t *testing.T) { assert.Equal(cut1.All(), k2.All(), "conf map mismatch") } +func TestRaw_YamlTypes(t *testing.T) { + var ( + assert = assert.New(t) + k = koanf.New(delim) + ) + + assert.Nil(k.Load(file.Provider(mockYAML), yaml.Parser()), + "error loading file") + raw := k.Raw() + + i, ok := raw["ints"] + assert.True(ok, "ints key does not exist in the map") + + arr, ok := i.([]interface{}) + assert.True(ok, "arr slice is not array of integers") + + for _, integer := range arr { + if _, ok := integer.(int); !ok { + assert.Failf("failure", "%v not an integer but %T", integer, integer) + } + } +} + +func TestRaw_JsonTypes(t *testing.T) { + var ( + assert = assert.New(t) + k = koanf.New(delim) + ) + + assert.Nil(k.Load(file.Provider(mockJSON), json.Parser()), + "error loading file") + raw := k.Raw() + + i, ok := raw["intbools"] + assert.True(ok, "ints key does not exist in the map") + + arr, ok := i.([]interface{}) + assert.True(ok, "arr slice is not array of integers") + + for _, integer := range arr { + if _, ok := integer.(float64); !ok { + assert.Failf("failure", "%v not an integer but %T", integer, integer) + } + } +} + func TestMergeAt(t *testing.T) { var ( assert = assert.New(t) diff --git a/maps/maps.go b/maps/maps.go index 44b68fc3..a192b9f9 100644 --- a/maps/maps.go +++ b/maps/maps.go @@ -4,8 +4,8 @@ package maps import ( - "encoding/json" "fmt" + "github.com/mitchellh/copystructure" "strings" ) @@ -190,10 +190,11 @@ func Search(mp map[string]interface{}, path []string) interface{} { // map[string]interface{} and not map[interface{}]interface{}. // Use IntfaceKeysToStrings() to convert if necessary. func Copy(mp map[string]interface{}) map[string]interface{} { - var out map[string]interface{} - b, _ := json.Marshal(mp) - json.Unmarshal(b, &out) - return out + out, _ := copystructure.Copy(&mp) + if res, ok := out.(*map[string]interface{}); ok { + return *res + } + return map[string]interface{}{} } // IntfaceKeysToStrings recursively converts map[interface{}]interface{} to diff --git a/mock/mock.yml b/mock/mock.yml index 63d0c32d..6143d097 100644 --- a/mock/mock.yml +++ b/mock/mock.yml @@ -69,3 +69,4 @@ strbools: strbool: "1" time: "2019-01-01" duration: "3s" +ints: [1,2,3] From 77c5ac039127164e32a490ebd756717462652a8b Mon Sep 17 00:00:00 2001 From: Daniel Gront Date: Mon, 22 Feb 2021 09:54:31 +0200 Subject: [PATCH 2/6] fix failing unit tests Ints should avoid calling Ints64s to avoid unneeded conversion between []in64 and []int. MergeAt unit test cannot be tested against JSON file since JSON treats numbers as float64 and confmap.Provider/YAML treats them as integers. --- getters.go | 41 +++++++++++++++++++++++++++++++++++------ koanf.go | 3 +++ koanf_test.go | 4 ++-- mock/mock.yml | 1 - 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/getters.go b/getters.go index ff4ffed8..2f94e7ac 100644 --- a/getters.go +++ b/getters.go @@ -36,6 +36,19 @@ func (ko *Koanf) Int64s(path string) []int64 { var out []int64 switch v := o.(type) { + case []int: + out = make([]int64, 0, len(v)) + for _, vi := range v { + i, err := toInt64(vi) + + // On error, return as it's not a valid + // int slice. + if err != nil { + return []int64{} + } + out = append(out, i) + } + return out case []interface{}: out = make([]int64, 0, len(v)) for _, vi := range v { @@ -128,16 +141,31 @@ func (ko *Koanf) MustInt(path string) int { // empty []int slice if the path does not exist or if the value // is not a valid int slice. func (ko *Koanf) Ints(path string) []int { - ints := ko.Int64s(path) - if len(ints) == 0 { + o := ko.Get(path) + if o == nil { return []int{} } - out := make([]int, len(ints)) - for i, v := range ints { - out[i] = int(v) + var out []int + switch v := o.(type) { + case []int: + return v + case []interface{}: + out = make([]int, 0, len(v)) + for _, vi := range v { + i, err := toInt64(vi) + + // On error, return as it's not a valid + // int slice. + if err != nil { + return []int{} + } + out = append(out, int(i)) + } + return out } - return out + + return []int{} } // MustInts returns the []int slice value of a given key path or panics @@ -379,6 +407,7 @@ func (ko *Koanf) Strings(path string) []string { copy(out[:], v[:]) return out } + return []string{} } diff --git a/koanf.go b/koanf.go index b7e99d31..6a53f46b 100644 --- a/koanf.go +++ b/koanf.go @@ -301,6 +301,9 @@ func (ko *Koanf) Get(path string) interface{} { } out, _ := copystructure.Copy(&res) + if ptrOut, ok := out.(*interface{}); ok { + return *ptrOut + } return out } diff --git a/koanf_test.go b/koanf_test.go index 4b05e2fc..1e8925a6 100644 --- a/koanf_test.go +++ b/koanf_test.go @@ -624,7 +624,7 @@ func TestRaw_YamlTypes(t *testing.T) { "error loading file") raw := k.Raw() - i, ok := raw["ints"] + i, ok := raw["intbools"] assert.True(ok, "ints key does not exist in the map") arr, ok := i.([]interface{}) @@ -665,7 +665,7 @@ func TestMergeAt(t *testing.T) { assert = assert.New(t) k = koanf.New(delim) ) - assert.Nil(k.Load(file.Provider(mockJSON), json.Parser()), + assert.Nil(k.Load(file.Provider(mockYAML), yaml.Parser()), "error loading file") // Get expected koanf, and root data diff --git a/mock/mock.yml b/mock/mock.yml index 6143d097..63d0c32d 100644 --- a/mock/mock.yml +++ b/mock/mock.yml @@ -69,4 +69,3 @@ strbools: strbool: "1" time: "2019-01-01" duration: "3s" -ints: [1,2,3] From 3f357751432e4c185496fd622d28628eab739461 Mon Sep 17 00:00:00 2001 From: Rohan Verma Date: Mon, 22 Feb 2021 14:53:10 +0530 Subject: [PATCH 3/6] chore: update Go version for Travis CI This commit adds the latest Go versions supported by the Gimme travis-ci tool build matrix. --- .travis.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.travis.yml b/.travis.yml index e8b36ead..36a39876 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,6 +12,10 @@ os: go: - 1.11.x - 1.12.x + - 1.13.x + - 1.14.x + - 1.15.x + - 1.16.x install: go get -v ./... script: go test -v -cover ./... From 5625550d01ddcd7e2c47b28114b95ab3790e1dd9 Mon Sep 17 00:00:00 2001 From: gront Date: Mon, 22 Feb 2021 12:09:51 +0200 Subject: [PATCH 4/6] add unit tests to cover edge cases JSON treats int array as float64 array, this tests checks that there is no issue between type conversions in Load Merge. --- koanf_test.go | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/koanf_test.go b/koanf_test.go index 1e8925a6..0d4bddea 100644 --- a/koanf_test.go +++ b/koanf_test.go @@ -323,6 +323,58 @@ func TestLoadFileAllKeys(t *testing.T) { } } +func TestLoadMergeYamlJson(t *testing.T) { + var ( + assert = assert.New(t) + k = koanf.New(delim) + ) + + assert.NoError(k.Load(file.Provider(mockYAML), yaml.Parser()), + "error loading file") + // loading json after yaml causes the intbools to be loaded as []float64 + assert.NoError(k.Load(file.Provider(mockJSON), yaml.Parser()), + "error loading file") + + // checking that there is no issues with expecting it to be an []int64 + v := k.Int64s("intbools") + assert.Len(v, 3) + + defer func() { + if err := recover(); err != nil { + assert.Failf("panic", "received panic: %v", err) + } + }() + + v2 := k.MustInt64s("intbools") + assert.Len(v2, 3) +} + +func TestLoadMergeJsonYaml(t *testing.T) { + var ( + assert = assert.New(t) + k = koanf.New(delim) + ) + + assert.NoError(k.Load(file.Provider(mockJSON), yaml.Parser()), + "error loading file") + // loading yaml after json causes the intbools to be loaded as []int after json loaded it with []float64 + assert.NoError(k.Load(file.Provider(mockYAML), yaml.Parser()), + "error loading file") + + // checking that there is no issues with expecting it to be an []float64 + v := k.Float64s("intbools") + assert.Len(v, 3) + + defer func() { + if err := recover(); err != nil { + assert.Failf("panic", "received panic: %v", err) + } + }() + + v2 := k.MustFloat64s("intbools") + assert.Len(v2, 3) +} + func TestWatchFile(t *testing.T) { var ( assert = assert.New(t) From 33a8f097e000dd09d82cb59f8e2127e7acc487b1 Mon Sep 17 00:00:00 2001 From: grount Date: Mon, 22 Feb 2021 17:07:15 +0200 Subject: [PATCH 5/6] update function docs --- maps/maps.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/maps/maps.go b/maps/maps.go index a192b9f9..891447f7 100644 --- a/maps/maps.go +++ b/maps/maps.go @@ -182,9 +182,7 @@ func Search(mp map[string]interface{}, path []string) interface{} { return nil } -// Copy returns a copy of a conf map by doing a JSON marshal+unmarshal -// pass. Inefficient, but creates a true deep copy. There is a side -// effect, that is, all numeric types change to float64. +// Copy returns a deep copy of a conf map. // // It's important to note that all nested maps should be // map[string]interface{} and not map[interface{}]interface{}. From b97eac0e1068dc872f5d6d40c193feac49090797 Mon Sep 17 00:00:00 2001 From: grount Date: Tue, 23 Feb 2021 11:40:52 +0200 Subject: [PATCH 6/6] add missing getters type conversions --- getters.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/getters.go b/getters.go index 2f94e7ac..81a2cebb 100644 --- a/getters.go +++ b/getters.go @@ -36,6 +36,8 @@ func (ko *Koanf) Int64s(path string) []int64 { var out []int64 switch v := o.(type) { + case []int64: + return v case []int: out = make([]int64, 0, len(v)) for _, vi := range v { @@ -150,6 +152,12 @@ func (ko *Koanf) Ints(path string) []int { switch v := o.(type) { case []int: return v + case []int64: + out = make([]int, 0, len(v)) + for _, vi := range v { + out = append(out, int(vi)) + } + return out case []interface{}: out = make([]int, 0, len(v)) for _, vi := range v { @@ -233,6 +241,8 @@ func (ko *Koanf) Float64s(path string) []float64 { var out []float64 switch v := o.(type) { + case []float64: + return v case []interface{}: out = make([]float64, 0, len(v)) for _, vi := range v { @@ -243,7 +253,7 @@ func (ko *Koanf) Float64s(path string) []float64 { if err != nil { return []float64{} } - out = append(out, float64(i)) + out = append(out, i) } return out }