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

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
+}
 
-// 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 @@ 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)
+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 {
+	for li, line := range lines {
+		lineNo = li + 1
+		m := LineMatcher{
+			FileError:  le,
+			LineNumber: lineNo,
+			Offset:     posBytes,
+			Line:       line,
+		}
+		if errCtx.Pos == -1 && matches(m) {
+			errCtx.LineNumber = lineNo
 			break
 		}
 
-		if errCtx.Pos == -1 && matches(le, lineNo, txt) {
-			errCtx.Pos = i
-			errCtx.LineNumber = lineNo
-		}
-
-		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
-
-			i++
-		}
-	}
-
-	if errCtx.Pos != -1 {
-		low := errCtx.Pos - 2
+	if errCtx.LineNumber != -1 {
+		low := errCtx.LineNumber - 3
 		if low < 0 {
 			low = 0
 		}
-		high := i
-		errCtx.Lines = buff[low:high]
 
-	} else {
-		errCtx.Pos = -1
-		errCtx.LineNumber = -1
+		if errCtx.LineNumber > 2 {
+			errCtx.Pos = 2
+		} else {
+			errCtx.Pos = errCtx.LineNumber - 1
+		}
+
+		high := errCtx.LineNumber + 2
+		if high > len(lines) {
+			high = len(lines)
+		}
+
+		errCtx.Lines = lines[low:high]
+
 	}
 
 	return errCtx

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)
+
 }

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 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, ""
+	}
 }

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))
 	}
+
 }

go.sum

@@ -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=

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.

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)
@@ -170,6 +169,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,
@@ -246,6 +259,16 @@ description = "Descriptioon"
 Some content.
 
 
+`))
+
+		b.WithContent("myjson.md", f(tomlcontent, `{
+	"title": "This is a title",
+	"description": "This is a description."
+}
+
+Some content.
+
+
 `))
 
 		createErr := b.CreateSitesE()

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)
 
 }

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

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
 			}
 		}