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.

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

[yaml]: Update YAML parser to yaml.v3 #72

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ require (
github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348
go.starlark.net v0.0.0-20190604130855-6ddc71c0ba77
gopkg.in/yaml.v2 v2.2.1
gopkg.in/yaml.v3 v3.0.0-20200121175148-a6ecf24a6d71
)

replace github.com/kylelemons/godebug => github.com/jmillikin-stripe/godebug v0.0.0-20180620173319-8279e1966bc1
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,5 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v2 v2.2.1 h1:mUhvW9EsL+naU5Q3cakzfE91YhliOondGd6ZrsDBHQE=
gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why these gopkg.in/yaml.v2 v2.2.1 lines stuck around? Is there an import still pulling in yaml.v2 somewhere?

gopkg.in/yaml.v3 v3.0.0-20200121175148-a6ecf24a6d71 h1:Xe2gvTZUJpsvOWUnvmL/tmhVBZUmHSvLbMjRj6NUUKo=
gopkg.in/yaml.v3 v3.0.0-20200121175148-a6ecf24a6d71/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
182 changes: 134 additions & 48 deletions internal/go/skycfg/yaml.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ import (
"bytes"
"fmt"
"reflect"
"strings"

"go.starlark.net/starlark"
yaml "gopkg.in/yaml.v2"
"gopkg.in/yaml.v3"
)

// YamlModule returns a Starlark module for YAML helpers.
Expand All @@ -44,24 +45,101 @@ func yamlMarshal() starlark.Callable {
return starlark.NewBuiltin("yaml.marshal", fnYamlMarshal)
}

