feat: support for CBOR by munna0908 · Pull Request #32 · logos-storage/nim-serde

munna0908 · 2025-05-22T11:20:57Z

This PR introduces support for canonical CBOR serialization in compliance with RFC 8949.
Note that this implementation does not yet support serde features such as OptIn, OptOut, Strict, or serialize/deserialize pragma annotations

gmega

Besides the comments, I strongly believe we need to deal with the nim-serde pragmas in some way, even if they're not supported. A few options:

issue an error (preferably a compilation error, if possible) if you try to use an annotated type with CBOR. This is what I prefer. Safest, least likely to cause surprise;
add documentation describing the behavior. Better than nothing, can still cause surprise, at least is written somewhere.

Either way, the documentation needs to be updated. I'm sure @emizzle will have better suggestions than mine as to how to make this integration as seamless as possible.

gmega · 2025-05-22T21:33:59Z

+    return failure(newCborError("Expected positive integer, got " & $c.kind))
+  let val = c.intVal.BiggestUInt
+
+  ?c.next()


I'm honestly not very fond of this. Took me a bit to figure out what this was doing, and I don't think I've seen this being used like that elsewhere.

@gmega In my opinion, this is the cleanest way to handle the "return on failure" scenario.
Based on my discussions with @dryajov, we hadn't used this approach earlier because it didn't support Futures. Now that it does, we’ll be using this syntax more extensively

gmega · 2025-05-22T22:29:16Z

+      let n = c.s.readData(buf[0].addr, buf.len)
+      if n != buf.len:
+        return failure(newCborError("truncated read of CBOR data"))
+    tryNext(c)


Yeah... I don't like those things that look like proc calls syntactically but then stuff a return into the code. It makes things very hard to read. It makes things look more like exception-based code (i.e. a tad cleaner), but it's surprising behavior when using result types IMHO.

If you really want to do something like that, it has to at least look like an explicit return IMHO. Something like:

returnOnFail c.next()

and then have returnOnFail as your template.

I have removed this template and replaced with ? template.

gmega · 2025-05-22T22:37:37Z

+
+func arrayLen*(c: CborParser): int =
+  ## Return the length of the array that the parser is positioned on.
+  assert(c.kind == CborEventKind.cborArray, $c.kind)


Why did you decide to throw an exception here instead of using result as you did with bytesLen? Also, I don't see this proc being used anywhere (nor bytestLen)?

gmega · 2025-05-23T16:29:29Z

+import ./errors
+import ./deserializer
+
+proc toJsonHook*(n: CborNode): JsonNode =


I'd call this toJson and keep it uniform with nim-serde API.

gmega · 2025-05-23T16:30:12Z

+  except Exception as e:
+    return failure(e.msg)
+
+proc writeCborHook*(str: Stream; dt: DateTime): ?!void =


Would probably rename all of those to toCbor.

gmega · 2025-05-23T16:32:12Z

-func keysNotIn[T](json: JsonNode, obj: T): HashSet[string] =
-  let jsonKeys = json.keys.toSeq.toHashSet
-  let objKeys = obj.fieldKeys.toHashSet
-  difference(jsonKeys, objKeys)


I wouldn't move those around for no reason. It's actually confusing cause I thought you were using them in the CBOR code, but then later realized you're not.

@gmega I've moved this to serde/json/helper.nim to separate helper functions from the core logic. This helps improve the overall structure and maintainability of the code

gmega · 2025-05-23T16:38:23Z

+import pkg/serde/cbor
+import pkg/questionable
+import pkg/questionable/results
+


Pretty awesome test!

…nterface

… type

…tency

…OR deserializer

…as in CBOR

gmega

Great job, I think this is much better now. I think the documentation still needs work to be easier to follow (I'm also not sure I'm so fond of the separate READMEs), but I don't have the time to work on it now. :-( Plus - all the info is there, so good enough for now I suppose.

gmega · 2025-06-03T22:23:29Z

+## Supported Wire Formats

-Opt-in serialization by default:
+nim-serde currently supports the following wire formats:


Suggested change

nim-serde currently supports the following wire formats:

nim-serde currently supports the following serialization formats:

gmega · 2025-06-03T22:24:17Z

 # nim-serde

-Easy-to-use json serialization capabilities, and a drop-in replacement for `std/json`.
+A serialization and deserialization library for Nim supporting multiple wire formats.


Suggested change

