Skip to content

Commit

Permalink
fix: deep copy amendable nodes during walk
Browse files Browse the repository at this point in the history
  • Loading branch information
@smrz2001
smrz2001 committed Jul 16, 2023
1 parent 122d13b commit b22bcc3
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 53 deletions.
57 changes: 16 additions & 41 deletions traversal/focus_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,7 @@ func TestGetWithLinkLoading(t *testing.T) {

func TestFocusedTransform(t *testing.T) {
t.Run("UpdateMapEntry", func(t *testing.T) {
mapNode := duplicateMapNode(rootNode)
n, err := traversal.FocusedTransform(mapNode, datamodel.ParsePath("plain"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
n, err := traversal.FocusedTransform(rootNode, datamodel.ParsePath("plain"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "plain")
qt.Check(t, must.String(prev), qt.Equals, "olde string")
nb := prev.Prototype().NewBuilder()
Expand All @@ -205,15 +204,14 @@ func TestFocusedTransform(t *testing.T) {
// updated value should be there
qt.Check(t, must.Node(n.LookupByString("plain")), nodetests.NodeContentEquals, basicnode.NewString("new string!"))
// everything else should be there
qt.Check(t, must.Node(n.LookupByString("linkedString")), qt.Equals, must.Node(mapNode.LookupByString("linkedString")))
qt.Check(t, must.Node(n.LookupByString("linkedMap")), qt.Equals, must.Node(mapNode.LookupByString("linkedMap")))
qt.Check(t, must.Node(n.LookupByString("linkedList")), qt.Equals, must.Node(mapNode.LookupByString("linkedList")))
qt.Check(t, must.Node(n.LookupByString("linkedString")), qt.Equals, must.Node(rootNode.LookupByString("linkedString")))
qt.Check(t, must.Node(n.LookupByString("linkedMap")), qt.Equals, must.Node(rootNode.LookupByString("linkedMap")))
qt.Check(t, must.Node(n.LookupByString("linkedList")), qt.Equals, must.Node(rootNode.LookupByString("linkedList")))
// everything should still be in the same order
qt.Check(t, keys(n), qt.DeepEquals, []string{"plain", "linkedString", "linkedMap", "linkedList"})
})
t.Run("UpdateDeeperMap", func(t *testing.T) {
mapNode := duplicateMapNode(middleMapNode)
n, err := traversal.FocusedTransform(mapNode, datamodel.ParsePath("nested/alink"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
n, err := traversal.FocusedTransform(middleMapNode, datamodel.ParsePath("nested/alink"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "nested/alink")
qt.Check(t, prev, nodetests.NodeContentEquals, basicnode.NewLink(leafAlphaLnk))
return basicnode.NewString("new string!"), nil
Expand All @@ -223,14 +221,13 @@ func TestFocusedTransform(t *testing.T) {
// updated value should be there
qt.Check(t, must.Node(must.Node(n.LookupByString("nested")).LookupByString("alink")), nodetests.NodeContentEquals, basicnode.NewString("new string!"))
// everything else in the parent map should should be there!
qt.Check(t, must.Node(n.LookupByString("foo")), qt.Equals, must.Node(mapNode.LookupByString("foo")))
qt.Check(t, must.Node(n.LookupByString("bar")), qt.Equals, must.Node(mapNode.LookupByString("bar")))
qt.Check(t, must.Node(n.LookupByString("foo")), qt.Equals, must.Node(middleMapNode.LookupByString("foo")))
qt.Check(t, must.Node(n.LookupByString("bar")), qt.Equals, must.Node(middleMapNode.LookupByString("bar")))
// everything should still be in the same order
qt.Check(t, keys(n), qt.DeepEquals, []string{"foo", "bar", "nested"})
})
t.Run("AppendIfNotExists", func(t *testing.T) {
mapNode := duplicateMapNode(rootNode)
n, err := traversal.FocusedTransform(mapNode, datamodel.ParsePath("newpart"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
n, err := traversal.FocusedTransform(rootNode, datamodel.ParsePath("newpart"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "newpart")
qt.Check(t, prev, qt.IsNil) // REVIEW: should datamodel.Absent be used here? I lean towards "no" but am unsure what's least surprising here.
// An interesting thing to note about inserting a value this way is that you have no `prev.Prototype().NewBuilder()` to use if you wanted to.
Expand All @@ -245,8 +242,7 @@ func TestFocusedTransform(t *testing.T) {
qt.Check(t, keys(n), qt.DeepEquals, []string{"plain", "linkedString", "linkedMap", "linkedList", "newpart"})
})
t.Run("CreateParents", func(t *testing.T) {
mapNode := duplicateMapNode(rootNode)
n, err := traversal.FocusedTransform(mapNode, datamodel.ParsePath("newsection/newpart"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
n, err := traversal.FocusedTransform(rootNode, datamodel.ParsePath("newsection/newpart"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "newsection/newpart")
qt.Check(t, prev, qt.IsNil) // REVIEW: should datamodel.Absent be used here? I lean towards "no" but am unsure what's least surprising here.
return basicnode.NewString("new string!"), nil
Expand All @@ -264,16 +260,14 @@ func TestFocusedTransform(t *testing.T) {
qt.Check(t, keys(n2), qt.DeepEquals, []string{"newpart"})
})
t.Run("CreateParentsRequiresPermission", func(t *testing.T) {
mapNode := duplicateMapNode(rootNode)
_, err := traversal.FocusedTransform(mapNode, datamodel.ParsePath("newsection/newpart"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
_, err := traversal.FocusedTransform(rootNode, datamodel.ParsePath("newsection/newpart"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, true, qt.IsFalse) // ought not be reached
return nil, nil
}, false)
qt.Check(t, err.Error(), qt.Equals, "transform: parent position at \"newsection\" did not exist (and createParents was false)")
})
t.Run("UpdateListEntry", func(t *testing.T) {
listNode := duplicateListNode(middleListNode)
n, err := traversal.FocusedTransform(listNode, datamodel.ParsePath("2"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
n, err := traversal.FocusedTransform(middleListNode, datamodel.ParsePath("2"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "2")
qt.Check(t, prev, nodetests.NodeContentEquals, basicnode.NewLink(leafBetaLnk))
return basicnode.NewString("new string!"), nil
Expand All @@ -289,8 +283,7 @@ func TestFocusedTransform(t *testing.T) {
qt.Check(t, must.Node(n.LookupByIndex(3)), nodetests.NodeContentEquals, basicnode.NewLink(leafAlphaLnk))
})
t.Run("AppendToList", func(t *testing.T) {
listNode := duplicateListNode(middleListNode)
n, err := traversal.FocusedTransform(listNode, datamodel.ParsePath("-"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
n, err := traversal.FocusedTransform(middleListNode, datamodel.ParsePath("-"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "4")
qt.Check(t, prev, qt.IsNil)
return basicnode.NewString("new string!"), nil
Expand All @@ -303,16 +296,14 @@ func TestFocusedTransform(t *testing.T) {
qt.Check(t, n.Length(), qt.Equals, int64(5))
})
t.Run("ListBounds", func(t *testing.T) {
listNode := duplicateListNode(middleListNode)
_, err := traversal.FocusedTransform(listNode, datamodel.ParsePath("4"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
_, err := traversal.FocusedTransform(middleListNode, datamodel.ParsePath("4"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, true, qt.IsFalse) // ought not be reached
return nil, nil
}, false)
qt.Check(t, err, qt.ErrorMatches, "transform: cannot navigate path segment \"4\" at \"\" because it is beyond the list bounds")
})
t.Run("ReplaceRoot", func(t *testing.T) { // a fairly degenerate case and no reason to do this, but should work.
listNode := duplicateListNode(middleListNode)
n, err := traversal.FocusedTransform(listNode, datamodel.ParsePath(""), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
n, err := traversal.FocusedTransform(middleListNode, datamodel.ParsePath(""), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "")
qt.Check(t, prev, nodetests.NodeContentEquals, middleListNode)
nb := basicnode.Prototype.Any.NewBuilder()
Expand All @@ -336,10 +327,9 @@ func TestFocusedTransformWithLinks(t *testing.T) {
LinkTargetNodePrototypeChooser: basicnode.Chooser,
}
t.Run("UpdateMapBeyondLink", func(t *testing.T) {
mapNode := duplicateMapNode(rootNode)
n, err := traversal.Progress{
Cfg: &cfg,
}.FocusedTransform(mapNode, datamodel.ParsePath("linkedMap/nested/nonlink"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
}.FocusedTransform(rootNode, datamodel.ParsePath("linkedMap/nested/nonlink"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "linkedMap/nested/nonlink")
qt.Check(t, must.String(prev), qt.Equals, "zoo")
qt.Check(t, progress.LastBlock.Path.String(), qt.Equals, "linkedMap")
Expand All @@ -356,11 +346,10 @@ func TestFocusedTransformWithLinks(t *testing.T) {
store2 = memstore.Store{}
})
t.Run("UpdateNotBeyondLink", func(t *testing.T) {
mapNode := duplicateMapNode(rootNode)
// This is replacing a link with a non-link. Doing so shouldn't hit storage.
n, err := traversal.Progress{
Cfg: &cfg,
}.FocusedTransform(mapNode, datamodel.ParsePath("linkedMap"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
}.FocusedTransform(rootNode, datamodel.ParsePath("linkedMap"), func(progress traversal.Progress, prev datamodel.Node) (datamodel.Node, error) {
qt.Check(t, progress.Path.String(), qt.Equals, "linkedMap")
nb := prev.Prototype().NewBuilder()
nb.AssignString("new string!")
Expand All @@ -385,17 +374,3 @@ func keys(n datamodel.Node) []string {
}
return v
}

func duplicateMapNode(n datamodel.Node) datamodel.Node {
amender := basicnode.Prototype.Map.AmendingBuilder(nil)
// Make a deep copy of the node
datamodel.Copy(n, amender)
return amender.Build()
}

func duplicateListNode(n datamodel.Node) datamodel.Node {
amender := basicnode.Prototype.List.AmendingBuilder(nil)
// Make a deep copy of the node
datamodel.Copy(n, amender)
return amender.Build()
}
26 changes: 16 additions & 10 deletions traversal/walk.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,13 +494,21 @@ func (prog Progress) walkTransforming(n datamodel.Node, s selector.Selector, fn
nk := n.Kind()
switch nk {
case datamodel.Kind_List:
if _, castOk := n.Prototype().(datamodel.NodePrototypeSupportingListAmend); castOk {
return prog.walk_transform_iterateAmendableList(n, s, fn, s.Interests())
if np, castOk := n.Prototype().(datamodel.NodePrototypeSupportingListAmend); castOk {
a := np.AmendingBuilder(nil)
if err := datamodel.Copy(n, a); err != nil {
return nil, err
}
return prog.walk_transform_iterateAmendableList(a, s, fn, s.Interests())

Check warning on line 502 in traversal/walk.go

View check run for this annotation

Codecov / codecov/patch

traversal/walk.go#L497-L502 

Added lines #L497 - L502 were not covered by tests
}
return prog.walk_transform_iterateList(n, s, fn, s.Interests())
case datamodel.Kind_Map:
if _, castOk := n.Prototype().(datamodel.NodePrototypeSupportingMapAmend); castOk {
return prog.walk_transform_iterateAmendableMap(n, s, fn, s.Interests())
if np, castOk := n.Prototype().(datamodel.NodePrototypeSupportingMapAmend); castOk {
a := np.AmendingBuilder(nil)
if err := datamodel.Copy(n, a); err != nil {
return nil, err
}

Check warning on line 510 in traversal/walk.go

View check run for this annotation

Codecov / codecov/patch

traversal/walk.go#L509-L510 

Added lines #L509 - L510 were not covered by tests
return prog.walk_transform_iterateAmendableMap(a, s, fn, s.Interests())
}
return prog.walk_transform_iterateMap(n, s, fn, s.Interests())
default:
Expand Down Expand Up @@ -579,9 +587,8 @@ func (prog Progress) walk_transform_iterateList(n datamodel.Node, s selector.Sel
return bldr.Build(), nil
}

func (prog Progress) walk_transform_iterateAmendableList(n datamodel.Node, s selector.Selector, fn TransformFn, attn []datamodel.PathSegment) (datamodel.Node, error) {
listAmender := n.Prototype().(datamodel.NodePrototypeSupportingListAmend).AmendingBuilder(n)

func (prog Progress) walk_transform_iterateAmendableList(listAmender datamodel.ListAmender, s selector.Selector, fn TransformFn, attn []datamodel.PathSegment) (datamodel.Node, error) {
n := listAmender.Build()
for itr := selector.NewSegmentIterator(n); !itr.Done(); {
ps, v, err := itr.Next()
if err != nil {
Expand Down Expand Up @@ -706,9 +713,8 @@ func (prog Progress) walk_transform_iterateMap(n datamodel.Node, s selector.Sele
return bldr.Build(), nil
}

func (prog Progress) walk_transform_iterateAmendableMap(n datamodel.Node, s selector.Selector, fn TransformFn, attn []datamodel.PathSegment) (datamodel.Node, error) {
mapAmender := n.Prototype().(datamodel.NodePrototypeSupportingMapAmend).AmendingBuilder(n)

func (prog Progress) walk_transform_iterateAmendableMap(mapAmender datamodel.MapAmender, s selector.Selector, fn TransformFn, attn []datamodel.PathSegment) (datamodel.Node, error) {
n := mapAmender.Build()
for itr := selector.NewSegmentIterator(n); !itr.Done(); {
ps, v, err := itr.Next()
if err != nil {
Expand Down
3 changes: 1 addition & 2 deletions traversal/walk_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -781,8 +781,7 @@ func TestWalkTransforming(t *testing.T) {
s, err := ss.Selector()
qt.Assert(t, err, qt.IsNil)
var order int
mapNode := duplicateMapNode(middleMapNode)
n, err := traversal.WalkTransforming(mapNode, s, func(prog traversal.Progress, n datamodel.Node) (datamodel.Node, error) {
n, err := traversal.WalkTransforming(middleMapNode, s, func(prog traversal.Progress, n datamodel.Node) (datamodel.Node, error) {
switch order {
case 0:
qt.Check(t, n, nodetests.NodeContentEquals, basicnode.NewBool(true))
Expand Down

0 comments on commit b22bcc3

Please sign in to comment.