Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error when use Codable in class inheritance #24

Open
nightwill opened this issue Dec 30, 2024 · 7 comments
Open

Error when use Codable in class inheritance #24

nightwill opened this issue Dec 30, 2024 · 7 comments

Comments

@nightwill
Copy link

I'm trying to use this wonderful library in a class that inherits from the base Codable class.

public override func encode(to encoder: any Encoder) throws {
    try super.encode(to: encoder)
    var container = encoder.container(keyedBy: CodingKeys.self)
    try container.encode(id, forKey: .id)
}

And the error appears Multiple calls to container(keyedBy:), unkeyedContainer(), or singleValueContainer()

@nightwill
Copy link
Author

I was able to solve this problem this way. First I added the auxiliary protocol.

private protocol _KeyedEncoder: AnyObject {
    var encodedValues: [HashableKey : EncodableContainer] { get set }
}

extension KeyedEncoder: _KeyedEncoder { }

Then

private func assign<T>(_ valueCreator: @autoclosure () -> T) -> T where T: EncodableContainer {
    // Prevent multiple calls with different containers container(keyedBy:), unkeyedContainer(), or singleValueContainer()
    if let encodedValue = encodedValue as? T {
        return encodedValue
    } else if let previousKeyedContainer = encodedValue as? _KeyedEncoder, T.self is _KeyedEncoder.Type {
        let value = valueCreator()
        (value as! _KeyedEncoder).encodedValues = previousKeyedContainer.encodedValues
        encodedValue = value
        return value
    } else {
        if encodedValue != nil {
            hasMultipleCalls = true
        }
        let value = valueCreator()
        encodedValue = value
        return value
    }
}

And then just remove private for encodedValues in KeyedEncoder

@christophhagen
Copy link
Owner

Interesting. So your goal is to encode values into the same keyed container in both the Superclass and the Subclass?

When I implemented the logic, it seemed reasonable to only one call to a container (single, keyed, or unkeyed).
The codable protocol provides the superEncoder() method, which seemed sufficient for data structures with inheritance.
But I did a few quick tests, and it seems that I was more strict than e.g. JSONDecoder, which allows:

singleValueContainer()

Multiple calls allowed, but only one value can be encoded (-> Exception)

unkeyedContainer()

Multiple calls allowed. All values are encoded sequentially, independent of which container is used to call encode().

keyedContainer()

Multiple calls allowed, even with different CodingKey types. The last value is used when encoding values with the same raw key.

Mixing containers

It's not allowed (Exception) to call any combination of different containers

I'll have to think about the best way to handle this. Some of these "features" can't be integrated directly, since each encoding node manages its own storage. Using multiple containers at the same time requires them to share the same backing storage.

The solution you used works for your case, but once the second call to keyedContainer() is made, the first one is no longer considered:

func encode(to encoder: any Encoder) throws {
    var container = encoder.container(keyedBy: CodingKeys.self)
    var container2 = encoder.container(keyedBy: CodingKeys.self)
    try container.encode(a, forKey: .a) // Not saved
    try container2.encode(b, forKey: .b)
}

I think it's possible to make this work, but it will require larger changes to the code base.

@nightwill
Copy link
Author

Hi Christoph. Yes, that’s true and it’s not an easy task. I did something similar a long time ago, but only for a specialized data format and it’s already difficult for me to remember exactly how it all works. I'll give some code snippets, maybe it will help you somehow. But probably the main idea is to store data by reference and then container and container2 will both write the data (from your example)

private enum RootObject {
    case groupObject(GroupObject) // GroupObject is a reference type
    case dataset(Dataset)
    case attr(Attribute)
    case value(Any)
}

func container<Key>(keyedBy type: Key.Type) throws -> KeyedDecodingContainer<Key> where Key : CodingKey {
        switch rootObject {
        case .groupObject(let groupObject):
            let container = GroupObjectContainer<Key>(groupObject: groupObject, context: context)
            return .init(container)
        case .value(let value):
            guard let dict = value as? [String: Any] else {
                throw "Should be Group Object or Dict"
            }
            let container = DictionaryContainer<Key>(values: dict, context: context)
            return .init(container)
        case .attr(let attribute):
            guard let dict = try attribute.readAny() as? [String: Any] else {
                throw "Attribute should be Compound Type"
            }
            let container = DictionaryContainer<Key>(values: dict, context: context)
            return .init(container)
        default:
            throw "Should be Group Object or Dict"
        }
}

And one more thing, decoding with inheritance also did not work correctly.

And thank you for your work and such a cool package. This should be part of the standard library someday 🙂

@christophhagen
Copy link
Owner

Thanks for the input.
I agree that we need a reference type backing the Encoder, with multiple containers writing to the same underlying storage.
Should be possible, once I find a bit of time :)

You mentioned that decoding with inheritance didn't work, do you have an example?
There are a few basic tests regarding that, but inheritance can get arbitrarily complex, so it's likely that there are edge cases that don't work.

@nightwill
Copy link
Author

Thank you for your answer. How great it would be if we all had more free time 🙂 You're right that this is a borderline case and it's not urgent, you already did a great job.

When I was preparing an example for you, I noticed something - if you do not use the custom init(from: Decoder) encode(to: Encoder) then everything works fine (apparently Swift generates the code somehow differently, perhaps adding all the CodingKeys from the base class ). But here’s an almost real example when I need to serialize a picture

import BinaryCodable
import AppKit
import Foundation

class ParentClass: Codable {
    var text: String = ""
}

final class ChildClass: ParentClass {
    var image: NSImage?

    enum CodingKeys: String, CodingKey {
        case image
    }

    override init() {
        self.image = nil
        super.init()
    }

    required init(from decoder: any Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        if let data = try container.decodeIfPresent(Data.self, forKey: .image) {
            self.image = NSImage(data: data)
        } else {
            self.image = nil
        }
        try super.init(from: decoder)
    }

    override func encode(to encoder: any Encoder) throws {
        try super.encode(to: encoder)
        var container = encoder.container(keyedBy: CodingKeys.self)
        try container.encodeIfPresent(image?.tiffRepresentation, forKey: .image)
    }
}

let child = ChildClass()
child.image = NSImage(systemSymbolName: "plus", accessibilityDescription: nil)

let data1 = try! JSONEncoder().encode(child)
let restored1 = try! JSONDecoder().decode(ChildClass.self, from: data1)

print(restored1)

let data2 = try! BinaryEncoder().encode(child)
let restored2 = try! BinaryDecoder().decode(ChildClass.self, from: data2)

print(restored2)

As you can see through JSONDecoder, the image is successfully restored, but BinaryDecoder fails with an error.

@christophhagen
Copy link
Owner

Thanks for the example. It looks like the root cause for the inheritance issue is the same, where it was previously not possible to call container(keyedBy:) twice on the same encoder.
I've pushed changes to the library on the branch multiple-containers (pull request #25), which fixes these issues according to the test cases I created.

It wasn't actually that difficult. I simply extracted the encoded data storage to separate classes and simply return wrapper structs to it.
The behaviour should now be very similar to JSONEncoder.
Feel free to check it out, I will merge it into master in a few days after some more testing, there is an additional issue I wanted to investigate as well.

Thanks for bringing this issue to my attention.

@nightwill
Copy link
Author

Hi Christoph.

I tested it on my code and everything works fine, thank you very much 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants