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

Swift OpenAPI Generator Fails to Decode anyOf Structure for nullable Field #286

Open
brandonmaul opened this issue Sep 20, 2023 · 41 comments
Labels
area/generator Affects: plugin, CLI, config file. kind/support Adopter support requests. status/blocked Waiting for another issue.

Comments

@brandonmaul
Copy link

brandonmaul commented Sep 20, 2023

When using the Swift OpenAPI Generator and runtime to decode a JSON payload from an API endpoint, a decoding error occurs for a field defined with the anyOf keyword in the OpenAPI spec. The error message is "The anyOf structure did not decode into any child schema."

Here is a simplified snippet of the OpenAPI spec that defines the response schema:

"ResponseSchema": {
  "properties": {
    "status": { "type": "string", "title": "Status" },
    "timestampUTC": {
      "type": "string",
      "format": "date-time",
      "title": "Timestamputc"
    },
    "errorCode": {
      "anyOf": [{ "type": "integer" }, { "type": "null" }],
      "title": "Errorcode"
    },
    "errorMessage": {
      "anyOf": [{ "type": "string" }, { "type": "null" }],
      "title": "Errormessage"
    },
    "data": {
      "type": "object"
    }
  },
  "type": "object",
  "required": [
    "status",
    "timestampUTC",
    "errorCode",
    "errorMessage",
    "data"
  ]
}

Here is a snippet of the JSON payload received from the API endpoint:

{
  "status": "success",
  "timestampUTC": "2023-09-20T12:20:29.606380Z",
  "errorCode": null,
  "errorMessage": null,
  "data": {}
}

Error Message:

DecodingError: valueNotFound errorCodePayload - at CodingKeys(stringValue: "errorCode", intValue: nil): The anyOf structure did not decode into any child schema. (underlying error: <nil>)

Steps to Reproduce:

Generate Swift code using the provided OpenAPI spec.
Make an API request to receive the JSON payload.
Attempt to decode the JSON payload using the generated Swift code.
Expected Behavior:
The JSON payload should be successfully decoded into the corresponding Swift model.

Actual Behavior:

Decoding fails with the error message mentioned above.

Environment:

Swift OpenAPI Generator version: 0.2.2
Swift version: 5.9
Runtime: 0.2.3
OpenAPI Spec version: 3.1.0

OpenAPI Generator config:

generate:
  - types
  - client
featureFlags:
  - nullableSchemas
@czechboy0
Copy link
Contributor

Just to be safe, can you try with the latest 0.2.x version and the feature flag nullableSchemas? It might not resolve it, but I'd like to make sure. Thanks 🙏

@czechboy0 czechboy0 added the status/triage Collecting information required to triage the issue. label Sep 20, 2023
@brandonmaul
Copy link
Author

Updated to version 0.2.X, but still seeing the same behavior. updated original description

@czechboy0
Copy link
Contributor

@brandonmaul one more question, is your document using OpenAPI 3.0 or 3.1?

@brandonmaul
Copy link
Author

Ah! 3.1.0 Apologies I missed putting this in the original description

@czechboy0
Copy link
Contributor

Gotcha, thanks. @mattpolzin what's your read on how that should be represented in OpenAPIKit? I haven't checked yet what we get today.

@brandonmaul our of curiosity, why not spell this as a nullable value instead of the anyof?

@brandonmaul
Copy link
Author

brandonmaul commented Sep 20, 2023

@czechboy0 Honestly, that's a great question, and I'm unsure of why it's generated like that. The response model is generated from the definition below. We're using FastAPI with Pydantic for this.

from typing import Optional, Any, Generic, TypeVar
from datetime import datetime, timezone
from pydantic import BaseModel, validator

ModelType = TypeVar('ModelType')

class ResponseSchema(BaseModel, Generic[ModelType]):
    status: str
    timestampUTC: datetime 
    errorCode: Optional[int] = None
    errorMessage: Optional[str] = None
    data: Optional[ModelType]

    @validator('errorCode', always=True)
    def check_consistency(cls, v, values):
        if v is not None and values['status'] == "success" :
            raise ValueError('Cannot provide both an errorCode and success status in the same response')
        if v is None and values.get('status') == "error":
            raise ValueError('Must provide an errorCode in response when there is an error')
        
        return v

It's my understanding that when Pydantic serialises Python Optional types to OpenAPI, it uses the anyOf construct to indicate that the field can be either of the specified type or null. Perhaps it's because anyOf is more general and can handle more complex scenarios? Obviously for this it seems to be a bit of overkill for simple nullable fields.

