tpl/tplimpl: Fix template truth logic
authorBjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com>
Wed, 6 Mar 2019 08:07:49 +0000 (09:07 +0100)
committerBjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com>
Wed, 6 Mar 2019 21:52:38 +0000 (22:52 +0100)
Before this commit, due to a bug in Go's `text/template` package, this would print different output for typed nil interface values:

```
{{ if .AuthenticatedUser }}User is authenticated!{{ else }}{{ end }}
{{ if not .AuthenticatedUser }}{{ else }}}User is authenticated!{{ end }}
```

This commit works around this by wrapping every `if` and `with` with a custom `getif` template func with truth logic that matches `not`, `and` and `or`.

Those 3 template funcs from Go's stdlib are now pulled into Hugo's source tree and adjusted to support custom zero values, e.g. types that implement `IsZero`.

This means that you can now do:

```
{{ with .Date }}{{ . }}{{ end }}
```

And it would work as expected.

Fixes #5738

common/hreflect/helpers.go [new file with mode: 0644]
common/hreflect/helpers_test.go [new file with mode: 0644]
common/types/types.go
go.sum
tpl/compare/init.go
tpl/compare/truth.go [new file with mode: 0644]
tpl/compare/truth_test.go [new file with mode: 0644]
tpl/tplimpl/template_ast_transformers.go
tpl/tplimpl/template_ast_transformers_test.go

diff --git a/common/hreflect/helpers.go b/common/hreflect/helpers.go
new file mode 100644 (file)
index 0000000..db7b208
--- /dev/null
@@ -0,0 +1,91 @@
+// Copyright 2019 The Hugo Authors. All rights reserved.
+// Some functions in this file (see comments) is based on the Go source code,
+// copyright The Go Authors and  governed by a BSD-style license.
+//
+// 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 hreflect contains reflect helpers.
+package hreflect
+
+import (
+       "reflect"
+
+       "github.com/gohugoio/hugo/common/types"
+)
+
+// IsTruthful returns whether in represents a truthful value.
+// See IsTruthfulValue
+func IsTruthful(in interface{}) bool {
+       switch v := in.(type) {
+       case reflect.Value:
+               return IsTruthfulValue(v)
+       default:
+               return IsTruthfulValue(reflect.ValueOf(in))
+       }
+
+}
+
+var zeroType = reflect.TypeOf((*types.Zeroer)(nil)).Elem()
+
+// IsTruthfulValue returns whether the given value has a meaningful truth value.
+// This is based on template.IsTrue in Go's stdlib, but also considers
+// IsZero and any interface value will be unwrapped before it's considered
+// for truthfulness.
+//
+// Based on:
+// https://github.com/golang/go/blob/178a2c42254166cffed1b25fb1d3c7a5727cada6/src/text/template/exec.go#L306
+func IsTruthfulValue(val reflect.Value) (truth bool) {
+       val = indirectInterface(val)
+
+       if !val.IsValid() {
+               // Something like var x interface{}, never set. It's a form of nil.
+               return
+       }
+
+       if val.Type().Implements(zeroType) {
+               return !val.Interface().(types.Zeroer).IsZero()
+       }
+
+       switch val.Kind() {
+       case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
+               truth = val.Len() > 0
+       case reflect.Bool:
+               truth = val.Bool()
+       case reflect.Complex64, reflect.Complex128:
+               truth = val.Complex() != 0
+       case reflect.Chan, reflect.Func, reflect.Ptr, reflect.Interface:
+               truth = !val.IsNil()
+       case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+               truth = val.Int() != 0
+       case reflect.Float32, reflect.Float64:
+               truth = val.Float() != 0
+       case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
+               truth = val.Uint() != 0
+       case reflect.Struct:
+               truth = true // Struct values are always true.
+       default:
+               return
+       }
+
+       return
+}
+
+// Based on: https://github.com/golang/go/blob/178a2c42254166cffed1b25fb1d3c7a5727cada6/src/text/template/exec.go#L931
+func indirectInterface(v reflect.Value) reflect.Value {
+       if v.Kind() != reflect.Interface {
+               return v
+       }
+       if v.IsNil() {
+               return reflect.Value{}
+       }
+       return v.Elem()
+}
diff --git a/common/hreflect/helpers_test.go b/common/hreflect/helpers_test.go
new file mode 100644 (file)
index 0000000..3c91793
--- /dev/null
@@ -0,0 +1,42 @@
+// Copyright 2019 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 hreflect
+
+import (
+       "reflect"
+       "testing"
+       "time"
+
+       "github.com/stretchr/testify/require"
+)
+
+func TestIsTruthful(t *testing.T) {
+       assert := require.New(t)
+
+       assert.True(IsTruthful(true))
+       assert.False(IsTruthful(false))
+       assert.True(IsTruthful(time.Now()))
+       assert.False(IsTruthful(time.Time{}))
+}
+
+func BenchmarkIsTruthFul(b *testing.B) {
+       v := reflect.ValueOf("Hugo")
+
+       b.ResetTimer()
+       for i := 0; i < b.N; i++ {
+               if !IsTruthfulValue(v) {
+                       b.Fatal("not truthful")
+               }
+       }
+}
index ca74391f8419334f491b5ae2700c93d2dac0907a..95e72d99b6ca3840e3e854759a0c5a0da19ee12d 100644 (file)
@@ -50,3 +50,9 @@ func NewKeyValuesStrings(key string, values ...string) KeyValues {
        }
        return KeyValues{Key: key, Values: iv}
 }
