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

Cid.Undef IPLD de-serialization error #246

Closed
adlrocha opened this issue Sep 7, 2021 · 4 comments · Fixed by #433
Closed

Cid.Undef IPLD de-serialization error #246

adlrocha opened this issue Sep 7, 2021 · 4 comments · Fixed by #433
Assignees
Labels
help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up

Comments

@adlrocha
Copy link
Contributor

adlrocha commented Sep 7, 2021

If we use cid.Undef to create a new datamodel.Link with cidLink, it is serialized as "/": "b", which means that when trying to de-serialize it, we get cid is too short. I don't think we should error in this case. We should probably not serialize cid.Undef, or use a Null type, to allow de-serialization in the other end. I don't know if this the expected behavior we wanted for cid.Undef or it is open to discussion.

Background

I was serializing an IPLD node map with a bunch of links, some of them were cid.Undef, and I was getting cid is too short. I was blaming the Links which were actually set, when in the end it was the cid.Undef the one to blame.

@rvagg
Copy link
Member

rvagg commented Sep 8, 2021

🤯

Undef can be used to represent a nil or undefined Cid

so yeah, this should probably be interpreted as Absent somehow, and then error if the schema doesn't allow for it in that place. Maybe if there's a union that allows for Null in that position it might make sense, but that seems like a stretch.

OR, just error earlier, or with a more informative message to discourage use of Undef or better checking upstream.

@willscott
Copy link
Member

Note that part of this duality is because of the quirk in how we represent Cid's in Golang. Since they are strings, a primitive type, they cannot be 'nil', and so Undef gets commonly used for that 'not yet set' case that may represent either explicit Absentness or Null

@BigLep BigLep added P2 Medium: Good to have, but can wait until someone steps up help wanted Seeking public contribution on this issue labels May 10, 2022
@rvagg
Copy link
Member

rvagg commented May 10, 2022

#196 possibly helps with this?

@rvagg
Copy link
Member

rvagg commented May 23, 2022

Revisiting this, I think maybe our codecs need to special-case this and convert to Null, like we're currently doing for Absent; needs a PR to propose and test that approach. Assigning myself.

@rvagg rvagg self-assigned this May 23, 2022
rvagg added a commit that referenced this issue May 24, 2022
Otherwise we end up with invalid output.

dag-cbor: 0xd82a4100

  d8 2a                                           #   tag(42)
    41                                            #     bytes(1)
      00                                          #       "\x00"

dag-json: {"/":"b"}

Fixes: #246
rvagg added a commit that referenced this issue May 24, 2022
Otherwise we end up with invalid output.

dag-cbor: 0xd82a4100

  d8 2a                                           #   tag(42)
    41                                            #     bytes(1)
      00                                          #       "\x00"

dag-json: {"/":"b"}

Fixes: #246
rvagg added a commit that referenced this issue Jun 1, 2022
Otherwise we end up with invalid output.

dag-cbor: 0xd82a4100

  d8 2a                                           #   tag(42)
    41                                            #     bytes(1)
      00                                          #       "\x00"

dag-json: {"/":"b"}

Fixes: #246
@rvagg rvagg closed this as completed in #433 Jun 1, 2022
rvagg added a commit that referenced this issue Jun 1, 2022
Otherwise we end up with invalid output.

dag-cbor: 0xd82a4100

  d8 2a                                           #   tag(42)
    41                                            #     bytes(1)
      00                                          #       "\x00"

dag-json: {"/":"b"}

Fixes: #246
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up
Projects
Status: 🎉 Done
Development

Successfully merging a pull request may close this issue.

4 participants