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

Tag binary values in cbor if set #2244

Merged
merged 2 commits into from
Jul 17, 2020
Merged

Conversation

matthewbauer
Copy link
Contributor

@matthewbauer matthewbauer commented Jul 6, 2020

CBOR has tags, which work similarly to "subtype"s:

https://www.iana.org/assignments/cbor-tags/cbor-tags.xhtml

Unsure if this makes sense. Note that the subtype must just be one
byte wide.


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

CBOR has tags, which work similarly to "subtype"s:

https://www.iana.org/assignments/cbor-tags/cbor-tags.xhtml

Unsure if this makes sense. Note that the subtype must just be one
byte wide.
matthewbauer added a commit to obsidiansystems/nix that referenced this pull request Jul 6, 2020
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one question (see comment). Furthermore, test cases are missing. Maybe with some examples, I understand the goal of this PR.

@matthewbauer
Copy link
Contributor Author

Note this only works for encoding subtypes, not decoding. Decoding is hard because the tag can come for any data type - not just binary. So we would need to add more state to handle it properly.

@coveralls
Copy link

coveralls commented Jul 6, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling dd08f77 on matthewbauer:tag-cbor into efcc826 on nlohmann:develop.

@Ericson2314
Copy link

For context, here is an issue from a Rust library about this exact issue pyfisch/cbor#157 . Serde also wasn't originally made with CBOR tags in mind, so again the sticking point is derialization without polluting everything else with CBOR-specific concepts.

@nlohmann
Copy link
Owner

nlohmann commented Jul 9, 2020

As the library does not support tags in the moment (#1968), it would fail to parse its own output, or am I mistaken?

@matthewbauer
Copy link
Contributor Author

As the library does not support tags in the moment (#1968), it would fail to parse its own output, or am I mistaken?

Yes - you wouldn't be able to parse a cbor with a subtype since it would have that tag.

@Ericson2314
Copy link

Yes this change just attempts serialization for now, as a stop-gap which is good enough for our purposes.

@nlohmann
Copy link
Owner

Hm. Then I think it would be helpful to also adjust the parser to skip tags, because otherwise roundtripping would not work any more. This does not need to be done in this PR, of course, but I'm still re-opening #1968.

@Ericson2314
Copy link

Oh! We'll if you are fine doing it after is there anything more this PR needs?

@nlohmann
Copy link
Owner

nlohmann commented Jul 15, 2020

@Ericson2314 @matthewbauer It would be great if you could have a look at #2273.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann self-assigned this Jul 17, 2020
@nlohmann nlohmann added this to the Release 3.8.1 milestone Jul 17, 2020
@nlohmann nlohmann merged commit a10d486 into nlohmann:develop Jul 17, 2020
@nlohmann
Copy link
Owner

Thanks!

@nlohmann
Copy link
Owner


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description


@Ericson2314
Copy link

Thank you!

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

Successfully merging this pull request may close these issues.

4 participants