+
+// Zeroer, as implemented by time.Time, will be used by the truth template
+// funcs in Hugo (if, with, not, and, or).
+type Zeroer interface {
+       IsZero() bool
+}
diff --git a/go.sum b/go.sum
index 79d7c1eb0b87c31b428afb6312d2c3828f930b74..0227e3b9cc9e55a150854689195acd32509a3324 100644 (file)
--- a/go.sum
+++ b/go.sum
@@ -76,6 +76,7 @@ github.com/magefile/mage v1.4.0 h1:RI7B1CgnPAuu2O9lWszwya61RLmfL0KCdo+QyyI/Bhk=
 github.com/magefile/mage v1.4.0/go.mod h1:IUDi13rsHje59lecXokTfGX0QIzO45uVPlXnJYsXepA=
 github.com/magiconair/properties v1.8.0 h1:LLgXmsheXeRoUOBOjtwPQCWIYqM/LU1ayDtDePerRcY=
 github.com/magiconair/properties v1.8.0/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
+github.com/markbates/inflect v0.0.0-20171215194931-a12c3aec81a6 h1:LZhVjIISSbj8qLf2qDPP0D8z0uvOWAW5C85ly5mJW6c=
 github.com/markbates/inflect v0.0.0-20171215194931-a12c3aec81a6/go.mod h1:oTeZL2KHA7CUX6X+fovmK9OvIOFuqu0TwdQrZjLTh88=
 github.com/matryer/try v0.0.0-20161228173917-9ac251b645a2/go.mod h1:0KeJpeMD6o+O4hW7qJOT7vyQPKrWmj26uf5wMc/IiIs=
 github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
index f766ef890f90a27444f85c7043461b75cfc7b8b2..6192932036c137570480c54e543e64e9d290b19f 100644 (file)
@@ -71,6 +71,27 @@ func init() {
                        [][2]string{},
                )
 
