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

[BUG][Rust] oneOf with array produces invalid code #17896

Closed
5 of 6 tasks
DDtKey opened this issue Feb 19, 2024 · 1 comment
Closed
5 of 6 tasks

[BUG][Rust] oneOf with array produces invalid code #17896

DDtKey opened this issue Feb 19, 2024 · 1 comment

Comments

@DDtKey
Copy link
Contributor

DDtKey commented Feb 19, 2024

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

It's nice that basic support of oneOf for rust has landed as part of #13970, but there are some issues.
It generates broken code if one of elements is "array", because it uses type-name as variant name for enum without normalization.

It leads to structures like this:

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
#[serde(untagged)]
pub enum OneOfWithArray {
    SomeObject(Box<SomeObject>),
    Vec<crate::models::SomeObject>(Box<Vec<crate::models::SomeObject>>), // invalid rust code
}
openapi-generator version

latest master:

openapi-generator-cli 7.4.0-SNAPSHOT
  commit : 76d743b
OpenAPI declaration file content or url

Can be reproduced with existing modules/openapi-generator/src/test/resources/3_0/oneOfArrayMapImport.yaml

Or another MRE:

{
  "openapi": "3.0.3",
  "info": {
    "title": "Omitted",
    "description": "Omitted",
    "license": {
      "name": ""
    },
    "version": "0.1.0"
  },
  "servers": [
    {
      "url": "http://localhost:8080",
      "description": "Omitted"
    }
  ],
  "paths": {},
  "components": {
    "schemas": {
      "OneOfWithArray": {
        "type": "object",
        "oneOf": [
          {
            "$ref": "#/components/schemas/SomeObject"
          },
          {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/SomeObject"
            }
          }
        ]
      },
      "SomeObject": {
        "type": "object",
        "properties": {
          "a": {
            "type": "integer"
          }
        }
      }
    },
    "responses": {}
  }
}
Generation Details

Just
openapi-generator generate -i test-spec.json -g rust --package-name test-one-of --additional-properties=supportMiddleware=true -o ./test

Steps to reproduce

Just try to generate rust client for provided spec.

Related issues/PRs

#13970 - the behavior was introduced there, but it's initial version of oneOf support.

#17869 - related issue with mention of wrong String import

Suggest a fix

Instead of using type-name as enum variant name, for such cases it could be normalization. I.e replace special chars and paths, for example VecSomeObject instead of Vec<crate::models::SomeObject>(..) or even just SomeObjects. Not sure about complexity though.
And not really related to this case, but if there is a descriminator - its values can be used instead of type names

@DDtKey DDtKey changed the title [BUG][Rust] oneOf [BUG][Rust] oneOf with array produces invalid code Feb 19, 2024
DDtKey added a commit to DDtKey/openapi-generator that referenced this issue Feb 21, 2024
Solves OpenAPITools#17869 and OpenAPITools#17896 and also includes unmerged $17898

Unfortunately it affects quite a lot of code, but we can see that only client-side models were affected by re-generation.
I tried to split this PR to several, but they're really coupled and hard to create a chain of PRs.
@DDtKey
Copy link
Contributor Author

DDtKey commented Feb 22, 2024

#17915 solves this and covers with tests

wing328 pushed a commit that referenced this issue Feb 24, 2024
* fix(rust): discriminator mapping to serde rename

Discriminator mapping has been ignored in some cases.
Even existing samples had wrong definition in some cases

This PR addresses this

* fix(rust): `oneOf` generation for client

Solves #17869 and #17896 and also includes unmerged $17898

Unfortunately it affects quite a lot of code, but we can see that only client-side models were affected by re-generation.
I tried to split this PR to several, but they're really coupled and hard to create a chain of PRs.

* fix: indentation in `impl Default`

* missing fixes

* fix: correct typeDeclaration with unaliased schema

* style: improve indentation for models

* fix: user toModelName for aliases of oneOf

* refactor: unify `getTypeDeclaration` for rust

* cover the case when `mapping` has the same `ref` for different mapping names

* test: add test for previous change

* style: remove extra qualified path to models

* add some comments

* fix(build): use method of `List` instead of specific for `LinkedList`
@DDtKey DDtKey closed this as completed Feb 24, 2024
oscarr-reyes pushed a commit to oscarr-reyes/openapi-generator that referenced this issue Feb 25, 2024
* fix(rust): discriminator mapping to serde rename

Discriminator mapping has been ignored in some cases.
Even existing samples had wrong definition in some cases

This PR addresses this

* fix(rust): `oneOf` generation for client

Solves OpenAPITools#17869 and OpenAPITools#17896 and also includes unmerged $17898

Unfortunately it affects quite a lot of code, but we can see that only client-side models were affected by re-generation.
I tried to split this PR to several, but they're really coupled and hard to create a chain of PRs.

* fix: indentation in `impl Default`

* missing fixes

* fix: correct typeDeclaration with unaliased schema

* style: improve indentation for models

* fix: user toModelName for aliases of oneOf

* refactor: unify `getTypeDeclaration` for rust

* cover the case when `mapping` has the same `ref` for different mapping names

* test: add test for previous change

* style: remove extra qualified path to models

* add some comments

* fix(build): use method of `List` instead of specific for `LinkedList`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant