christophhagen / BinaryCodable

A binary encoder for Swift Codable types
MIT License
92 stars 7 forks source link

BinaryCodable/ValueEncoder.swift:40: Fatal error: Unexpectedly found nil while unwrapping an Optional value #6

Closed bryceco closed 9 months ago

bryceco commented 1 year ago

I'm not able to encode a class containing an optional string property. See attached test case.

Test.txt

christophhagen commented 1 year ago

From how I understand the documentation of KeyedEncodingContainer and KeyedDecodingContainer provided by Apple, the correct function to use for encoding optionals would be

func encodeIfPresent(String?, forKey: KeyedEncodingContainer<K>.Key) throws

Another option would be to use encodeNil(forKey:).

The corresponding function for decoding is

func decodeIfPresent(String.Type, forKey: KeyedDecodingContainer<K>.Key) throws -> String?

Your implementation would then look like this:

final class TestClass: Codable {
    let name: String
    let date: String?

    enum CodingKeys: String, CodingKey {
        case name
        case date
    }

    convenience init(from decoder: Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        let name = try container.decode(String.self, forKey: .name)
                // Note the use of `String` instead of `String?`
        let date = try container.decodeIfPresent(String.self, forKey: .date)
        self.init(withName: name, endDate: date)
    }

    func encode(to encoder: Encoder) throws {
        var container = encoder.container(keyedBy: CodingKeys.self)
        try container.encode(name, forKey: .name)
        try container.encodeIfPresent(date, forKey: .date)
    }

    init(withName name: String, endDate: String?) {
        self.name = name
        date = endDate
    }
}
bryceco commented 1 year ago

There's a minor difference when you use encodeIfPresent vs encode. Using encode with a null value will encode "value=null", while encodeIfPresent simple doesn't encode any value at all. If I rewrite my code like:

let v1: String = "Bob"
let v2: String? = nil
let v3: String? = nil

func encode(to encoder: Encoder) throws {
    var container = encoder.container(keyedBy: CodingKeys.self)
    try container.encode(v1, forKey: .v1)
    try container.encodeIfPresent(v2, forKey: .v2)
    try container.encode(v3, forKey: .v3)
}

And then encode using JSONEncoder() I get:

{"v1":"Bob","v3":null}
christophhagen commented 1 year ago

That's an interesting corner case. I've pushed some code to explicitly detect Optionals so as to at least not crash in your case, but it would still omit the key. I guess we could treat Optionals as separate containers, but that would add another byte to the encoded data. I'll have to think about it.

Do you have an explicit need to encode nil, instead of omitting the key? I think the auto-generated Codable conformance would also omit it.

On another note: Did you have a look at CBOR? It's more cross-platform than my framework, and there are different Swift implementations: PotentCodables, CBORCoding. Maybe a good alternative, and using something more mature?

bryceco commented 1 year ago

I don't need to encode the nil, but I did expect the behavior to be a identical to how other encoders behave so it can be used as a drop-in replacement. During decoding there might also be the expectation that it will throw if the nil is missing.

Since an optional is just syntactic sugar for an enum I'm a little surprised that it requires special handling, but I'm not an expert in Swift.

christophhagen commented 1 year ago

I would agree with you that there should be a difference when writing custom decoding routines between decode() and decodeIfPresent(). Apple also states in the documentation, that decode()should throw an error if the container doesn't have an entry for the given key.

I haven't considered that during implementation, and it would be possible to fix that, by adding an additional wrapper around explicitly encoded optionals to indicate if they are nil, or contain some value.

The problem is that the Codable protocols to implement do not treat Optionals merely as syntactic sugar. Functions like decodeIfPresent() or encodeNil() show that quite clearly. Within the encoding and decoding routines, it's sometimes impossible to determine if a given value is actually the .some(value) case of an optional or just value.

I'll have to look more into these specific cases. There may be more complex cases, as it's also possible to encounter things like String??.

Regarding your JSON example above, I having trouble thinking of any situation where it's actually useful to distinguish between an implicit nil value (missing key), to an explicit nil. The former is certainly more efficient. It makes sense to fail if a non-optional is missing, but the whole point of an optional is that it doesn't necessarily need to have a value.

christophhagen commented 1 year ago

I did some more digging, and it seems that the implemented logic is hard to adapt to your use case. It also doesn't support double optionals, meaning that encoding a value of let value: String?? = .some(nil) will be wrongfully decoded as just nil. I can fix all of this with a bit of time.

But at this point, I'm thinking about deprecating this library in favour of other binary encoders like PotentCodables and CBORCoding. I initially wrote this library because I didn't know these existed, and my library has few advantages over them. On the contrary, these alternatives are mature and available on other systems.

bryceco commented 1 year ago

No problem, I've only started looking at various binary coding solutions. I'll investigate CBOR or a BSON solution.

bryceco commented 1 year ago

I played around with this a little starting with Mike Ash's implementation I came up with this method of dealing with optionals that seems to work:

protocol OptionalProtocol {
    static var wrappedType: Any.Type { get }
    var isNil: Bool { get }
    var value: Any { get }
    static var nilValue: Any { get }
}

extension Optional: OptionalProtocol {
    static var wrappedType: Any.Type { return Wrapped.self }
    var isNil: Bool { return self == nil }
    var value: Any { return self! }
    static var nilValue: Any { Wrapped?(nil) as Any }
}

    func encode(_ encodable: Encodable) throws {
        switch encodable {
        case let optional as OptionalProtocol:
            if optional.isNil {
                try encode(false)
            } else {
                try encode(true)
                try encode(optional.value as! BinaryEncodable)
            }
            return
        case let v as Int:
etc.

    func decode<T: Decodable>(_ type: T.Type) throws -> T {
        switch type {
        case let valueT as OptionalProtocol.Type:
            let present = try decode(Bool.self)
            if present {
                let t = valueT.wrappedType as! BinaryDecodable.Type
                let v = try t.init(fromBinary: self)
                return v as! T
            } else {
                return valueT.nilValue as! T
            }
        case is Int.Type:
etc.
christophhagen commented 1 year ago

I'm using a very similar protocol AnyOptional to handle optionals. But the implementation is a bit different (encoding an additional bool), I'll have a look at it.

christophhagen commented 1 year ago

The new release 2.0.0 should fix the issues with optionals. It uses a different format with an additional byte to indicate an optional.

However, for custom implementations using KeyedEncodingContainer, nil values are not included in the binary data, so encodeNil() and encodeIfPresent(nil) produce the same binary. If you explicitly need to encode nil for a key, directly pass the Optional to encode(), like in your first implementation:

let name: String? = nil
try container.encode(name, forKey: .name)

This may be a bit different to other encoders, but it's not easily implementable with this binary format and the constraints from the Codable framework.