@czechboy0
Copy link
Contributor

Ah, it's not a hand-written document, I see.

I'll try to repro locally and it'll likely be on us to fix. Thanks for reporting!

@mattpolzin
Copy link

Just to speak for OpenAPIKit, I try not to implement this particular kind of heuristic (turning the above into a nullable string instead of leaving it as an anyOf) because I think the sheer number of ways anyOf might be used would make that difficult to do generally and not sustainable if tackled one specific case at a time.

The fact that JSON schemas can often represent simpler scenarios as more complex schemas was the motivation behind the OpenAPIKit code that "simplifies" schemas; this code only currently simplifies allOf schemas but I always wanted it to handle anyOf and others as well.

I realize I've walked a line here in the past since I do handle e.g. type: [string, null] as a nullable string rather than strictly storing the two types it can handle. Perhaps that is arguably a step too far as well but the redesign would have been more work and less backwards compatible so for now it will have to remain.

@mattpolzin
Copy link

I didn't even really answer the question. In OpenAPIKit this is represented as an anyOf with two schemas within it, no funny business.

@mattpolzin
Copy link

Would it be helpful if I applied a change that propagated nullable such that "if any schema in an anyOf is nullable then the parent anyOf is nullable?" That I could do. I do that with allOf.

@czechboy0
Copy link
Contributor

Yeah, and remove the null case from the child schemas. I don't think it should stop being an anyof, but it'd be easier if this was parsed as a nullable anyof of a single child schema. Since Swift doesn't have the equivalent of NSNull, I don't think it makes sense to treat null as a separate child schema of the anyof, as I'm not sure what its associated value would even be.

@mattpolzin
Copy link

mattpolzin commented Sep 20, 2023

How about this compromise: I’ll propagate nullable to the root of an anyOf and then swift-openapi-generator can choose to ignore .null json schemas. Roughly what you said but minus me removing the child schema.

@czechboy0
Copy link
Contributor

Yup that works, it's trivial to filter out the null for us.

@brandonmaul
Copy link
Author

brandonmaul commented Oct 3, 2023

@czechboy0 I'd love to help out in solving this if I can. I've built my career on Swift, but I've not much experience with Open Source software development, especially on a project this big.

Any pointers on the what to do to get started? (in this project or OpenAPIKit)

@mattpolzin
Copy link

The necessary change to OpenAPIKit was made with the newest release candidate. This project's package manifest has already been updated to use that version, so the remaining work will be to filter out .null schemas under .anyOf schemas within this project.

That may not fully answer the question but I'm not as familiar with the internals of the generator so I'll quit while I'm ahead. 🙂

@czechboy0
Copy link
Contributor

Hi @brandonmaul - a good start is to update the reference tests and create a failing unit test that shows an example of where this doesn't work. Then work backwards. I haven't looked into this yet, so feel free to ask more questions as you investigate this.

But Matt is basically right, I think filtering out .null and adding a test might be enough, unless we discover some other impact.

@brandonmaul
Copy link
Author

brandonmaul commented Oct 3, 2023

As I was attempting to build the test case (and determine exactly what it is I want to see generated) I ran into a few questions.
Given this OpenAPISpec schema

"ResponseSchema": {
        "properties": {
          "status": { "type": "string", "title": "Status" },
          "timestampUTC": { "type": "string", "title": "Timestamputc" },
          "errorCode": {
            "anyOf": [{ "type": "integer" }, { "type": "null" }],
            "title": "Errorcode"
          },
          "errorMessage": {
            "anyOf": [{ "type": "string" }, { "type": "null" }],
            "title": "Errormessage"
          },
          "data": { "anyOf": [{}, { "type": "null" }], "title": "Data" }
        },
        "type": "object",
        "required": [
          "status",
          "timestampUTC",
          "errorCode",
          "errorMessage",
          "data"
        ],
        "title": "ResponseSchema"
      }

this was the generated response:

