Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cbor: decode undefined as null #54

Merged
merged 1 commit into from Dec 3, 2018
Merged

cbor: decode undefined as null #54

merged 1 commit into from Dec 3, 2018

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Nov 30, 2018

See: ipfs/kubo#5776

This won't round-trip undef, it'll just convert it to null (which is what most decoders do anyways).

also fixes #53

See: ipfs/kubo#5776

This won't round-trip undef, it'll just convert it to null (which is what most
decoders do anyways).
@ghost ghost assigned Stebalien Nov 30, 2018
@ghost ghost added the status/in-progress In progress label Nov 30, 2018
@Stebalien
Copy link
Member Author

CC: @vmx, @mikeal.

@vmx
Copy link
Member

vmx commented Nov 30, 2018

Sounds good to me. undefined signals an encoding error so I think interpreting it as null is fair (although I don't know the exact semantics of null in Go).

Encoding is kind of related. We should make sure that we normally don't ever end up with having undefined encoded in the first place. There's a consensus at dignifiedquire/borc#36 that the JS implementation of the encoder will error if it encodes a JavaScript undefined rather then using the CBOR undefined for it.

@Stebalien
Copy link
Member Author

(although I don't know the exact semantics of null in Go).

We'll get a "zero" (empty) value.

@mikeal
Copy link

mikeal commented Nov 30, 2018

I didn't realize cbor had an undefined type.

I'm fine with de-serializing undefined from cbor to null. However, we should make sure that we never serialize undefined in any of our encoders, any undefined value should be interpreted as not actually existing (the key name should be unset), the only exception being if we need to encode a sparse array.

@Stebalien
Copy link
Member Author

@mikeal take a look at the issue @vmx linked.

CBOR undefined != JavaScript undefined. In CBOR, undefined means "failed to encode" (our JS library was just misusing it). The alternative here is to just drop CBOR values with undefined fields.

@mikeal
Copy link

mikeal commented Nov 30, 2018

Interesting. I don't think we should drop them in decoders, but I do think the JS encoder should throw when it has a "failed to encode" state.

@Stebalien
Copy link
Member Author

Got it. Should we just say that this is implementation defined behavior? That is, implementations may choose to decode undefined however they'd like (in go, we'll decode to "null").

@warpfork
Copy link
Member

warpfork commented Dec 2, 2018

Should we just say that this is implementation defined behavior?

Yeah, I think for the specification of how dag-cbor relates to IPLD, calling this implementation defined is reasonable.

Although (and maybe this is obvious, but for clarity's sake) that also needs to come with a caveat that cbor which contains these 'undefined' values is not canonical.

@mikeal
Copy link

mikeal commented Dec 3, 2018

Should we just say that this is implementation defined behavior?

This sounds right. Languages diff enough that it's appropriate to allow each one to determine how to interpret and serialize to cbor's undefined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

not enough arguments in call to cbor.NewUnmarshallerAtlased
4 participants