Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow to get source position of unmarshal errors (enhanced version) #901

Open
wants to merge 6 commits into
base: v3
Choose a base branch
from
Open

Allow to get source position of unmarshal errors (enhanced version) #901

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5fb3956
Remember positions of unmarshal errors
rhysd Jun 30, 2021
412f013
Use strings.Builder to make error message efficiently
rhysd Jun 30, 2021
27c67bd
Implement unwrapping of terminal errors
andig Sep 18, 2022
df93c53
Test ParserError
andig Sep 19, 2022
9dda667
Merge branch 'go-yaml:v3' into error-position-plus-failures
andig Apr 20, 2024
1ff5761
Allow unwrapping of non-terminal TypeErrors (#1)
andig May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 22 additions & 11 deletions decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"io"
"math"
"reflect"
"strconv"
"time"
)

Expand Down Expand Up @@ -107,7 +106,6 @@ func (p *parser) peek() yaml_event_type_t {
}

func (p *parser) fail() {
var where string
var line int
if p.parser.context_mark.line != 0 {
line = p.parser.context_mark.line
Expand All @@ -122,16 +120,13 @@ func (p *parser) fail() {
line++
}
}
if line != 0 {
where = "line " + strconv.Itoa(line) + ": "
}
var msg string
if len(p.parser.problem) > 0 {
msg = p.parser.problem
} else {
msg = "unknown problem parsing YAML content"
}
failf("%s%s", where, msg)
fail(&ParserError{msg, line})
}

