Use go-fetcher, unixfsnode, and ipld-prime to resolve paths. #34
Conversation
This comment has been minimized.
This comment has been minimized.
| func NewBasicResolver(bs blockservice.BlockService) *Resolver { | ||
| fc := fetcher.NewFetcherConfig(bs) |
There was a problem hiding this comment.
is it possible we'll want a single fetcher both for pathing and other things, and will want to pass in a fetchconfig here directly? (i guess we can make a Resolver struct if we need to do that)
There was a problem hiding this comment.
It's a little tricky. FetcherConfig owns the PrototypeChooser and go-path needs a custom PrototypeChooser to ensure it can path into dagpb nodes using the unixfsnode ADL. We probably don't want to push responsibility for constructing the correct chooser to the caller and we can't have go-path reconfigure a shared FetchConfig.
We could potentially pass the chooser into the GetSession call in go-fetcher (which would be called with a specific chooser in go-path/BasicResolver, but I'd want to get a better sense of the kinds of things that might need to be shared in a FetchConfig (other than blockservice) before making that kind of refactor.
| default: | ||
| return nd.Cid(), p, nil | ||
| clnk, ok := lnk.(cidlink.Link) | ||
| if !ok { |
There was a problem hiding this comment.
eventually we may want to have a slow fallback of
cid.ParseString(lnk.String()) to support other concrete cid types.
cc @warpfork on thoughts on if needed / other ways to make our life remain extensible here.
There was a problem hiding this comment.
nah, honestly I don't even know if the whole , ok and branch seems relevant to me.
It doesn't hurt anything to ask the cast rather than assert it, but I'd say that it's legitimately a compile-time bug if a program encounters an unexpected concrete type inside this interface, and so if this wanted to panic when that fails, that would make sense to me.
Co-authored-by: Eric Myhre <hash@exultant.us>
| github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= | ||
| github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98= | ||
| github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= |
There was a problem hiding this comment.
This isn't really constructive criticism for this PR, per se (because a lot of these existed before this PR too, and I'm just noticing them for the first time)... but... I want to make this comment somewhere, if for no other reason that being able to refer to it in other discussions in the future...
holy smokes. These dependencies. These are not things that I would expect.
github.com/envoyproxy/go-control-plane is a doozy. github.com/client9/misspell is also a doozy. github.com/btcsuite/btcd is a doozy. github.com/gopherjs/gopherjs is a superdoozy. And there's some CLI flag libraries. And a DNS library. And... I'm drowning in doozies.
It's not a job for this PR, but some kind of understanding about what the heck is going on with this module and its transitive dependencies needs to be reached in the future. Something's gone significantly off the rails, fallen into a wormhole, and appeared in the andromeda galaxy at some point.
| // Resolver provides path resolution to IPFS | ||
| // It has a pointer to a DAGService, which is uses to resolve nodes. | ||
| // It has a pointer to a FetcherConfig, which is uses to resolve nodes. | ||
| // TODO: now that this is more modular, try to unify this code with the | ||
| // the resolvers in namesys | ||
| type Resolver struct { |
There was a problem hiding this comment.
I could use more docs on this. I see there weren't a ton before, either, but if we're gonna lug this into the future, it would be good to have some comments on whether its shape is intentful or primarily legacy driven; any notes about what it doesn't do, etc.
There was a problem hiding this comment.
I'm kinda wondering, throughout this whole file:
What's a Resolver for, vs using traverse.* on the nodes? (Is it just legacy API satisfying to try to reduce change splash size? Fine, but we should mark wherever that reasoning is used; that information failing to transfer to the next person working in this area would be devastating to that person's ability to plan changes.)
Is FetcherConfig adding any key data vs traversal.Config? Or did we accidentally create a similar abstraction twice, and we should try to simplify it back to one? If there is key data it adds that makes it suitable for Resolver -- what is it?
There was a problem hiding this comment.
Should we be trying to push a lot harder on the ADL'ization of pathing over unixfsv1 stuff? If we do so, can we make this Resolver thing a much thinner facade over that?
remove ResolveOnce as it's no longer used and is just confusing
* first pass * Update resolver/resolver.go Co-authored-by: Eric Myhre <hash@exultant.us> * update dependencies to tagged versions * correctly handles nested nodes within blocks * return link from resolve path so we can fetch container block * return expected NoSuchLink error * more accurate errors * feat(resolver): remove resolve once remove ResolveOnce as it's no longer used and is just confusing Co-authored-by: acruikshank <acruikshank@example.com> Co-authored-by: Eric Myhre <hash@exultant.us> Co-authored-by: hannahhoward <hannah@hannahhoward.net>
* Use go-fetcher, unixfsnode, and ipld-prime to resolve paths. (#34) * first pass * Update resolver/resolver.go Co-authored-by: Eric Myhre <hash@exultant.us> * update dependencies to tagged versions * correctly handles nested nodes within blocks * return link from resolve path so we can fetch container block * return expected NoSuchLink error * more accurate errors * feat(resolver): remove resolve once remove ResolveOnce as it's no longer used and is just confusing Co-authored-by: acruikshank <acruikshank@example.com> Co-authored-by: Eric Myhre <hash@exultant.us> Co-authored-by: hannahhoward <hannah@hannahhoward.net> * fix(update to tagged branches): update to tagged branches and use node reifier * fix(deps): update go-unixfsnode * fix(deps): update to latest go fetcher (#37) * feat(resolver): take fetcher config as parameter (#38) * fix(deps): switch to tagged go-fetcher * fix(resolver): removed ipldcbor dependency * fix(mod): remove unneeded deps * fix(resolver): correct comments * test(resolver): add test verifying ErrNoLink functionality * fix(lint): fix lint errors resolve go vet and staticcheck issues. note we had to ignore two lines that use deprecated behavior, but which replacing could have unintended effects * fix(resolver): LookupBySegment to handle list indexes as well as map fields (#42) * fix(resolver): LookupBySegment to handle list indexes as well as map fields * Add test for /mixed/path/segment/types/1/2/3 * feat(resolver): address more PR comments * style(tests): add clarification * style(lint): fix lint errors, redo test fix * fix(deps): update deps to tagged version Co-authored-by: Alex Cruikshank <169613+acruikshank@users.noreply.github.com> Co-authored-by: acruikshank <acruikshank@example.com> Co-authored-by: Eric Myhre <hash@exultant.us> Co-authored-by: Rod Vagg <rod@vagg.org>
Motivation
We are moving IPFS to use ipld-prime (via go-fetcher) for graph traversal. This PR replaces the path traversal algorithm in go-path's resolver with ipld-prime queries using a path selector.
This PR depends on linksystem changes to ipld-prime and must remain a draft until that PR lands.
What's in this PR