public struct ResponseSchema: Codable, Hashable, Sendable {
    /// - Remark: Generated from `#/components/schemas/ResponseSchema/status`.
    public var status: Swift.String
    /// - Remark: Generated from `#/components/schemas/ResponseSchema/timestampUTC`.
    public var timestampUTC: Swift.String
    /// - Remark: Generated from `#/components/schemas/ResponseSchema/errorCode`.
    public struct errorCodePayload: Codable, Hashable, Sendable {
        /// - Remark: Generated from `#/components/schemas/ResponseSchema/errorCode/value1`.
        public var value1: Swift.Int?
        /// - Remark: Generated from `#/components/schemas/ResponseSchema/errorCode/value2`.
        public var value2: OpenAPIRuntime.OpenAPIValueContainer?
        /// Creates a new `errorCodePayload`.
        ///
        /// - Parameters:
        ///   - value1:
        ///   - value2:
        public init(value1: Swift.Int? = nil, value2: OpenAPIRuntime.OpenAPIValueContainer? = nil) {
            self.value1 = value1
            self.value2 = value2
        }
        public init(from decoder: any Decoder) throws {
            value1 = try? decoder.decodeFromSingleValueContainer()
            value2 = try? .init(from: decoder)
            try DecodingError.verifyAtLeastOneSchemaIsNotNil(
                [value1, value2],
                type: Self.self,
                codingPath: decoder.codingPath
            )
        }
        public func encode(to encoder: any Encoder) throws {
            try encoder.encodeFirstNonNilValueToSingleValueContainer([value1])
            try value2?.encode(to: encoder)
        }
    }
    /// - Remark: Generated from `#/components/schemas/ResponseSchema/errorCode`.
    public var errorCode: Components.Schemas.ResponseSchema.errorCodePayload
    /// - Remark: Generated from `#/components/schemas/ResponseSchema/errorMessage`.
    public struct errorMessagePayload: Codable, Hashable, Sendable {
        /// - Remark: Generated from `#/components/schemas/ResponseSchema/errorMessage/value1`.
        public var value1: Swift.String?
        /// - Remark: Generated from `#/components/schemas/ResponseSchema/errorMessage/value2`.
        public var value2: OpenAPIRuntime.OpenAPIValueContainer?
        /// Creates a new `errorMessagePayload`.
        ///
        /// - Parameters:
        ///   - value1:
        ///   - value2:
        public init(value1: Swift.String? = nil, value2: OpenAPIRuntime.OpenAPIValueContainer? = nil) {
            self.value1 = value1
            self.value2 = value2
        }
        public init(from decoder: any Decoder) throws {
            value1 = try? decoder.decodeFromSingleValueContainer()
            value2 = try? .init(from: decoder)
            try DecodingError.verifyAtLeastOneSchemaIsNotNil(
                [value1, value2],
                type: Self.self,
                codingPath: decoder.codingPath
            )
        }
        public func encode(to encoder: any Encoder) throws {
            try encoder.encodeFirstNonNilValueToSingleValueContainer([value1])
            try value2?.encode(to: encoder)
        }
    }
    /// - Remark: Generated from `#/components/schemas/ResponseSchema/errorMessage`.
    public var errorMessage: Components.Schemas.ResponseSchema.errorMessagePayload
    /// - Remark: Generated from `#/components/schemas/ResponseSchema/data`.
    public struct dataPayload: Codable, Hashable, Sendable {
        /// - Remark: Generated from `#/components/schemas/ResponseSchema/data/value1`.
        public var value1: OpenAPIRuntime.OpenAPIValueContainer?
        /// - Remark: Generated from `#/components/schemas/ResponseSchema/data/value2`.
        public var value2: OpenAPIRuntime.OpenAPIValueContainer?
        /// Creates a new `dataPayload`.
        ///
        /// - Parameters:
        ///   - value1:
        ///   - value2:
        public init(
            value1: OpenAPIRuntime.OpenAPIValueContainer? = nil,
            value2: OpenAPIRuntime.OpenAPIValueContainer? = nil
        ) {
            self.value1 = value1
            self.value2 = value2
        }
        public init(from decoder: any Decoder) throws {
            value1 = try? .init(from: decoder)
            value2 = try? .init(from: decoder)
            try DecodingError.verifyAtLeastOneSchemaIsNotNil(
                [value1, value2],
                type: Self.self,
                codingPath: decoder.codingPath
            )
        }
        public func encode(to encoder: any Encoder) throws {
            try value1?.encode(to: encoder)
            try value2?.encode(to: encoder)
        }
    }
    /// - Remark: Generated from `#/components/schemas/ResponseSchema/data`.
    public var data: Components.Schemas.ResponseSchema.dataPayload
    /// Creates a new `ResponseSchema`.
    ///
    /// - Parameters:
    ///   - status:
    ///   - timestampUTC:
    ///   - errorCode:
    ///   - errorMessage:
    ///   - data:
    public init(
        status: Swift.String,
        timestampUTC: Swift.String,
        errorCode: Components.Schemas.ResponseSchema.errorCodePayload,
        errorMessage: Components.Schemas.ResponseSchema.errorMessagePayload,
        data: Components.Schemas.ResponseSchema.dataPayload
    ) {
        self.status = status
        self.timestampUTC = timestampUTC
        self.errorCode = errorCode
        self.errorMessage = errorMessage
        self.data = data
    }
    public enum CodingKeys: String, CodingKey {
        case status
        case timestampUTC
        case errorCode
        case errorMessage
        case data
    }
}

