minicbor-serde: add support for serializing / deserializing custom CBOR tags#49
Open
mpanav wants to merge 4 commits intotwittner:developfrom
Open
minicbor-serde: add support for serializing / deserializing custom CBOR tags#49mpanav wants to merge 4 commits intotwittner:developfrom
mpanav wants to merge 4 commits intotwittner:developfrom
Conversation
Patch introduces wrapper tag types to enable deserialization and serialization of tagged values. Signed-off-by: Panav Munshi <mpanav@amazon.com>
Signed-off-by: Panav Munshi <mpanav@amazon.com>
Signed-off-by: Panav Munshi <mpanav@amazon.com>
Signed-off-by: Panav Munshi <mpanav@amazon.com>
twittner
reviewed
Jan 4, 2026
Owner
twittner
left a comment
twittner
left a comment
There was a problem hiding this comment.
Thanks! Overall this looks good to me. Just left some minor comments.
Comment on lines
+351
to
+352
| match self.tagged { | ||
| true => { |
| } | ||
|
|
||
| enum State { | ||
| Tag(u64), |
Owner
There was a problem hiding this comment.
Why not a minicbor::data::Tag?
Suggested change
| Tag(u64), | |
| Tag(Tag), |
|
|
||
| struct EnumTagAccess<'a, 'de: 'a> { | ||
| deserializer: &'a mut Deserializer<'de>, | ||
| tag: Option<u64>, |
Owner
There was a problem hiding this comment.
Suggested change
| tag: Option<u64>, | |
| tag: Option<Tag>, |
| { | ||
| match self.state { | ||
| State::Tag(tag) => { | ||
| self.state = State::Value; |
Owner
There was a problem hiding this comment.
Perhaps the state should change after the tag was successfully deserialised? Probably does not make much of a difference in practice.
| /// custom aliases to ensure that [`crate::de::Deserializer`] is able to drive | ||
| /// the [`minicbor::Decoder`] correctly. | ||
| enum TagContainer<T> { | ||
| Tag(u64, T), |
Owner
There was a problem hiding this comment.
Suggested change
| Tag(u64, T), | |
| Tag(Tag, T), |
Comment on lines
+99
to
+100
| found: u64, | ||
| expected: u64, |
Owner
There was a problem hiding this comment.
Suggested change
| found: u64, | |
| expected: u64, | |
| found: Tag, | |
| expected: Tag, |
| } | ||
|
|
||
| impl TagMismatchError { | ||
| fn new(found: u64, expected: u64) -> Self { |
Owner
There was a problem hiding this comment.
Suggested change
| fn new(found: u64, expected: u64) -> Self { | |
| fn new<A: Into<Tag>, B: Into<Tag>>(found: A, expected: B) -> Self { |
| /// | ||
| /// Tags will always be emitted during serialization | ||
| #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] | ||
| pub struct Required<const TAG: u64, T>(pub T); |
Owner
There was a problem hiding this comment.
Suggested change
| pub struct Required<const TAG: u64, T>(pub T); | |
| pub struct Required<const TAG: u64, T>(T); |
I would prefer not to expose the field publicly. Required is essentially Tagged, so the impl blocks from Tagged could be copied over here.
| /// | ||
| /// Tag will be emitted during serialization, if present | ||
| #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] | ||
| pub struct Optional<const TAG: u64, T>(pub Option<u64>, pub T); |
Owner
There was a problem hiding this comment.
Suggested change
| pub struct Optional<const TAG: u64, T>(pub Option<u64>, pub T); | |
| pub struct Optional<const TAG: u64, T>(Option<Tag>, T); | |
| impl<const TAG: u64, T> Optional<TAG, T> { | |
| pub fn tagged(val: T) -> Self { | |
| Self(Some(Tag::new(TAG)), val) | |
| } | |
| pub fn untagged(val: T) -> Self { | |
| Self(None, val) | |
| } | |
| pub fn tag(&self) -> Option<Tag> { | |
| self.0 | |
| } | |
| pub fn into_value(self) -> T { | |
| self.1 | |
| } | |
| } | |
| impl<const TAG: u64, T> Deref for Optional<TAG, T> { | |
| type Target = T; | |
| fn deref(&self) -> &Self::Target { | |
| &self.1 | |
| } | |
| } | |
| impl<const TAG: u64, T> DerefMut for Optional<TAG, T> { | |
| fn deref_mut(&mut self) -> &mut Self::Target { | |
| &mut self.1 | |
| } | |
| } |
Optional::tagged guarantees that the tag value matches the const TAG.
| /// | ||
| /// Tag will be emitted during serialization, if present | ||
| #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] | ||
| pub struct Any<T>(pub Option<u64>, pub T); |
Owner
There was a problem hiding this comment.
Suggested change
| pub struct Any<T>(pub Option<u64>, pub T); | |
| pub struct Any<T>(Option<Tag>, T); | |
| impl<T> Any<T> { | |
| pub fn tagged<N: Into<Tag>>(t: N, v: T) -> Self { | |
| Self(Some(t.into()), v) | |
| } | |
| pub fn untagged(v: T) -> Self { | |
| Self(None, v) | |
| } | |
| pub fn tag(&self) -> Option<Tag> { | |
| self.0 | |
| } | |
| pub fn into_value(self) -> T { | |
| self.1 | |
| } | |
| } | |
| impl<T> Deref for Any<T> { | |
| type Target = T; | |
| fn deref(&self) -> &Self::Target { | |
| &self.1 | |
| } | |
| } | |
| impl<T> DerefMut for Any<T> { | |
| fn deref_mut(&mut self) -> &mut Self::Target { | |
| &mut self.1 | |
| } | |
| } |
Author
|
Thanks for the comments! I'll work through these and have a revision out soon :) |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Patch series adds support to deserialize / serialize custom tags in minicbor-serde.
See #44 for more details