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

Encoding and subsequent decoding of an ipld schema'd struct needs to work #190

Open
willscott opened this issue Jun 8, 2021 · 2 comments

Comments

@willscott
Copy link
Member

Suppose you have a struct define as

	ts.Accumulate(schema.SpawnStruct("Example", []schema.StructField{
		schema.SpawnStructField("Optional", "String", true, false),
	}, schema.SpawnStructRepresentationMap(map[string]string{})))

and you serialize it:

node := _Example {
  Optional: _String__Maybe{v:schema.Maybe_Abscent}
}
linkPrototype.Build(ctx, ipld.LinkContext{}, node, storer)

This will serialize (with the link prototype being dagjson) to

{
  "Optional": null
}

which.. is wrong?

on attempting to decode it, it will then fail.

@mvdan
Copy link
Contributor

mvdan commented Jun 8, 2021

From my node-schema talks with @warpfork, I've come to understand that a struct's MapIterator will only skip optional fields which are absent when one is using the node's representation. It seems to be by design that, without the representation view, absent fields are included.

I agree it seems inconsistent that the non-repr node then does not allow assigning null. I think it should be a property that marshaling a node with a well-designed codec, and unmarshaling back into the same node's prototype, should not error. I'm less sure that it should give exactly the same data in memory, but at least decoding shouldn't error if encoding worked.

I think, in practice, everyone is expected to use representation nodes when encoding, and representation prototypes when decoding. A few weeks ago I argued that this should be done by default by all codecs, but Eric mentioned that maybe one does want to encode or decode a node without its repr view, e.g. for debugging purposes. Though I do personally think that the codecs should, by default, choose the repr view.

@warpfork
Copy link
Collaborator

warpfork commented Jun 9, 2021

#191 is also now about this.

Re: unmarshalling back into the same node prototype: yeah, I agree that no matter what, we should get this to be more symmetrical. I'm afraid unmarshalling symmetrically isn't generally possible though: if there was a field that's optional nullable (both) then the coersion of absent to null was a lossy one.

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

No branches or pull requests

3 participants