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

Should the JSON represenetation of CID always be formatted as links. #76

Open
kevina opened this issue Sep 22, 2018 · 6 comments
Open

Comments

@kevina
Copy link
Contributor

kevina commented Sep 22, 2018

My understanding is that the idea behind {"/": <cid-string>} is when Cid are represented as links. Should they always be serialized like this. In many cases a Cid acts more as an identifier than a link.

I see it just adding additional overhead to the JSON stream, espacally when returning a list of CIDs

Also it seams that even the idea of encoding them this was in conversational, at least based on @Stebalien comments in ipld/specs#70 when he said:

I'm really not happy baking this into IPLD. {'/': ...} was a hack to get JSON working.

This will need some strong arguments/motivations.

and

this was never intended to be the canonical representation. It was a hack to get JSON working.

@kevina kevina changed the title Should the JSON represenetation on CID always be formatted as links. Should the JSON represenetation of CID always be formatted as links. Sep 22, 2018
@Stebalien
Copy link
Member

Really, this is the correct format for DagJSON (that is the JSON IPLD format). Unfortunately, our APIs don't use DagJSON (they should but it's hard to fix that now).

We did the {"/": ...} dance to add a new "primitive" type (the CID) to JSON.


In practice, this allows us to write:

type Person struct {
  name string
  friends map[string]cid.Cid
}

And have json.Marshal(somePerson) spit out valid IPLD.

@kevina
Copy link
Contributor Author

kevina commented Sep 26, 2018

@Stebalien that may be true, but now that we can put strings in maps how will map[cid.Cid]string serialize since the key is a JSON object and not a string?

@Stebalien
Copy link
Member

It won't. That's not valid DagJSON. It could serialize to CBOR (in theory) but we currently restrict map keys to be strings (IIRC, this is a hold-over from the days when we wanted all map keys to be valid path components).

@kevina

This comment has been minimized.

@kevina
Copy link
Contributor Author

kevina commented Sep 26, 2018

So after testing and a little more research it seams map[cid.Cid]string won't serialize anyway. To fix this we would need to implement the encoding.TextMarshaler and encoding.TextUnmarshaler interface which just convert the Cid to string without a path comment.

@hsanjuan
Copy link
Contributor

I was going to open an issue to discuss that { "/" : <cid> } is quite weird and I think it's inconsistent in some places in the go-ipfs API how CIDs are presented. While correct in the IPLD context, it difficults using CIDs in non-IPLD contexts. CIDs are the core identifier for IPLD, but must they have a IPLD-based JSON output?

Note how this requires that every language implements additional json handling for CIDs just because it does not come as a simple string that can be thrown to Decode(). It just becomes harder to work with CIDs and it's not obvious why things are as they are.

Would having a global switch (variable) that just outputs them as a string be an option? It's a bad solution, but I can't think of anything non-breaking and would help cluster keep API compatibility if we let Cids marshal themselves to JSON in the future.

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