+               ns.AddMethodMapping(ctx.And,
+                       []string{"and"},
+                       [][2]string{},
+               )
+
+               ns.AddMethodMapping(ctx.Or,
+                       []string{"or"},
+                       [][2]string{},
+               )
+
+               // getif is used internally by Hugo. Do not document.
+               ns.AddMethodMapping(ctx.getIf,
+                       []string{"getif"},
+                       [][2]string{},
+               )
+
+               ns.AddMethodMapping(ctx.Not,
+                       []string{"not"},
+                       [][2]string{},
+               )
+
                ns.AddMethodMapping(ctx.Conditional,
                        []string{"cond"},
                        [][2]string{
diff --git a/tpl/compare/truth.go b/tpl/compare/truth.go
new file mode 100644 (file)
index 0000000..85ee221
--- /dev/null
@@ -0,0 +1,73 @@
+// Copyright 2019 The Hugo Authors. All rights reserved.
+// The functions in this file is based on the Go source code, copyright
+// The Go Authors and  governed by a BSD-style license.
+//
+// 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 compare provides template functions for comparing values.
+package compare
+
+import (
+       "reflect"
+
+       "github.com/gohugoio/hugo/common/hreflect"
+)
+
+// Boolean logic, based on:
+// https://github.com/golang/go/blob/178a2c42254166cffed1b25fb1d3c7a5727cada6/src/text/template/funcs.go#L302
+
+func truth(arg reflect.Value) bool {
+       return hreflect.IsTruthfulValue(arg)
+}
+
+// getIf will return the given arg if it is considered truthful, else an empty string.
+func (*Namespace) getIf(arg reflect.Value) reflect.Value {
+       if truth(arg) {
+               return arg
+       }
+       return reflect.ValueOf("")
+}
+
+// And computes the Boolean AND of its arguments, returning
+// the first false argument it encounters, or the last argument.
+func (*Namespace) And(arg0 reflect.Value, args ...reflect.Value) reflect.Value {
+       if !truth(arg0) {
+               return arg0
+       }
+       for i := range args {
+               arg0 = args[i]
+               if !truth(arg0) {
+                       break
+               }
+       }
+       return arg0
+}
+
+// Or computes the Boolean OR of its arguments, returning
+// the first true argument it encounters, or the last argument.
+func (*Namespace) Or(arg0 reflect.Value, args ...reflect.Value) reflect.Value {
+       if truth(arg0) {
+               return arg0
+       }
+       for i := range args {
+               arg0 = args[i]
+               if truth(arg0) {
+                       break
+               }
+       }
+       return arg0
+}
+
+// Not returns the Boolean negation of its argument.
+func (*Namespace) Not(arg reflect.Value) bool {
+       return !truth(arg)
+}
diff --git a/tpl/compare/truth_test.go b/tpl/compare/truth_test.go
new file mode 100644 (file)
index 0000000..04d8972
--- /dev/null
@@ -0,0 +1,60 @@
+// Copyright 2019 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 compare
+
+import (
+       "reflect"
+       "testing"
+       "time"
+
+       "github.com/gohugoio/hugo/common/hreflect"
+       "github.com/stretchr/testify/require"
+)
+
+func TestTruth(t *testing.T) {
+       n := New()
+
+       truthv, falsev := reflect.ValueOf(time.Now()), reflect.ValueOf(false)
+
+       assertTruth := func(t *testing.T, v reflect.Value, expected bool) {
+               if hreflect.IsTruthfulValue(v) != expected {
+                       t.Fatal("truth mismatch")
+               }
+       }
+
+       t.Run("And", func(t *testing.T) {
+               assertTruth(t, n.And(truthv, truthv), true)
+               assertTruth(t, n.And(truthv, falsev), false)
+
+       })
+
+       t.Run("Or", func(t *testing.T) {
+               assertTruth(t, n.Or(truthv, truthv), true)
+               assertTruth(t, n.Or(falsev, truthv, falsev), true)
+               assertTruth(t, n.Or(falsev, falsev), false)
+       })
+
+       t.Run("Not", func(t *testing.T) {
+               assert := require.New(t)
+               assert.True(n.Not(falsev))
+               assert.False(n.Not(truthv))
+       })
+
+       t.Run("getIf", func(t *testing.T) {
+               assert := require.New(t)
+               assertTruth(t, n.getIf(reflect.ValueOf(nil)), false)
+               s := reflect.ValueOf("Hugo")
+               assert.Equal(s, n.getIf(s))
+       })
+}
index f32b189ffbc7b5a1ec82c8b113af96e9d1addead..e1cfb1aa427b0cde0e25adb23c92f06aa3309b90 100644 (file)
@@ -85,31 +85,59 @@ func applyTemplateTransformers(templ *parse.Tree, lookupFn func(name string) *pa
 
        c := newTemplateContext(lookupFn)
 
-       c.paramsKeysToLower(templ.Root)
+       c.applyTransformations(templ.Root)
 
        return nil
 }
 
-// paramsKeysToLower is made purposely non-generic to make it not so tempting
-// to do more of these hard-to-maintain AST transformations.
-func (c *templateContext) paramsKeysToLower(n parse.Node) {
+// The truth logic in Go's template package is broken for certain values
+// for the if and with keywords. This works around that problem by wrapping
+// the node passed to if/with in a getif conditional.
+// getif works slightly different than the Go built-in in that it also
+// considers any IsZero methods on the values (as in time.Time).
+// See https://github.com/gohugoio/hugo/issues/5738
+func (c *templateContext) wrapWithGetIf(p *parse.PipeNode) {
+       if len(p.Cmds) == 0 {
+               return
+       }
+
+       // getif will return an empty string if not evaluated as truthful,
+       // which is when we need the value in the with clause.
+       firstArg := parse.NewIdentifier("getif")
+       secondArg := p.CopyPipe()
+       newCmd := p.Cmds[0].Copy().(*parse.CommandNode)
+
+       // secondArg is a PipeNode and will behave as it was wrapped in parens, e.g:
+       // {{ getif (len .Params | eq 2) }}
+       newCmd.Args = []parse.Node{firstArg, secondArg}
+
+       p.Cmds = []*parse.CommandNode{newCmd}
+
+}
+
+// applyTransformations do two things:
+// 1) Make all .Params.CamelCase and similar into lowercase.
+// 2) Wraps every with and if pipe in getif
+func (c *templateContext) applyTransformations(n parse.Node) {
        switch x := n.(type) {
        case *parse.ListNode:
                if x != nil {
-                       c.paramsKeysToLowerForNodes(x.Nodes...)
+                       c.applyTransformationsToNodes(x.Nodes...)
                }
        case *parse.ActionNode:
-               c.paramsKeysToLowerForNodes(x.Pipe)
+               c.applyTransformationsToNodes(x.Pipe)
        case *parse.IfNode:
-               c.paramsKeysToLowerForNodes(x.Pipe, x.List, x.ElseList)
+               c.applyTransformationsToNodes(x.Pipe, x.List, x.ElseList)
+               c.wrapWithGetIf(x.Pipe)
        case *parse.WithNode:
-               c.paramsKeysToLowerForNodes(x.Pipe, x.List, x.ElseList)
+               c.applyTransformationsToNodes(x.Pipe, x.List, x.ElseList)
+               c.wrapWithGetIf(x.Pipe)
        case *parse.RangeNode:
-               c.paramsKeysToLowerForNodes(x.Pipe, x.List, x.ElseList)
+               c.applyTransformationsToNodes(x.Pipe, x.List, x.ElseList)
        case *parse.TemplateNode:
                subTempl := c.getIfNotVisited(x.Name)
                if subTempl != nil {
-                       c.paramsKeysToLowerForNodes(subTempl.Root)
+                       c.applyTransformationsToNodes(subTempl.Root)
                }
        case *parse.PipeNode:
                if len(x.Decl) == 1 && len(x.Cmds) == 1 {
@@ -118,7 +146,7 @@ func (c *templateContext) paramsKeysToLower(n parse.Node) {
                }
 
                for _, cmd := range x.Cmds {
-                       c.paramsKeysToLower(cmd)
+                       c.applyTransformations(cmd)
                }
 
        case *parse.CommandNode:
@@ -129,7 +157,7 @@ func (c *templateContext) paramsKeysToLower(n parse.Node) {
                        case *parse.VariableNode:
                                c.updateIdentsIfNeeded(an.Ident)
                        case *parse.PipeNode:
-                               c.paramsKeysToLower(an)
+                               c.applyTransformations(an)
                        case *parse.ChainNode:
                                // site.Params...
                                if len(an.Field) > 1 && an.Field[0] == paramsIdentifier {
@@ -140,9 +168,9 @@ func (c *templateContext) paramsKeysToLower(n parse.Node) {
        }
 }
 
-func (c *templateContext) paramsKeysToLowerForNodes(nodes ...parse.Node) {
+func (c *templateContext) applyTransformationsToNodes(nodes ...parse.Node) {
        for _, node := range nodes {
-               c.paramsKeysToLower(node)
+               c.applyTransformations(node)
        }
 }
 
index 45cf4399a9b77c8a29dfaa6abb0dc3bb931116df..611f5d8caa292cec06d96675a391f3dfd14a0a15 100644 (file)
@@ -15,9 +15,14 @@ package tplimpl
 import (
        "bytes"
        "fmt"
+       "html/template"
        "testing"
+       "time"
 
-       "html/template"
+       "github.com/gohugoio/hugo/tpl"
+
+       "github.com/gohugoio/hugo/deps"
+       "github.com/gohugoio/hugo/hugofs"
 
        "github.com/spf13/cast"
 
@@ -26,6 +31,7 @@ import (
 
 var (
        testFuncs = map[string]interface{}{
+               "getif":  func(v interface{}) interface{} { return v },
                "ToTime": func(v interface{}) interface{} { return cast.ToTime(v) },
                "First":  func(v ...interface{}) interface{} { return v[0] },
                "Echo":   func(v interface{}) interface{} { return v },
@@ -183,7 +189,7 @@ func TestParamsKeysToLower(t *testing.T) {
 
        require.Equal(t, -1, c.decl.indexOfReplacementStart([]string{}))
 
-       c.paramsKeysToLower(templ.Tree.Root)
+       c.applyTransformations(templ.Tree.Root)
 
        var b bytes.Buffer
 
@@ -265,7 +271,7 @@ func BenchmarkTemplateParamsKeysToLower(b *testing.B) {
 
        for i := 0; i < b.N; i++ {
                c := newTemplateContext(createParseTreeLookup(templates[i]))
-               c.paramsKeysToLower(templ.Tree.Root)
+               c.applyTransformations(templ.Tree.Root)
        }
 }
 
@@ -304,7 +310,7 @@ Pretty First3: {{ $__amber_4.COLORS.PRETTY.FIRST}}
 
        c := newTemplateContext(createParseTreeLookup(templ))
 
-       c.paramsKeysToLower(templ.Tree.Root)
+       c.applyTransformations(templ.Tree.Root)
 
        var b bytes.Buffer
 
@@ -348,7 +354,7 @@ P2: {{ .Params.LOWER }}
 
        c := newTemplateContext(createParseTreeLookup(overlayTpl))
 
-       c.paramsKeysToLower(overlayTpl.Tree.Root)
+       c.applyTransformations(overlayTpl.Tree.Root)
 
        var b bytes.Buffer
 
@@ -377,6 +383,78 @@ func TestTransformRecursiveTemplate(t *testing.T) {
        require.NoError(t, err)
 
        c := newTemplateContext(createParseTreeLookup(templ))
-       c.paramsKeysToLower(templ.Tree.Root)
+       c.applyTransformations(templ.Tree.Root)
+
+}
+
+type I interface {
+       Method0()
+}
+
+type T struct {
+       NonEmptyInterfaceTypedNil I
+}
+
+func (T) Method0() {
+}
+
+func TestInsertIsZeroFunc(t *testing.T) {
+       t.Parallel()
+
+       assert := require.New(t)
+
+       var (
+               ctx = map[string]interface{}{
+                       "True":     true,
+                       "Now":      time.Now(),
+                       "TimeZero": time.Time{},
+                       "T":        &T{NonEmptyInterfaceTypedNil: (*T)(nil)},
+               }
+
+               templ = `
+{{ if .True }}.True: TRUE{{ else }}.True: FALSE{{ end }}
+{{ if .TimeZero }}.TimeZero1: TRUE{{ else }}.TimeZero1: FALSE{{ end }}
+{{ if (.TimeZero) }}.TimeZero2: TRUE{{ else }}.TimeZero2: FALSE{{ end }}
+{{ if not .TimeZero }}.TimeZero3: TRUE{{ else }}.TimeZero3: FALSE{{ end }}
+{{ if .Now }}.Now: TRUE{{ else }}.Now: FALSE{{ end }}
+{{ with .TimeZero }}.TimeZero1 with: {{ . }}{{ else }}.TimeZero1 with: FALSE{{ end }}
+{{ template "mytemplate" . }}
+{{ if .T.NonEmptyInterfaceTypedNil }}.NonEmptyInterfaceTypedNil: TRUE{{ else }}.NonEmptyInterfaceTypedNil: FALSE{{ end }}
+
+
+{{ define "mytemplate" }}
+{{ if .TimeZero }}.TimeZero1: mytemplate: TRUE{{ else }}.TimeZero1: mytemplate: FALSE{{ end }}
+{{ end }}
+
+`
+       )
+
+       v := newTestConfig()
+       fs := hugofs.NewMem(v)
+
+       depsCfg := newDepsConfig(v)
+       depsCfg.Fs = fs
+       d, err := deps.New(depsCfg)
+       assert.NoError(err)
+
+       provider := DefaultTemplateProvider
+       provider.Update(d)
+
+       h := d.Tmpl.(handler)
+
+       assert.NoError(h.addTemplate("mytemplate.html", templ))
+
+       tt, _ := d.Tmpl.Lookup("mytemplate.html")
+       result, err := tt.(tpl.TemplateExecutor).ExecuteToString(ctx)
+       assert.NoError(err)
+
+       assert.Contains(result, ".True: TRUE")
+       assert.Contains(result, ".TimeZero1: FALSE")
+       assert.Contains(result, ".TimeZero2: FALSE")
+       assert.Contains(result, ".TimeZero3: TRUE")
+       assert.Contains(result, ".Now: TRUE")
+       assert.Contains(result, "TimeZero1 with: FALSE")
+       assert.Contains(result, ".TimeZero1: mytemplate: FALSE")
+       assert.Contains(result, ".NonEmptyInterfaceTypedNil: FALSE")
 
 }