Im noticing the try DecodingError.verifyAtLeastOneSchemaIsNotNil in the initialiser of the anyOf properties. I think that may be part of the problem? Shouldn't the initialiser allow the value be to null if null is included in anyOf? It's at this point I got lost trying to figure out what the "correctly" generated code should look like.

Another question I have is, why are the null options in the anyOf get converted into the type OpenAPIRuntime.OpenAPIValueContainer??

For example, "anyOf": [{ "type": "string" }, { "type": "null" }], gets turned into

public var value1: Swift.String?
public var value2: OpenAPIRuntime.OpenAPIValueContainer?

I don't think I fully understand what the OpenAPIRuntime.OpenAPIValueContainer type is, or at least I don't see the relationship it has with null. Upon further inspection, Im noticing that type is also used for the empty object that the data property could be. Im guessing it's just a type eraser for any type that could expressed in JSON?

@czechboy0
Copy link
Contributor

Correct, it's a free-form JSON value.

Why it's being used for null here? I don't know, that isn't intentional, so warrants further investigation.

And I think you're right, if null is one of the types, we should not generate the verify call at the end of the initializer.

Seems you're on the right track!

In the generated types for the properties, I'd expect only one property, not two. That's where the filtering out of null should come in, but we need to also keep track of null being one of the options so that we skip generating the verify call.

@brandonmaul
Copy link
Author

Awesome! I'll keep investigating. Thanks for the info!

@brandonmaul
Copy link
Author

@czechboy0 what are your thoughts on what the "correct" output should be for the translation?
I have this idea that
"anyOf": [{ "type": "string" }, { "type": "null" }]
should just be translated to

public var value1: Swift.String?

Basically, ignoring the "null" value entirely, and then later removing the check to make sure at least 1 isn't null. I mulled over the idea of using CFNull or NSNull as the type for "value2", but this seems cleaner.

Upon more investigation, I noticed that actually the "null" schema in the anyOf is being translated into the OpenAPIKit JSONSchema.fragment type.
Perhaps this screenshot shows how I arrived at that.

Screenshot 2023-10-04 at 10 42 07 am

Is this correct? I see there's a JSONSchema.null type that I would believe makes more sense.

@czechboy0
Copy link
Contributor

@czechboy0 what are your thoughts on what the "correct" output should be for the translation? I have this idea that "anyOf": [{ "type": "string" }, { "type": "null" }] should just be translated to

public var value1: Swift.String?

Basically, ignoring the "null" value entirely, and then later removing the check to make sure at least 1 isn't null.

Yup that's what I was thinking as well. Filter it out, just keep track of the fact so that you also omit the verify call at the end of the decoding initializer.

@mattpolzin
Copy link

Upon more investigation, I noticed that actually the "null" schema in the anyOf is being translated into the OpenAPIKit JSONSchema.fragment type.
Is this correct? I see there's a JSONSchema.null type that I would believe makes more sense.

Yeah, that looks like not what I would expect. In fact, it seems contradictory to a test case in OpenAPIKit where I assert that "anyOf": [{ "type": "string" }, { "type": "null" }] becomes a nullable anyOf with .string and .null beneath it...

Can you confirm for me that your local copy of OpenAPIKit resolved for swift-package-generator is 3.0.0-rc.2 @brandonmaul? What you're seeing looks like a bug, but first I need to figure out how it's possible that my test case missed the mark.

@mattpolzin
Copy link

Filter it out, just keep track of the fact so that you also omit the verify call at the end of the decoding initializer.

RE keeping track of having filtered it out, this should be one of two viable options, the other one being checking the coreContext of the anyOf schema to see if it is nullable: true, which may be more direct or lower overhead (but whichever works best here).

