Skip to content

Commit

Permalink
tpl/tplimpl: Reimplement the ".Params tolower" template transformer
Browse files Browse the repository at this point in the history
All `.Params` are stored lowercase, but it should work to access them `.Page.camelCase` etc. There was, however, some holes in the logic with the old transformer.

This commit fixes that by applying a blacklist instead of the old whitelist logic. `.Param` is a very distinct key. The original case will be kept in `.Data.Params.myParam`, but other than that it will be lowercased.

This implementation is also faster:

```bash
benchmark                                old ns/op     new ns/op     delta
BenchmarkTemplateParamsKeysToLower-4     15237         9581          -37.12%

benchmark                                old allocs     new allocs     delta
BenchmarkTemplateParamsKeysToLower-4     92             40             -56.52%

benchmark                                old bytes     new bytes     delta
BenchmarkTemplateParamsKeysToLower-4     2656          1360          -48.80%
```

Fixes gohugoio#5068
  • Loading branch information
bep committed Aug 14, 2018
1 parent 78f8475 commit 01ececd
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 72 deletions.
155 changes: 83 additions & 72 deletions tpl/tplimpl/template_ast_transformers.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,14 @@ import (
// decl keeps track of the variable mappings, i.e. $mysite => .Site etc.
type decl map[string]string

var paramsPaths = [][]string{
{"Params"},
{"Site", "Params"},

// Site and Pag referenced from shortcodes
{"Page", "Site", "Params"},
{"Page", "Params"},
const (
paramsIdentifier = "Params"
)

{"Site", "Language", "Params"},
// Containers that may contain Params that we will not touch.
var reservedContainers = map[string]bool{
// Aka .Site.Data.Params which must stay case sensitive.
"Data": true,
}

type templateContext struct {
Expand Down Expand Up @@ -155,6 +154,7 @@ func (c *templateContext) updateIdentsIfNeeded(idents []string) {
for i := index; i < len(idents); i++ {
idents[i] = strings.ToLower(idents[i])
}

}

// indexOfReplacementStart will return the index of where to start doing replacement,
Expand All @@ -167,31 +167,88 @@ func (d decl) indexOfReplacementStart(idents []string) int {
return -1
}

first := idents[0]
firstIsVar := first[0] == '$'
if l == 1 {
first := idents[0]
if first == "" || first == paramsIdentifier || first[0] == '$' {
// This can not be a Params.x
return -1
}
}

if l == 1 && !firstIsVar {
// This can not be a Params.x
var lookFurther bool
var needsVarExpansion bool
for _, ident := range idents {
if ident[0] == '$' {
lookFurther = true
needsVarExpansion = true
} else if ident == paramsIdentifier {
lookFurther = true
break
}
}

if !lookFurther {
return -1
}

if !firstIsVar {
found := false
for _, paramsPath := range paramsPaths {
if first == paramsPath[0] {
found = true
break
var resolvedIdents []string

if !needsVarExpansion {
resolvedIdents = idents
} else {
var ok bool
resolvedIdents, ok = d.resolveVariables(idents)
if !ok {
return -1
}
}

var paramFound bool
for i, ident := range resolvedIdents {
if ident == paramsIdentifier {
if i > 0 {
container := resolvedIdents[i-1]
if reservedContainers[container] {
// .Data.Params.someKey
return -1
}
}
paramFound = true

break
}
if !found {
return -1
}

if !paramFound {
return -1
}

var paramSeen bool
idx := -1
for i, ident := range idents {
if ident == "" || ident[0] == '$' {
continue
}
if ident == paramsIdentifier {
paramSeen = true
idx = -1
} else {
if paramSeen {
return i
}
if idx == -1 {
idx = i
}
}
}
return idx

}

func (d decl) resolveVariables(idents []string) ([]string, bool) {
var (
resolvedIdents []string
replacements []string
replaced []string
replacements []string
replaced []string
)

// An Ident can start out as one of
Expand All @@ -206,7 +263,7 @@ func (d decl) indexOfReplacementStart(idents []string) int {

if i > 20 {
// bail out
return -1
return nil, false
}

potentialVar := replacements[i]
Expand All @@ -225,7 +282,7 @@ func (d decl) indexOfReplacementStart(idents []string) int {

if !ok {
// Temporary range vars. We do not care about those.
return -1
return nil, false
}

replacement = strings.TrimPrefix(replacement, ".")
Expand All @@ -242,52 +299,6 @@ func (d decl) indexOfReplacementStart(idents []string) int {
}
}

resolvedIdents = append(replaced, idents[1:]...)

for _, paramPath := range paramsPaths {
if index := indexOfFirstRealIdentAfterWords(resolvedIdents, idents, paramPath...); index != -1 {
return index
}
}

return -1
return append(replaced, idents[1:]...), true

}

func indexOfFirstRealIdentAfterWords(resolvedIdents, idents []string, words ...string) int {
if !sliceStartsWith(resolvedIdents, words...) {
return -1
}

for i, ident := range idents {
if ident == "" || ident[0] == '$' {
continue
}
found := true
for _, word := range words {
if ident == word {
found = false
break
}
}
if found {
return i
}
}

return -1
}

func sliceStartsWith(slice []string, words ...string) bool {

if len(slice) < len(words) {
return false
}

for i, word := range words {
if word != slice[i] {
return false
}
}
return true
}
27 changes: 27 additions & 0 deletions tpl/tplimpl/template_ast_transformers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ var (
"Slice": []int{1, 3},
"Params": map[string]interface{}{
"lower": "P1L",
"slice": []int{1, 3},
},
"CurrentSection": map[string]interface{}{
"Params": map[string]interface{}{
"lower": "pcurrentsection",
},
},
"Site": map[string]interface{}{
"Params": map[string]interface{}{
Expand Down Expand Up @@ -58,6 +64,7 @@ var (
{{ $data := .Site.Data }}
{{ $notparam := .NotParam }}
PCurrentSection: {{ .CurrentSection.Params.LOWER }}
P1: {{ .Params.LOWER }}
P1_2: {{ $.Params.LOWER }}
P1_3: {{ $page.Params.LOWER }}
Expand Down Expand Up @@ -109,6 +116,8 @@ RANGE: {{ . }}: {{ $.Params.LOWER }}
F1: {{ printf "themes/%s-theme" .Site.Params.LOWER }}
F2: {{ Echo (printf "themes/%s-theme" $lower) }}
F3: {{ Echo (printf "themes/%s-theme" .Site.Params.LOWER) }}
PSLICE: {{ range .Params.SLICE }}PSLICE{{.}}|{{ end }}
`
)

Expand Down Expand Up @@ -164,6 +173,11 @@ func TestParamsKeysToLower(t *testing.T) {
require.Contains(t, result, "F2: themes/P2L-theme")
require.Contains(t, result, "F3: themes/P2L-theme")

require.Contains(t, result, "PSLICE: PSLICE1|PSLICE3|")

// Issue #5068
require.Contains(t, result, "PCurrentSection: pcurrentsection")

}

func BenchmarkTemplateParamsKeysToLower(b *testing.B) {
Expand Down Expand Up @@ -197,6 +211,9 @@ func TestParamsKeysToLowerVars(t *testing.T) {
"Params": map[string]interface{}{
"colors": map[string]interface{}{
"blue": "Amber",
"pretty": map[string]interface{}{
"first": "Indigo",
},
},
},
}
Expand All @@ -205,8 +222,14 @@ func TestParamsKeysToLowerVars(t *testing.T) {
paramsTempl = `
{{$__amber_1 := .Params.Colors}}
{{$__amber_2 := $__amber_1.Blue}}
{{$__amber_3 := $__amber_1.Pretty}}
{{$__amber_4 := .Params}}
Color: {{$__amber_2}}
Blue: {{ $__amber_1.Blue}}
Pretty First1: {{ $__amber_3.First}}
Pretty First2: {{ $__amber_1.Pretty.First}}
Pretty First3: {{ $__amber_4.COLORS.PRETTY.FIRST}}
`
)

Expand All @@ -225,6 +248,10 @@ Blue: {{ $__amber_1.Blue}}
result := b.String()

require.Contains(t, result, "Color: Amber")
require.Contains(t, result, "Blue: Amber")
require.Contains(t, result, "Pretty First1: Indigo")
require.Contains(t, result, "Pretty First2: Indigo")
require.Contains(t, result, "Pretty First3: Indigo")

}

Expand Down

0 comments on commit 01ececd

Please sign in to comment.