Skip to content

Commit

Permalink
Replace use of yaml.v2 library with yaml.v3 (#7091)
Browse files Browse the repository at this point in the history
Some quirks encountered in the upgrade:
- Errors reported by the parser are now zero-indexed
- A few errors seemingly reported on the line after the
  error rather than where it happened
- Also a few tests where the line number reported
  *previously* seemed wrong but now is right
- Something different in how the parser unmarshalled to
  the "raw" schema annotation type we used. Changed to
  use a a map instead of that type alias.. but I'm not
  really sure why that had to be done

Perhaps worth looking into this further. But pushing this
now to start that discussion.

Fixes #7090

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert authored Oct 6, 2024
1 parent 61c551c commit 1d51c1d
Show file tree
Hide file tree
Showing 24 changed files with 36 additions and 10,057 deletions.
24 changes: 11 additions & 13 deletions ast/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
"strings"
"unicode/utf8"

"gopkg.in/yaml.v2"
"gopkg.in/yaml.v3"

"github.com/open-policy-agent/opa/ast/internal/scanner"
"github.com/open-policy-agent/opa/ast/internal/tokens"
Expand Down Expand Up @@ -2309,12 +2309,10 @@ type rawAnnotation struct {
Organizations []string `yaml:"organizations"`
RelatedResources []interface{} `yaml:"related_resources"`
Authors []interface{} `yaml:"authors"`
Schemas []rawSchemaAnnotation `yaml:"schemas"`
Schemas []map[string]any `yaml:"schemas"`
Custom map[string]interface{} `yaml:"custom"`
}

type rawSchemaAnnotation map[string]interface{}

type metadataParser struct {
buf *bytes.Buffer
comments []*Comment
Expand Down Expand Up @@ -2345,9 +2343,8 @@ func (b *metadataParser) Parse() (*Annotations, error) {
var comment *Comment
match := yamlLineErrRegex.FindStringSubmatch(err.Error())
if len(match) == 2 {
n, err2 := strconv.Atoi(match[1])
index, err2 := strconv.Atoi(match[1])
if err2 == nil {
index := n - 1 // line numbering is 1-based so subtract one from row
if index >= len(b.comments) {
comment = b.comments[len(b.comments)-1]
} else {
Expand Down Expand Up @@ -2397,7 +2394,7 @@ func (b *metadataParser) Parse() (*Annotations, error) {
if err != nil {
return nil, err
}
case map[interface{}]interface{}:
case map[string]any:
w, err := convertYAMLMapKeyTypes(v, nil)
if err != nil {
return nil, fmt.Errorf("invalid schema definition: %w", err)
Expand Down Expand Up @@ -2446,8 +2443,9 @@ func (b *metadataParser) Parse() (*Annotations, error) {
return &result, nil
}

// augmentYamlError augments a YAML error with hints intended to help the user figure out the cause of an otherwise cryptic error.
// These are hints, instead of proper errors, because they are educated guesses, and aren't guaranteed to be correct.
// augmentYamlError augments a YAML error with hints intended to help the user figure out the cause of an otherwise
// cryptic error. These are hints, instead of proper errors, because they are educated guesses, and aren't guaranteed
// to be correct.
func augmentYamlError(err error, comments []*Comment) error {
// Adding hints for when key/value ':' separator isn't suffixed with a legal YAML space symbol
for _, comment := range comments {
Expand Down Expand Up @@ -2601,11 +2599,11 @@ func parseAuthorString(s string) (*AuthorAnnotation, error) {
return &AuthorAnnotation{Name: name, Email: email}, nil
}

func convertYAMLMapKeyTypes(x interface{}, path []string) (interface{}, error) {
func convertYAMLMapKeyTypes(x any, path []string) (any, error) {
var err error
switch x := x.(type) {
case map[interface{}]interface{}:
result := make(map[string]interface{}, len(x))
case map[any]any:
result := make(map[string]any, len(x))
for k, v := range x {
str, ok := k.(string)
if !ok {
Expand All @@ -2617,7 +2615,7 @@ func convertYAMLMapKeyTypes(x interface{}, path []string) (interface{}, error) {
}
}
return result, nil
case []interface{}:
case []any:
for i := range x {
x[i], err = convertYAMLMapKeyTypes(x[i], append(path, fmt.Sprintf("%d", i)))
if err != nil {
Expand Down
28 changes: 24 additions & 4 deletions ast/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5052,7 +5052,7 @@ public_servers[server] {
ports[k].networks[l] = networks[m].id;
networks[m].public = true
}`,
expError: "test.rego:14: rego_parse_error: yaml: line 7: could not find expected ':'",
expError: "test.rego:14: rego_parse_error: yaml: line 6: could not find expected ':'",
},
{
note: "Ill-structured (invalid) annotation document path",
Expand Down Expand Up @@ -5146,7 +5146,7 @@ public_servers[server] {
networks[m].public = true
}`,
expNumComments: 6,
expError: "rego_parse_error: yaml: line 3: did not find expected key",
expError: "rego_parse_error: yaml: line 2: did not find expected key",
},
{
note: "Multiple rules with and without metadata",
Expand Down Expand Up @@ -6058,7 +6058,7 @@ func TestAnnotationsAugmentedError(t *testing.T) {
"# scope: rule\n" +
"p := true\n",
expErrorHint: "Hint: on line 4, symbol(s) ['\\u00a0'] immediately following a key/value separator ':' is not a legal yaml space character",
expErrorRow: 5,
expErrorRow: 6, // Should be 5 really, and yaml.v2 reported the error as on line 1, but v3 on line 3..
},
{
note: "thin whitespace (\\u2009) after key/value separator",
Expand Down Expand Up @@ -6101,7 +6101,7 @@ func TestAnnotationsAugmentedError(t *testing.T) {
"p := true\n",
expErrorHint: "Hint: on line 4, symbol(s) ['\\u3000'] immediately following a key/value separator ':' is not a legal yaml space character\n" +
" Hint: on line 6, symbol(s) ['\\u2009'] immediately following a key/value separator ':' is not a legal yaml space character",
expErrorRow: 5,
expErrorRow: 6,
},
}

Expand Down Expand Up @@ -6130,6 +6130,26 @@ func TestAnnotationsAugmentedError(t *testing.T) {
}
}

func TestAnnotationsAreParsedAsYamlv1_2(t *testing.T) {
policy := `package p
# METADATA
# custom:
# string: yes
is_string := rego.metadata.rule().custom.string == "yes"
`
mod := MustParseModuleWithOpts(policy, ParserOptions{ProcessAnnotation: true})

if len(mod.Annotations) != 1 {
t.Fatalf("Expected exactly one annotation but got %v", len(mod.Annotations))
}

anno := mod.Annotations[0]
if value, ok := anno.Custom["string"].(string); !ok || value != "yes" {
t.Fatalf("Expected custom.string to be 'yes' but got %v", value)
}
}

// https://github.com/open-policy-agent/opa/issues/6587
func TestAnnotationsParseErrorOnFirstRowGetsCorrectLocation(t *testing.T) {
module := `# METADATA
Expand Down
1 change: 0 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ require (
golang.org/x/time v0.6.0
google.golang.org/grpc v1.67.1
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c
gopkg.in/yaml.v2 v2.4.0
gopkg.in/yaml.v3 v3.0.1
oras.land/oras-go/v2 v2.3.1
sigs.k8s.io/yaml v1.4.0
Expand Down
1 change: 0 additions & 1 deletion go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,6 @@ gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EV
gopkg.in/ini.v1 v1.67.0 h1:Dgnx+6+nfE+IfzjUEISNeydPJh9AXNNsWbGP9KzCsOA=
gopkg.in/ini.v1 v1.67.0/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k=
gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY=
gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
Expand Down
2 changes: 1 addition & 1 deletion internal/gqlparser/parser/testrunner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (

"github.com/andreyvit/diff"
"github.com/open-policy-agent/opa/internal/gqlparser/gqlerror"
"gopkg.in/yaml.v2"
"gopkg.in/yaml.v3"
)

type Features map[string][]Spec
Expand Down
17 changes: 0 additions & 17 deletions vendor/gopkg.in/yaml.v2/.travis.yml

This file was deleted.

201 changes: 0 additions & 201 deletions vendor/gopkg.in/yaml.v2/LICENSE

This file was deleted.

Loading

0 comments on commit 1d51c1d

Please sign in to comment.