diff options
author | Bjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com> | 2018-10-23 08:54:10 +0200 |
---|---|---|
committer | Bjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com> | 2018-10-23 14:35:43 +0200 |
commit | f669ef6bec25155d015b6ab231c53caef4fa5cdc (patch) | |
tree | a76f3843a7249ccbc61ec6c8a20ac2c38e8518cc /common/herrors | |
parent | ed7b3e261909fe425ef64216f12806840c45b205 (diff) |
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
Diffstat (limited to 'common/herrors')
-rw-r--r-- | common/herrors/error_locator.go | 134 | ||||
-rw-r--r-- | common/herrors/error_locator_test.go | 54 | ||||
-rw-r--r-- | common/herrors/file_error.go | 70 | ||||
-rw-r--r-- | common/herrors/file_error_test.go | 25 |
4 files changed, 177 insertions, 106 deletions
diff --git a/common/herrors/error_locator.go b/common/herrors/error_locator.go index 306f8f46b..3f1aae689 100644 --- 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 @@ import ( 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 +} + +// LineMatcherFn is used to match a line with an error. +type LineMatcherFn func(m LineMatcher) bool -// 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 +// 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 @@ func (e *ErrorWithFileContext) Cause() error { // 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 @@ func WithFileContextForFile(e error, realFilename, filename string, fs afero.Fs, // 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 @@ func WithFileContext(e error, realFilename string, r io.Reader, matcher LineMatc } } - 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 @@ func chromaLexerFromFilename(filename string) string { 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) - - errCtx.ColumnNumber = 1 - if le != nil { - errCtx.ColumnNumber = le.ColumnNumber() +func locateError(r io.Reader, le FileError, matches LineMatcherFn) ErrorContext { + if le == nil { + panic("must provide an error") } - lineNo := 0 + errCtx := ErrorContext{LineNumber: -1, ColumnNumber: 1, Pos: -1} + + b, err := ioutil.ReadAll(r) + if err != nil { + return errCtx + } - var buff [6]string - i := 0 - errCtx.Pos = -1 + lines := strings.Split(string(b), "\n") - for s.Scan() { - lineNo++ - txt := s.Text() - buff[i] = txt + if le != nil && le.ColumnNumber() >= 0 { + errCtx.ColumnNumber = le.ColumnNumber() + } - if errCtx.Pos != -1 && i >= 5 { - break + lineNo := 0 + posBytes := 0 + + 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 diff --git a/common/herrors/error_locator_test.go b/common/herrors/error_locator_test.go index caa6e6385..e7bc3cb19 100644 --- a/common/herrors/error_locator_test.go +++ b/common/herrors/error_locator_test.go @@ -24,8 +24,8 @@ import ( 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 7 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 @@ J`, lineMatcher) 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) @@ -109,4 +111,18 @@ C 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) + } diff --git a/common/herrors/file_error.go b/common/herrors/file_error.go index 86ccfcefb..49b9f808a 100644 --- 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 @@ var _ causer = (*fileError)(nil) 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 @@ type FileError interface { var _ FileError = (*fileError)(nil) type fileError struct { + offset int lineNumber int columnNumber int fileType string @@ -39,10 +51,23 @@ type fileError struct { 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 @@ func (f *fileError) Cause() error { } // 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 @@ func UnwrapFileError(err error) FileError { 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 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, "" } - // Fall back to the original. - return err } diff --git a/common/herrors/file_error_test.go b/common/herrors/file_error_test.go index 8b1674ba1..6acb49603 100644 --- 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 @@ func TestToLineNumberError(t *testing.T) { 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 @@ func TestToLineNumberError(t *testing.T) { {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)) } + } |