From 3de0cdd959692d67ebc425a0165598b6933347b0 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 9 Mar 2023 20:07:49 +0000 Subject: [PATCH 1/4] Improve template error reporting There are multiple duplicate reports of errors during template rendering due to broken custom templates. Unfortunately the error returned here is somewhat difficult for users to understand and it doesn't return the context of the error. This PR attempts to parse the error returned by the template renderer to add in some further context including the filename of the template AND the preceding lines within that template file. Ref #23274 Signed-off-by: Andrew Thornton --- modules/context/context.go | 28 ++++++++++++++++++++ modules/templates/htmlrenderer.go | 43 ++++++++++++++++++------------- 2 files changed, 53 insertions(+), 18 deletions(-) diff --git a/modules/context/context.go b/modules/context/context.go index 50c34edae2e53..27226a5b189b0 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -16,8 +16,10 @@ import ( "net/http" "net/url" "path" + "regexp" "strconv" "strings" + texttemplate "text/template" "time" "code.gitea.io/gitea/models/db" @@ -213,6 +215,8 @@ func (ctx *Context) RedirectToFirst(location ...string) { ctx.Redirect(setting.AppSubURL + "/") } +var templateErr = regexp.MustCompile(`^template: (.*):([1-9][0-9]*):([1-9][0-9]*): executing (?:"(.*)" at <(.*)>: )?`) + // HTML calls Context.HTML and renders the template to HTTP response func (ctx *Context) HTML(status int, name base.TplName) { log.Debug("Template: %s", name) @@ -228,6 +232,30 @@ func (ctx *Context) HTML(status int, name base.TplName) { ctx.PlainText(http.StatusInternalServerError, "Unable to find status/500 template") return } + if _, ok := err.(texttemplate.ExecError); ok { + if groups := templateErr.FindStringSubmatch(err.Error()); len(groups) > 0 { + templateName, lineStr, posStr := groups[1], groups[2], groups[3] + target := "http" + if len(groups) == 6 { + target = groups[5] + } + line, lineErr := strconv.Atoi(lineStr) + if lineErr != nil { + line = -1 + } + pos, posErr := strconv.Atoi(posStr) + if posErr != nil { + pos = -1 + } + if line >= 0 { + filename, filenameErr := templates.GetAssetFilename("templates/" + templateName + ".tmpl") + if filenameErr != nil { + filename = "(template) " + templateName + } + err = fmt.Errorf("%w\nin template file %s:\n%s", err, filename, templates.GetLineFromTemplate(templateName, line, target, pos)) + } + } + } ctx.ServerError("Render failed", err) } } diff --git a/modules/templates/htmlrenderer.go b/modules/templates/htmlrenderer.go index 5a328043ebfe4..96dc010796ef9 100644 --- a/modules/templates/htmlrenderer.go +++ b/modules/templates/htmlrenderer.go @@ -118,7 +118,7 @@ func handleGenericTemplateError(err error) (string, []interface{}) { lineNumber, _ := strconv.Atoi(lineNumberStr) - line := getLineFromAsset(templateName, lineNumber, "") + line := GetLineFromTemplate(templateName, lineNumber, "", -1) return "PANIC: Unable to compile templates!\n%s in template file %s at line %d:\n\n%s\nStacktrace:\n\n%s", []interface{}{message, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} } @@ -140,7 +140,7 @@ func handleNotDefinedPanicError(err error) (string, []interface{}) { lineNumber, _ := strconv.Atoi(lineNumberStr) - line := getLineFromAsset(templateName, lineNumber, functionName) + line := GetLineFromTemplate(templateName, lineNumber, functionName, -1) return "PANIC: Unable to compile templates!\nUndefined function %q in template file %s at line %d:\n\n%s", []interface{}{functionName, filename, lineNumber, log.NewColoredValue(line, log.Reset)} } @@ -161,7 +161,7 @@ func handleUnexpected(err error) (string, []interface{}) { lineNumber, _ := strconv.Atoi(lineNumberStr) - line := getLineFromAsset(templateName, lineNumber, unexpected) + line := GetLineFromTemplate(templateName, lineNumber, unexpected, -1) return "PANIC: Unable to compile templates!\nUnexpected %q in template file %s at line %d:\n\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset)} } @@ -181,14 +181,15 @@ func handleExpectedEnd(err error) (string, []interface{}) { lineNumber, _ := strconv.Atoi(lineNumberStr) - line := getLineFromAsset(templateName, lineNumber, unexpected) + line := GetLineFromTemplate(templateName, lineNumber, unexpected, -1) return "PANIC: Unable to compile templates!\nMissing end with unexpected %q in template file %s at line %d:\n\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset)} } const dashSeparator = "----------------------------------------------------------------------\n" -func getLineFromAsset(templateName string, targetLineNum int, target string) string { +// GetLineFromTemplate returns a line from a template with some context +func GetLineFromTemplate(templateName string, targetLineNum int, target string, position int) string { bs, err := GetAsset("templates/" + templateName + ".tmpl") if err != nil { return fmt.Sprintf("(unable to read template file: %v)", err) @@ -229,23 +230,29 @@ func getLineFromAsset(templateName string, targetLineNum int, target string) str // If there is a provided target to look for in the line add a pointer to it // e.g. ^^^^^^^ if target != "" { - idx := bytes.Index(lineBs, []byte(target)) - - if idx >= 0 { - // take the current line and replace preceding text with whitespace (except for tab) - for i := range lineBs[:idx] { - if lineBs[i] != '\t' { - lineBs[i] = ' ' - } + targetPos := bytes.Index(lineBs, []byte(target)) + if targetPos >= 0 { + position = targetPos + } + } + if position >= 0 { + // take the current line and replace preceding text with whitespace (except for tab) + for i := range lineBs[:position] { + if lineBs[i] != '\t' { + lineBs[i] = ' ' } + } - // write the preceding "space" - _, _ = sb.Write(lineBs[:idx]) + // write the preceding "space" + _, _ = sb.Write(lineBs[:position]) - // Now write the ^^ pointer - _, _ = sb.WriteString(strings.Repeat("^", len(target))) - _ = sb.WriteByte('\n') + // Now write the ^^ pointer + targetLen := len(target) + if targetLen == 0 { + targetLen = 1 } + _, _ = sb.WriteString(strings.Repeat("^", targetLen)) + _ = sb.WriteByte('\n') } // Finally write the footer From f8e25c019557fd77d53da4cb34108a592e8995cb Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 9 Mar 2023 20:21:04 +0000 Subject: [PATCH 2/4] Update modules/context/context.go --- modules/context/context.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/context/context.go b/modules/context/context.go index 27226a5b189b0..897799d761b20 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -235,7 +235,7 @@ func (ctx *Context) HTML(status int, name base.TplName) { if _, ok := err.(texttemplate.ExecError); ok { if groups := templateErr.FindStringSubmatch(err.Error()); len(groups) > 0 { templateName, lineStr, posStr := groups[1], groups[2], groups[3] - target := "http" + target := "" if len(groups) == 6 { target = groups[5] } From e2e94128d2840195ae05cb08197608615806c58d Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 10 Mar 2023 18:15:54 +0000 Subject: [PATCH 3/4] as per lunny Signed-off-by: Andrew Thornton --- modules/context/context.go | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/modules/context/context.go b/modules/context/context.go index 897799d761b20..7ee327c43e84b 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -215,7 +215,7 @@ func (ctx *Context) RedirectToFirst(location ...string) { ctx.Redirect(setting.AppSubURL + "/") } -var templateErr = regexp.MustCompile(`^template: (.*):([1-9][0-9]*):([1-9][0-9]*): executing (?:"(.*)" at <(.*)>: )?`) +var templateExecutingErr = regexp.MustCompile(`^template: (.*):([1-9][0-9]*):([1-9][0-9]*): executing (?:"(.*)" at <(.*)>: )?`) // HTML calls Context.HTML and renders the template to HTTP response func (ctx *Context) HTML(status int, name base.TplName) { @@ -233,27 +233,22 @@ func (ctx *Context) HTML(status int, name base.TplName) { return } if _, ok := err.(texttemplate.ExecError); ok { - if groups := templateErr.FindStringSubmatch(err.Error()); len(groups) > 0 { - templateName, lineStr, posStr := groups[1], groups[2], groups[3] + if groups := templateExecutingErr.FindStringSubmatch(err.Error()); len(groups) > 0 { + errorTemplateName, lineStr, posStr := groups[1], groups[2], groups[3] target := "" if len(groups) == 6 { target = groups[5] } - line, lineErr := strconv.Atoi(lineStr) - if lineErr != nil { - line = -1 + line, _ := strconv.Atoi(lineStr) // Cannot error out as groups[2] is [1-9][0-9]* + pos, _ := strconv.Atoi(posStr) // Cannot error out as groups[3] is [1-9][0-9]* + filename, filenameErr := templates.GetAssetFilename("templates/" + errorTemplateName + ".tmpl") + if filenameErr != nil { + filename = "(template) " + errorTemplateName } - pos, posErr := strconv.Atoi(posStr) - if posErr != nil { - pos = -1 - } - if line >= 0 { - filename, filenameErr := templates.GetAssetFilename("templates/" + templateName + ".tmpl") - if filenameErr != nil { - filename = "(template) " + templateName - } - err = fmt.Errorf("%w\nin template file %s:\n%s", err, filename, templates.GetLineFromTemplate(templateName, line, target, pos)) + if errorTemplateName != string(name) { + filename += " (subtemplate of " + string(name) + ")" } + err = fmt.Errorf("%w\nin template file %s:\n%s", err, filename, templates.GetLineFromTemplate(errorTemplateName, line, target, pos)) } } ctx.ServerError("Render failed", err) From 163bc838450eb3f0627756f66104e865ab757058 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 10 Mar 2023 20:27:00 +0000 Subject: [PATCH 4/4] And handle the other types of ExecError Signed-off-by: Andrew Thornton --- modules/context/context.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/modules/context/context.go b/modules/context/context.go index 7ee327c43e84b..5876e23cc40a2 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -232,7 +232,7 @@ func (ctx *Context) HTML(status int, name base.TplName) { ctx.PlainText(http.StatusInternalServerError, "Unable to find status/500 template") return } - if _, ok := err.(texttemplate.ExecError); ok { + if execErr, ok := err.(texttemplate.ExecError); ok { if groups := templateExecutingErr.FindStringSubmatch(err.Error()); len(groups) > 0 { errorTemplateName, lineStr, posStr := groups[1], groups[2], groups[3] target := "" @@ -249,6 +249,15 @@ func (ctx *Context) HTML(status int, name base.TplName) { filename += " (subtemplate of " + string(name) + ")" } err = fmt.Errorf("%w\nin template file %s:\n%s", err, filename, templates.GetLineFromTemplate(errorTemplateName, line, target, pos)) + } else { + filename, filenameErr := templates.GetAssetFilename("templates/" + execErr.Name + ".tmpl") + if filenameErr != nil { + filename = "(template) " + execErr.Name + } + if execErr.Name != string(name) { + filename += " (subtemplate of " + string(name) + ")" + } + err = fmt.Errorf("%w\nin template file %s", err, filename) } } ctx.ServerError("Render failed", err)