dagcbor: coerce undef to null. #308
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CBOR's "undefined" token -- byte 23; see https://datatracker.ietf.org/doc/html/rfc8949#section-5.7 or https://datatracker.ietf.org/doc/html/rfc8949#section-3.3 table 4 -- is not generally considered part of DAG-CBOR (which is generally a subset of CBOR). However, it may be desirable to allow parsing data flagged as DAG-CBOR while coercing any "undefined" tokens to a "null", even if considering it noncanonical. Because DAG-CBOR was not well specified early on, and because libraries existed for a considerable period of time which would readily emit data that we would not call DAG-CBOR, data exists in the wild which encoded these tokens, and links to them with an indicator that it's DAG-CBOR. We may wish to continue allowing these documents to be parsed. This does *not* mean that our DAG-CBOR codec will emit "undefined" tokens. That would not be neither canonical nor valid DAG-CBOR, so we don't want to; and it would also not be possible to describe with this library in the first place (it's not in the IPLD data model; so how would you?). This does *not* mean that you can *read* DAG-CBOR data with this codec and see "undefined" tokens distinctly. Again, they're not in the IPLD data model, so this library API (intentionally) does not have the ability to describe that. With this change, an "undefined" token will be allowed by the parser, but will be described as "null" in terms of the IPLD data model, and not be distinguishable from any other null. A change to the DAG-CBOR specs may be required if we decide to embrace this behavior and expansion of what we consider non-canonical-but-valid DAG-CBOR.
(Made a one-off test for this. It should also be a fixture in the ipld/ipld metarepo, but we don't already have those wired up, so I'm making the more incremental diff today.)
|
noting that the actual logic change appears to be from 2018 in polydawn/refmt#45 |
willscott
approved these changes
Dec 7, 2021
mvdan
approved these changes
Dec 7, 2021
rvagg
added a commit
to ipld/js-dag-cbor
that referenced
this pull request
Dec 8, 2021
rvagg
added a commit
to ipld/js-dag-cbor
that referenced
this pull request
Dec 8, 2021
rvagg
added a commit
to ipld/js-dag-cbor
that referenced
this pull request
Dec 13, 2021
For legacy data since this has been allowed by dag-cbor for a long time. New data should _not_ take advantage of this, `undefined` should not appear in IPLD data structures as it is not part of the IPLD data model. Fixes: #44 Ref: ipld/go-ipld-prime#308
rvagg
added a commit
to ipld/js-dag-cbor
that referenced
this pull request
Dec 13, 2021
For legacy data since this has been allowed by dag-cbor for a long time. New data should _not_ take advantage of this, `undefined` should not appear in IPLD data structures as it is not part of the IPLD data model. Fixes: #44 Ref: ipld/go-ipld-prime#308
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CBOR's "undefined" token -- byte 23; see https://datatracker.ietf.org/doc/html/rfc8949#section-5.7 or https://datatracker.ietf.org/doc/html/rfc8949#section-3.3 table 4 -- is not generally considered part of DAG-CBOR (which is generally a subset of CBOR).
However, it may be desirable to allow parsing data flagged as DAG-CBOR while coercing any "undefined" tokens to a "null", even if considering it noncanonical.
Because DAG-CBOR was not well specified early on, and because libraries existed for a considerable period of time which would readily emit data that we would not call DAG-CBOR, data exists in the wild which encoded these tokens, and links to them with an indicator that it's DAG-CBOR. We may wish to continue allowing these documents to be parsed. (Related: ipld/js-dag-cbor#44)
This does not mean that our DAG-CBOR codec will emit "undefined" tokens. That would not be neither canonical nor valid DAG-CBOR, so we don't want to; and it would also not be possible to describe with this library in the first place (it's not in the IPLD data model; so how would you?).
This does not mean that you can read DAG-CBOR data with this codec and see "undefined" tokens distinctly. Again, they're not in the IPLD data model, so this library API (intentionally) does not have the ability to describe that. With this change, an "undefined" token will be allowed by the parser, but will be described as "null" in terms of the IPLD data model, and not be distinguishable from any other null.
A change to the DAG-CBOR specs may be required if we decide to embrace this behavior and expansion of what we consider non-canonical-but-valid DAG-CBOR.