ipfs / go-unixfsnode

An ADL IPLD prime node that wraps go-codec-dagpb's implementation of protobuf to enable pathing
Other
11 stars 5 forks source link

HAMT Preloader does not return error even if blocks do not load #54

Closed aschmahmann closed 1 year ago

aschmahmann commented 1 year ago

The HAMT Preloader ADL is meant to load all the blocks in the HAMT without following any links in the map's values.

Unfortunately, the way it does the preloading is by calling https://github.com/ipfs/go-unixfsnode/blob/364a5496925b291900d67c35b8b898e1a30b7f0a/hamt/shardeddir.go#L68-L71

While this would be fine if in fact -1 was returned during an error loading the HAMT.

However, it does not because the contract for that function for this function is that -1 is only returned if the node is not a list or a map and we know it is. https://github.com/ipld/go-ipld-prime/blob/65bfa53512f2328d19273e471ce4fd6d964055a2/datamodel/node.go#L131-L133

This means that when we have trouble loading a block we just continue on as if nothing happened which does not fulfill the contract of the preloader function

https://github.com/ipfs/go-unixfsnode/blob/364a5496925b291900d67c35b8b898e1a30b7f0a/hamt/shardeddir.go#L280-L283


This seems most readily resolvable by just using a different code path than Length() for doing the enumeration. Two that come to mind are:

  1. Copying/reusing the Length code but where we return errors
  2. Going through a map iterator that doesn't load the map value nodes
willscott commented 1 year ago

This makes sense. either of the options you propose sound pretty plausible. I'll take a look at a PR