ensure that rooted Sets and Lists don't have keys #3684
Conversation
| @@ -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 { | |||
There was a problem hiding this comment.
Two things:
- I think this condition should be a panic, not just silently ignoring the problem
- 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
keysbut 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.
There was a problem hiding this comment.
PS: Thanks for the patch!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
rooted Sets and Lists will panic if keys are passed into SetInsert or ListAppend due to the leaf chunk functions trying to assert types.mapEntry as types.Value