Open aschmahmann opened 1 year ago
I agree that this is a problem, and it would be good to resolve it without too much of a hack.
I know that during development there was an attempt to reign in the verbosity of the API—it's already very verbose but it could be a lot more by allowing everything to error .. but maybe they should in a world where we're building multi-layered abstractions over these things with complex behaviour underneath.
In terms of options:
- Change
Length
to return an error
I'd prefer this, but it's going to be very painful breaking that contract, with everyone needing to make the jump together. Are we prepared to deal with that pain?
- Change the contract to allow -1 to mean any type of error
This could end up being more dangerous; a length is often used for iteration. Silently changing contract on this could leave users exposed to surprising, or damaging, behaviour. It might be best to be noisy about it.
- Add a new function (name TBD) that does return an error and deprecate Length
Does this buy us anything that option 1 doesn't give us? It breaks the interface so the same dependency upgrade pain will exist, we just get to wriggle out of having to check for err
except that you get deprecation warnings when you don't! So it seems like a worse version of 1.
Another option exists, to ignore the problem and keep on papering over it. The unixfs-preload one is pretty annoying because one way to handle problems like this is to hold on to an error and return it on a later call to an error returning API call, but with unixfs-preload, no more calls to the Node are made so it gets lost entirely!
https://github.com/ipld/go-ipld-prime/pull/532, just to see what that would look like internally
Noticed while discovering https://github.com/ipfs/go-unixfsnode/issues/54
The contract on
node.Length()
is https://github.com/ipld/go-ipld-prime/blob/65bfa53512f2328d19273e471ce4fd6d964055a2/datamodel/node.go#L131-L133This means that if there is any sort of lazy loading or computation involved in working with a node that is definitely a list or map that there is nothing valid to return.
-1
but that's a contract violationI suspect the options here are:
Length
to return an error-1
to mean any type of errorLength
While 2 seems the easiest, it has the ability to break a bunch of code without people noticing. Simple compile-time checked fixes like 1 seem a safer option.
Whether 1 or 3 is preferred seems like a matter of taste.
Note: the other functions on the Node interface which could potentially run into a similar issue are
Kind()
,IsAbsent()
, andIsNull()
However, it seems like these are less likely to be lazily constructed thanLength
and therefore less likely to be a problem.Similarly, the map and list iterators can return errors once they're in use (and they can be checked in advance with
Kind()
) and so are also less problematic.