Fix JSON Parsing

[keks]

@Stebalien reported a panic here that is caused by the cmds layer to not correctly parse a value that results in a zero value.

The problem is that I tried to make a type that takes a set of interface{} values, tries to decode the JSON into each of them and returns the first one that doesn't throw an error or result in a zero value. If this doesn't work for any of the supplied types, instead unmarshal into a map[string]interface{}.
This approach obviously fails if we want to parse a value that actually results in a zero value. This is what happens in the issue mentioned above.

So instead I made a type that expects either a cmdkit.Error or a value, and unmarshals into the value if unmarshaling into an error fails.

I used this version of go-ipfs-cmds to build go-ipfs and the panic does not occur anymore.

If the PR is accepted I'll squash and gx publish.

Regarding the tests, CI will only work after at cmds0.5 is done, which I'm working on when I'm not busy fixing panics ;)

[Stebalien]

Are we mixing errors and values with no type indication?

[keks]

Are we mixing errors and values with no type indication?

Ish. cmdkit.Error is the kind of error a command returns when it fails, so it is one form of result a command can give. That's why it's treated like a regular value.
Also we want to enable commands to emit non-fatal errors this way. Many commands that don't output a single value but a stream of values have ad-hoc types for returning errors to the caller. With the cmds rewrite, everything is a stream of interface{} values, that can contain both errors and values emitted by the command.

[Stebalien]

Yes but we have no type information. I could just as easily decide to return a value with the same fields as an error. Is there any way to wrap this in a type? I.e., wrap it in an object that contains a type: ID field?

[keks]

I agree that having type hints would be great. That's why I added a "Type":"error" to the JSON representation of cmdkit.Error. Unfortunately our options are somewhat limited because the API has been fixed for a while now and we can't do arbitrary changes.
The good news is that (at the time of writing) we only ever need to distinguish an error and a "good" value, so we can first check whether decoding into an error works and in case it doesn't fall back to the non-error value.

[Stebalien]

That doesn't help. Try:

> ipfs dag get zdpuAxiFRrxtmMmFRZx145LufQsKTw8is1ZgfTQX4vgzgb8DB

[keks]

I get error: the api is broken. Wireshark tells me that the response is a regular cmdkit.Errror. I don't think I get your point, can you elaborate? But please keep in mind we can't make breaking changes to the API, no matter how clumsy it is.
I propose to instead fixing the apparent problems with it and start building a clean websocket handler and client.

Regarding the CI complaining: That will be fixed with cmds0.5 which is well on the way.

[Stebalien]

I don't think I get your point, can you elaborate?

#31

That is, there are valid IPLD objects that will cause the API to return an error.

---

However, I understand that it's a bit late to fix this in API v0. It's just frustrating as this shouldn't have happened.

[Stebalien]

As a fix, this looks good to me.

[Stebalien]

can we not assume that it's one or the other? (i.e., if Error is nil, it's a value)?

[keks]

hmm, Error is a struct so I can't check for nil. I could check if the message is empty but that seems like a bad hack. Checking whether Value is nil won't work because it's supposed to be set to a non-nil value by the user. So inferring whether we have an error or not is not possible in a clean way without isError.

I think the danger of incosistencies (i.e. isError is false but we in fact do have an error) is very small, so I'd like to stick with it.

[Stebalien]

I see. Sounds good.

[Stebalien]

It looks like we do actually need this logic.

[keks]

I don't get it. Why? nevermind.