ref: f669ef6bec25155d015b6ab231c53caef4fa5cdc
parent: ed7b3e261909fe425ef64216f12806840c45b205
author: Bjørn Erik Pedersen <[email protected]>
date: Tue Oct 23 04:54:10 EDT 2018
herrors: Improve handling of JSON errors `*json.UnmarshalTypeError` and `*json.SyntaxError` has a byte `Offset`, so use that. This commit also reworks/simplifies the errror line matching logic. This also makes the file reading unbuffered, but that should be fine in this error case. See #5324
--- a/common/herrors/error_locator.go
+++ b/common/herrors/error_locator.go
@@ -15,9 +15,9 @@
package herrors
import (
- "bufio"
"fmt"
"io"
+ "io/ioutil"
"strings"
"github.com/gohugoio/hugo/helpers"
@@ -27,13 +27,20 @@
var fileErrorFormat = "\"%s:%d:%d\": %s"
-// LineMatcher is used to match a line with an error.
-type LineMatcher func(le FileError, lineNumber int, line string) bool
+// LineMatcher contains the elements used to match an error to a line
+type LineMatcher struct {+ FileError FileError
+ LineNumber int
+ Offset int
+ Line string
+}
-// SimpleLineMatcher matches if the current line number matches the line number
-// in the error.
-var SimpleLineMatcher = func(le FileError, lineNumber int, line string) bool {- return le.LineNumber() == lineNumber
+// LineMatcherFn is used to match a line with an error.
+type LineMatcherFn func(m LineMatcher) bool
+
+// SimpleLineMatcher simply matches by line number.
+var SimpleLineMatcher = func(m LineMatcher) bool {+ return m.FileError.LineNumber() == m.LineNumber
}
// ErrorContext contains contextual information about an error. This will
@@ -79,7 +86,7 @@
// WithFileContextForFile will try to add a file context with lines matching the given matcher.
// If no match could be found, the original error is returned with false as the second return value.
-func WithFileContextForFile(e error, realFilename, filename string, fs afero.Fs, matcher LineMatcher) (error, bool) {+func WithFileContextForFile(e error, realFilename, filename string, fs afero.Fs, matcher LineMatcherFn) (error, bool) {f, err := fs.Open(filename)
if err != nil {return e, false
@@ -90,11 +97,12 @@
// WithFileContextForFile will try to add a file context with lines matching the given matcher.
// If no match could be found, the original error is returned with false as the second return value.
-func WithFileContext(e error, realFilename string, r io.Reader, matcher LineMatcher) (error, bool) {+func WithFileContext(e error, realFilename string, r io.Reader, matcher LineMatcherFn) (error, bool) { if e == nil { panic("error missing")}
le := UnwrapFileError(e)
+
if le == nil {var ok bool
if le, ok = ToFileError("", e).(FileError); !ok {@@ -102,13 +110,27 @@
}
}
- errCtx := locateError(r, le, matcher)
- errCtx.Filename = realFilename
+ var errCtx ErrorContext
+ if le.Offset() != -1 {+ errCtx = locateError(r, le, func(m LineMatcher) bool {+ if le.Offset() >= m.Offset && le.Offset() < m.Offset+len(m.Line) {+ fe := m.FileError
+ m.FileError = ToFileErrorWithOffset(fe, -fe.LineNumber()+m.LineNumber)
+ }
+ return matcher(m)
+ })
+
+ } else {+ errCtx = locateError(r, le, matcher)
+ }
+
if errCtx.LineNumber == -1 {return e, false
}
+ errCtx.Filename = realFilename
+
if le.Type() != "" {errCtx.ChromaLexer = chromaLexerFromType(le.Type())
} else {@@ -151,72 +173,66 @@
return chromaLexerFromType(ext)
}
-func locateErrorInString(le FileError, src string, matcher LineMatcher) ErrorContext {- return locateError(strings.NewReader(src), nil, matcher)
+func locateErrorInString(src string, matcher LineMatcherFn) ErrorContext {+ return locateError(strings.NewReader(src), &fileError{}, matcher)}
-func locateError(r io.Reader, le FileError, matches LineMatcher) ErrorContext {- var errCtx ErrorContext
- s := bufio.NewScanner(r)
+func locateError(r io.Reader, le FileError, matches LineMatcherFn) ErrorContext {+ if le == nil {+ panic("must provide an error")+ }
- errCtx.ColumnNumber = 1
- if le != nil {+ errCtx := ErrorContext{LineNumber: -1, ColumnNumber: 1, Pos: -1}+
+ b, err := ioutil.ReadAll(r)
+ if err != nil {+ return errCtx
+ }
+
+ lines := strings.Split(string(b), "\n")
+
+ if le != nil && le.ColumnNumber() >= 0 {errCtx.ColumnNumber = le.ColumnNumber()
}
lineNo := 0
+ posBytes := 0
- var buff [6]string
- i := 0
- errCtx.Pos = -1
-
- for s.Scan() {- lineNo++
- txt := s.Text()
- buff[i] = txt
-
- if errCtx.Pos != -1 && i >= 5 {- break
+ for li, line := range lines {+ lineNo = li + 1
+ m := LineMatcher{+ FileError: le,
+ LineNumber: lineNo,
+ Offset: posBytes,
+ Line: line,
}
-
- if errCtx.Pos == -1 && matches(le, lineNo, txt) {- errCtx.Pos = i
+ if errCtx.Pos == -1 && matches(m) {errCtx.LineNumber = lineNo
+ break
}
- if errCtx.Pos == -1 && i == 2 {- // Shift left
- buff[0], buff[1] = buff[i-1], buff[i]
- } else {- i++
- }
+ posBytes += len(line)
}
- // Go's template parser will typically report "unexpected EOF" errors on the
- // empty last line that is supressed by the scanner.
- // Do an explicit check for that.
- if errCtx.Pos == -1 {- lineNo++
- if matches(le, lineNo, "") {- buff[i] = ""
- errCtx.Pos = i
- errCtx.LineNumber = lineNo
+ if errCtx.LineNumber != -1 {+ low := errCtx.LineNumber - 3
+ if low < 0 {+ low = 0
+ }
- i++
+ if errCtx.LineNumber > 2 {+ errCtx.Pos = 2
+ } else {+ errCtx.Pos = errCtx.LineNumber - 1
}
- }
- if errCtx.Pos != -1 {- low := errCtx.Pos - 2
- if low < 0 {- low = 0
+ high := errCtx.LineNumber + 2
+ if high > len(lines) {+ high = len(lines)
}
- high := i
- errCtx.Lines = buff[low:high]
- } else {- errCtx.Pos = -1
- errCtx.LineNumber = -1
+ errCtx.Lines = lines[low:high]
+
}
return errCtx
--- a/common/herrors/error_locator_test.go
+++ b/common/herrors/error_locator_test.go
@@ -24,8 +24,8 @@
func TestErrorLocator(t *testing.T) {assert := require.New(t)
- lineMatcher := func(le FileError, lineno int, line string) bool {- return strings.Contains(line, "THEONE")
+ lineMatcher := func(m LineMatcher) bool {+ return strings.Contains(m.Line, "THEONE")
}
lines := `LINE 1
@@ -38,49 +38,51 @@
LINE 8
`
- location := locateErrorInString(nil, lines, lineMatcher)
+ location := locateErrorInString(lines, lineMatcher)
assert.Equal([]string{"LINE 3", "LINE 4", "This is THEONE", "LINE 6", "LINE 7"}, location.Lines)assert.Equal(5, location.LineNumber)
assert.Equal(2, location.Pos)
- assert.Equal([]string{"This is THEONE"}, locateErrorInString(nil, `This is THEONE`, lineMatcher).Lines)+ assert.Equal([]string{"This is THEONE"}, locateErrorInString(`This is THEONE`, lineMatcher).Lines)- location = locateErrorInString(nil, `L1
+ location = locateErrorInString(`L1
This is THEONE
L2
`, lineMatcher)
+ assert.Equal(2, location.LineNumber)
assert.Equal(1, location.Pos)
- assert.Equal([]string{"L1", "This is THEONE", "L2"}, location.Lines)+ assert.Equal([]string{"L1", "This is THEONE", "L2", ""}, location.Lines)- location = locateErrorInString(nil, `This is THEONE
+ location = locateErrorInString(`This is THEONE
L2
`, lineMatcher)
assert.Equal(0, location.Pos)
- assert.Equal([]string{"This is THEONE", "L2"}, location.Lines)+ assert.Equal([]string{"This is THEONE", "L2", ""}, location.Lines)- location = locateErrorInString(nil, `L1
+ location = locateErrorInString(`L1
This THEONE
`, lineMatcher)
- assert.Equal([]string{"L1", "This THEONE"}, location.Lines)+ assert.Equal([]string{"L1", "This THEONE", ""}, location.Lines)assert.Equal(1, location.Pos)
- location = locateErrorInString(nil, `L1
+ location = locateErrorInString(`L1
L2
This THEONE
`, lineMatcher)
- assert.Equal([]string{"L1", "L2", "This THEONE"}, location.Lines)+ assert.Equal([]string{"L1", "L2", "This THEONE", ""}, location.Lines)assert.Equal(2, location.Pos)
- location = locateErrorInString(nil, "NO MATCH", lineMatcher)
+ location = locateErrorInString("NO MATCH", lineMatcher)assert.Equal(-1, location.LineNumber)
assert.Equal(-1, location.Pos)
assert.Equal(0, len(location.Lines))
- lineMatcher = func(le FileError, lineno int, line string) bool {- return lineno == 6
+ lineMatcher = func(m LineMatcher) bool {+ return m.LineNumber == 6
}
- location = locateErrorInString(nil, `A
+
+ location = locateErrorInString(`A
B
C
D
@@ -96,11 +98,11 @@
assert.Equal(2, location.Pos)
// Test match EOF
- lineMatcher = func(le FileError, lineno int, line string) bool {- return lineno == 4
+ lineMatcher = func(m LineMatcher) bool {+ return m.LineNumber == 4
}
- location = locateErrorInString(nil, `A
+ location = locateErrorInString(`A
B
C
`, lineMatcher)
@@ -108,5 +110,19 @@
assert.Equal([]string{"B", "C", ""}, location.Lines)assert.Equal(4, location.LineNumber)
assert.Equal(2, location.Pos)
+
+ offsetMatcher := func(m LineMatcher) bool {+ return m.Offset == 1
+ }
+
+ location = locateErrorInString(`A
+B
+C
+D
+E`, offsetMatcher)
+
+ assert.Equal([]string{"A", "B", "C", "D"}, location.Lines)+ assert.Equal(2, location.LineNumber)
+ assert.Equal(1, location.Pos)
}
--- a/common/herrors/file_error.go
+++ b/common/herrors/file_error.go
@@ -13,6 +13,12 @@
package herrors
+import (
+ "encoding/json"
+
+ "github.com/pkg/errors"
+)
+
var _ causer = (*fileError)(nil)
// FileError represents an error when handling a file: Parsing a config file,
@@ -20,9 +26,14 @@
type FileError interface {error
+ // Offset gets the error location offset in bytes, starting at 0.
+ // It will return -1 if not provided.
+ Offset() int
+
// LineNumber gets the error location, starting at line 1.
LineNumber() int
+ // Column number gets the column location, starting at 1.
ColumnNumber() int
// A string identifying the type of file, e.g. JSON, TOML, markdown etc.
@@ -32,6 +43,7 @@
var _ FileError = (*fileError)(nil)
type fileError struct {+ offset int
lineNumber int
columnNumber int
fileType string
@@ -39,10 +51,23 @@
cause error
}
+type fileErrorWithLineOffset struct {+ FileError
+ offset int
+}
+
+func (e *fileErrorWithLineOffset) LineNumber() int {+ return e.FileError.LineNumber() + e.offset
+}
+
func (e *fileError) LineNumber() int {return e.lineNumber
}
+func (e *fileError) Offset() int {+ return e.offset
+}
+
func (e *fileError) ColumnNumber() int {return e.columnNumber
}
@@ -63,8 +88,8 @@
}
// NewFileError creates a new FileError.
-func NewFileError(fileType string, lineNumber, columnNumber int, err error) FileError {- return &fileError{cause: err, fileType: fileType, lineNumber: lineNumber, columnNumber: columnNumber}+func NewFileError(fileType string, offset, lineNumber, columnNumber int, err error) FileError {+ return &fileError{cause: err, fileType: fileType, offset: offset, lineNumber: lineNumber, columnNumber: columnNumber}}
// UnwrapFileError tries to unwrap a FileError from err.
@@ -83,24 +108,37 @@
return nil
}
-// ToFileError will try to convert the given error to an error supporting
-// the FileError interface.
-// If will fall back to returning the original error if a line number cannot be extracted.
-func ToFileError(fileType string, err error) error {- return ToFileErrorWithOffset(fileType, err, 0)
+// ToFileErrorWithOffset will return a new FileError with a line number
+// with the given offset from the original.
+func ToFileErrorWithOffset(fe FileError, offset int) FileError {+ return &fileErrorWithLineOffset{FileError: fe, offset: offset}}
-// ToFileErrorWithOffset will try to convert the given error to an error supporting
-// the FileError interface. It will take any line number offset given into account.
-// If will fall back to returning the original error if a line number cannot be extracted.
-func ToFileErrorWithOffset(fileType string, err error, offset int) error {+// ToFileError will convert the given error to an error supporting
+// the FileError interface.
+func ToFileError(fileType string, err error) FileError { for _, handle := range lineNumberExtractors {-
lno, col := handle(err)
- if lno > 0 {- return NewFileError(fileType, lno+offset, col, err)
+ offset, typ := extractOffsetAndType(err)
+ if fileType == "" {+ fileType = typ
}
+ if lno > 0 || offset != -1 {+ return NewFileError(fileType, offset, lno, col, err)
+ }
}
- // Fall back to the original.
- return err
+ // Fall back to the pointing to line number 1.
+ return NewFileError(fileType, -1, 1, 1, err)
+}
+
+func extractOffsetAndType(e error) (int, string) {+ e = errors.Cause(e)
+ switch v := e.(type) {+ case *json.UnmarshalTypeError:
+ return int(v.Offset), "json"
+ case *json.SyntaxError:
+ return int(v.Offset), "json"
+ default:
+ return -1, ""
+ }
}
--- a/common/herrors/file_error_test.go
+++ b/common/herrors/file_error_test.go
@@ -14,11 +14,11 @@
package herrors
import (
- "errors"
"fmt"
- "strconv"
"testing"
+ "github.com/pkg/errors"
+
"github.com/stretchr/testify/require"
)
@@ -33,7 +33,7 @@
lineNumber int
columnNumber int
}{- {errors.New("no line number for you"), 0, -1, 1},+ {errors.New("no line number for you"), 0, 1, 1}, {errors.New(`template: _default/single.html:4:15: executing "_default/single.html" at <.Titles>: can't evaluate field Titles in type *hugolib.PageOutput`), 0, 4, 15}, {errors.New("parse failed: template: _default/bundle-resource-meta.html:11: unexpected in operand"), 0, 11, 1}, {errors.New(`failed:: template: _default/bundle-resource-meta.html:2:7: executing "main" at <.Titles>`), 0, 2, 7},@@ -41,18 +41,19 @@
{errors.New(`failed to load translations: (6, 7): was expecting token =, but got "g" instead`), 0, 6, 7}, } {- got := ToFileErrorWithOffset("template", test.in, test.offset)+ got := ToFileError("template", test.in)+ if test.offset > 0 {+ got = ToFileErrorWithOffset(got.(FileError), test.offset)
+ }
errMsg := fmt.Sprintf("[%d][%T]", i, got)le, ok := got.(FileError)
+ assert.True(ok)
- if test.lineNumber > 0 {- assert.True(ok, errMsg)
- assert.Equal(test.lineNumber, le.LineNumber(), errMsg)
- assert.Equal(test.columnNumber, le.ColumnNumber(), errMsg)
- assert.Contains(got.Error(), strconv.Itoa(le.LineNumber()))
- } else {- assert.False(ok)
- }
+ assert.True(ok, errMsg)
+ assert.Equal(test.lineNumber, le.LineNumber(), errMsg)
+ assert.Equal(test.columnNumber, le.ColumnNumber(), errMsg)
+ assert.Error(errors.Cause(got))
}
+
}
--- a/go.sum
+++ b/go.sum
@@ -65,6 +65,7 @@
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/mattn/go-isatty v0.0.4 h1:bnP0vzxcAdeI1zdubAl5PjU6zsERjGZb7raWodagDYs=
github.com/mattn/go-isatty v0.0.4/go.mod h1:M+lRXTBqGeGNdLjl/ufCoiOlB5xdOkqRJdNxMWT7Zi4=
--- a/hugolib/hugo_sites.go
+++ b/hugolib/hugo_sites.go
@@ -1,4 +1,4 @@
-// Copyright 2016-present The Hugo Authors. All rights reserved.
+// Copyright 2018 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.
--- a/hugolib/hugo_sites_build_errors_test.go
+++ b/hugolib/hugo_sites_build_errors_test.go
@@ -150,8 +150,7 @@
name: "Invalid YAML front matter",
fileType: yamlcontent,
fileFixer: func(content string) string {- // TODO(bep) 2errors YAML line numbers seems to be off by one for > 1 line.
- return strings.Replace(content, "title:", "title", 1)
+ return strings.Replace(content, "title:", "title: %foo", 1)
},
assertBuildError: func(a testSiteBuildErrorAsserter, err error) {a.assertLineNumber(2, err)
@@ -171,6 +170,20 @@
},
},
{+ name: "Invalid JSON front matter",
+ fileType: tomlcontent,
+ fileFixer: func(content string) string {+ return strings.Replace(content, "\"description\":", "\"description\"", 1)
+ },
+ assertBuildError: func(a testSiteBuildErrorAsserter, err error) {+ fe := a.getFileError(err)
+
+ assert.Equal(3, fe.LineNumber)
+ assert.Equal("json", fe.ErrorContext.ChromaLexer)+
+ },
+ },
+ {name: "Panic in template Execute",
fileType: single,
fileFixer: func(content string) string {@@ -242,6 +255,16 @@
p3 = "v"
description = "Descriptioon"
+++
+
+Some content.
+
+
+`))
+
+ b.WithContent("myjson.md", f(tomlcontent, `{+ "title": "This is a title",
+ "description": "This is a description."
+}
Some content.
--- a/hugolib/page_content.go
+++ b/hugolib/page_content.go
@@ -89,7 +89,11 @@
f := metadecoders.FormatFromFrontMatterType(it.Type)
m, err := metadecoders.UnmarshalToMap(it.Val, f)
if err != nil {- return herrors.ToFileErrorWithOffset(string(f), err, iter.LineNumber()-1)
+ if fe, ok := err.(herrors.FileError); ok {+ return herrors.ToFileErrorWithOffset(fe, iter.LineNumber()-1)
+ } else {+ return err
+ }
}
if err := p.updateMetaData(m); err != nil {return err
@@ -192,6 +196,6 @@
input = input[:pos]
lineNumber := bytes.Count(input, lf) + 1
endOfLastLine := bytes.LastIndex(input, lf)
- return herrors.NewFileError("md", lineNumber, pos-endOfLastLine, err)+ return herrors.NewFileError("md", -1, lineNumber, pos-endOfLastLine, err)}
--- a/parser/metadecoders/decoder.go
+++ b/parser/metadecoders/decoder.go
@@ -17,6 +17,8 @@
"encoding/json"
"fmt"
+ "github.com/gohugoio/hugo/common/herrors"
+
"github.com/BurntSushi/toml"
"github.com/chaseadamsio/goorgeous"
"github.com/pkg/errors"
@@ -59,7 +61,7 @@
case ORG:
vv, err := goorgeous.OrgHeaders(data)
if err != nil {- return errors.Wrap(err, "failed to unmarshal ORG headers")
+ return toFileError(f, errors.Wrap(err, "failed to unmarshal ORG headers"))
}
switch v.(type) { case *map[string]interface{}:@@ -74,7 +76,7 @@
case YAML:
err = yaml.Unmarshal(data, v)
if err != nil {- return errors.Wrap(err, "failed to unmarshal YAML")
+ return toFileError(f, errors.Wrap(err, "failed to unmarshal YAML"))
}
// To support boolean keys, the YAML package unmarshals maps to
@@ -103,8 +105,16 @@
return errors.Errorf("unmarshal of format %q is not supported", f)}
- return errors.Wrap(err, "unmarshal failed")
+ if err == nil {+ return nil
+ }
+ return toFileError(f, errors.Wrap(err, "unmarshal failed"))
+
+}
+
+func toFileError(f Format, err error) error {+ return herrors.ToFileError(string(f), err)
}
// stringifyMapKeys recurses into in and changes all instances of
--- a/tpl/template.go
+++ b/tpl/template.go
@@ -162,8 +162,8 @@
// Since this can be a composite of multiple template files (single.html + baseof.html etc.)
// we potentially need to look in both -- and cannot rely on line number alone.
- lineMatcher := func(le herrors.FileError, lineNumber int, line string) bool {- if le.LineNumber() != lineNumber {+ lineMatcher := func(m herrors.LineMatcher) bool {+ if m.FileError.LineNumber() != m.LineNumber {return false
}
if !hasMaster {@@ -170,10 +170,10 @@
return true
}
- identifiers := t.extractIdentifiers(le.Error())
+ identifiers := t.extractIdentifiers(m.FileError.Error())
for _, id := range identifiers {- if strings.Contains(line, id) {+ if strings.Contains(m.Line, id) {return true
}
}