// starlarkToYAMLNode parses v into *yaml.Node.
func starlarkToYAMLNode(v starlark.Value) (*yaml.Node, error) {
switch v := v.(type) {
case starlark.NoneType:
return &yaml.Node{
Kind: yaml.ScalarNode,
Value: "null",
}, nil
case starlark.Bool:
return &yaml.Node{
Kind: yaml.ScalarNode,
// Both "True" and "true" are valid YAML but we'll use
// lowercase to stay consistent with older API (yaml.v2).
Value: strings.ToLower(v.String()),
}, nil
case starlark.Int:
return &yaml.Node{
Kind: yaml.ScalarNode,
Value: v.String(),
}, nil
case starlark.Float:
return &yaml.Node{
Kind: yaml.ScalarNode,
Value: fmt.Sprintf("%g", v),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to verify -- the Go docs for %g say it may format in scientific notation for some values. Does Go's format match YAML? yaml/pyyaml#173 suggests that at least some popular YAML implementations don't handle this well.

Good test cases would be a long positive number (enough to trigger the Go science mode), a long negative number, checking both the output text and asserting that go-yaml can parse the formatted value.

}, nil
case starlark.String:
return &yaml.Node{
Kind: yaml.ScalarNode,
Value: string(v),
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens for special-ish strings like v == "0" or v == "true"? go-yaml/yaml#490 suggests that gopkg.in/yaml.v3 might not support this properly.

A test case with some known-challenging strings would be useful to make sure Skycfg's user-visible behavior doesn't regress here.

}, nil
case starlark.Indexable: // Tuple, List
var elems []*yaml.Node
for i, n := 0, starlark.Len(v); i < n; i++ {
nn, err := starlarkToYAMLNode(v.Index(i))
if err != nil {
return nil, fmt.Errorf("failed to convert %d-th element of %v to YAML: %v", i, v, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to result in really really really big error messages for nested structures? Since the container gets printed in its entirety plus the nested error, which would be the entire second-level container plus its nested error, etc.

}
elems = append(elems, nn)
}
return &yaml.Node{
Kind: yaml.SequenceNode,
Value: v.String(),
Content: elems,
}, nil
case *starlark.Dict:
var elems []*yaml.Node
for _, pair := range v.Items() {
key, err := starlarkToYAMLNode(pair[0])
if err != nil {
return nil, fmt.Errorf("failed to convert key %v to YAML: %v", pair[0], err)
}
if key.Kind != yaml.ScalarNode {
return nil, fmt.Errorf("key `%v' is not scalar", key.Value)
}
elems = append(elems, key)

val, err := starlarkToYAMLNode(pair[1])
if err != nil {
return nil, fmt.Errorf("failed to convert value %v to YAML: %v", pair[1], err)
}
elems = append(elems, val)
}
return &yaml.Node{
Kind: yaml.MappingNode,
Value: v.String(),
Content: elems,
}, nil
default:
return nil, fmt.Errorf("TypeError: value %s (type `%s') can't be converted to YAML", v.String(), v.Type())
}
}

func fnYamlMarshal(t *starlark.Thread, fn *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
var v starlark.Value
if err := starlark.UnpackArgs(fn.Name(), args, kwargs, "value", &v); err != nil {
return nil, err
}
var buf bytes.Buffer
if err := writeJSON(&buf, v); err != nil {
return nil, err
}
var jsonObj interface{}
if err := yaml.Unmarshal(buf.Bytes(), &jsonObj); err != nil {

node, err := starlarkToYAMLNode(v)
if err != nil {
return nil, err
}
yamlBytes, err := yaml.Marshal(jsonObj)
if err != nil {

buf := bytes.Buffer{}
enc := yaml.NewEncoder(&buf)
enc.SetIndent(2)
if err := enc.Encode(&yaml.Node{
Kind: yaml.DocumentNode,
Content: []*yaml.Node{node},
}); err != nil {
return nil, err
}
return starlark.String(yamlBytes), nil
enc.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the error from Close() need to be checked?


return starlark.String(buf.String()), nil
}

// yamlUnmarshal returns a Starlark function for unmarshaling yaml content to
Expand All @@ -77,71 +155,79 @@ func fnYamlUnmarshal(t *starlark.Thread, fn *starlark.Builtin, args starlark.Tup
if err := starlark.UnpackPositionalArgs(fn.Name(), args, nil, 1, &blob); err != nil {
return nil, err
}
var inflated interface{}
if err := yaml.Unmarshal([]byte(blob), &inflated); err != nil {
var doc yaml.Node
if err := yaml.Unmarshal([]byte(blob), &doc); err != nil {
return nil, err
}
return toStarlarkValue(inflated)
return toStarlarkValue(doc.Content[0])
}

// toStarlarkScalarValue converts a scalar [obj] value to its starlark Value
func toStarlarkScalarValue(obj interface{}) (starlark.Value, bool){
// toStarlarkScalarValue converts a scalar node value to corresponding
// starlark.Value.
func toStarlarkScalarValue(node *yaml.Node) (starlark.Value, error) {
var obj interface{}
if err := node.Decode(&obj); err != nil {
return nil, err
}

if obj == nil {
return starlark.None, true
return starlark.None, nil
}
rt := reflect.TypeOf(obj)

t := reflect.TypeOf(obj)
v := reflect.ValueOf(obj)
switch rt.Kind() {
switch t.Kind() {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return starlark.MakeInt64(v.Int()), true
return starlark.MakeInt64(v.Int()), nil
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
return starlark.MakeUint64(v.Uint()), true
return starlark.MakeUint64(v.Uint()), nil
case reflect.Bool:
return starlark.Bool(v.Bool()), true
return starlark.Bool(v.Bool()), nil
case reflect.Float32, reflect.Float64:
return starlark.Float(v.Float()), true
return starlark.Float(v.Float()), nil
case reflect.String:
return starlark.String(v.String()), true
return starlark.String(v.String()), nil
default:
return nil, false
return nil, fmt.Errorf("unsupported type: %v", t)
}
}

// toStarlarkValue is a DFS walk to translate the DAG from go to starlark
func toStarlarkValue(obj interface{}) (starlark.Value, error) {
if objval, ok := toStarlarkScalarValue(obj); ok {
return objval, nil
}
rt := reflect.TypeOf(obj)
switch rt.Kind() {
case reflect.Map:
ret := &starlark.Dict{}
for k, v := range obj.(map[interface{}]interface{}) {
keyval, ok := toStarlarkScalarValue(k)
if !ok {
return nil, fmt.Errorf("%s (%v) is not a supported key type", rt.Kind(), obj)
// toStarlarkValue is a DFS walk to translate the DAG from Go to Starlark.
func toStarlarkValue(node *yaml.Node) (starlark.Value, error) {
switch node.Kind {
case yaml.ScalarNode:
return toStarlarkScalarValue(node)
case yaml.MappingNode:
out := &starlark.Dict{}
for ki, vi := 0, 1; vi < len(node.Content); ki, vi = ki+2, vi+2 {
k, v := node.Content[ki], node.Content[vi]

kv, err := toStarlarkScalarValue(k)
if err != nil {
return nil, fmt.Errorf("`%s' not a supported key type: %v", k.Value, err)
}
starval, err := toStarlarkValue(v)

vv, err := toStarlarkValue(v)
if err != nil {
return nil, err
}
if err = ret.SetKey(keyval, starval); err != nil {

if err = out.SetKey(kv, vv); err != nil {
return nil, err
}
}
return ret, nil
case reflect.Slice:
slice := obj.([]interface{})
starvals := make([]starlark.Value, len(slice))
for i, element := range slice {
v, err := toStarlarkValue(element)
return out, nil
case yaml.SequenceNode:
out := make([]starlark.Value, len(node.Content))
for i, e := range node.Content {
vv, err := toStarlarkValue(e)
if err != nil {
return nil, err
}
starvals[i] = v
out[i] = vv
}
return starlark.NewList(starvals), nil
return starlark.NewList(out), nil
default:
return nil, fmt.Errorf("%s (%v) is not a supported type", rt.Kind(), obj)
return nil, fmt.Errorf("`%s' not a supported value", node.Value)
}
}
16 changes: 8 additions & 8 deletions internal/go/skycfg/yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,20 @@ func TestSkyToYaml(t *testing.T) {
`,
},
YamlTestCase{
skyExpr: `{"a": 5, 13: 2, "k": {"k2": "v"}}`,
skyExpr: `{13: 2, "k": {"k2": "v"}, "a": 5.11}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

What caused this reordering? Is something changing in how map values get parsed/serialized?

expOutput: `13: 2
a: 5
k:
k2: v
a: 5.11
`,
},
YamlTestCase{
skyExpr: `[1, 2, 3, "abc", None, 15, True, False, {"k": "v"}]`,
skyExpr: `[1, 2, 3, None, "abc", 15, True, False, {"k": "v"}]`,
expOutput: `- 1
- 2
- 3
- abc
- null
- abc
- 15
- true
- false
Expand Down Expand Up @@ -198,8 +198,8 @@ func TestYamlToSky(t *testing.T) {
},
{
name: "negative Int64 key mapped to String",
key: starlark.MakeInt64(-2147483649),
want: `"nInt64Key"`,
key: starlark.MakeInt64(-2147483649),
want: `"nInt64Key"`,
},
{
name: "Uint key mapped to String",
Expand Down Expand Up @@ -230,9 +230,9 @@ func TestYamlToSky(t *testing.T) {
if testCase.want != got.String() {
t.Error(
"Bad return value from yaml.unmarshal",
"\nExpected:",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this squash the test failure message onto one line? That can be hard to read for the larger input expressions.

"Expected:",
testCase.want,
"\nGot:",
"Got:",
got,
)
}
Expand Down