CBOR Bindings #300

waahm7 · 2024-11-21T23:27:06Z

Issue #, if available:
#297

Description of changes:

Adds bindings for CBOR v2.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

waahm7 · 2024-11-21T23:30:29Z

Source/AwsCommonRuntimeKit/crt/CBOR.swift

+                else {
+                    throw CommonRunTimeError.crtError(.makeFromLastError())
+                }
+                return .int(-(Int64(out_value + 1)))


Please verify the math here.

I think this looks mathematically correct, but do we need to worry about integer overflow if out_value is the largest Int64 and then you try to add 1 to it?

Thanks, that's a great point. I am looking into it and will add tests for this.

waahm7 · 2024-11-21T23:36:09Z

Source/AwsCommonRuntimeKit/crt/CBOR.swift

+            }
+        case .date(let value):
+            do {
+                aws_cbor_encoder_write_tag(self.rawValue, UInt64(AWS_CBOR_TAG_EPOCH_TIME))


Please validate.

waahm7 · 2024-11-21T23:37:12Z

Source/AwsCommonRuntimeKit/crt/CBOR.swift

+            }
+        case AWS_CBOR_TYPE_TAG:
+            var out_value: UInt64 = 0
+            guard


Please validate. I am asserting here that the tag will always be 1, and the next value can only be an int, double, or uint64, which is then converted to a time.

waahm7 · 2024-11-21T23:41:30Z

Source/AwsCommonRuntimeKit/crt/CBOR.swift

+    /// Array type
+    case array(_ value: [CBORType])
+    /// Map type
+    case map(_ value: [String: CBORType])


Please validate, asserting that map keys will always be string. For indefinite map, it's up to the users.

TingDaoK · 2024-11-22T19:14:36Z

Source/AwsCommonRuntimeKit/crt/CBOR.swift

+    /// UINT64 type for positive numbers.
+    case uint64(_ value: UInt64)
+    /// INT64 type for negative numbers. If the number is positive, it will be encoded as UINT64 type.
+    case int(_ value: Int64)
+    /// Double type. It might be encoded as an integer if possible without loss of precision. Half-precision floats are not supported.
+    case double(_ value: Double)


worth to mention that the numbers will be encoded as "smallest possible".

I don't get why we have uint64 and int

Either we just do int64 for both positive or negative, which means we only supports [-2^63; 2^63-1] range. (If SDKs are okay with that.)

Or we be clear about unsgined 64 and negative 64, which both take unsigned int 64 to meet the range cbor supports, you can see here, cbor supports range from [-2^64; 2^64-1] .

Or we do both, be clear about unsigned and negative, and then a helper to just take int. So that we can support the range cbor required, while provides an easier to use API.

Thanks, I didn't realize -2^64 is a possibility. I think we should just keep it simple and expose Int64 and UInt64 since they are Swift types. Maybe we can use better names like uint and int?

Either we just do int64 for both positive or negative, which means we only supports [-2^63; 2^63-1] range. (If SDKs are okay with that.)

UInt64.max is a valid value, so we should support encoding/decoding that.

Or we be clear about unsgined 64 and negative 64, which both take unsigned int 64 to meet the range cbor supports, you can see here, cbor supports range from [-2^64; 2^64-1] .

The problem with -2^64 is that there is no way to represent that in Swift in a type-safe way. During encoding, I don't think there is any way for Swift to have a -2^64 integer unless it does what CBOR is doing which is UInt64 named NegInt.

During decoding, yeah, we might encounter -2^64 since CBOR allows it, but we should just error out instead of passing that complexity to the users to deal with. This should never happen in practice because I don't think anyone would be using -2^64 numbers. We can add the UInt64 named NegInt type in the future if needed.

@dayaffe What do you think?

Actually I thought of that again.

It seems to be a one-way door as it's an enum, that we cannot add the UInt64 named NegInt type in the future, since how will you deal with the value that's possible to fit into two different types??

Like currently, looks like you have both int64 and uint64 for the positive number, but during decode, you only give the negative value to the int64, and all the positive value will be uin64.

It's arguably working as expected? But, seems more reasonable to just have two different type for negative and positive?

Maybe something like

public func fromInt64(int) throws -> CBORType { }

as a helper to take the int and convert it to the type???

Discussed offline, it's not a one-way door since we can add a negint type in the future and decode it as either int or negint. I have added a warning that this enum is non_exhaustive. It is not ideal because of different types, but we already have this issue with bytes, text, date, and double types, etc., so it's not too bad.

as a helper to take the int and convert it to the type???

