diff options
author | Bjørn Erik Pedersen <[email protected]> | 2018-10-23 08:54:10 +0200 |
---|---|---|
committer | Bjørn Erik Pedersen <[email protected]> | 2018-10-23 14:35:43 +0200 |
commit | f669ef6bec25155d015b6ab231c53caef4fa5cdc (patch) | |
tree | a76f3843a7249ccbc61ec6c8a20ac2c38e8518cc | |
parent | ed7b3e261909fe425ef64216f12806840c45b205 (diff) | |
download | hugo-f669ef6bec25155d015b6ab231c53caef4fa5cdc.tar.gz hugo-f669ef6bec25155d015b6ab231c53caef4fa5cdc.zip |
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
-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 | ||||
-rw-r--r-- | go.sum | 1 | ||||
-rw-r--r-- | hugolib/hugo_sites.go | 2 | ||||
-rw-r--r-- | hugolib/hugo_sites_build_errors_test.go | 27 | ||||
-rw-r--r-- | hugolib/page_content.go | 8 | ||||
-rw-r--r-- | parser/metadecoders/decoder.go | 16 | ||||
-rw-r--r-- | tpl/template.go | 8 |
10 files changed, 227 insertions, 118 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)) } + } @@ -65,6 +65,7 @@ github.com/magefile/mage v1.4.0 h1:RI7B1CgnPAuu2O9lWszwya61RLmfL0KCdo+QyyI/Bhk= 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= diff --git a/hugolib/hugo_sites.go b/hugolib/hugo_sites.go index a184e8877..65e3260f6 100644 --- 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. diff --git a/hugolib/hugo_sites_build_errors_test.go b/hugolib/hugo_sites_build_errors_test.go index f290022e0..1e53eb3c4 100644 --- a/hugolib/hugo_sites_build_errors_test.go +++ b/hugolib/hugo_sites_build_errors_test.go @@ -150,8 +150,7 @@ func TestSiteBuildErrors(t *testing.T) { 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 @@ func TestSiteBuildErrors(t *testing.T) { }, }, { + 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 { @@ -248,6 +261,16 @@ Some content. `)) + b.WithContent("myjson.md", f(tomlcontent, `{ + "title": "This is a title", + "description": "This is a description." +} + +Some content. + + +`)) + createErr := b.CreateSitesE() if test.assertCreateError != nil { test.assertCreateError(errorAsserter, createErr) diff --git a/hugolib/page_content.go b/hugolib/page_content.go index 8c20db761..be015253b 100644 --- a/hugolib/page_content.go +++ b/hugolib/page_content.go @@ -89,7 +89,11 @@ Loop: 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 @@ func parseError(err error, input []byte, pos int) error { 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) } diff --git a/parser/metadecoders/decoder.go b/parser/metadecoders/decoder.go index 0cb6afa5b..47d8af912 100644 --- a/parser/metadecoders/decoder.go +++ b/parser/metadecoders/decoder.go @@ -17,6 +17,8 @@ import ( "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 @@ func unmarshal(data []byte, f Format, v interface{}) error { 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 @@ func unmarshal(data []byte, f Format, v interface{}) error { 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 @@ func unmarshal(data []byte, f Format, v interface{}) error { 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 diff --git a/tpl/template.go b/tpl/template.go index 09710206e..12a4607fb 100644 --- a/tpl/template.go +++ b/tpl/template.go @@ -162,18 +162,18 @@ func (t *TemplateAdapter) addFileContext(name string, inerr error) error { // 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 { 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 } } |