valpackett / SwiftCBOR

A CBOR implementation for Swift
The Unlicense
132 stars 74 forks source link

Error on encoding empty Array inside a Map #58

Closed lvaccaro closed 2 years ago

lvaccaro commented 4 years ago

Hi, I am using array inside a generic map object: it works good for not empty array, but just in case of an empty array c inside a Map, it will be encoded by encodeMap() as [0x97, 0x99, 0x64] instead of [0x97, 0x99, 0x128]. Forcing in the definition the element type as "c": [String]() or "c": [Int]() has no effects.

I think it could be related to https://github.com/myfreeweb/SwiftCBOR/blob/97a3d94906db62899688b332e5eda8436bfd8530/Sources/SwiftCBOR/CBOREncoder.swift#L290 the following conditions are all true for an empty array.

(lldb) print (any is [UInt8])
(Bool) $R2 = (_value = 1)
(lldb) print (any is [String])
(Bool) $R4 = (_value = 1)
(lldb) print (any is [Int])
(Bool) $R6 = (_value = 1)
(lldb) print (any is [Any])
(Bool) $R14 = (_value = 1)

Do you have any suggestions?

Test example:

        let mapToAnyEmpty: [String: Any] = [
            "a": 1,
            "b": [2, 3],
            "c": []
        ]
        let encodedMapToAnyEmpty = try! CBOR.encodeMap(mapToAnyEmpty)
        XCTAssertEqual(encodedMapToAnyEmpty, [0xa2, 0x61, 0x61, 0x01, 0x61, 0x62, 0x82, 0x02, 0x03, 0x61, 0x63, 0x80])

Assert failure:

error: -[SwiftCBORTests.CBOREncoderTests testEncodeMaps] : XCTAssertEqual failed: ("[163, 97, 97, 1, 97, 98, 130, 2, 3, 97, 99, 64]") is not equal to ("[162, 97, 97, 1, 97, 98, 130, 2, 3, 97, 99, 128]")
hamchapman commented 4 years ago

Good find. This is, as I think you've spotted, because "byte strings" are represented here as [UInt8]. As such, when you have an empty array it is impossible to distinguish between an empty array of UInt8s and an empty array of any other type.

The behaviour could be changed to default to represent any empty array as an empty array (rather than an empty byte string as it's currently doing it) by changing this code:

https://github.com/myfreeweb/SwiftCBOR/blob/97a3d94906db62899688b332e5eda8436bfd8530/Sources/SwiftCBOR/CBOREncoder.swift#L289-L307

As long as the case for [Any] is before the case for [UInt8] then an empty array will default to an empty array.

I think that might make more sense.

As a quick point it looks like your test would fail even if an empty array was encoded as 0x80 because your first byte in the expected value is 0xa2 (a map with two keys), but I think you want it to be 0xa3 (a map with three keys) 👍

lvaccaro commented 4 years ago

Thanks for the quick reply. Have right, 0xa3 in the test :-)

If [Any] is moved before of [UInt8], do you think the [UInt8] case is still accessible?

hamchapman commented 4 years ago

Ah of course. It won't ever get reached if [Any] is above it.

I'm not immediately sure what the best solution is here.

hamchapman commented 2 years ago

Looking at this again, I think the best solution is probably to not handle the [UInt8] case and instead rely on the fact that Data is handled and so anytime someone wants a byte string then they should use Data instead of [UInt8]. That way an empty array will be appropriately encoded