mattpolzin / OpenAPIKit

Codable Swift OpenAPI implementation.
MIT License
276 stars 35 forks source link

Incorrect decoding of `type: null` from Yaml #364

Closed brandonbloom closed 3 months ago

brandonbloom commented 3 months ago

This test exists:

    func test_decodeNullType() throws {
        let nullTypeData = """
        {
          "type": "null"
        }
        """.data(using: .utf8)!

        let decoded = try orderUnstableDecode(JSONSchema.self, from: nullTypeData)

        XCTAssertEqual(decoded, .null())
    }

However, adding a similar test for Yaml highlights the bug:

diff --git a/Tests/OpenAPIKitTests/Schema Object/SchemaObjectYamsTests.swift b/Tests/OpenAPIKitTests/Schema Object/SchemaObjectYamsTests.swift
index 12c7c10c..1d32f5d7 100644
--- a/Tests/OpenAPIKitTests/Schema Object/SchemaObjectYamsTests.swift   
+++ b/Tests/OpenAPIKitTests/Schema Object/SchemaObjectYamsTests.swift   
@@ -16,6 +16,20 @@ import OpenAPIKit
 import Yams

 final class SchemaObjectYamsTests: XCTestCase {
+    func test_nullTypeDecode() throws {
+        let nullString =
+        """
+        type: 'null'
+        """
+
+        let null = try YAMLDecoder().decode(JSONSchema.self, from: nullString)
+
+        XCTAssertEqual(
+            null,
+            JSONSchema.null()
+        )
+    }
+
     func test_floatingPointWholeNumberIntegerDecode() throws {
         let integerString =
         """

In short, nil is returned, when JSONType.null is expected.

brandonbloom commented 3 months ago

I've been looking at this a little more and have narrowed the problem down to decodeIfPresent for JSONType. This is used in a few places (decodeNullable and decodeTypes) and behaves incorrectly for type: null and type: 'null'. Interestingly, container.contains(.type) ? container.decode(JSONType.self, forKey: .type) : nil works as expected.

mattpolzin commented 3 months ago

So this is actually related to a Yams bug that has been fixed as of version 5.1.0 of Yams. I had purposefully avoided writing a test for Yams decoding similar to the one you wrote because I knew it would not pass. Now that Yams 5.1.0 is released, it can be added (and passes): https://github.com/mattpolzin/OpenAPIKit/pull/365

Interestingly, the Yams fix also only works for Swift releases later than 5.4, it appears. A bit surprising. That means that I can't bump the locked version of Yams for the project prior to OpenAPIKit 4.0.0.

Here's a little more background: https://github.com/apple/swift-openapi-generator/issues/286#issuecomment-1749127706. In that comment, you can focus on the second paragraph where I explain and link to a Yams bug that describes the behavior you've described in the OP for this issue.

mattpolzin commented 3 months ago

I see you came to the same conclusion as I was typing up my response!

mattpolzin commented 3 months ago

Your PR looks good to me if you rebase it against release/4_0.

brandonbloom commented 3 months ago

Rebase: Done.