From 9b04b5e0f2983ecc64e675c695fe51ceda1625c4 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 5 Oct 2022 19:04:48 +0100 Subject: [PATCH 1/5] Add nicer error handling on template compile errors There are repeated issues reported whereby users are unable to interpret the template errors. This PR adds some (somewhat complex) error handling to the panic recovery for template renderering but hopefully makes the interpretation of the error easier. Reference #21344 Signed-off-by: Andrew Thornton --- modules/templates/dynamic.go | 15 +++ modules/templates/htmlrenderer.go | 172 +++++++++++++++++++++++++++++- modules/templates/static.go | 11 ++ 3 files changed, 197 insertions(+), 1 deletion(-) diff --git a/modules/templates/dynamic.go b/modules/templates/dynamic.go index 4896580f6249f..a86e71a8c8b0d 100644 --- a/modules/templates/dynamic.go +++ b/modules/templates/dynamic.go @@ -33,6 +33,21 @@ func GetAsset(name string) ([]byte, error) { return os.ReadFile(filepath.Join(setting.StaticRootPath, name)) } +// GetAssetFilename returns the filename of the provided asset +func GetAssetFilename(name string) (string, error) { + filename := filepath.Join(setting.CustomPath, name) + _, err := os.Stat(filename) + if err != nil && !os.IsNotExist(err) { + return filename, err + } else if err == nil { + return filename, nil + } + + filename = filepath.Join(setting.StaticRootPath, name) + _, err = os.Stat(filename) + return filename, err +} + // walkTemplateFiles calls a callback for each template asset func walkTemplateFiles(callback func(path, name string, d fs.DirEntry, err error) error) error { if err := walkAssetDir(filepath.Join(setting.CustomPath, "templates"), true, callback); err != nil && !os.IsNotExist(err) { diff --git a/modules/templates/htmlrenderer.go b/modules/templates/htmlrenderer.go index 210bb5e73c7e1..2a46f4f0e5343 100644 --- a/modules/templates/htmlrenderer.go +++ b/modules/templates/htmlrenderer.go @@ -5,7 +5,12 @@ package templates import ( + "bytes" "context" + "fmt" + "regexp" + "strconv" + "strings" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -14,7 +19,14 @@ import ( "github.com/unrolled/render" ) -var rendererKey interface{} = "templatesHtmlRendereer" +var ( + rendererKey interface{} = "templatesHtmlRenderer" + + templateError = regexp.MustCompile(`^template: (.*):([0-9]+): (.*)`) + notDefinedError = regexp.MustCompile(`^template: (.*):([0-9]+): function "(.*)" not defined`) + unexpectedError = regexp.MustCompile(`^template: (.*):([0-9]+): unexpected "(.*)" in operand`) + expectedEndError = regexp.MustCompile(`^template: (.*):([0-9]+): expected end; found (.*)`) +) // HTMLRenderer returns the current html renderer for the context or creates and stores one within the context for future use func HTMLRenderer(ctx context.Context) (context.Context, *render.Render) { @@ -32,6 +44,25 @@ func HTMLRenderer(ctx context.Context) (context.Context, *render.Render) { } log.Log(1, log.DEBUG, "Creating "+rendererType+" HTML Renderer") + compilingTemplates := true + defer func() { + if !compilingTemplates { + return + } + + panicked := recover() + if panicked == nil { + return + } + + // OK try to handle the panic... + err, ok := panicked.(error) + if ok { + handlePanicError(err) + } + log.Fatal("PANIC: Unable to compile templates: %v\nStacktrace:\n%s", panicked, log.Stack(2)) + }() + renderer := render.New(render.Options{ Extensions: []string{".tmpl"}, Directory: "templates", @@ -42,6 +73,7 @@ func HTMLRenderer(ctx context.Context) (context.Context, *render.Render) { IsDevelopment: false, DisableHTTPErrorRendering: true, }) + compilingTemplates = false if !setting.IsProd { watcher.CreateWatcher(ctx, "HTML Templates", &watcher.CreateWatcherOpts{ PathsCallback: walkTemplateFiles, @@ -50,3 +82,141 @@ func HTMLRenderer(ctx context.Context) (context.Context, *render.Render) { } return context.WithValue(ctx, rendererKey, renderer), renderer } + +func handlePanicError(err error) { + wrapFatal(handleNotDefinedPanicError(err)) + wrapFatal(handleUnexpected(err)) + wrapFatal(handleExpectedEnd(err)) + wrapFatal(handleGenericTemplateError(err)) +} + +func wrapFatal(format string, args ...interface{}) { + if format == "" { + return + } + log.Fatal(format, args...) +} + +func handleGenericTemplateError(err error) (string, []interface{}) { + groups := templateError.FindStringSubmatch(err.Error()) + if len(groups) != 4 { + return "", nil + } + + templateName, lineNumberStr, message := groups[1], groups[2], groups[3] + + filename, assetErr := GetAssetFilename("templates/" + templateName + ".tmpl") + if assetErr != nil { + return "", nil + } + + lineNumber, _ := strconv.Atoi(lineNumberStr) + + line := getLineFromAsset(templateName, lineNumber, "") + + return "PANIC: Unable to compile templates due to: %s in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{message, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} +} + +func handleNotDefinedPanicError(err error) (string, []interface{}) { + groups := notDefinedError.FindStringSubmatch(err.Error()) + if len(groups) != 4 { + return "", nil + } + + templateName, lineNumberStr, functionName := groups[1], groups[2], groups[3] + + functionName, _ = strconv.Unquote(`"` + functionName + `"`) + + filename, assetErr := GetAssetFilename("templates/" + templateName + ".tmpl") + if assetErr != nil { + return "", nil + } + + lineNumber, _ := strconv.Atoi(lineNumberStr) + + line := getLineFromAsset(templateName, lineNumber, functionName) + + return "PANIC: Unable to compile templates due to undefined function %q in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{functionName, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} +} + +func handleUnexpected(err error) (string, []interface{}) { + groups := unexpectedError.FindStringSubmatch(err.Error()) + if len(groups) != 4 { + return "", nil + } + + templateName, lineNumberStr, unexpected := groups[1], groups[2], groups[3] + unexpected, _ = strconv.Unquote(`"` + unexpected + `"`) + + filename, assetErr := GetAssetFilename("templates/" + templateName + ".tmpl") + if assetErr != nil { + return "", nil + } + + lineNumber, _ := strconv.Atoi(lineNumberStr) + + line := getLineFromAsset(templateName, lineNumber, unexpected) + + return "PANIC: Unable to compile templates due to unexpected %q in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} +} + +func handleExpectedEnd(err error) (string, []interface{}) { + groups := expectedEndError.FindStringSubmatch(err.Error()) + if len(groups) != 4 { + return "", nil + } + + templateName, lineNumberStr, unexpected := groups[1], groups[2], groups[3] + + filename, assetErr := GetAssetFilename("templates/" + templateName + ".tmpl") + if assetErr != nil { + return "", nil + } + + lineNumber, _ := strconv.Atoi(lineNumberStr) + + line := getLineFromAsset(templateName, lineNumber, unexpected) + + return "PANIC: Unable to compile templates due to missing end with unexpected %q in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} +} + +func getLineFromAsset(templateName string, lineNumber int, functionName string) string { + bs, err := GetAsset("templates/" + templateName + ".tmpl") + if err != nil { + return fmt.Sprintf("(unable to read template file: %v)", err) + } + + sb := &strings.Builder{} + start := 0 + var lineBs []byte + for i := 0; i < lineNumber && start < len(bs); i++ { + end := bytes.IndexByte(bs[start:], '\n') + if end < 0 { + end = len(bs) + } else { + end += start + } + lineBs = bs[start:end] + if lineNumber-i < 4 { + _, _ = sb.Write(lineBs) + _ = sb.WriteByte('\n') + } + start = end + 1 + } + + if functionName != "" { + idx := strings.Index(string(lineBs), functionName) + for i := range lineBs[:idx] { + if lineBs[i] != '\t' { + lineBs[i] = ' ' + } + } + _, _ = sb.Write(lineBs[:idx]) + if idx >= 0 { + _, _ = sb.WriteString(strings.Repeat("^", len(functionName))) + } + _ = sb.WriteByte('\n') + } + + return strings.Repeat("-", 70) + "\n" + sb.String() + strings.Repeat("-", 70) +} diff --git a/modules/templates/static.go b/modules/templates/static.go index 3265bd9cfcbc4..7cf4f2b8b338c 100644 --- a/modules/templates/static.go +++ b/modules/templates/static.go @@ -31,6 +31,17 @@ func GlobalModTime(filename string) time.Time { return timeutil.GetExecutableModTime() } +// GetAssetFilename returns the filename of the provided asset +func GetAssetFilename(name string) (string, error) { + _, err := os.Stat(filepath.Join(setting.CustomPath, name)) + if err != nil && !os.IsNotExist(err) { + return name, err + } else if err == nil { + return filepath.Join(setting.CustomPath, name), nil + } + return "(builtin) " + name, nil +} + // GetAsset get a special asset, only for chi func GetAsset(name string) ([]byte, error) { bs, err := os.ReadFile(filepath.Join(setting.CustomPath, name)) From be8d4351e8f23966f6689061e049386bd0b861b3 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 6 Oct 2022 20:51:46 +0100 Subject: [PATCH 2/5] as per delvh Signed-off-by: Andrew Thornton --- modules/templates/htmlrenderer.go | 61 ++++++++++++++++++++++--------- modules/templates/static.go | 5 ++- 2 files changed, 47 insertions(+), 19 deletions(-) diff --git a/modules/templates/htmlrenderer.go b/modules/templates/htmlrenderer.go index 2a46f4f0e5343..12296f7193f5d 100644 --- a/modules/templates/htmlrenderer.go +++ b/modules/templates/htmlrenderer.go @@ -90,7 +90,7 @@ func handlePanicError(err error) { wrapFatal(handleGenericTemplateError(err)) } -func wrapFatal(format string, args ...interface{}) { +func wrapFatal(format string, args []interface{}) { if format == "" { return } @@ -180,43 +180,70 @@ func handleExpectedEnd(err error) (string, []interface{}) { return "PANIC: Unable to compile templates due to missing end with unexpected %q in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} } -func getLineFromAsset(templateName string, lineNumber int, functionName string) string { +const dashSeparator = "----------------------------------------------------------------------\n" + +func getLineFromAsset(templateName string, targetLineNum int, target string) string { bs, err := GetAsset("templates/" + templateName + ".tmpl") if err != nil { return fmt.Sprintf("(unable to read template file: %v)", err) } sb := &strings.Builder{} - start := 0 + + // Write the header + sb.WriteString(dashSeparator) + var lineBs []byte - for i := 0; i < lineNumber && start < len(bs); i++ { + + // Iterate through the lines from the asset file to find the target line + for start, currentLineNum := 0, 1; currentLineNum <= targetLineNum && start < len(bs); currentLineNum++ { + // Find the next new line end := bytes.IndexByte(bs[start:], '\n') + + // adjust the end to be a direct pointer in to []byte if end < 0 { end = len(bs) } else { end += start } + + // set lineBs to the current line []byte lineBs = bs[start:end] - if lineNumber-i < 4 { + + // move start to after the current new line position + start = end + 1 + + // Write 2 preceding lines + the target line + if targetLineNum-currentLineNum < 3 { _, _ = sb.Write(lineBs) _ = sb.WriteByte('\n') } - start = end + 1 } - if functionName != "" { - idx := strings.Index(string(lineBs), functionName) - for i := range lineBs[:idx] { - if lineBs[i] != '\t' { - lineBs[i] = ' ' - } - } - _, _ = sb.Write(lineBs[:idx]) + // 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 { - _, _ = sb.WriteString(strings.Repeat("^", len(functionName))) + // take the current line and replace preceding text with whitespace (except for tab) + for i := range lineBs[:idx] { + if lineBs[i] != '\t' { + lineBs[i] = ' ' + } + } + + // write the preceding "space" + _, _ = sb.Write(lineBs[:idx]) + + // Now write the ^^ pointer + _, _ = sb.WriteString(strings.Repeat("^", len(target))) + _ = sb.WriteByte('\n') } - _ = sb.WriteByte('\n') } - return strings.Repeat("-", 70) + "\n" + sb.String() + strings.Repeat("-", 70) + // Finally write the footer + sb.WriteString(dashSeparator) + + return sb.String() } diff --git a/modules/templates/static.go b/modules/templates/static.go index 7cf4f2b8b338c..f655bb0ace3ec 100644 --- a/modules/templates/static.go +++ b/modules/templates/static.go @@ -33,11 +33,12 @@ func GlobalModTime(filename string) time.Time { // GetAssetFilename returns the filename of the provided asset func GetAssetFilename(name string) (string, error) { - _, err := os.Stat(filepath.Join(setting.CustomPath, name)) + filename := filepath.Join(setting.CustomPath, name) + _, err := os.Stat(filename) if err != nil && !os.IsNotExist(err) { return name, err } else if err == nil { - return filepath.Join(setting.CustomPath, name), nil + return filepath.Join(filename), nil } return "(builtin) " + name, nil } From 5d086bb854cd658ac0c9659fd0d99b06082cbd6d Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 6 Oct 2022 22:01:25 +0100 Subject: [PATCH 3/5] Update modules/templates/static.go Co-authored-by: delvh --- modules/templates/static.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/templates/static.go b/modules/templates/static.go index f655bb0ace3ec..7f7cbe702fb1c 100644 --- a/modules/templates/static.go +++ b/modules/templates/static.go @@ -38,7 +38,7 @@ func GetAssetFilename(name string) (string, error) { if err != nil && !os.IsNotExist(err) { return name, err } else if err == nil { - return filepath.Join(filename), nil + return filename, nil } return "(builtin) " + name, nil } From 62fd9263c42d425808d1e686c3a9421b4e33d34e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 6 Oct 2022 22:10:31 +0100 Subject: [PATCH 4/5] remove stacktrace and slightly adjust message Signed-off-by: Andrew Thornton --- modules/templates/htmlrenderer.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/templates/htmlrenderer.go b/modules/templates/htmlrenderer.go index 12296f7193f5d..6f84b42ec1a9e 100644 --- a/modules/templates/htmlrenderer.go +++ b/modules/templates/htmlrenderer.go @@ -94,7 +94,7 @@ func wrapFatal(format string, args []interface{}) { if format == "" { return } - log.Fatal(format, args...) + log.FatalWithSkip(1, format, args...) } func handleGenericTemplateError(err error) (string, []interface{}) { @@ -114,7 +114,7 @@ func handleGenericTemplateError(err error) (string, []interface{}) { line := getLineFromAsset(templateName, lineNumber, "") - return "PANIC: Unable to compile templates due to: %s in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{message, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} + 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)} } func handleNotDefinedPanicError(err error) (string, []interface{}) { @@ -136,7 +136,7 @@ func handleNotDefinedPanicError(err error) (string, []interface{}) { line := getLineFromAsset(templateName, lineNumber, functionName) - return "PANIC: Unable to compile templates due to undefined function %q in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{functionName, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} + 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)} } func handleUnexpected(err error) (string, []interface{}) { @@ -157,7 +157,7 @@ func handleUnexpected(err error) (string, []interface{}) { line := getLineFromAsset(templateName, lineNumber, unexpected) - return "PANIC: Unable to compile templates due to unexpected %q in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} + 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)} } func handleExpectedEnd(err error) (string, []interface{}) { @@ -177,7 +177,7 @@ func handleExpectedEnd(err error) (string, []interface{}) { line := getLineFromAsset(templateName, lineNumber, unexpected) - return "PANIC: Unable to compile templates due to missing end with unexpected %q in template file %s at line %d:\n%s\nStacktrace:\n%s", []interface{}{unexpected, filename, lineNumber, log.NewColoredValue(line, log.Reset), log.Stack(2)} + 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" From 297f862b05116446ff05cc166ce8bc819fc63b99 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 6 Oct 2022 22:16:34 +0100 Subject: [PATCH 5/5] And handle the final panic too Signed-off-by: Andrew Thornton --- modules/templates/htmlrenderer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/templates/htmlrenderer.go b/modules/templates/htmlrenderer.go index 6f84b42ec1a9e..81ea66016140e 100644 --- a/modules/templates/htmlrenderer.go +++ b/modules/templates/htmlrenderer.go @@ -60,7 +60,7 @@ func HTMLRenderer(ctx context.Context) (context.Context, *render.Render) { if ok { handlePanicError(err) } - log.Fatal("PANIC: Unable to compile templates: %v\nStacktrace:\n%s", panicked, log.Stack(2)) + log.Fatal("PANIC: Unable to compile templates!\n%v\n\nStacktrace:\n%s", panicked, log.Stack(2)) }() renderer := render.New(render.Options{