Skip to content

Conversation

jimschubert
Copy link
Contributor

@jimschubert jimschubert commented Jan 14, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Previous implementation didn't support multiple levels of array with
array items as OpenAPI spec supports. This means an object defined as
type: array with items = type: array|items=double (which is common in
GIS) would not be possible.

This implementation assumes generics in the nested type definitions, so
the above would generate List<List<double?>> for model parent types as
well as property type declarations.

This should resolve multiple issues as presented in #7218:

As part of this change, I've also removed the case-sensitive comparison for reserved word evaluation. User has a Break model which was being renamed to ModelBreak. This shouldn't be necessary in C#, especially because we can escape reserved words in C# 3.x+ by preceding the reserved word with an @. I haven't done this in this commit since it seems that all generated examples with reserved words use the title cased representation, but this should be a simple fix if it is required for parameter names.

/cc @mandrean

@wing328 I've opened this PR so @karussell might evaluate all fixes in one. It may be easier to merge this and close the other two.

Previous implementation assumed specification only supports polymorphic
associations (via discrimator), although the code didn't seem to be
setup correctly for that in the first place. That is, the parent object
must define the discriminator (see
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#models-with-polymorphism-support),
so NOT HAS parent AND HAS discriminator doesn't make sense.

From a C# perspective, base classes should have the method marked
virtual and derived classes should override the method. This supports
both composition and polymorphic definitions.
Unprefixed Configuration property access leads to ambiguous references
when spec defines a Configuration model.
Previous implementation didn't support multiple levels of array with
array items as OpenAPI spec supports. This means an object defined as
type: array with items = type: array|items=double (which is common in
GIS) would not be possible.

This implementation assumes generics in the nested type definitions, so
the above would generate List<List<double?>> for model parent types as
well as property type declarations.
@karussell
Copy link
Contributor

I can confirm our library now compiles with this on Linux via mono and also the created tests do pass.

@wing328 wing328 changed the base branch from master to 2.4.0 January 15, 2018 15:53
@wing328 wing328 added this to the v2.4.0 milestone Jan 15, 2018
@theason
Copy link

theason commented Jan 19, 2018

For my Swagger based API I am trying to generate a C# client for, it has a root type for "Configuration", and I noticed that even with this branch you've committed the code still fails to build. Can you review the enclosed json file that I am trying to generate against please?

The checkin you made did a great job of cleaning up where you need to use Client.Configuration, but in ConfigurationApi.cs that get's generated you also need to use Model.Configuration at times to resolve the ambiguity in the other direction.

However, I am left with a ton of "The parameter name "Type" is a duplicate about 116 times in the project.

final.json.txt

?

@theason
Copy link

theason commented Jan 19, 2018

Update, it looks like the json uses a parameter called "Type" over and over...

"parameters": [
{
"in": "query",
"description": "A list of words to search for, separated by spaces. Prefer complete words or the start of words.",
"name": "q",
"type": "string"
},
{
"in": "query",
"description": "A list of item types to search among. If no types are given all item types will be included in the search.",
"name": "type",
"type": "array",
"items": {
"type": "string"
},
"collectionFormat": "multi"
}
],

@jimschubert
Copy link
Contributor Author

@theason I've looked at the swagger definition you shared.

The reason you're getting Type repeated is because your spec has a string of allOf property inclusions where type is getting duplicated.

Here's an example:

SwitchGear is defined as:

    "SwitchGear": {
      "allOf": [
        {
          "required": [
            "type"
          ],
          "properties": {
            "floorMounted": {
              "$ref": "#/definitions/FloorMounted"
            },
            "type": {
              "type": "string"
            }
          },
          "discriminator": "type"
        },
        {
          "$ref": "#/definitions/DataCenterEquipment"
        }
      ]
    },

The allOf includes one schema defining name=type, type=string, then continues to reference DataCenterEquipment, defined as:

"DataCenterEquipment": {
      "allOf": [
        {
          "required": [
            "type"
          ],
          "properties": {
            "modelName": {
              "type": "string"
            },
            "serialNumber": {
              "type": "string"
            },
            "partNumberId": {
              "type": "integer",
              "format": "int32"
            },
            "weight": {
              "type": "number",
              "format": "double"
            },
            "description": {
              "type": "string"
            },
            "partNumber": {
              "type": "string"
            },
            "type": {
              "type": "string"
            },
            "barcode": {
              "type": "string"
            },
            "manufacturer": {
              "type": "string"
            },
            "height": {
              "type": "number",
              "format": "double"
            }
          },
          "discriminator": "type"
        },
        {
          "$ref": "#/definitions/dataCenterItem"
        }
      ]
    }

This also contains a property defined as name=type, type=string.

discriminator is defined in Open API 2.0 as:

Adds support for polymorphism. The discriminator is the schema property name that is used to differentiate between other schema that inherit this schema. The property name used MUST be defined at this schema and it MUST be in the required property list. When used, the value MUST be the name of this schema or any schema that inherits it.

discriminator is used to basically create a base type with a named type that identifies subtypes. I've never seen it used in multiple allOf schemas, possibly because I mainly support C# which doesn't support multiple inheritance.

To check, I've also compiled the Java generator. It doesn't seem to have a similar issue with compilation because there are no constructors enforcing the required arguments. It also doesn't appear to me that it handles the type property issue as would be expected because it results in a type property in the super type and another in the sub type which shadows the super type's property. In C#, there are additional constraints for shadowing which would require us to use override or new explicitly.

What are your expectations for when your model defines type in two different models, both as discriminators? Since I don't know what the correct behavior should be, and your spec doesn't match how I've seen discriminators done elsewhere, you may want to open a new issue for this.

To address your issue with Configuration conflicting, you could either modify the template and change the Configuration property in api class to something else, or you could use --model-name-suffix=Model to append Model to all model names. You could also use a using alias on the imported Configuration model and refer to your model by alias:

using ConfigurationModel = MyApi.Model.Configuration

@theason
Copy link

theason commented Jan 20, 2018

Thank you so much for reviewing my problem! Unfortunately, this is a REST API that I am trying to consume from a 3rd party product I have no control over. I had a hunch that their system didn't quite conform to the OpenAPI standard. I won't call them out by name here; suffice to say they are a multibillion dollar organization that I have almost no sway over (but if you search the json for a URL you'll find it).

It would seem that auto-generating a client for this API isn't a possibility given the disconnected decisions they made.

@wing328 wing328 changed the base branch from 2.4.0 to master January 22, 2018 02:53
@wing328 wing328 merged commit 8724719 into swagger-api:master Jan 22, 2018
@karussell
Copy link
Contributor

@wing328 will it go into the next release 2.4.0 or only 3.0.0?

@karussell
Copy link
Contributor

Ups, I can now see the milestone v2.4.0, please ignore.

jimschubert added a commit to jimschubert/swagger-codegen that referenced this pull request Jan 22, 2018
* master: (133 commits)
  add a link to ebook (polish version)
  Add R namespace file (swagger-api#7467)
  Add Spring Petstore samples (async, java8-localdatetime) to CircleCI (swagger-api#7468)
  Revised core team members
  Ada code generator corrected: "=>" instead of "->". Fixes swagger-api#7450 (swagger-api#7456)
  Fixes issue swagger-api#7177 (SpringCodeGen dateLibrary "java8-localdatetime" option is ignored). (swagger-api#7178)
  Issue-7438 Fix that prevents generating interfaces when interfaceOnly is false. (swagger-api#7439)
  swagger-api#7093 - Add maven wrapper (swagger-api#7356)
  [csharp] Support arrays of arrays for properties and models (swagger-api#7400)
  [csharp] Fix ToJson to work with composition and polymorphism (swagger-api#7399)
  [csharp] Reference this.Configuration in client api template (swagger-api#7394)
  [JAX-RS][Spec] Removes throws Exception. (swagger-api#7437)
  Use supportsES6 flag in ts compilation for language typescript-angular (swagger-api#7408)
  Fix 7457: [Ada] wrong order for generated structures in *-models.ads (swagger-api#7462)
  Fix 7459: [Ada] wrong JSON in POST operations (swagger-api#7460)
  [erlang-client] Erlang request utils (swagger-api#7257)
  reenable pushing snapshot to maven repo
  Create CODE_OF_CONDUCT.md
  comment out update to docker image
  deleted unnecessary notes (swagger-api#7454)
  ...
viclovsky pushed a commit to viclovsky/swagger-codegen that referenced this pull request Jan 23, 2018
…api#7400)

* [csharp] Support composition on toJson

Previous implementation assumed specification only supports polymorphic
associations (via discrimator), although the code didn't seem to be
setup correctly for that in the first place. That is, the parent object
must define the discriminator (see
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#models-with-polymorphism-support),
so NOT HAS parent AND HAS discriminator doesn't make sense.

From a C# perspective, base classes should have the method marked
virtual and derived classes should override the method. This supports
both composition and polymorphic definitions.

* [csharp] this.Configuration in api template

Unprefixed Configuration property access leads to ambiguous references
when spec defines a Configuration model.

* [csharp] Models/properties support nested arrays

Previous implementation didn't support multiple levels of array with
array items as OpenAPI spec supports. This means an object defined as
type: array with items = type: array|items=double (which is common in
GIS) would not be possible.

This implementation assumes generics in the nested type definitions, so
the above would generate List<List<double?>> for model parent types as
well as property type declarations.

* [csharp] Regenerate integration test sample

* [csharp] Set "Client" case sensitive as reserved

* [csharp] Regenerate security sample

* [csharp] Regenerate samples
@jimschubert jimschubert deleted the csharp-multi-7218 branch February 3, 2018 20:17
@mikeclemson
Copy link

I'm consuming JSON from a third party, and I receive the same error for Configuration and for OperatingSystem (C# and PowerShell clients). By using this PR and making new builds, I fix the Configuration issue, but the OperatingSystem ambiguous reference remains. Should this be part of this issue, a new issue, or is there another way to solve it?

@jimschubert
Copy link
Contributor Author

I think that issue is related to a current feature request for fully qualified models. The issue in this pr fix was a conflict with a property in the API class. OperatingSystem would be a conflict with imports, so just a little different.

You could workaround this by passing --model-name-prefix=Model or --model-name-suffix=Model. Check the CLI help to verify these are the correct switches for your version of swagger codegen.

You could also look into extending the templates. I think OperatingSystem is under System, and that may be difficult to work around without fully qualified model names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants