-
Notifications
You must be signed in to change notification settings - Fork 50
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
test(basicnode): Increase test coverage for int and map types #454
Conversation
These tests build a struct with a union field. They all work except when the union is built as a typed node, and the top-level struct is built via representation. The union is copied using representation, and its assembler is incomplete, so the test fails.
I wonder whether we have this wrong at a lower level:
schemaType = ts.TypeByName("Animal")
proto := bindnode.Prototype(nil, schemaType).Representation()
// ...
animalNode := b.Build()
actual = printer.Sprint(animalNode)
expect = `struct<Animal>{
Name: string<String>{"eel"}
Action: union<Behavior>{string<Movement>{"swim"}}
}`
qt.Check(t, expect, qt.Equals, actual) is kind of surprising to me and I wonder if it's as it should be. If I'm building using the Perhaps bindnode, through taking shortcuts for efficiency reasons, is not properly keeping representation-level things at the representation-level (note in the code there's a lot of switching back and forth from the But then back to what you're trying to fix here: schemaType = ts.TypeByName("Animal")
proto := bindnode.Prototype(nil, schemaType).Representation()
// ...
schemaType := ts.TypeByName("Behavior")
typedProto := bindnode.Prototype(nil, schemaType)
b := typedProto.NewBuilder()
//...
a := ma.AssembleKey()
must.NotError(a.AssignString("Movement"))
a = ma.AssembleValue()
must.NotError(a.AssignString("swim"))
must.NotError(ma.Finish())
behaviorNode := b.Build()
//...
a = ma.AssembleKey()
must.NotError(a.AssignString("Action"))
a = ma.AssembleValue()
must.NotError(a.AssignNode(behaviorNode)) With the last line being the error being addressed here. But we're mixing up a representation form of So this works with the current code: must.NotError(a.AssignNode(behaviorNode.(schema.TypedNode).Representation())) Which seems appropriate to me. Whether it should be cleverer than that is the question - perhaps it should be able to work with both forms and just figure it out? I'm not sure. I'll have more of a think about this and try and get some input from others. |
Sure thing. For some background on the motivation here, I've been working off of this document: https://github.com/ipld/go-datalark/blob/master/docs/using-complex.md#kitchen-sink-constructed-by-mix-of-levels-explicitly (specifically, this test is covering the behavior from the "Kitchen Sink constructed by Mix of Levels (Explicitly)") written by @warpfork. My job has been to bridge those requirements with the functionality provided by |
On the build calls returning the type level value, even when the builder was for the representation form and fed values accordingly: Yeah, that's the way it is. The main reason for this is: it's always pretty clear to gear-shift down (e.g. on a typed node, ask for the (I agree that this is kind of surprising -- it sort of feels like it "breaks" the conventions about "stay on the level you're on" -- but don't know how else we could do it.) |
OK, so as per @warpfork's comment above the But on the other point, setting a type level Node onto a representation level Assembler where they don't meet the requirements is not something we should be enabling (if we can help it). I can see how https://github.com/ipld/go-datalark/blob/master/docs/using-complex.md#kitchen-sink-constructed-by-mix-of-levels-explicitly might lead to wanting that here but I don't think that's going to be a happy move in terms of the integrity of the layering. If it really is needed for datalark then I think it is probably going to have to manage it over there instead? The layering in go-ipld-prime means that a graph is going to have different properties when viewed at different layers, and the builders likewise are the same. A typed stringprefix union appears as a map Node at the type level, but at the representation layer it appears as a string Node. In theory there's also another possible layer on top of that if we were to make an ADL out of it so it could appear as something else. When building with a prototype's Assembler, that should also operate at the layer that it's supposed to. So a stringprefix union's Assembler should assemble as a map, but the prototype's representation layer form should assemble as a string. There is a possibility of inserting I know it pushed the complexity back on you, but can you see a path to achieving it upstream from here? Aside from the bindnode repr.go changes this seems mostly good to me, as unhelpful as that might be for what you're trying to achieve. Sorry. |
// test that AssignNode will fail if called twice, it expects an empty | ||
// node to assign to | ||
func TestMapCantAssignNodeTwice(t *testing.T) { | ||
b := basicnode.Prototype.Map.NewBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for almost all of these (I pick just one instance at random to comment on), I wonder if we can immediately generalize them to TestTheTrait(t *testing.T, np datamodel.NodePrototype)
?
It would need:
- the reusable test would need comments on it on what kind of NodePrototype it's expecting (in this case: a map)
- another quick func of boilerplate for
TestTheTrait(t *testing.T) { sharedtests.TestTheTrait(t, basicnode.Prototype.Map) }
- ... that's about it?
Not sure if you saw it, but we have the go-ipld-prime//node/tests
package for some of these already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It's also very likely that this will run into issues being applied on more than one node implementation, especially in the area of error message string matching... but I'd like to be aspirational about that!)
node/bindnode/repr.go
Outdated
case schema.UnionRepresentation_Stringprefix: | ||
// NOTE: duplicated in node.go, unionAssembler.AssembleValue | ||
// consider refactoring these | ||
name := w.curKey.val.String() | ||
var idx int | ||
var mtyp schema.Type | ||
for i, member := range w.schemaType.Members() { | ||
if member.Name() == name { | ||
idx = i | ||
mtyp = member | ||
break | ||
} | ||
} | ||
if mtyp == nil { | ||
return _errorAssembler{fmt.Errorf("bindnode TODO: missing member %s in %s", name, w.schemaType.Name())} | ||
} | ||
|
||
goType := w.val.Field(idx).Type().Elem() | ||
valPtr := reflect.New(goType) | ||
finish := func() error { | ||
unionSetMember(w.val, idx, valPtr) | ||
return nil | ||
} | ||
return &_assembler{ | ||
cfg: w.cfg, | ||
schemaType: mtyp, | ||
val: valPtr.Elem(), | ||
finish: finish, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and the related switch cases added to AssembleKey and Finish) doesn't belong here. If someone is working at the representation level, and the strategy is stringprefix
, the only thing it should accept is AssignString
, or AssignNode
given something that reports Kind() == Kind_String
.
Yep, I think I holistically agree with @rvagg . I do think giving our starlark bindings over in the go-datalark repo some magic "DWIM" sauce is a good and fun idea -- but we should do that over there, and we should keep the behavior of the lower level components in this repo as un-clever as possible. "APIs should stick to one level at a time; gearshifting requires explicit steps" seems like a good heuristic. (Confessedly, this can be a little confusing right now when looking at the code, because... well, there are bad examples. We haven't always followed that heuristic. In particular, I know I scattered a bunch of code in various |
The increases in test coverage are wonderful and always welcome! I'd still love to merge those parts. |
Ok, so is the plan to revert the changes to I admit I'm a bit lost on how to implement the fixes for go-datalark. I'm not sure what sort of "magic" is being discussed, and could use some clarity there. I had assumed |
Yeah, for datalark, exactly, I think that's what datalark should do -- if a datalark constructor is called, and knows its working on representation level, and it's been handed a node that's already all processed (e.g., an ipld.Node, as opposed to just other starlark primitives)... then it can detect that situation, do a feature-detection for (I'm okay with putting that logic in datalark because it's a higher level. I'd probably thumbs up making functions in go-ipld-prime that do that kind of detection and "DTRT" logic too -- just not putting them in node implementations themselves; that pushes it too deep / makes more work for every node implementation / creates more places for things to go wrong and get weird if all the node implementations don't perfectly implement all the edge cases.) |
Reverted as specified, looking for a merge when you get a chance. |
As part of the work I've been doing on go-datalark I encountered a panic happening in
AssignNode
. The issue was caused by a surprising interaction between union nodes that were built as typed, but were then being copied into struct nodes that were being built by representation. Turns out the fix was just needing to implement theAssembleKey
andAssembleValue
for the union repr assembler. To document this behavior, better understand the APIs involved here, and generally increase test coverage, I added a number of basic, straight-forward tests tobasicnode
andbindnode
.Increases
node/basicnode/map.go
's coverage from 42.9% to 65.6%.