@brandonmaul
Copy link
Author

brandonmaul commented Oct 4, 2023

@mattpolzin Yep confirmed, the screenshot of the Xcode window shows that version under the package dependencies on the left

@mattpolzin
Copy link

@brandonmaul ah yes, the screenshot does have that info. thanks. is it possible you are inspecting another part of the same OpenAPI Document in the given screenshot? I notice the schemas under the anOf are .integer and .fragment whereas we've been mostly discussing .string (not to say it isn't the exact same kind of anyOf but just for a nullable integer instead of a string).

Assuming this is something like "anyOf": [{ "type": "integer" }, { "type": "null" }], do you mind sharing the exact schema contents so I can dig into why my test cases don't cover the situation you are encountering?

@brandonmaul
Copy link
Author

brandonmaul commented Oct 4, 2023

@mattpolzin

is it possible you are inspecting another part of the same OpenAPI Document in the given screenshot?

I don't think so (although I very well may be wrong). The test case the breakpoint was hit on is located on a test case I added: https://github.com/brandonmaul/swift-openapi-generator/blob/81b0cc0dee366e8a50255f925a2563c87ba7c8a9/Tests/OpenAPIGeneratorReferenceTests/SnippetBasedReferenceTests.swift#L431C2-L431C2

and in the debugger screenshot I think I confirmed it's looking at the "errorCode" property for ResponseSchema.

@mattpolzin
Copy link

mattpolzin commented Oct 4, 2023

@brandonmaul very strange. you've got me stumped; l was able to reproduce your experience by running locally myself and using Xcode to set breakpoints and inspect values.

However, I have to conclude that this is a bug with Xcode and/or the Swift language debugging tools, because the following added to the top of your new test case will produce the correct and expected results:

        let t = """
            {
                "properties": {
                    "status": { "type": "string", "title": "Status" },
                    "timestampUTC": { "type": "string", "title": "Timestamputc" },
                    "errorCode": {
                        "anyOf": [{ "type": "integer" }, { "type": "null" }],
                        "title": "Errorcode"
                    },
                    "errorMessage": {
                        "anyOf": [{ "type": "string" }, { "type": "null" }],
                        "title": "Errormessage"
                    },
                    "data": { "anyOf": [{}, { "type": "null" }], "title": "Data" }
                },
                "type": "object",
                "required": [
                    "status",
                    "timestampUTC",
                    "errorCode",
                    "errorMessage",
                    "data"
                ],
                "title": "ResponseSchema"
            }
            """.data(using: .utf8)!
        let u = try JSONDecoder().decode(OpenAPIKit.JSONSchema.self, from: t)
        print(String(describing: u))

Specifically, this appears in the String description:

"errorCode": OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.any(of: [OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.integer(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.IntegerFormat>(format: OpenAPIKitCore.Shared.IntegerFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: nil, description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]), OpenAPIKit.JSONSchema.IntegerContext(multipleOf: nil, maximum: nil, minimum: nil))), OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.null(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: true, _permissions: nil, _deprecated: nil, title: nil, description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:])))], core: OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: true, _permissions: nil, _deprecated: nil, title: Optional("Errorcode"), description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:])))

That's a mess to look at, no doubt, but I see an integer schema, a null schema, and a core context that has nullable: true and title: Optional("Errorcode").

@brandonmaul
Copy link
Author

Well thats confusing! Okay... I'll proceed assuming the JSONSchemas being evaluated are correct. Thanks so much for taking the time to confirm @mattpolzin!

@brandonmaul
Copy link
Author

brandonmaul commented Oct 5, 2023

Hey there again. So, although Matt proved yesterday that a "normal" decoding of the test case to JSONSchema should provide an core object context with nullable: true, I'm actually not seeing that when running the test case with assertSchemasTranslation.

I went to where I think the JSONSchema is originally being decoded. Here's a screenshot showing this:
Screenshot 2023-10-05 at 9 01 43 am

And here's the full output of po String(describing: translator.components.schemas.first!). If you do a search for "nullable: true", it doesn't appear once in this output. All the core context's in this type all have "nullable: false".

