Fix selector helpers #24
Conversation
aschmahmann
force-pushed
the
fix/selector-helpers
branch
from
8382466
to
b6e7cc8
Compare
… selector now always compiles
aschmahmann
force-pushed
the
fix/selector-helpers
branch
from
b6e7cc8
to
760b004
Compare
| var matchAllSelector ipld.Node | ||
|
|
||
| func init() { | ||
| ssb := builder.NewSelectorSpecBuilder(basicnode.Prototype.Any) | ||
| matchAllSelector = ssb.ExploreRecursive(selector.RecursionLimitNone(), ssb.ExploreUnion( | ||
| ssb.Matcher(), | ||
| ssb.ExploreAll(ssb.ExploreRecursiveEdge()), | ||
| )).Node() | ||
| } |
There was a problem hiding this comment.
AFAICT this selector means matchAll and there's no reason to rebuild it continuously given that it's a static thing that shouldn't eat much memory.
There was a problem hiding this comment.
FWIW, I also added one of these upstream recently, here: https://github.com/ipld/go-ipld-prime/blob/df94e6a99727d6dfd9f3381f0f407f3df368178a/traversal/selector/parse/selector_parse.go#L94
Though I'm fine seeing a construction snippet like this recur locally in places, as needed, too. It's more antifragile that way. (I added that precompiled one to upstream as much for an example as for any other purpose; I don't know if it actually really "belongs" in the API surface area up there.)
| // BlockAll traverses all nodes in the graph linked by root. The nodes will be untyped and send over the results | ||
| // channel. | ||
| func BlockAll(ctx context.Context, f fetcher.Fetcher, root ipld.Link, cb fetcher.FetchCallback) error { |
There was a problem hiding this comment.
We could probably just kill this function in exchange for exporting the selector itself but it shouldn't be a big deal.
|
|
||
| // BlockAllOfType traverses all nodes in the graph linked by root. The nodes will typed according to ptype | ||
| // and send over the results channel. | ||
| func BlockAllOfType(ctx context.Context, f fetcher.Fetcher, root ipld.Link, ptype ipld.NodePrototype, cb fetcher.FetchCallback) error { |
There was a problem hiding this comment.
IIUC this function seems actively harmful. Even if this function were fixed up it would be special casing the first node in the traversal which feels like bugs just waiting to happen.
| _ ipld.NodePrototype, cb fetcher.FetchCallback) error { | ||
|
|
||
| // retrieve first node | ||
| node, err := f.BlockOfType(ctx, root, ptype) | ||
| prototype, err := f.PrototypeFromLink(root) |
There was a problem hiding this comment.
Special casing the type of the first node over the others seems harmful/bug prone. We can just use the linksystem to figure out what it should be.
It seems like we could probably also just change the name and signature of this function in the Fetcher interface to drop this. I'm not sure if it would cause much propagation damage and should wait for a follow-up set of PRs though. If we're not going to do this let's at least add a comment to the interface saying that the prototype should be ignored.
There was a problem hiding this comment.
I'm not sure what the issue is here? And I'm not sure LinkSystem is that smart?
One typically wants a NodePrototype parameter to either A) say what kind of memory layout to use, or B) say what kind of types to use, if it's a schema node (and this, then, can go on to hint where ADLs should be later, etc, though we do not use this anywhere in IPFS yet, and it may be some time before we do).
LinkSystem has a NodeReifier callback, yes, but it's not meant to do half of those jobs.
(All of this should be prefixed with "I think". I don't understand big picture what this function is supposed to be doing.)
There was a problem hiding this comment.
... Looking yet again, I set the strength of my previous comment to zero. Because I agree with Will's comment that PrototypeFromLink doesn't seem to belong here at all, I just can't make sense of this, so if you're telling me this fixed something, I can't protest.
|
I read through this proposed change and am generally okay with switching the interface to not take the explicit prototype constructor to use. That said, i would prefer to let @warpfork or @hannahhoward have a chance to review, since I know some thought and iteration has gone into this interface. I do still think |
|
I'm also gonna punt to @hannahhoward. I don't see anything to object to in the diffs, but I don't feel ownership here either. (As I touched lightly on in the megapost in go-path (which ended up reviewing a lot of the whole module graph), my current understanding of go-fetcher is that it contains a lot of glue code; and my hope is in the future it either {develops a stronger identity and clear scope}, or {we fold the reusable parts into upstreams and fold the glue parts into downstreams}... so I don't really have a good sense of what's appropriate abstraction in this repo right now and what's not.) |
There was a problem hiding this comment.
We all seem to be in a "no objections" state, so let's call this good and move forward.
- The CI shows downstream having problems before this: https://circleci.com/gh/ipfs/go-ipfs/55708 , described in IPLD Prime In IPFS: Target Merge Branch kubo#7976 (comment)
- The CI shows a commit downstream getting green by this change: ipfs/kubo@7a975b0
- It's unfortunate that there's no tests in this repo that demonstrate the issue or the fix, but I'm unwilling to block us on that given the general confidence in this API as written.
- My main worry would be that we're going to merge this and tag and it bubble it up and then find it doesn't fix anything, but that appears addressed by the above existing CI observations from the only downstream that matters.
|
IIUC, the path to bubble this up into go-ipfs is just:
|
Fix selector helpers This commit was moved from ipfs/go-fetcher@86467b9
Per ipfs/kubo#7976 (comment) there seem to be some issues with the traversal code here, and probably with the Fetcher interface as well.
This is an attempt to resolve them, but I'm not very familiar with the codebase so please lmk if I'm just missing things.