func (p *parser) anchor(n *Node, anchor []byte) {
Expand Down Expand Up @@ -310,7 +305,7 @@ func (p *parser) mapping() *Node {
type decoder struct {
doc *Node
aliases map[*Node]bool
terrors []string
terrors []UnmarshalError

stringMapType reflect.Type
generalMapType reflect.Type
Expand Down Expand Up @@ -354,7 +349,11 @@ func (d *decoder) terror(n *Node, tag string, out reflect.Value) {
value = " `" + value + "`"
}
}
d.terrors = append(d.terrors, fmt.Sprintf("line %d: cannot unmarshal %s%s into %s", n.Line, shortTag(tag), value, out.Type()))
d.terrors = append(d.terrors, UnmarshalError{
Message: fmt.Sprintf("cannot unmarshal %s%s into %s", shortTag(tag), value, out.Type()),
Line: n.Line,
Column: n.Column,
})
}

func (d *decoder) callUnmarshaler(n *Node, u Unmarshaler) (good bool) {
Expand Down Expand Up @@ -768,7 +767,11 @@ func (d *decoder) mapping(n *Node, out reflect.Value) (good bool) {
for j := i + 2; j < l; j += 2 {
nj := n.Content[j]
if ni.Kind == nj.Kind && ni.Value == nj.Value {
d.terrors = append(d.terrors, fmt.Sprintf("line %d: mapping key %#v already defined at line %d", nj.Line, nj.Value, ni.Line))
d.terrors = append(d.terrors, UnmarshalError{
Message: fmt.Sprintf("mapping key %#v already defined at line %d", nj.Value, ni.Line),
Line: nj.Line,
Column: nj.Column,
})
}
}
}
Expand Down Expand Up @@ -888,7 +891,11 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
if info, ok := sinfo.FieldsMap[name.String()]; ok {
if d.uniqueKeys {
if doneFields[info.Id] {
d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s already set in type %s", ni.Line, name.String(), out.Type()))
d.terrors = append(d.terrors, UnmarshalError{
Message: fmt.Sprintf("field %s already set in type %s", name.String(), out.Type()),
Line: ni.Line,
Column: ni.Column,
})
continue
}
doneFields[info.Id] = true
Expand All @@ -908,7 +915,11 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
d.unmarshal(n.Content[i+1], value)
inlineMap.SetMapIndex(name, value)
} else if d.knownFields {
d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s not found in type %s", ni.Line, name.String(), out.Type()))
d.terrors = append(d.terrors, UnmarshalError{
Message: fmt.Sprintf("field %s not found in type %s", name.String(), out.Type()),
Line: ni.Line,
Column: ni.Column,
})
}
}
return true
Expand Down
28 changes: 20 additions & 8 deletions decode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,6 +979,18 @@ func (s *S) TestDecoderErrors(c *C) {
}
}

func (s *S) TestParserError(c *C) {
var v struct {
A, B int
}
data := "a: 1\n=\nb: 2"
err := yaml.Unmarshal([]byte(data), &v)
c.Assert(err, DeepEquals, &yaml.ParserError{
Message: "could not find expected ':'",
Line: 2,
})
}

var unmarshalerTests = []struct {
data, tag string
value interface{}
Expand Down Expand Up @@ -1111,8 +1123,8 @@ func (s *S) TestUnmarshalerWholeDocument(c *C) {
}

func (s *S) TestUnmarshalerTypeError(c *C) {
unmarshalerResult[2] = &yaml.TypeError{[]string{"foo"}}
unmarshalerResult[4] = &yaml.TypeError{[]string{"bar"}}
unmarshalerResult[2] = &yaml.TypeError{[]yaml.UnmarshalError{{"foo", 1, 1}}}
unmarshalerResult[4] = &yaml.TypeError{[]yaml.UnmarshalError{{"bar", 1, 1}}}
defer func() {
delete(unmarshalerResult, 2)
delete(unmarshalerResult, 4)
Expand All @@ -1129,8 +1141,8 @@ func (s *S) TestUnmarshalerTypeError(c *C) {
c.Assert(err, ErrorMatches, ""+
"yaml: unmarshal errors:\n"+
" line 1: cannot unmarshal !!str `A` into int\n"+
" foo\n"+
" bar\n"+
" line 1: foo\n"+
" line 1: bar\n"+
" line 1: cannot unmarshal !!str `B` into int")
c.Assert(v.M["abc"], NotNil)
c.Assert(v.M["def"], IsNil)
Expand All @@ -1142,8 +1154,8 @@ func (s *S) TestUnmarshalerTypeError(c *C) {
}

func (s *S) TestObsoleteUnmarshalerTypeError(c *C) {
unmarshalerResult[2] = &yaml.TypeError{[]string{"foo"}}
unmarshalerResult[4] = &yaml.TypeError{[]string{"bar"}}
unmarshalerResult[2] = &yaml.TypeError{[]yaml.UnmarshalError{{"foo", 1, 1}}}
unmarshalerResult[4] = &yaml.TypeError{[]yaml.UnmarshalError{{"bar", 1, 1}}}
defer func() {
delete(unmarshalerResult, 2)
delete(unmarshalerResult, 4)
Expand All @@ -1160,8 +1172,8 @@ func (s *S) TestObsoleteUnmarshalerTypeError(c *C) {
c.Assert(err, ErrorMatches, ""+
"yaml: unmarshal errors:\n"+
" line 1: cannot unmarshal !!str `A` into int\n"+
" foo\n"+
" bar\n"+
" line 1: foo\n"+
" line 1: bar\n"+
" line 1: cannot unmarshal !!str `B` into int")
c.Assert(v.M["abc"], NotNil)
c.Assert(v.M["def"], IsNil)
Expand Down
44 changes: 39 additions & 5 deletions yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"fmt"
"io"
"reflect"
"strconv"
"strings"
"sync"
"unicode/utf8"
Expand Down Expand Up @@ -288,7 +289,7 @@ func (e *Encoder) Close() (err error) {

func handleErr(err *error) {
if v := recover(); v != nil {
if e, ok := v.(yamlError); ok {
if e, ok := v.(*yamlError); ok {
*err = e.err
} else {
panic(v)
Expand All @@ -300,24 +301,57 @@ type yamlError struct {
err error
}

func (e *yamlError) Unwrap() error {
return e.err
}

func fail(err error) {
panic(yamlError{err})
panic(&yamlError{err})
}

func failf(format string, args ...interface{}) {
panic(yamlError{fmt.Errorf("yaml: "+format, args...)})
panic(&yamlError{fmt.Errorf("yaml: "+format, args...)})
}

// ParserError is each error with a source line position found by the parser.
// Unlike UnmarshalErrors, it is only used for terminal failures.
type ParserError struct {
Message string
Line int
}

func (e *ParserError) Error() string {
var b strings.Builder
b.WriteString("yaml: ")
if e.Line != 0 {
b.WriteString("line " + strconv.Itoa(e.Line) + ": ")
}
b.WriteString(e.Message)
return b.String()
}

// UnmarshalError is each error with a source position found by Unmarshal.
type UnmarshalError struct {
Message string
Line int
Column int
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding a way to differentiate what type of error actually happened, because not all errors have the same severity. For instance, an error because the file contains an unknown field may only constitute printing a warning, while not being able to unmarshal a known field would probably be reason enough to stop processing the file altogether.

I added a snippet of one possible solution that could plug into the current code. Another would be to create separate Go types for each error, although that would be a bigger change.

Suggested change
type UnmarshalError struct {
Message string
Line int
Column int
}
type UnmarshalError struct {
Type UnmarshalErrorType
Message string
Line int
Column int
}
type UnmarshalErrorType int
const (
UnknownField UnmarshalErrorType = iota
CannotUnmarshal UnmarshalErrorType
// ...
)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose to do this in a separate PR as you'll also need to touch the places where these types are generated?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First a disclaimer: I'm not a maintainer of this project, so you don't get the wrong idea, my comments don't have a lot of weight 🙂

The type would only have to be added to the lines where UnmarshalError is created, so all the lines that would need to change are already touched in this PR. That said, a separate PR sounds reasonable to me! I might create it and base it on your PR since I need the functionality in my project.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had something like this in mind: lovromazgon@456dd73

It changes UnmarshalError to an interface and creates specific types for each error. This allows you to know exactly what error happened and act accordingly.

I won't create a PR until @niemeyer responds to this PR though, to not create more noise and confusion.


// A TypeError is returned by Unmarshal when one or more fields in
// the YAML document cannot be properly decoded into the requested
// types. When this error is returned, the value is still
// unmarshaled partially.
type TypeError struct {
Errors []string
Errors []UnmarshalError
}

func (e *TypeError) Error() string {
return fmt.Sprintf("yaml: unmarshal errors:\n %s", strings.Join(e.Errors, "\n "))
var b strings.Builder
b.WriteString("yaml: unmarshal errors:")
for _, err := range e.Errors {
b.WriteString(fmt.Sprintf("\n line %d: %s", err.Line, err.Message))
}
return b.String()
}

type Kind uint32
Expand Down