"(key: OpenAPIKitCore.Shared.ComponentKey(rawValue: \"ResponseSchema\"), value: OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.object(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.ObjectFormat>(format: OpenAPIKitCore.Shared.ObjectFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: Optional(\"ResponseSchema\"), description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]), OpenAPIKit.JSONSchema.ObjectContext(maxProperties: nil, _minProperties: nil, properties: OpenAPIKitCore.OrderedDictionary<Swift.String, OpenAPIKit.JSONSchema>(orderedKeys: [\"status\", \"timestampUTC\", \"errorCode\", \"errorMessage\", \"data\"], unorderedHash: [\"data\": OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.any(of: [OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.fragment(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: nil, description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]))), OpenAPIKit.JSONSchema(warnings: [Inconsistency encountered when parsing `OpenAPI Schema`: Found nothing but unsupported attributes..], value: OpenAPIKit.JSONSchema.Schema.fragment(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: nil, description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:])))], core: OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: Optional(\"Data\"), description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]))), \"status\": OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.string(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.StringFormat>(format: OpenAPIKitCore.Shared.StringFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: Optional(\"Status\"), description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]), OpenAPIKitCore.Shared.StringContext(maxLength: nil, _minLength: nil, pattern: nil))), \"errorMessage\": OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.any(of: [OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.string(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.StringFormat>(format: OpenAPIKitCore.Shared.StringFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: nil, description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]), OpenAPIKitCore.Shared.StringContext(maxLength: nil, _minLength: nil, pattern: nil))), OpenAPIKit.JSONSchema(warnings: [Inconsistency encountered when parsing `OpenAPI Schema`: Found nothing but unsupported attributes..], value: OpenAPIKit.JSONSchema.Schema.fragment(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: nil, description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:])))], core: OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: Optional(\"Errormessage\"), description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]))), \"timestampUTC\": OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.string(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.StringFormat>(format: OpenAPIKitCore.Shared.StringFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: Optional(\"Timestamputc\"), description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]), OpenAPIKitCore.Shared.StringContext(maxLength: nil, _minLength: nil, pattern: nil))), \"errorCode\": OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.any(of: [OpenAPIKit.JSONSchema(warnings: [], value: OpenAPIKit.JSONSchema.Schema.integer(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.IntegerFormat>(format: OpenAPIKitCore.Shared.IntegerFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: nil, description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:]), OpenAPIKit.JSONSchema.IntegerContext(multipleOf: nil, maximum: nil, minimum: nil))), OpenAPIKit.JSONSchema(warnings: [Inconsistency encountered when parsing `OpenAPI Schema`: Found nothing but unsupported attributes..], value: OpenAPIKit.JSONSchema.Schema.fragment(OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: nil, description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:])))], core: OpenAPIKit.JSONSchema.CoreContext<OpenAPIKitCore.Shared.AnyFormat>(format: OpenAPIKitCore.Shared.AnyFormat.generic, required: true, nullable: false, _permissions: nil, _deprecated: nil, title: Optional(\"Errorcode\"), description: nil, externalDocs: nil, discriminator: nil, allowedValues: nil, defaultValue: nil, examples: [], vendorExtensions: [:])))], _warnings: []), additionalProperties: nil))))"

Do I need to provide any feature flags or anything when testing? I noticed that the "nullableSchemas" feature flag I'd normally put the generator config isn't a FeatureFlag enum option. There's only "empty".

The reason I'm fixated on this OpenAPIKit.JSONSchema.CoreContext nullable property is because I'm almost certain I'll need to use it when the translation is actually performed.

Any guidance on how to proceed?

@mattpolzin
Copy link

This is a pretty twisty rabbit hole. I was wrong when I blamed the Swift tooling or Xcode before; the difference was that I was using the JSONDecoder to prove out the parsed result from OpenAPIKit whereas the test case is using the YAMLDecoder. The breakpoint value inspector was right all along, when decoding your snippet via YAML OpenAPIKit throws a few warnings and comes up with a best-guess result that is not what we want.

Unfortunately, this bug comes from the decoder, not OpenAPIKit. Specifically, this bug is tracked here: jpsim/Yams#301. In that bug ticket's description, it mentions that Yams will parse the string value "null" as an actual nil. That is not correct behavior. Specifying that a type is allowed to be null within JSON Schema is done with the string value "null" not the nil value, so OpenAPIKit has no choice but to indicate that it was expecting a string indicating the type of a property but got nil instead.

@czechboy0
Copy link
Contributor

Thanks for the writeup, @mattpolzin - what do you propose we do here? Should OpenAPIKit check for this special case and translate it to .null? Or should we try to detect it in Swift OpenAPI Generator? We already do something similar for Void.

