From 604146ce8961d32f410949015fc8ee31f9052209 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 19 Oct 2016 10:27:05 -0400 Subject: [PATCH] html/template, text/template: docs and fixes for template redefinition All prior versions of Go have allowed redefining empty templates to become non-empty. Unfortunately, that has never consistently taken effect in html/template after the first execution: // define and execute t := template.New("root") t.Parse(`{{define "T"}}{{end}}`) t.Execute(w, nil) // // redefine t.Parse(`{{define "T"}}my.url{{end}}`) // succeeds, but ignored t.Execute(w, nil) // When Go 1.6 added {{block...}} to text/template, that loosened the redefinition rules to allow redefinition at any time. The loosening was undone a bit in html/template, although inconsistently: // define and execute t := template.New("root") t.Parse(`{{define "T"}}body{{end}}`) t.Lookup("T").Execute(ioutil.Discard, nil) // attempt to redefine t.Parse(`{{define "T"}}body{{end}}`) // rejected in all Go versions t.Lookup("T").Parse("body") // OK as of Go 1.6, likely unintentionally Like in the empty->non-empty case, whether future execution takes notice of a redefinition basically can't be explained without going into the details of the template escape analysis. Address both the original inconsistencies in whether a redefinition would have any effect and the new inconsistencies about whether a redefinition is allowed by adopting a new rule: no parsing or modifying any templates after the first execution of any template in the same set. Template analysis begins at first execution, and once template analysis has begun, we simply don't have the right logic to update the analysis for incremental modifications (and never have). If this new rule breaks existing uses of templates that we decide need to be supported, we can try to invalidate all escape analysis for the entire set after any modifications. But let's wait on that until we know we need to and why. Also fix documentation of text/template redefinition policy (redefinition is always OK). Fixes #15761. Change-Id: I7d58d7c08a7d9df2440ee0d651a5b2ecaff3006c Reviewed-on: https://go-review.googlesource.com/31464 Run-TryBot: Russ Cox TryBot-Result: Gobot Gobot Reviewed-by: Andrew Gerrand --- src/html/template/template.go | 64 +++++++++----- src/html/template/template_test.go | 130 ++++++++++++++++++++++++++++- src/text/template/template.go | 10 +-- 3 files changed, 178 insertions(+), 26 deletions(-) diff --git a/src/html/template/template.go b/src/html/template/template.go index f83e6d22d8..9eaab4be6a 100644 --- a/src/html/template/template.go +++ b/src/html/template/template.go @@ -33,8 +33,9 @@ var escapeOK = fmt.Errorf("template escaped correctly") // nameSpace is the data structure shared by all templates in an association. type nameSpace struct { - mu sync.Mutex - set map[string]*Template + mu sync.Mutex + set map[string]*Template + escaped bool } // Templates returns a slice of the templates associated with t, including t @@ -74,10 +75,25 @@ func (t *Template) Option(opt ...string) *Template { return t } +// checkCanParse checks whether it is OK to parse templates. +// If not, it returns an error. +func (t *Template) checkCanParse() error { + if t == nil { + return nil + } + t.nameSpace.mu.Lock() + defer t.nameSpace.mu.Unlock() + if t.nameSpace.escaped { + return fmt.Errorf("html/template: cannot Parse after Execute") + } + return nil +} + // escape escapes all associated templates. func (t *Template) escape() error { t.nameSpace.mu.Lock() defer t.nameSpace.mu.Unlock() + t.nameSpace.escaped = true if t.escapeErr == nil { if t.Tree == nil { return fmt.Errorf("template: %q is an incomplete or empty template%s", t.Name(), t.DefinedTemplates()) @@ -124,6 +140,7 @@ func (t *Template) ExecuteTemplate(wr io.Writer, name string, data interface{}) func (t *Template) lookupAndEscapeTemplate(name string) (tmpl *Template, err error) { t.nameSpace.mu.Lock() defer t.nameSpace.mu.Unlock() + t.nameSpace.escaped = true tmpl = t.set[name] if tmpl == nil { return nil, fmt.Errorf("html/template: %q is undefined", name) @@ -155,21 +172,22 @@ func (t *Template) DefinedTemplates() string { // define additional templates associated with t and are removed from the // definition of t itself. // +// Templates can be redefined in successive calls to Parse, +// before the first use of Execute on t or any associated template. // A template definition with a body containing only white space and comments -// is considered empty and is not recorded as the template's body. -// Each template can be given a non-empty definition at most once. -// That is, Parse may be called multiple times to parse definitions of templates -// to associate with t, but at most one such call can include a non-empty body for -// t itself, and each named associated template can be given at most one -// non-empty definition. +// is considered empty and will not replace an existing template's body. +// This allows using Parse to add new named template definitions without +// overwriting the main template body. func (t *Template) Parse(text string) (*Template, error) { - t.nameSpace.mu.Lock() - t.escapeErr = nil - t.nameSpace.mu.Unlock() + if err := t.checkCanParse(); err != nil { + return nil, err + } + ret, err := t.text.Parse(text) if err != nil { return nil, err } + // In general, all the named templates might have changed underfoot. // Regardless, some new ones may have been defined. // The template.Template set has been updated; update ours. @@ -180,11 +198,7 @@ func (t *Template) Parse(text string) (*Template, error) { tmpl := t.set[name] if tmpl == nil { tmpl = t.new(name) - } else if tmpl.escapeErr != nil { - return nil, fmt.Errorf("html/template: cannot redefine %q after it has executed", name) } - // Restore our record of this text/template to its unescaped original state. - tmpl.escapeErr = nil tmpl.text = v tmpl.Tree = v.Tree } @@ -194,13 +208,14 @@ func (t *Template) Parse(text string) (*Template, error) { // AddParseTree creates a new template with the name and parse tree // and associates it with t. // -// It returns an error if t has already been executed. +// It returns an error if t or any associated template has already been executed. func (t *Template) AddParseTree(name string, tree *parse.Tree) (*Template, error) { + if err := t.checkCanParse(); err != nil { + return nil, err + } + t.nameSpace.mu.Lock() defer t.nameSpace.mu.Unlock() - if t.escapeErr != nil { - return nil, fmt.Errorf("html/template: cannot AddParseTree to %q after it has executed", t.Name()) - } text, err := t.text.AddParseTree(name, tree) if err != nil { return nil, err @@ -368,6 +383,8 @@ func ParseFiles(filenames ...string) (*Template, error) { // // When parsing multiple files with the same name in different directories, // the last one mentioned will be the one that results. +// +// ParseFiles returns an error if t or any associated template has already been executed. func (t *Template) ParseFiles(filenames ...string) (*Template, error) { return parseFiles(t, filenames...) } @@ -375,6 +392,10 @@ func (t *Template) ParseFiles(filenames ...string) (*Template, error) { // parseFiles is the helper for the method and function. If the argument // template is nil, it is created from the first file. func parseFiles(t *Template, filenames ...string) (*Template, error) { + if err := t.checkCanParse(); err != nil { + return nil, err + } + if len(filenames) == 0 { // Not really a problem, but be consistent. return nil, fmt.Errorf("html/template: no files named in call to ParseFiles") @@ -429,12 +450,17 @@ func ParseGlob(pattern string) (*Template, error) { // // When parsing multiple files with the same name in different directories, // the last one mentioned will be the one that results. +// +// ParseGlob returns an error if t or any associated template has already been executed. func (t *Template) ParseGlob(pattern string) (*Template, error) { return parseGlob(t, pattern) } // parseGlob is the implementation of the function and method ParseGlob. func parseGlob(t *Template, pattern string) (*Template, error) { + if err := t.checkCanParse(); err != nil { + return nil, err + } filenames, err := filepath.Glob(pattern) if err != nil { return nil, err diff --git a/src/html/template/template_test.go b/src/html/template/template_test.go index 46df1f8d49..90c5a73ba7 100644 --- a/src/html/template/template_test.go +++ b/src/html/template/template_test.go @@ -1,7 +1,13 @@ -package template +// Copyright 2016 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package template_test import ( "bytes" + . "html/template" + "strings" "testing" ) @@ -27,3 +33,125 @@ func TestTemplateClone(t *testing.T) { t.Fatalf("got %q; want %q", got, want) } } + +func TestRedefineNonEmptyAfterExecution(t *testing.T) { + c := newTestCase(t) + c.mustParse(c.root, `foo`) + c.mustExecute(c.root, nil, "foo") + c.mustNotParse(c.root, `bar`) +} + +func TestRedefineEmptyAfterExecution(t *testing.T) { + c := newTestCase(t) + c.mustParse(c.root, ``) + c.mustExecute(c.root, nil, "") + c.mustNotParse(c.root, `foo`) + c.mustExecute(c.root, nil, "") +} + +func TestRedefineAfterNonExecution(t *testing.T) { + c := newTestCase(t) + c.mustParse(c.root, `{{if .}}<{{template "X"}}>{{end}}{{define "X"}}foo{{end}}`) + c.mustExecute(c.root, 0, "") + c.mustNotParse(c.root, `{{define "X"}}bar{{end}}`) + c.mustExecute(c.root, 1, "<foo>") +} + +func TestRedefineAfterNamedExecution(t *testing.T) { + c := newTestCase(t) + c.mustParse(c.root, `<{{template "X" .}}>{{define "X"}}foo{{end}}`) + c.mustExecute(c.root, nil, "<foo>") + c.mustNotParse(c.root, `{{define "X"}}bar{{end}}`) + c.mustExecute(c.root, nil, "<foo>") +} + +func TestRedefineNestedByNameAfterExecution(t *testing.T) { + c := newTestCase(t) + c.mustParse(c.root, `{{define "X"}}foo{{end}}`) + c.mustExecute(c.lookup("X"), nil, "foo") + c.mustNotParse(c.root, `{{define "X"}}bar{{end}}`) + c.mustExecute(c.lookup("X"), nil, "foo") +} + +func TestRedefineNestedByTemplateAfterExecution(t *testing.T) { + c := newTestCase(t) + c.mustParse(c.root, `{{define "X"}}foo{{end}}`) + c.mustExecute(c.lookup("X"), nil, "foo") + c.mustNotParse(c.lookup("X"), `bar`) + c.mustExecute(c.lookup("X"), nil, "foo") +} + +func TestRedefineSafety(t *testing.T) { + c := newTestCase(t) + c.mustParse(c.root, `{{define "X"}}{{end}}`) + c.mustExecute(c.root, nil, ``) + // Note: Every version of Go prior to Go 1.8 accepted the redefinition of "X" + // on the next line, but luckily kept it from being used in the outer template. + // Now we reject it, which makes clearer that we're not going to use it. + c.mustNotParse(c.root, `{{define "X"}}" bar="baz{{end}}`) + c.mustExecute(c.root, nil, ``) +} + +func TestRedefineTopUse(t *testing.T) { + c := newTestCase(t) + c.mustParse(c.root, `{{template "X"}}{{.}}{{define "X"}}{{end}}`) + c.mustExecute(c.root, 42, `42`) + c.mustNotParse(c.root, `{{define "X"}}