Skip to content
Merged
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
31 changes: 25 additions & 6 deletions kyaml/yaml/rnode.go
Original file line number Diff line number Diff line change
Expand Up @@ -718,10 +718,11 @@ func (rn *RNode) MustString() string {

// Content returns Node Content field.
func (rn *RNode) Content() []*yaml.Node {
if rn == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Please remain to check if rn == nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koba1t do you still want this check retained with the knowledge that:

// YNode returns the yaml.Node value.  If the yaml.Node value is a DocumentNode,
// YNode will return the DocumentNode Content entry instead of the DocumentNode.
func (rn *RNode) YNode() *yaml.Node {
	if rn == nil || rn.value == nil {
		return nil
	}

will achieve the same?

Copy link
Member

Choose a reason for hiding this comment

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

I think so.

Additionally, Please add a test to check if rn == nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a test for that @koba1t.

The test I already added has two cases, one of which is where rn == nil.

yNode := rn.YNode()
if yNode == nil {
return nil
}
return rn.YNode().Content
return yNode.Content
}

// Fields returns the list of field names for a MappingNode.
Expand Down Expand Up @@ -756,7 +757,11 @@ func (rn *RNode) FieldRNodes() ([]*RNode, error) {
// Field returns a fieldName, fieldValue pair for MappingNodes.
// Returns nil for non-MappingNodes.
func (rn *RNode) Field(field string) *MapNode {
if rn.YNode().Kind != yaml.MappingNode {
yNode := rn.YNode()
if yNode == nil {
return nil
}
if yNode.Kind != yaml.MappingNode {
return nil
}
var result *MapNode
Expand Down Expand Up @@ -892,7 +897,11 @@ func (rn *RNode) ElementValuesList(keys []string) ([][]string, error) {
// Element returns the element in the list which contains the field matching the value.
// Returns nil for non-SequenceNodes or if no Element matches.
func (rn *RNode) Element(key, value string) *RNode {
if rn.YNode().Kind != yaml.SequenceNode {
yNode := rn.YNode()
if yNode == nil {
return nil
}
if yNode.Kind != yaml.SequenceNode {
return nil
}
elem, err := rn.Pipe(MatchElement(key, value))
Expand All @@ -906,7 +915,11 @@ func (rn *RNode) Element(key, value string) *RNode {
// corresponding values[i].
// Returns nil for non-SequenceNodes or if no Element matches.
func (rn *RNode) ElementList(keys []string, values []string) *RNode {
if rn.YNode().Kind != yaml.SequenceNode {
yNode := rn.YNode()
if yNode == nil {
return nil
}
if yNode.Kind != yaml.SequenceNode {
return nil
}
elem, err := rn.Pipe(MatchElementList(keys, values))
Expand Down Expand Up @@ -960,12 +973,17 @@ func (rn *RNode) GetAssociativeKey() string {

// MarshalJSON creates a byte slice from the RNode.
func (rn *RNode) MarshalJSON() ([]byte, error) {
yNode := rn.YNode()
if yNode == nil {
return []byte("null"), nil
}

s, err := rn.String()
if err != nil {
return nil, err
}

if rn.YNode().Kind == SequenceNode {
if yNode.Kind == SequenceNode {
var a []interface{}
if err := Unmarshal([]byte(s), &a); err != nil {
return nil, err
Expand All @@ -977,6 +995,7 @@ func (rn *RNode) MarshalJSON() ([]byte, error) {
if err := Unmarshal([]byte(s), &m); err != nil {
return nil, err
}

return json.Marshal(m)
}

Expand Down
67 changes: 67 additions & 0 deletions kyaml/yaml/rnode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2356,3 +2356,70 @@ metadata:
require.NoError(t, err)
require.Equal(t, "hello-world-foo", fooAppName) // no field named 'foo.appname'
}

func TestRNode_nilSafety(t *testing.T) {
// Both of these scenarios should cause rn.YNode() to return nil.
nodesToTest := [...]struct {
name string
rn *RNode
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test case that rn is nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one :) it tests both:

		{"nil *RNode receiver", nil},
		{"RNode with nil internal node", &RNode{value: nil}},

}{
{"nil *RNode receiver", nil},
{"RNode with nil internal node", &RNode{value: nil}},
}

t.Run("Content", func(t *testing.T) {
for _, tc := range nodesToTest {
t.Run(tc.name, func(t *testing.T) {
assert.NotPanics(t, func() {
assert.Nil(t, tc.rn.Content())
})
})
}
})

t.Run("Field", func(t *testing.T) {
for _, tc := range nodesToTest {
t.Run(tc.name, func(t *testing.T) {
assert.NotPanics(t, func() {
assert.Nil(t, tc.rn.Field("any-field"))
})
})
}
})

t.Run("Element", func(t *testing.T) {
for _, tc := range nodesToTest {
t.Run(tc.name, func(t *testing.T) {
assert.NotPanics(t, func() {
assert.Nil(t, tc.rn.Element("any-key", "any-value"))
})
})
}
})

t.Run("ElementList", func(t *testing.T) {
for _, tc := range nodesToTest {
t.Run(tc.name, func(t *testing.T) {
assert.NotPanics(t, func() {
assert.Nil(t, tc.rn.ElementList([]string{"any-key"}, []string{"any-value"}))
})
})
}
})

t.Run("MarshalJSON", func(t *testing.T) {
for _, tc := range nodesToTest {
t.Run(tc.name, func(t *testing.T) {
var (
jsonData []byte
err error
)
assert.NotPanics(t, func() {
jsonData, err = tc.rn.MarshalJSON()
})
require.NoError(t, err)
assert.Equal(t, []byte("null"), jsonData)
})
}
})
}
Loading