diff options
author | Bjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com> | 2018-10-08 10:25:15 +0200 |
---|---|---|
committer | Bjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com> | 2018-10-08 12:30:50 +0200 |
commit | 31a8bb8c071c6f2ca8cbd73057912932a1e7e943 (patch) | |
tree | 698f8d82cf1dc2bb6c56ba3a615ef4b441361c6a /common | |
parent | 8e825ddf5b31ce5fe570c78ac7a78c8056fb60f9 (diff) |
common/maps: Improve append in Scratch
This commit consolidates the reflective collections handling in `.Scratch` vs the `tpl` package so they use the same code paths.
This commit also adds support for a corner case where a typed slice is appended to a nil or empty `[]interface{}`.
Fixes #5275
Diffstat (limited to 'common')
-rw-r--r-- | common/collections/append.go | 83 | ||||
-rw-r--r-- | common/collections/append_test.go | 70 | ||||
-rw-r--r-- | common/collections/collections.go | 7 | ||||
-rw-r--r-- | common/collections/slice.go | 66 | ||||
-rw-r--r-- | common/collections/slice_test.go | 125 | ||||
-rw-r--r-- | common/maps/scratch.go | 11 | ||||
-rw-r--r-- | common/maps/scratch_test.go | 47 |
7 files changed, 384 insertions, 25 deletions
diff --git a/common/collections/append.go b/common/collections/append.go new file mode 100644 index 000000000..e1008843b --- /dev/null +++ b/common/collections/append.go @@ -0,0 +1,83 @@ +// Copyright 2018 The Hugo Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package collections + +import ( + "fmt" + "reflect" +) + +// Append appends from to a slice to and returns the resulting slice. +// If lenght of from is one and the only element is a slice of same type as to, +// it will be appended. +func Append(to interface{}, from ...interface{}) (interface{}, error) { + tov, toIsNil := indirect(reflect.ValueOf(to)) + + toIsNil = toIsNil || to == nil + var tot reflect.Type + + if !toIsNil { + if tov.Kind() != reflect.Slice { + return nil, fmt.Errorf("expected a slice, got %T", to) + } + + tot = tov.Type().Elem() + toIsNil = tov.Len() == 0 + + if len(from) == 1 { + fromv := reflect.ValueOf(from[0]) + if fromv.Kind() == reflect.Slice { + if toIsNil { + // If we get nil []string, we just return the []string + return from[0], nil + } + + fromt := reflect.TypeOf(from[0]).Elem() + + // If we get []string []string, we append the from slice to to + if tot == fromt { + return reflect.AppendSlice(tov, fromv).Interface(), nil + } + } + } + } + + if toIsNil { + return Slice(from...), nil + } + + for _, f := range from { + fv := reflect.ValueOf(f) + if tot != fv.Type() { + return nil, fmt.Errorf("append element type mismatch: expected %v, got %v", tot, fv.Type()) + } + tov = reflect.Append(tov, fv) + } + + return tov.Interface(), nil +} + +// indirect is borrowed from the Go stdlib: 'text/template/exec.go' +// TODO(bep) consolidate +func indirect(v reflect.Value) (rv reflect.Value, isNil bool) { + for ; v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface; v = v.Elem() { + if v.IsNil() { + return v, true + } + if v.Kind() == reflect.Interface && v.NumMethod() > 0 { + break + } + } + return v, false +} diff --git a/common/collections/append_test.go b/common/collections/append_test.go new file mode 100644 index 000000000..e3361fb26 --- /dev/null +++ b/common/collections/append_test.go @@ -0,0 +1,70 @@ +// Copyright 2018 The Hugo Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package collections + +import ( + "fmt" + "reflect" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestAppend(t *testing.T) { + t.Parallel() + + for i, test := range []struct { + start interface{} + addend []interface{} + expected interface{} + }{ + {[]string{"a", "b"}, []interface{}{"c"}, []string{"a", "b", "c"}}, + {[]string{"a", "b"}, []interface{}{"c", "d", "e"}, []string{"a", "b", "c", "d", "e"}}, + {[]string{"a", "b"}, []interface{}{[]string{"c", "d", "e"}}, []string{"a", "b", "c", "d", "e"}}, + {nil, []interface{}{"a", "b"}, []string{"a", "b"}}, + {nil, []interface{}{nil}, []interface{}{nil}}, + {[]interface{}{}, []interface{}{[]string{"c", "d", "e"}}, []string{"c", "d", "e"}}, + {tstSlicers{&tstSlicer{"a"}, &tstSlicer{"b"}}, + []interface{}{&tstSlicer{"c"}}, + tstSlicers{&tstSlicer{"a"}, &tstSlicer{"b"}, &tstSlicer{"c"}}}, + {&tstSlicers{&tstSlicer{"a"}, &tstSlicer{"b"}}, + []interface{}{&tstSlicer{"c"}}, + tstSlicers{&tstSlicer{"a"}, + &tstSlicer{"b"}, + &tstSlicer{"c"}}}, + // Errors + {"", []interface{}{[]string{"a", "b"}}, false}, + // No string concatenation. + {"ab", + []interface{}{"c"}, + false}, + } { + + errMsg := fmt.Sprintf("[%d]", i) + + result, err := Append(test.start, test.addend...) + + if b, ok := test.expected.(bool); ok && !b { + require.Error(t, err, errMsg) + continue + } + + require.NoError(t, err, errMsg) + + if !reflect.DeepEqual(test.expected, result) { + t.Fatalf("%s got\n%T: %v\nexpected\n%T: %v", errMsg, result, result, test.expected, test.expected) + } + } + +} diff --git a/common/collections/collections.go b/common/collections/collections.go index f2dd3071d..bb47c8acc 100644 --- a/common/collections/collections.go +++ b/common/collections/collections.go @@ -19,10 +19,3 @@ package collections type Grouper interface { Group(key interface{}, items interface{}) (interface{}, error) } - -// Slicer definse a very generic way to create a typed slice. This is used -// in collections.Slice template func to get types such as Pages, PageGroups etc. -// instead of the less useful []interface{}. -type Slicer interface { - Slice(items interface{}) (interface{}, error) -} diff --git a/common/collections/slice.go b/common/collections/slice.go new file mode 100644 index 000000000..380d3d329 --- /dev/null +++ b/common/collections/slice.go @@ -0,0 +1,66 @@ +// Copyright 2018 The Hugo Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package collections + +import ( + "reflect" +) + +// Slicer definse a very generic way to create a typed slice. This is used +// in collections.Slice template func to get types such as Pages, PageGroups etc. +// instead of the less useful []interface{}. +type Slicer interface { + Slice(items interface{}) (interface{}, error) +} + +// Slice returns a slice of all passed arguments. +func Slice(args ...interface{}) interface{} { + if len(args) == 0 { + return args + } + + first := args[0] + firstType := reflect.TypeOf(first) + + if firstType == nil { + return args + } + + if g, ok := first.(Slicer); ok { + v, err := g.Slice(args) + if err == nil { + return v + } + + // If Slice fails, the items are not of the same type and + // []interface{} is the best we can do. + return args + } + + if len(args) > 1 { + // This can be a mix of types. + for i := 1; i < len(args); i++ { + if firstType != reflect.TypeOf(args[i]) { + // []interface{} is the best we can do + return args + } + } + } + + slice := reflect.MakeSlice(reflect.SliceOf(firstType), len(args), len(args)) + for i, arg := range args { + slice.Index(i).Set(reflect.ValueOf(arg)) + } + return slice.Interface() +} diff --git a/common/collections/slice_test.go b/common/collections/slice_test.go new file mode 100644 index 000000000..1103e2fea --- /dev/null +++ b/common/collections/slice_test.go @@ -0,0 +1,125 @@ +// Copyright 2018 The Hugo Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package collections + +import ( + "errors" + "fmt" + "testing" + + "github.com/alecthomas/assert" +) + +var _ Slicer = (*tstSlicer)(nil) +var _ Slicer = (*tstSlicerIn1)(nil) +var _ Slicer = (*tstSlicerIn2)(nil) +var _ testSlicerInterface = (*tstSlicerIn1)(nil) +var _ testSlicerInterface = (*tstSlicerIn1)(nil) + +type testSlicerInterface interface { + Name() string +} + +type testSlicerInterfaces []testSlicerInterface + +type tstSlicerIn1 struct { + name string +} + +type tstSlicerIn2 struct { + name string +} + +type tstSlicer struct { + name string +} + +func (p *tstSlicerIn1) Slice(in interface{}) (interface{}, error) { + items := in.([]interface{}) + result := make(testSlicerInterfaces, len(items)) + for i, v := range items { + switch vv := v.(type) { + case testSlicerInterface: + result[i] = vv + default: + return nil, errors.New("invalid type") + } + + } + return result, nil +} + +func (p *tstSlicerIn2) Slice(in interface{}) (interface{}, error) { + items := in.([]interface{}) + result := make(testSlicerInterfaces, len(items)) + for i, v := range items { + switch vv := v.(type) { + case testSlicerInterface: + result[i] = vv + default: + return nil, errors.New("invalid type") + } + } + return result, nil +} + +func (p *tstSlicerIn1) Name() string { + return p.Name() +} + +func (p *tstSlicerIn2) Name() string { + return p.Name() +} + +func (p *tstSlicer) Slice(in interface{}) (interface{}, error) { + items := in.([]interface{}) + result := make(tstSlicers, len(items)) + for i, v := range items { + switch vv := v.(type) { + case *tstSlicer: + result[i] = vv + default: + return nil, errors.New("invalid type") + } + } + return result, nil +} + +type tstSlicers []*tstSlicer + +func TestSlice(t *testing.T) { + t.Parallel() + + for i, test := range []struct { + args []interface{} + expected interface{} + }{ + {[]interface{}{"a", "b"}, []string{"a", "b"}}, + {[]interface{}{&tstSlicer{"a"}, &tstSlicer{"b"}}, tstSlicers{&tstSlicer{"a"}, &tstSlicer{"b"}}}, + {[]interface{}{&tstSlicer{"a"}, "b"}, []interface{}{&tstSlicer{"a"}, "b"}}, + {[]interface{}{}, []interface{}{}}, + {[]interface{}{nil}, []interface{}{nil}}, + {[]interface{}{5, "b"}, []interface{}{5, "b"}}, + {[]interface{}{&tstSlicerIn1{"a"}, &tstSlicerIn2{"b"}}, testSlicerInterfaces{&tstSlicerIn1{"a"}, &tstSlicerIn2{"b"}}}, + {[]interface{}{&tstSlicerIn1{"a"}, &tstSlicer{"b"}}, []interface{}{&tstSlicerIn1{"a"}, &tstSlicer{"b"}}}, + } { + errMsg := fmt.Sprintf("[%d] %v", i, test.args) + + result := Slice(test.args...) + + assert.Equal(t, test.expected, result, errMsg) + } + + assert.Len(t, Slice(), 0) +} diff --git a/common/maps/scratch.go b/common/maps/scratch.go index 4b062d139..2972e2022 100644 --- a/common/maps/scratch.go +++ b/common/maps/scratch.go @@ -18,6 +18,7 @@ import ( "sort" "sync" + "github.com/gohugoio/hugo/common/collections" "github.com/gohugoio/hugo/common/math" ) @@ -40,14 +41,12 @@ func (c *Scratch) Add(key string, newAddend interface{}) (string, error) { if found { var err error - addendV := reflect.ValueOf(existingAddend) + addendV := reflect.TypeOf(existingAddend) if addendV.Kind() == reflect.Slice || addendV.Kind() == reflect.Array { - nav := reflect.ValueOf(newAddend) - if nav.Kind() == reflect.Slice || nav.Kind() == reflect.Array { - newVal = reflect.AppendSlice(addendV, nav).Interface() - } else { - newVal = reflect.Append(addendV, nav).Interface() + newVal, err = collections.Append(existingAddend, newAddend) + if err != nil { + return "", err } } else { newVal, err = math.DoArithmetic(existingAddend, newAddend, '+') diff --git a/common/maps/scratch_test.go b/common/maps/scratch_test.go index 8397ba830..bf37d79df 100644 --- a/common/maps/scratch_test.go +++ b/common/maps/scratch_test.go @@ -18,29 +18,31 @@ import ( "sync" "testing" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestScratchAdd(t *testing.T) { t.Parallel() + assert := require.New(t) + scratch := NewScratch() scratch.Add("int1", 10) scratch.Add("int1", 20) scratch.Add("int2", 20) - assert.Equal(t, int64(30), scratch.Get("int1")) - assert.Equal(t, 20, scratch.Get("int2")) + assert.Equal(int64(30), scratch.Get("int1")) + assert.Equal(20, scratch.Get("int2")) scratch.Add("float1", float64(10.5)) scratch.Add("float1", float64(20.1)) - assert.Equal(t, float64(30.6), scratch.Get("float1")) + assert.Equal(float64(30.6), scratch.Get("float1")) scratch.Add("string1", "Hello ") scratch.Add("string1", "big ") scratch.Add("string1", "World!") - assert.Equal(t, "Hello big World!", scratch.Get("string1")) + assert.Equal("Hello big World!", scratch.Get("string1")) scratch.Add("scratch", scratch) _, err := scratch.Add("scratch", scratch) @@ -53,12 +55,14 @@ func TestScratchAdd(t *testing.T) { func TestScratchAddSlice(t *testing.T) { t.Parallel() + assert := require.New(t) + scratch := NewScratch() _, err := scratch.Add("intSlice", []int{1, 2}) - assert.Nil(t, err) + assert.NoError(err) _, err = scratch.Add("intSlice", 3) - assert.Nil(t, err) + assert.NoError(err) sl := scratch.Get("intSlice") expected := []int{1, 2, 3} @@ -66,10 +70,9 @@ func TestScratchAddSlice(t *testing.T) { if !reflect.DeepEqual(expected, sl) { t.Errorf("Slice difference, go %q expected %q", sl, expected) } - _, err = scratch.Add("intSlice", []int{4, 5}) - assert.Nil(t, err) + assert.NoError(err) sl = scratch.Get("intSlice") expected = []int{1, 2, 3, 4, 5} @@ -77,29 +80,47 @@ func TestScratchAddSlice(t *testing.T) { if !reflect.DeepEqual(expected, sl) { t.Errorf("Slice difference, go %q expected %q", sl, expected) } +} + +// https://github.com/gohugoio/hugo/issues/5275 +func TestScratchAddTypedSliceToInterfaceSlice(t *testing.T) { + t.Parallel() + assert := require.New(t) + + scratch := NewScratch() + scratch.Set("slice", []interface{}{}) + + _, err := scratch.Add("slice", []int{1, 2}) + assert.NoError(err) + assert.Equal([]int{1, 2}, scratch.Get("slice")) } func TestScratchSet(t *testing.T) { t.Parallel() + assert := require.New(t) + scratch := NewScratch() scratch.Set("key", "val") - assert.Equal(t, "val", scratch.Get("key")) + assert.Equal("val", scratch.Get("key")) } func TestScratchDelete(t *testing.T) { t.Parallel() + assert := require.New(t) + scratch := NewScratch() scratch.Set("key", "val") scratch.Delete("key") scratch.Add("key", "Lucy Parsons") - assert.Equal(t, "Lucy Parsons", scratch.Get("key")) + assert.Equal("Lucy Parsons", scratch.Get("key")) } // Issue #2005 func TestScratchInParallel(t *testing.T) { var wg sync.WaitGroup scratch := NewScratch() + key := "counter" scratch.Set(key, int64(1)) for i := 1; i <= 10; i++ { @@ -142,13 +163,15 @@ func TestScratchGet(t *testing.T) { func TestScratchSetInMap(t *testing.T) { t.Parallel() + assert := require.New(t) + scratch := NewScratch() scratch.SetInMap("key", "lux", "Lux") scratch.SetInMap("key", "abc", "Abc") scratch.SetInMap("key", "zyx", "Zyx") scratch.SetInMap("key", "abc", "Abc (updated)") scratch.SetInMap("key", "def", "Def") - assert.Equal(t, []interface{}{0: "Abc (updated)", 1: "Def", 2: "Lux", 3: "Zyx"}, scratch.GetSortedMapValues("key")) + assert.Equal([]interface{}{0: "Abc (updated)", 1: "Def", 2: "Lux", 3: "Zyx"}, scratch.GetSortedMapValues("key")) } func TestScratchGetSortedMapValues(t *testing.T) { |