Allowing custom NavigableNode implementations

[Wondertan]

Currently, implementing custom NavigableNode has no point due to ExtractIPLDNode. It forces users(DAGReader) to stick with NavigableIPLDNode implementation through type assertion to get an actual Node. The PR is intended to fix this without any breaking changes by adding the 'GetIPLDNode' method to the interface.

My use case also requires DAGReader usage with custom NavigableNode, but it's constructor strict to single implementation. I propose to add options for DagReader to resolve the case. I will raise a PR there in case this one is accepted.

/cc @Stebalien

[welcome]

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

• The context for this PR is clear, with relevant discussion, decisions
  and stakeholders linked/mentioned.

• Your contribution itself is clear (code comments, self-review for the
  rest) and in its best form. Follow the code contribution
  guidelines
  if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

• A maintainer will triage and assign priority to this PR, commenting on
  any missing things and potentially assigning a reviewer for high
  priority items.

• The PR gets reviews, discussed and approvals as needed.

• The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

[Wondertan]

What about naming as IPLDNode or even just Node?

[schomatis]

Hey @Wondertan! Thanks for submitting the PR, could you expand a bit more on this please?

My use case also requires DAGReader usage with custom NavigableNode, but it's constructor strict to single implementation.

Just to add some context here, the original idea behind the DAGReader was to decouple navigation over an abstract graph from the actual implementation of it, in our case IPLD nodes. Originally this was a desirable feature to be able to reuse this structure with other DAGs, e.g., in dependency navigation (targeted mostly for the now deprecated Gx). At this point I don't think that will ever be the case so we could fully specialize the DAGReader to handle only IPLD nodes, thus avoiding ugly functions like ExtractIPLDNode (and the NavigableNode interface altogether).

In any case, I don't object this change, but until we fully specialize the DAGReader, if we want to add GetIPLDNode() to the NavigableNode interface we should allow it to return an error in case the underlying node is not an IPLD one.

[warpfork]

Without detracting from the conversation on whether this library should change -- Can I check if you have considered doing what you're doing using the go-ipld-prime libraries instead? They're an alternate set of interfaces for IPLD, and get a lot more development at present. (There's a little more explainer on the difference here.)

I feel this is important to point out as a possibility, because those libraries have been designed from the ground up in order to be very careful to have this kind of pluggability for the Node interfaces in all cases. So, while I don't know the details of your use case in detail, if it's possible to use the go-ipld-prime libraries, I think there's a fair bet they'll be a lot more conducive to what you're trying to accomplish.

The traversal package in the new libraries may be somewhat spartan, but they work cleanly over a Node interface, and we have much more specifications available now about what that means and what contracts it has to support. (This library, go-ipld-format, is based on roughly similar ideas, but hasn't been revised much since the push we've made on specifications, so I'm not sure things will be as clear in this code.)

If you're doing this for some piece of code that's directly integrating with go-ipfs or some other existing codebase that's using the go-ipld-format interfaces and it's not practical to migrate at this time, that's understandable too. (We are starting to also think about how to get the IPFS repos moving towards these newer, revamped libraries as well, fwiw -- but there's a fairly decent amount of work ahead there, and we're not in a radical hurry yet (since both libraries are handling the same serial data formats, there's not any data incompatibility at rest...)... but it is the direction we want to move in, long run, to focus development efforts.)

Anyway, I just wanted to make the possibility known. :) I know we might not have made a ton of documentation available around this transition yet, but we should soon. And if we can get more effort unified behind the new stuff, that'd be awesome(r). :)

[Wondertan]

@warpfork, Huge thanks to your extensive and clear answer, as well as to your work in go-ipld-prime. Of course, I am acquainted with it, but as you mentioned, migrating to it currently is not that easy and mostly impossible, due to lack of go-unixfsv2 implementation, what is definitely a dealbreaker, although go-ipld-prime has other amazing possibilities unavailable before. Also, IMHO, there are enough docs and specs for me to understand what happens with the new IPLD, so thanks for that too :)

[Wondertan]

@schomatis, thanks for the explanation.

Let me expand my motivation. I use an interface that allows me not to spawn goroutine per GetBlocks/GetNodes and request new blocks in-flight with CID chan:

type BlockStreamer interface {
    Stream(context.Context, chan<-[]cid.Cid) (<-chan blocks.Block, error)
}

Then, I realized the need to use it with IPLD and concretely with unixfs, where I got into the problem: the only customizable value in the whole stack is DAGservice which does not support any similar interface and session here won't help as well. Therefore, I got to the point where the only nonbreaking change was to implement custom NavigableNode over the BlockStreamer, and here we are.

However, my fervor to be up to date with your projects lead me to this issue that explains the motivation for a similar idea. In the thread @Stebalien mentioned that DAGService will be changed to allow Node streaming, what is definitely a good intention that gives more possibilities as well as optimizations. Furthermore, that change may encourage you to also implement Walker over the streaming interface, but that will require implementing a new NavigableNode or rewriting the existing one. Eventually, If you see that Walker over StreamNodes is reasonable for you then I'll wait or help you with that. In another case, I'll just maintain a bunch of forks.

[schomatis]

I see, you're right, indeed NavigableIPLDNode is doing much more than its name implies, as it has a very opinionated way of fetching the nodes inherited from the old reader's logic.

I still think that if we are tying the NavigableNode interface to IPLD (in this case by adding a GetIPLDNode() method to it) we should remove it altogether as it's no longer fulfilling its abstraction goal, but in any case this change is not that important and if it allows you to move forward with your effort we should prioritize that. Thanks for looking into this, you detailed explanations, and your work towards making IPFS even greater.