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

Latest yams results in incomplete generation of types #565

Open
dpasirst opened this issue Apr 18, 2024 · 20 comments
Open

Latest yams results in incomplete generation of types #565

dpasirst opened this issue Apr 18, 2024 · 20 comments
Labels
kind/enhacement Improvements to existing feature.

Comments

@dpasirst
Copy link
Contributor

dpasirst commented Apr 18, 2024

Description

I'm not exactly sure where this bug is arising from (e.g. swift-openapi-generator or one of its dependencies like OpenAPIKit).

In short, using swift-openapi-generator with when the yams version is 5.0.X, the correct swift code (Types) is generated as part of generating types and client. When yams is 5.1.X, the generated code is missing fields. This is with an OpenAPI 3.1 json.

Reproduction

I have attached an example openapi.json along with the difference in the resulting generated swift code. Specifically, in looking at Components.Schemas.NewAccount and Components.Schemas.Account you will see that the one created with yams 5.0.X contains a phoneNumberPayload as part of each. There is also a defined PhoneNumber struct and curiously it is not linked or used instead of phoneNumberPayload but that is perhaps a different improvement. For now, the phoneNumberPayload is usable with yams 5.0.X.

However, when building with yams 5.1.X, the generated NewAccount and Account do not contain phoneNumberPayload. It is dropped but should be there.

I have attached the example openapi.json and the example generated outputs.

openapi.json
generated_types_yams5.0.swift.txt
generated_types_yams5.1.swift.txt

I'm not sure if this is related but Yams 5.1.0 release notes the following breaking change:

Change how empty strings are decoded into nullable properties.
key: "" previously decoded into
struct Value: Codable { let key: String? } as Value(key: nil)
whereas after this change it decodes as Value(key: "").
This could be a breaking change if you were relying on the previous
semantics.

If it is helpful, the openapi.json was generated via Rust using Aide with Axum.

Rust struct definition
#[derive(Default, Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash, JsonSchema)]
pub enum Role {
    #[default]
    #[serde(rename="user")]
    User,
    #[serde(rename="admin")]
    Admin,
}

#[derive(Default, Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash, JsonSchema)]
pub struct PhoneNumber {
    /// country code must contain only a set of digits 0-9
    /// it should be formatted without leading zeros.
    /// for example: U.S.A. should be `1` without `+` and without `00` e.g. not `001`
    /// it should be assumed that the `+` would be added by the system
    #[serde(rename = "countryCode")]
    pub country_code: String,

    /// the number should be a set of digits "333555222"
    /// without parens or dashes. It should not contain the country code
    /// however it should be formatted such that combining a `+` countryCode number
    /// would result in a complete functional international phone number
    #[serde(rename = "number")]
    pub number: String
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash, JsonSchema)]
pub struct NewAccount {

    /// username as A-Za-z0-9
    #[serde(rename = "username")]
    pub username: String,

    /// email address
    #[serde(rename = "email")]
    pub email: Option<String>,

    /// phone number
    #[serde(rename = "phoneNumber")]
    pub phone_number: Option<PhoneNumber>,

    /// The code (string) for referrals
    #[serde(rename = "role")]
    pub role: Role,

}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash, JsonSchema)]
pub struct Account {

    /// internal uuid for the user
    #[serde(rename = "id")]
    pub id: Uuid,

    /// username as A-Za-z0-9
    #[serde(rename = "username")]
    pub username: String,

    /// email address
    #[serde(rename = "email")]
    pub email: Option<String>,

    /// phone number
    #[serde(rename = "phoneNumber")]
    pub phone_number: Option<PhoneNumber>,

    /// The code (string) for referrals
    #[serde(rename = "role")]
    pub role: Role,

}

Package version(s)

OpenAPIKit 3.1.3
swift-algorithms 1.2.0
swift-argument-parser 1.3.1
swift-collections 1.1.0
swift-http-types 1.0.3
swift-numerics 1.0.2
swift-openapi-generator 1.2.1
swift-openapi-runtime 1.4.0
swift-openapi-urlsession 1.0.1
Yams 5.0.6 or 5.1.0 as described resulting in the problem

Expected behavior

The generated types for Account and NewAccount should contain phoneNumberPayload respectively (or even better if it directly contained PhoneNumber) for any version of Yams.

Environment

$ swift -version
swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: x86_64-apple-macosx14.0

Additional information

No response

@dpasirst dpasirst added kind/bug Feature doesn't work as expected. status/triage Collecting information required to triage the issue. labels Apr 18, 2024
@czechboy0
Copy link
Contributor

Hi @dpasirst,

this sounds very similar to #553.

I looked at your doc, and I think the problem is that we're not handling the case of anyOf where one of the cases is just null. In Swift, it's not necessary to have that explicit null, because the fact that phoneNumber already is omitted from required in the Account schema, we'll already treat it as optional.