A serialization and deserialization library for Nim supporting multiple wire formats.

A serialization and deserialization library for Nim supporting multiple formats.

I wouldn't say "wire" as those might well be serialized for other reasons, not just sending over the wire.

gmega · 2025-06-04T21:58:03Z

+let userCborData = userStream.data
+```
+
+### Converting to CBOR with `toCbor`


I mean this is good enough as all the info needed to use the library is here. However: I'd probably use this as the initial example as it's the simplest API, and it's also what looks the most like the JSON API. Would then talk about the streaming API as a more advanced, CBOR-only feature and, finally, about working with CBOR node.

gmega · 2025-06-04T21:58:56Z

+
+The nim-serde CBOR serialization API provides several ways to convert Nim values to CBOR.
+
+### Basic Serialization with Stream API


Suggested change

### Basic Serialization with Stream API

### Stream API: Primitive Type and Sequence Serialization

gmega · 2025-06-04T22:28:47Z

+let jsonObj = %*{
+  "name": "John",
+  "age": 30,
+  "hobbies": ["reading", "coding"],
+  "address": {
+    "street": "123 Main St",
+    "city": "Anytown"
+  }
+}


Suggested change

let jsonObj = %*{

"name": "John",

"age": 30,

"hobbies": ["reading", "coding"],

"address": {

"street": "123 Main St",

"city": "Anytown"

}

}

let

age = 30

name = "John"

jsonObj = %*{

"name": name,

"age": age,

"hobbies": ["reading", "coding"],

"address": {

"street": "123 Main St",

"city": "Anytown"

}

}

gmega

I still think the examples in the detailed docs are a bit confusing and suggested some edits, but I guess we're getting to diminishing returns on review rounds. Since @emizzle also thinks this is good enough, I'm comfortable approving it.

Co-authored-by: Giuliano Mega <giuliano.mega@gmail.com>

munna0908 added 7 commits May 16, 2025 18:24

add cbor types

dcfec8a

feat: add CBOR serialization and deserialization support

f1ec805

cleanup test files

beac0e2

improve error handling for cbor serialization

52bc943

chore: fix test cases

eadc7e2

update license info

84943db

chore: cosmetic changes

587c834

munna0908 requested review from dryajov, emizzle and gmega May 22, 2025 11:20

munna0908 self-assigned this May 22, 2025

fix test filename

d3c3774

gmega requested changes May 23, 2025

View reviewed changes

munna0908 added 9 commits May 28, 2025 11:49

refactor: reimplement CBOR deserialization with type-based fromCbor i…

056a78e

…nterface

refactor: split writeCbor monolith into specialized type implementations

a969719

add tests for Time and TimeDate

ff37e61

feat: refactor CBOR parser to use direct exceptions instead of Result…

9d3da40

… type

refactor: rename encode to toCbor and toCbor to toCborNode for consis…

0e2fd5c

…tency

test changes

a4cd9a0

refactor: convert parseAssert to template and fix line wrapping in CB…

50000bc

…OR deserializer

add compile-time checks to prevent use of serialize/deserialize pragm…

0096796

…as in CBOR

update readme and add docs for cbor and json wireformats

dd00098

munna0908 marked this pull request as ready for review June 1, 2025 18:43

munna0908 requested a review from gmega June 1, 2025 18:43

gmega approved these changes Jun 4, 2025

View reviewed changes

munna0908 added 2 commits June 9, 2025 12:40

cleanup json and cbor readme

fc799d2

update readme with examples

6793ee5

munna0908 requested a review from gmega June 9, 2025 07:59

gmega approved these changes Jun 11, 2025

View reviewed changes

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread serde/json/README.md Outdated

Comment thread serde/json/README.md Outdated

Apply suggestions from code review

2d8fa4d

Co-authored-by: Giuliano Mega <giuliano.mega@gmail.com>

	nim-serde currently supports the following wire formats:
	nim-serde currently supports the following serialization formats:

	A serialization and deserialization library for Nim supporting multiple wire formats.
	A serialization and deserialization library for Nim supporting multiple formats.


		The nim-serde CBOR serialization API provides several ways to convert Nim values to CBOR.

		### Basic Serialization with Stream API

	### Basic Serialization with Stream API
	### Stream API: Primitive Type and Sequence Serialization

Conversation

munna0908 commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmega left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmega left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmega left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

munna0908 commented May 22, 2025 •

edited

Loading