Skip to content
This repository has been archived by the owner on Aug 28, 2021. It is now read-only.

ensure that rooted Sets and Lists don't have keys #3684

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ensure that rooted Sets and Lists don't have keys #3684

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion go/types/graph_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type GraphBuilder struct {
vrw ValueReadWriter
stack graphStack
mutex sync.Mutex
rootKind NomsKind
}

// NewGraphBuilder returns an new GraphBuilder object.
Expand All @@ -60,7 +61,7 @@ func NewGraphBuilder(vrw ValueReadWriter, rootKind NomsKind) *GraphBuilder {
}

func newGraphBuilder(vrw ValueReadWriter, opcStore opCacheStore, rootKind NomsKind) *GraphBuilder {
b := &GraphBuilder{oc: opcStore.opCache(), opcStore: opcStore, vrw: vrw}
b := &GraphBuilder{oc: opcStore.opCache(), opcStore: opcStore, vrw: vrw, rootKind: rootKind}
b.pushNewKeyOnStack(String("ROOT"), rootKind)
return b
}
Expand All @@ -82,6 +83,9 @@ func (b *GraphBuilder) SetInsert(keys []Value, v Value) {
if b.oc == nil {
d.Panic("Can't call SetInsert() again after Build()")
}
if b.rootKind != MapKind {
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. I think this condition should be a panic, not just silently ignoring the problem
  2. Doesn't this same problem apply at other points in the path other than the root? It seems like if at any point there's a key in keys but the thing at that level is a set or list, that should be an error. So it seems like the fix should be somewhere lower down.

Copy link
Contributor

Choose a reason for hiding this comment

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

PS: Thanks for the patch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like sometimes you want a key pointing to a set/list... like if you have a base map that has a nested set/list. (see types/graph_builder_test.go:ExampleGraphBuilder_Build()). But it's definitely a problem if the base type isn't a map since the base collection isn't nested.

I would've liked to have handled it deeper, but once it's been put into ldb it's hard to tell where it's at until everything is unwound through graphbuilder.Build().

I'm not married to the solution... just wanted to communicate the problem and suggest a workaround until I get better acquainted with the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider a case like this (psuedocode):

gb := new GraphBuilder(..., MapKind)
gb.ListAppend(["foo"], 42)
gb.MapSet(["foo", "bar", "baz"], "key", thing)

This is another example of trying to set a key ("bar") in a list. It's just further down in the tree. How do we handle this currently? Or do we not. If not, perhaps we can handle the two problems in the same way.

If we can't handle it until build that might be OK so that all error cases are handled in a consistent way.

keys = nil
}
b.oc.GraphSetInsert(keys, v)
}

Expand All @@ -94,6 +98,9 @@ func (b *GraphBuilder) ListAppend(keys []Value, v Value) {
if b.oc == nil {
d.Panic("Can't call ListAppend() again after Build()")
}
if b.rootKind != MapKind {
keys = nil
}
b.oc.GraphListAppend(keys, v)
}

Expand Down