So in your case, if you edit:

                    "phoneNumber": {
                        "description": "phone number",
                        "anyOf": [
                            {
                                "$ref": "#/components/schemas/PhoneNumber"
                            },
                            {
                                "type": "null"
                            }
                        ]
                    },

to:

                    "phoneNumber": {
                        "description": "phone number",
                        "$ref": "#/components/schemas/PhoneNumber"
                    },

It should start working again. Please let me know if that fixed it 🙂

@dpasirst
Copy link
Contributor Author

Hi @czechboy0 ,
To answer your question, it partially fixes it but not super friendly.

With your suggested change and yams 5.1.X, phoneNumber does indeed appear; however, it is of type OpenAPIValueContainer? and not type PhoneNumber as I would have expected.

In the original example (unmodified) with yams 5.0.X, phoneNumber is of type Components.Schemas.NewAccount.phoneNumberPayload? which is not ideal (would be nicest to just be PhoneNumber directly), but still easier to work with than OpenAPIValueContainer?.

Regards.

@czechboy0
Copy link
Contributor

That's strange, I would expect it to be of the PhoneNumber type. Can you post your updated OpenAPI doc here again please?

@dpasirst
Copy link
Contributor Author

@czechboy0 - as requested - openapi.json with the change you suggested applied in 2 places:
openapi.json

@czechboy0
Copy link
Contributor

@dpasirst Your OpenAPI JSON file seemed malformed, in both places you had:

"phoneNumber": {
    "#/components/schemas/PhoneNumber",
    "description": "phone number"
},

but should be:

"phoneNumber": {
    "$ref": "#/components/schemas/PhoneNumber",
    "description": "phone number"
},

When the generator runs, it emits warning and error diagnostics, can you check if it caught the malformed JSON? It's possible it didn't regenerate, showed an error, so your generated files would still be the old ones.

@dpasirst
Copy link
Contributor Author

@czechboy0 - great catch. I see that was caused by my manual edit to change the json per a few comments back, I seem to have been a little too aggressive in removing the "anyOf"... I was not sure where to look for the generator error diagnostic. With the errored json, the Xcode build screen does not seem to indicate such a diagnostic, instead, it lists "- Diagnostics output path: <none - logs to stderr>".

Indeed, this:

"phoneNumber": {
    "$ref": "#/components/schemas/PhoneNumber",
    "description": "phone number"
},

does appear to work with both yams 5.0.X and 5.1.X.

In this case, the original posted json containing the anyOf is generated from a Rust project using Aide...and as already demonstrated manual editing is error-prone and less than ideal. Is this something that needs to be fixed upstream in that project or here on the client side?

In reviewing, #553 , it seems the Aide generated openapi.json might be valid as it is generating with the "null" as quoted.

@mwildehahn
Copy link

mwildehahn commented Apr 23, 2024

👋 I think I'm running into something similar with these warnings: Schema "null" is not supported, reason: "schema type", skipping

With something like this schema:

      "Cursors": {
        "properties": {
          "next": {
            "anyOf": [
              {
                "type": "string"
              },
              {
                "type": "null"
              }
            ],
            "title": "Next"
          },
          "previous": {
            "anyOf": [
              {
                "type": "string"
              },
              {
                "type": "null"
              }
            ],
            "title": "Previous"
          }
        },
        "type": "object",
        "required": [
          "next",
          "previous"
        ],
        "title": "Cursors"
      },

I understand from reading a couple issues that the recommendation is to not include {"type": "null"} but I don't have direct control over this. This is a side effect of using pydantic + fastapi and dumping the openapi schema. In this specific case, I'm also relying on returning "null" although I could work around it if need be.

@mwildehahn
Copy link

I was able to work around this just by post-processing the schema to handle removing those anyOf types 👍

@dpasirst
Copy link
Contributor Author

for me, I can move forward for now thanks to @czechboy0 , but given the openapi.json is server generated, I'm hoping the fix to properly support the schema will not be too complicated and hopefully can be resolved quickly.

@IanHoar
Copy link

IanHoar commented May 3, 2024

Also having this issue. I'm using the schema directly from a django-ninja backend and it writes nullable types in the anyOf style. I'm getting dozens of warnings like Schema "null" is not supported, reason: "schema type" when generating. I'd like to not have to go trough and rewrite those optionals every time I update the schema.

Update: Forced Yams v5.0.6 in my Package.swift and generation is working now

Update 2: Generation worked, but now the app crashes when trying to parse the values that are defined in that anyOf style instead of the type style:

image_large_url:
  anyOf:
    - type: string
    - type: 'null'

needs to be this ⬇️

image_large_url:
  type: [string, 'null'] 

Update 3: Seems either is correct according to the OpenAPI maintainers: OAI/OpenAPI-Specification#3148

