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
This commit is contained in:
Bjørn Erik Pedersen 2018-10-08 10:25:15 +02:00
parent 8e825ddf5b
commit 31a8bb8c07
12 changed files with 391 additions and 192 deletions

View File

@ -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
}

View File

@ -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)
}
}
}

View File

@ -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)
}

View File

@ -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()
}

View File

@ -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)
}

View File

@ -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, '+')

View File

@ -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) {

View File

@ -15,8 +15,8 @@ package collections
import (
"errors"
"fmt"
"reflect"
"github.com/gohugoio/hugo/common/collections"
)
// Append appends the arguments up to the last one to the slice in the last argument.
@ -33,42 +33,6 @@ func (ns *Namespace) Append(args ...interface{}) (interface{}, error) {
to := args[len(args)-1]
from := args[:len(args)-1]
tov, toIsNil := indirect(reflect.ValueOf(to))
return collections.Append(to, from...)
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 {
// If we get []string []string, we append the from slice to to
fromv := reflect.ValueOf(from[0])
if fromv.Kind() == reflect.Slice {
fromt := reflect.TypeOf(from[0]).Elem()
if tot == fromt {
return reflect.AppendSlice(tov, fromv).Interface(), nil
}
}
}
}
if toIsNil {
return ns.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
}

View File

@ -18,11 +18,11 @@ import (
"reflect"
"testing"
"github.com/alecthomas/assert"
"github.com/gohugoio/hugo/deps"
"github.com/stretchr/testify/require"
)
// Also see tests in common/collection.
func TestAppend(t *testing.T) {
t.Parallel()
@ -36,16 +36,6 @@ func TestAppend(t *testing.T) {
{[]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}},
{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},
{[]string{"a", "b"}, []interface{}{}, false},
@ -73,5 +63,4 @@ func TestAppend(t *testing.T) {
}
}
assert.Len(t, ns.Slice(), 0)
}

View File

@ -136,7 +136,7 @@ func (ns *Namespace) lookupFunc(fname string) (reflect.Value, bool) {
return m, true
}
// indirect is taken from 'text/template/exec.go'
// indirect is borrowed from the Go stdlib: 'text/template/exec.go'
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() {

View File

@ -523,39 +523,7 @@ func (ns *Namespace) Slice(args ...interface{}) interface{} {
return args
}
first := args[0]
firstType := reflect.TypeOf(first)
if firstType == nil {
return args
}
if g, ok := first.(collections.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()
return collections.Slice(args...)
}
type intersector struct {

View File

@ -25,7 +25,6 @@ import (
"testing"
"time"
"github.com/gohugoio/hugo/common/collections"
"github.com/gohugoio/hugo/config"
"github.com/gohugoio/hugo/deps"
"github.com/gohugoio/hugo/helpers"
@ -644,83 +643,7 @@ func TestShuffleRandomising(t *testing.T) {
}
}
var _ collections.Slicer = (*tstSlicer)(nil)
var _ collections.Slicer = (*tstSlicerIn1)(nil)
var _ collections.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
// Also see tests in commons/collection.
func TestSlice(t *testing.T) {
t.Parallel()
@ -731,14 +654,10 @@ func TestSlice(t *testing.T) {
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{}{tstNoStringer{}}, []tstNoStringer{tstNoStringer{}}},
{[]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)