Yeah, we can do that, but I am not too concerned about the encoding part. The problem is during decoding; we can't map -2^64 to a Swift type, and I think we should try to hide this complexity from our users until needed. The API I am trying to design is to just encode Swift types and decode to Swift types to keep it simple and expected.

Source/AwsCommonRuntimeKit/crt/CBOR.swift

TingDaoK · 2024-11-22T19:29:35Z

Source/AwsCommonRuntimeKit/crt/CBOR.swift

+                else {
+                    throw CommonRunTimeError.crtError(.makeFromLastError())
+                }
+                return .int(Int64(-Int64(out_value) - 1))


The cbor negative int supports [-2^64; -1], it may overflow here. Check for overflow

Thanks, I have checked for overflow and am throwing an error for now. We can confirm with the SDK if they need -2^64; if yes, we can just return that.

TingDaoK · 2024-11-22T19:39:04Z

Source/AwsCommonRuntimeKit/crt/CBOR.swift

+            } else if case .uint64(let value) = timestamp {
+                return .date(Date.init(timeIntervalSince1970: Double(value)))
+            } else if case .int(let value) = timestamp {
+                return .date(Date.init(timeIntervalSince1970: Double(value)))


in C, converting from unsigned 64 or int 64 to double may loss the precision. Not sure about swift double, maybe worth to check. But seems like the timeIntervalSince1970 in swift only takes double, so, maybe just document it out that's a risk of loss of precision?

Thanks, I have added a comment.

TingDaoK · 2024-11-22T19:50:28Z

Source/AwsCommonRuntimeKit/crt/CBOR.swift

+    /// Date type. It will be encoded as epoch-based date/time.
+    case date(_ value: Date)


I'd add another case like undefined_tags (naming to be decided). So that we still have a way to support tags other than epoch-based time instead of just error out.
Unless SDK really don't want it and want to error out for tags other than epoch time.

Thanks, I have added a tag case.

Co-authored-by: Dengke Tang <dengket@amazon.com>

waahm7 added 15 commits November 20, 2024 10:58

initial

49f0563

wip encoder/decoder

b74e2a3

working basic types

fc48c40

working basic encode decode test

93d47e8

todos

04e6c1c

indef types

5a2800b

fix indef types

ff6035a

try init

544094b

don't crash for invalid cbor

4ccb796

pop next

d5ceda8

docs

ee746ea

cleanup

27b76c1

lint

01499ea

update test

e3c25d8

fix error todo

443bbc5

waahm7 marked this pull request as ready for review November 21, 2024 23:38

lint?

ae072fc

waahm7 commented Nov 21, 2024

View reviewed changes

edge cases

40eee2b

TingDaoK reviewed Nov 22, 2024

View reviewed changes

waahm7 and others added 6 commits November 22, 2024 15:17

Update Source/AwsCommonRuntimeKit/crt/CBOR.swift

0bf7b6e

Co-authored-by: Dengke Tang <dengket@amazon.com>

support tag type

d75f353

rename type

ceb373a

guard against Int64 max

6c89ebf

update docs

4bb0c00

adding warning

023c0d0

TingDaoK approved these changes Nov 23, 2024

View reviewed changes

use let

2ca60ac

waahm7 merged commit 37ab536 into main Nov 25, 2024
26 checks passed

waahm7 deleted the cbor-support branch November 25, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBOR Bindings #300

CBOR Bindings #300

waahm7 commented Nov 21, 2024 •

edited

Loading

waahm7 Nov 21, 2024

dayaffe Nov 22, 2024

waahm7 Nov 22, 2024

waahm7 Nov 21, 2024

waahm7 Nov 21, 2024

waahm7 Nov 21, 2024

TingDaoK Nov 22, 2024

TingDaoK Nov 22, 2024

waahm7 Nov 22, 2024

TingDaoK Nov 22, 2024

TingDaoK Nov 22, 2024 •

edited

Loading

waahm7 Nov 22, 2024 •

edited

Loading

TingDaoK Nov 22, 2024

waahm7 Nov 22, 2024

TingDaoK Nov 22, 2024

waahm7 Nov 23, 2024

TingDaoK Nov 22, 2024

waahm7 Nov 23, 2024

		/// Date type. It will be encoded as epoch-based date/time.
		case date(_ value: Date)

CBOR Bindings #300

CBOR Bindings #300

Conversation

waahm7 commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TingDaoK Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

waahm7 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waahm7 commented Nov 21, 2024 •

edited

Loading

TingDaoK Nov 22, 2024 •

edited

Loading

waahm7 Nov 22, 2024 •

edited

Loading