@mattpolzin
Copy link

I'm very conflicted on this one. Unfortunately, the interpretation that Yams gives can also be found in a document that is not valid OpenAPI (type: null) and should be rejected, so we cannot (without using a fixed version of Yams or an alternative YAML parser) work around this bug without also succeeding to parse legitimately invalid documents. Maybe this is sufficiently edge case to be acceptable, but it's a class of workarounds I have always been very against historically.

Without downplaying the massive amount of work that went into Yams and how incredibly grateful I am that it exists, I find myself wishing for a good way for consumers of my work (and more directly this generator and other downstream projects) to use a fixed fork of Yams or even a different project. I don't think that desire is very pragmatic, though, unless this generator project would like to spearhead a movement to a particular fork of that project or even depend on a fork of it temporarily (which may perhaps end up being more indefinite if the bug in question is destined to remain open on the upstream project). The bug has purportedly been fixed by someone else already, but I have not independently verified the fix referenced on that GitHub issue.

@czechboy0
Copy link
Contributor

Maybe add your take to jpsim/Yams#351? Not sure if there's another way to get attention there.

@mattpolzin
Copy link

I commented on the open PR in hopes that JP notices and has time to reply. I didn't suggest the possibility of a fork right-off because that's a proposed solution under only 1 of three possible scenarios and I wanted to ask which scenario we are looking at here ( (a) the open PR could be merged if conflicts were fixed, (b) JP would like a different solution to the problem, or (c) JP does not intend to merge any PRs solving the associated bug now or in the future).

@mattpolzin
Copy link

JP has merged the fix into Yams and plans to cut a release! Best case outcome. Once that release is cut that should unblock work on this ticket, although both OpenAPIKit and this project will need to accept the next major version of Yams first.

@czechboy0 czechboy0 added this to the 1.0 milestone Oct 15, 2023
@czechboy0
Copy link
Contributor

Great! Marking as a blocker for 1.0.

@brandonmaul
Copy link
Author

Thanks for investigating this deeper guys! Sounds like we have a plan for solving this. Really appreciate it! 😊

@czechboy0 czechboy0 added the status/blocked Waiting for another issue. label Oct 26, 2023
@czechboy0
Copy link
Contributor

Marking as blocked by Yams 6.

@czechboy0 czechboy0 added area/generator Affects: plugin, CLI, config file. kind/support Adopter support requests. and removed status/triage Collecting information required to triage the issue. labels Oct 26, 2023
@czechboy0
Copy link
Contributor

Not blocked on us, moving to post 1.0

@czechboy0 czechboy0 modified the milestones: 1.0, Post-1.0 Oct 27, 2023
@erikhric
Copy link

erikhric commented Dec 6, 2023

@czechboy0 I have the opposite error with oneOf usage. Decoding never fails for the first schema it tries.

in openAPI 3.0.0 on entity with oneOf without discriminator.

        content:
          type: object
          properties:
            data:
              oneOf:
                - $ref: '#/components/schemas/VideoPost'
                - $ref: '#/components/schemas/ImagePost'

Generated code tries to decoding in the same order as defined in yaml:

Screenshot 2023-12-06 at 09 13 47
The problem is that decoding VideoPost never fails even when it has non-nullable properties:

    VideoPost:
      type: object
      properties:
        text:
          type: string
          nullable: true
          description: The text content of the post
        video:
          type: object
          nullable: false
          properties:
            data:
              $ref: '#/components/schemas/ProcessedVideo'

and there are more non-nullable fields in refferenced ProcessedVideo.


video property is generated as optional same as all its non-nullable properties. Is it a bug or is there a config option to enforce proper translating of nullables to Optionals?

@czechboy0
Copy link
Contributor

czechboy0 commented Dec 6, 2023

@erikhric You'll need to add the required list to the object, it's a better way to express nullability than the nullable key (which is removed in OpenAPI 3.1.0 anyway, so moving to required is good to do anyway).

I'd rewrite the above as follows, which will work fine with Swift OpenAPI Generator:

VideoPost:
  type: object
  properties:
    text:
      type: string
      description: The text content of the post
    video:
      type: object
      properties:
        data:
          $ref: '#/components/schemas/ProcessedVideo'
      required:
        - data
  required:
    - video

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/generator Affects: plugin, CLI, config file. kind/support Adopter support requests. status/blocked Waiting for another issue.
Projects
None yet
Development

No branches or pull requests

4 participants