@czechboy0 czechboy0 added kind/enhacement Improvements to existing feature. and removed kind/bug Feature doesn't work as expected. status/triage Collecting information required to triage the issue. labels Oct 29, 2024
@czechboy0 czechboy0 changed the title bug/regression: latest yams results in incomplete generation of types Latest yams results in incomplete generation of types Oct 29, 2024
@atacan
Copy link

atacan commented Nov 3, 2024

I want to work with an API that returns null as value in JSON (without quotes).

So, in their case, when they use type: "null", it's not just the property is missing, so it can be optional nil in Swift, but rather it's null as value. Is it possible to support decoding this as well? or is this another topic?

response schema:

        text:
          type: [string, "null"]

        words:
          type: [array, "null"]
          items:
            $ref: "#/components/schemas/TranscriptWord"

returns

{
  "text": null,
  "words": null
}

update: I think this is the relevant issue: #419

update_2: It's already decoded actually.

@simonjbeaumont
Copy link
Contributor

update_2: It's already decoded actually.

@atacan just confirming: you're not currently blocked by this issue?

@czechboy0
Copy link
Contributor

@atacan Yes that's the same as #419

@atacan
Copy link

atacan commented Nov 4, 2024

@simonjbeaumont Hi, I removed the type: "null" declarations. These nullable properties were in the required list of properties. I also removed them from there. It is now decoded as nil by the Foundation's JSON decoder.

@toliv
Copy link

toliv commented Nov 14, 2024

Hi there, I'm still unclear on whether there is a fix or legitimate workaround for this issue, as it's causing a high amount of frustration not being able to declare/ use any nullable API types in our app.

Here's an example of a very contrived schema / route that returns a nullable string. The generated openapi.json file has the following:

"NullableSchemaResponse": {
        "properties": {
          "aNullableString": {
            "anyOf": [
              {
                "type": "string"
              },
              {
                "type": "null"
              }
            ],
            "title": "Anullablestring"
          },
          "id": {
            "type": "string",
            "title": "Id"
          },
        },
        "type": "object",
        "required": [
          "aNullableString",
          "id",
        ],
        "title": "NullableSchemaResponse"
      },

In this form, the generated Swift schema type completely misses the aNullableString.

/// - Remark: Generated from `#/components/schemas/NullableSchemaResponse`.
        internal struct NullableSchemaResponse: Codable, Hashable, Sendable {
            /// - Remark: Generated from `#/components/schemas/NullableSchemaResponse/id`.
            internal var id: Swift.String
            /// Creates a new `NullableSchemaResponse`.
            ///
            /// - Parameters:
            ///   - id:
            internal init(id: Swift.String) {
                self.id = id
            }
            internal enum CodingKeys: String, CodingKey {
                case id
            }
        }

Trying what was suggested in this thread, to replace the anyOf with the following

"aNullableString": {
            "type": "string" ,
            "title": "Anullablestring"
          },

results in runtime errors when my API actually returns an null response. For clarity, here's the response I get using curl

❯ curl http://localhost:8000/new_nullable
{"aNullableString":null,"id":"123"}%

Apologies if I've missed something in these comments, is there a recommended path or workaround here?

Once again, it's making this borderline unusable if we can't support a nullable field. Our janky solution right now is using array type almost everywhere.

@IanHoar
Copy link

IanHoar commented Nov 14, 2024

We're still having similar issues in our app. We have had to write scripts to convert the yaml before we feed it into the OpenAPI generator which is not ideal. We don't have any control over what Django Ninja outputs so we need to tackle massaging the formatting from the Swift side of things

@toliv
Copy link

toliv commented Nov 14, 2024

We're still having similar issues in our app. We have had to write scripts to convert the yaml before we feed it into the OpenAPI generator which is not ideal.

@IanHoar just out of curiosity, can I ask what you're doing for conversion that is getting around the issue?

@IanHoar
Copy link

IanHoar commented Nov 14, 2024

Here's the script we are using to strip the invalid nullables:

https://gist.github.com/IanHoar/7960d3598c718c831636b1a07e817d39

⚠️ this is not bug free, so use at your own risk. Its working okay in our project, but may not work in every situation

@toliv
Copy link

toliv commented Nov 14, 2024

Here's the script we are using to strip the invalid nullables:

https://gist.github.com/IanHoar/7960d3598c718c831636b1a07e817d39

⚠️ this is not bug free, so use at your own risk. Its working okay in our project, but may not work in every situation

@IanHoar Thank you so much for sharing! Hope we can get some clarity from maintainers soon.

@czechboy0
Copy link
Contributor

@toliv if you remove the property name from the required list, and remove the anyOf wrapping (it doesn't do anything here), you should get the expected String? property on the parent object that works at runtime.

Right now the problem is that the schema is marked as required but null - this is not a combination we support.

Hopefully the workaround will bridge you over until we have a chance to take another look here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhacement Improvements to existing feature.
Projects
None yet
Development

No branches or pull requests

7 participants