New AnyCodable implementation

[dankinsoid]

Problem Statement

The current `AnyCodable implementation, which relies on type casting, is inconsistent and has some hidden bugs.

Inconsistency Details

The init(from: Decoder) implementation casts to [String: Any?], while the Equatable implementation casts to [String: AnyCodable]. This causes an inconsistency.

Examples

Example 1

This simple test fails:

func testEqualityFromJSON() throws {
    let json = """
    {
        "boolean": true,
        "integer": 1,
        "string": "string",
        "array": [1, 2, 3],
        "nested": {
            "a": "alpha",
            "b": "bravo",
            "c": "charlie"
        }
    }
    """.data(using: .utf8)!
    let decoder = JSONDecoder()
    let anyCodable0 = try decoder.decode(AnyCodable.self, from: json)
    let anyCodable1 = try decoder.decode(AnyCodable.self, from: json)
    XCTAssertEqual(anyCodable0, anyCodable1)
}

This test tries to decode JSON into two instances of AnyCodable and compare them for equality. It fails because of the aforementioned inconsistency.

Example 2

This test fails because only a few native types can be encoded:

func test_encodable() throws {
   let value = SomeCustomEncodableStruct()
   let anyCodable = AnyCodable(value)
   _ = try JSONEncoder().encode(anyCodable)
}

Proposed Solution

I propose a new AnyCodable enum that is both consistent and backward compatible.

Testing

The new implementation can be tested by updated AnyCodableTests

[mattpolzin]

Thanks for the PR. I'm going to need to give this a proper read through (and figure out why Bitrise is failing to build this with a mysterious error); I am heading out for a trip and leaving the laptop at home, so I wanted to let you know I won't be able to get back to this PR for a week or two, but it'll be on my todo list as soon as I return from vacation.

[mattpolzin]

I see that OpenAPIKit's relatively old Swift version support is causing some headache. Hard to code in an old style of Swift and forget the new stuff you've learned since 5.1.

I do think I want to retain Swift 5.1 support for version 3 of OpenAPIKit. I think that version 4, which could come much faster on version 3's tail than the current major version bump, would be a nice place to specifically focus on bumping the Swift version requirement of the library. Doing it separately and explicitly as a goal of the version bump will motivate making broad changes to the codebase instead of having older parts of the codebase use older Swift coding styles than newer parts of the codebase.

Anyway, I see you're already working on adapting to the old version of Swift, I just figured I would give some thoughts on the subject of supporting Swift 5.1.

[dankinsoid]

@mattpolzin Hello! I have some troubles with code coverage. I got 97.95% yesterday, I added some tests and then I got 97.92% 🤔 Maybe exclude tests themself from codecov? Also I notice, codecov takes into account even private API, imho it shouldn't.

[mattpolzin]

@dankinsoid some parts of OpenAPIKit's test framework involves test helpers that I've on occasion discovered are not being called because I've failed to test a situation I intended to test so including tests in the coverage numbers has proven useful to me.

If you feel good about test coverage I'll take a closer look at this PR and consider adjusting the requirement downward. In my experience decoder/encoder implementations are definitely not areas I've cared to cover to as high of a percentage by their nature.

[mattpolzin]

This is an impressive chunk of work! I agree whole-heartedly with the goals laid out in the PR description.

I don't really have any critiques about the code you have here, but I was able to uncover a bit more work needed to support all of the things the old AnyCodable type supported. The easiest way to spot things was to pull your branch into my other downstream projects where I get compilation errors in the following spots within the OpenAPIReflection library:

1. Sources/OpenAPIReflection/AnyJSONCaseIterable.swift:48
2. Sources/OpenAPIReflection/AnyJSONCaseIterable.swift:74

The first error is no exact matches in call to initializer and the second error is no exact matches in call to instance method 'map'. The first error is reported when passing the AnyCodable() constructor a value from the allCases property of some type conforming to CaseIterable. The second error is reported when passing the AnyCodable.init constructor values of type Any.

[mattpolzin]

Suggested change

	var dictionary = ["a": 1, "b": 2, "c": 3]
	var dictionary = ["a": 1, "b": 2, "c": 3]
	var data = "hello".data(using: .utf8)
	var decimal = Decimal(10)

This should be the way to test the other two special cases in your func encode(_ value: Encodable) throws -> AnyCodable function. The first property works once the additional field is added to the relevant test cases above. The second property appears to expose a bug in the encode function because Decimal gets encoded as a String value which then will fail to decode as a Decimal.

[dankinsoid]

@mattpolzin Thank you very much, I'll check it all soon

[dankinsoid]

I've observed that certain Foundation types (URL, Decimal, Data) don't have optimal native encoding/decoding methods. While JSONEncoder/JSONDecoder offer custom encoding strategies for these types, I believe the AnyCodable encoder should also provide a flexible means to customize encoding strategies for any type. Hence, I've introduced the ValueEncodingStrategy.

[dankinsoid]

I checked your OpenAPIReflection cases, now there is .init(Encodable), but it throws, because it uses encoder under hood. I can replace try encode() with try? encode() ?? .null.

[mattpolzin]

Will .init(Encodable) suffice for https://github.com/mattpolzin/OpenAPIReflection/blob/main//Sources/OpenAPIReflection/AnyJSONCaseIterable.swift#L74..L74 where a value happens to be Encodable but the compiler may only know it as Any?

Unless I am missing something, I don't see your new .init(Encodable) in this PR; have you pushed it up yet? I'd be curious to see why it is necessary to encode something on initialization instead of just when actually encoding it. Perhaps because part of the point is to make sure we have something equatable... Is this an indication that backwards compatibility is not worth it?

Maybe your new type is functionally just as good, if only you didn't need to support initialization from Any. Though I would need to think more about whether that really does serve enough use cases (without supporting Any) because I use AnyCodable pretty extensively in downstream projects (for the reflection library, yes, but also in libraries of mine that support generating Swift types from OpenAPI documents and even testing OpenAPI documents for consistency in their examples using those generated Swift types).

[dankinsoid]

Hi, I apologize for the delayed response. I've returned .init(Any) with your switch by type. In this case, encoding on init should occur rarely. I believe that encoding through AnyCodableEncoder is an extremely fast operation, but if it matters to you, I can make it lazy.

[mattpolzin]

Those older compiler versions were not nearly as good at type inference; I've noticed that on a few occasions recently.

The first thing I am doing with the OpenAPIKit v4 release branch is dropping support for anything prior to Swift 5.8; that'll be nice!

[mattpolzin]

I was curious about taking a different tactic to provide some comparison; whereas you've created a new Encoder which I think is the "rebuild from the ground up" tactic, I wanted to see how much could be accomplished by, roughly, "bolting something on."

I've managed something that passes the two test cases you gave in your initial description for this PR, but I would love if you had time to take a look and let me know if I have missed the point -- it seems very possible that I've fixed the narrow scope of the two examples you gave but not really managed to fix the bigger problems they represent.

My work is on this branch so you can pull it down and try to break it if you don't mind the exploration: https://github.com/mattpolzin/OpenAPIKit/compare/anycodable-correctness-exploration

[dankinsoid]

Hi @mattpolzin, yes, your fixes are much more shorter. I checked them and found some edge case, this test fails:

   func test_encodedDecodedQuality() throws {
        let value = URL(string: "https://www.google.com")
        let anyCodable = AnyCodable(value)
        
        let encodedValue = try JSONEncoder().encode(value)
        let encodedCodable = try JSONEncoder().encode(anyCodable)
        
        let decodedFromValue = try JSONDecoder().decode(AnyCodable.self, from: encodedValue)
        XCTAssertEqual(anyCodable, decodedFromValue)
        
        let decodedFromAnyCodable = try JSONDecoder().decode(AnyCodable.self, from: encodedCodable)
        XCTAssertEqual(anyCodable, decodedFromAnyCodable)
    }

I beleve it fails because URL, Decimal (and maybe Data, Date) encoding/decoding is customized in JSONEncoder/JSONDecoder, PropertyListEncoder/PropertyListDecoder. Also this may be due to the fact that the types list you consider in init(from decoder: Decoder) method are different from the types list you consider in encode(to encoder: Encoder)
I'll check how to fix it

[mattpolzin]

I've merged and released OpenAPIKit 3.0.0. That created some merge conflicts with this PR, though there's no fundamental incompatibility.

[dankinsoid]

I've merged and released OpenAPIKit 3.0.0. That created some merge conflicts with this PR, though there's no fundamental incompatibility.

Hi, I'll check conflicts, thank you

[dankinsoid]

I've merged and released OpenAPIKit 3.0.0. That created some merge conflicts with this PR, though there's no fundamental incompatibility.

Hi! I've updated the branch

[mattpolzin]

Heads up that your tests are failing on here (of course that wasn't clear until I accepted GitHub's request to run them again, a very annoying GitHub "feature").

I don't think you should spend the time to get the tests working again, though. After doing some more hard thinking, I have decided that I am very unlikely to want to incorporate another Encoder implementation into OpenAPIKit. It's quite a bit of maintenance overhead to add new Encoders, but there's also the fact that I think both your Encoder based implementation and my* non-Encoder based implementation have their own limitations.

*well, it's really mostly Flight School's implementation that I borrowed.

The limitations of my approach were uncovered by you (and I truly appreciate that): If you take an AnyCodable and you encode it and then decode it then it is not guaranteed to be equal to the original AnyCodable. The limitation of your approach (I believe) is that two AnyCodables created with different Swift types are equal sometimes even when the types they came from are different (e.g. String and URL). In different situations, either one of these is more important than the other. You could make the limitation of your Encodable based approach less surprising by not supporting e.g. .init(Encodable), but you break backwards compatibility (not a deal breaker) and you stop supporting use cases where it is beneficial to consider AnyCodables based on more than just the foundational types as distinct (arguably more of a deal breaker).

You brought to my attention more than just the encode -> decode equality limitation, though. I've patched all of the other issues you brought up (support for arbitrary Encodable types and equality of nested structures within AnyCodable) in a newly merged branch against v4 of OpenAPIKit here. That branch also adds a fair bit of README documentation around the limitations of AnyCodable and suggests a relatively simple function that essentially adopts the equality behavior you would like to see by encoding and then checking for equality -- in other words, where your approach encodes on init of AnyCodable, this function would support encoding specifically when you care about encode-equality and it does so using any existing Encoder (or a new one, if desired).

I want to stress my gratitude for you bringing these issues up; several of them were bugs I was very happy to patch and one of them was a more hard limitation on the approach I took which made for important additional documentation. I couldn't have made the decision I did without having seen all of your hard work in this branch, either. 🙏

[dankinsoid]

@mattpolzin, thank you very much. I'll try to adapt my VaporToOpenAPI for your v4 library, and if the equating works fine, I'll be happy with it.