Models are now typed as Object when properties of another Model

[jimschubert]

Description

This was asked in chat, and I've done some evaluation to determine what's going on. Models which are properties of other models are now considered Object rather than the property type.

diff --git a/samples/client/petstore/kotlin/docs/Pet.md b/samples/client/petstore/kotlin/docs/Pet.md
index ec7756007..4b25ccb3e 100644
--- a/samples/client/petstore/kotlin/docs/Pet.md
+++ b/samples/client/petstore/kotlin/docs/Pet.md
@@ -5,7 +5,7 @@
 Name | Type | Description | Notes
 ------------ | ------------- | ------------- | -------------
 **id** | **kotlin.Long** |  |  [optional]
-**category** | [**Category**](Category.md) |  |  [optional]
+**category** | [**kotlin.Any**](kotlin.Any.md) | A category for a pet |  [optional]
 **name** | **kotlin.String** |  | 
 **photoUrls** | **kotlin.Array<kotlin.String>** |  | 
 **tags** | [**kotlin.Array<Tag>**](Tag.md) |  |  [optional]
diff --git a/samples/client/petstore/kotlin/src/main/kotlin/org/openapitools/client/models/Pet.kt b/samples/client/petstore/kotlin/src/main/kotlin/org/openapitools/client/models/Pet.kt
index 583dd3fb3..3ca832e7b 100644
--- a/samples/client/petstore/kotlin/src/main/kotlin/org/openapitools/client/models/Pet.kt
+++ b/samples/client/petstore/kotlin/src/main/kotlin/org/openapitools/client/models/Pet.kt
@@ -11,14 +11,13 @@
 */
 package org.openapitools.client.models
 
-import org.openapitools.client.models.Category
 import org.openapitools.client.models.Tag
 
 import com.squareup.moshi.Json
 /**
  * A pet for sale in the pet store
  * @param id 
- * @param category 
+ * @param category A category for a pet
  * @param name 
  * @param photoUrls 
  * @param tags 
@@ -28,7 +27,8 @@ data class Pet (
     val name: kotlin.String,
     val photoUrls: kotlin.Array<kotlin.String>,
     val id: kotlin.Long? = null,
-    val category: Category? = null,
+    /* A category for a pet */
+    val category: kotlin.Any? = null,
     val tags: kotlin.Array<Tag>? = null,
     /* pet status in the store */
     val status: Pet.Status? = null

Apparently, our petstore example for many generated samples only nest models once (Category under Pet#category). This bug appears to happen for any nested models. Nested enums and arrays of types are unaffected.

openapi-generator version

master (752b36e)

OpenAPI declaration file content or url

n/a

Command line used for generation

Any generator under ./bin with nested models

Steps to reproduce

• pull current master branch (752b36e)
• execute mvn clean package install
• execute ./bin/kotlin-client-petstore.sh
• execute cat samples/client/petstore/kotlin/src/main/kotlin/org/openapitools/client/models/Pet.kt

Notice that category is now kotlin.Any? rather than Category as it was previously.

Related issues/PRs

• Introduced via Consider minLength, maxLength and pattern in referenced schema #45

Suggest a fix/enhancement

Nothing substantial to suggest. Maybe copying non-empty properties to the new schema rather than replacing the reference?

---

@jmini you commented in the PR that the fix is temporary. However, I consider the issue a significant one.

It looks like this block is the bug:

+            Schema prop = entry.getValue();
+            if (allDefinitions != null && prop != null && StringUtils.isNotEmpty(prop.get$ref())) {
+                String refName = ModelUtils.getSimpleRef(prop.get$ref());
+                prop = allDefinitions.get(refName);
+            }

I haven't stepped through the code to verify, but it seems to me that this would be falling back to Object where it shouldn't be. I saw that you opened a bug with swagger-parser for the json-schema properties that don't get exposed as expected. I think that parser bug is unrelated to this bug.

For reference, I tracked this down via git bisect using:

• GOOD f22d0cf
• BAD b5a0d17
• RUN https://gist.github.com/jimschubert/38fe5491aa0c25df9d2c8c13d327a1b6

Would you mind giving this a quick look? I had planned to get into a fix, but I wasted a big of time with my evaluation script (running kotlin-client generator, then checking kotlin-server output produced only HEAD as the bad commit).

[jmini]

Sorry for the issue and thank you a lot for the analysis. This probably affects more generators than just Kotlin and need to be fixed.

I wrote in PR #45 that the code was not as nice as it could be, but my intention is to keep the feature.

I will fix it ASAP.

PS: I am sorry you lost so much time with the analysis. I have opened #80 to discuss possible solutions.

[jmini]

I have seen the change, by running the generator before and after my commit...

final String inputSpec = "____/openapi-generator/src/test/resources/2_0/petstore.yaml";

KotlinClientCodegen config = new KotlinClientCodegen();
config.setArtifactId(toArtifactId(inputSpec, config));
final String subFolder = toArtifactId(inputSpec, config);
final String outputDir = ROOT + subFolder;

config.setOutputDir(outputDir);

final OpenAPIParser openApiParser = new OpenAPIParser();
final ParseOptions options = new ParseOptions();
final OpenAPI openAPI = openApiParser.readLocation(inputSpec, null, options).getOpenAPI();

final ClientOptInput opts = new ClientOptInput();
opts.setConfig(config);
opts.setOpenAPI(openAPI);
opts.setOpts(new ClientOpts());
new DefaultGenerator().opts(opts).generate();

---

For the moment I do not understand where the type is changed.
fromModel(..) called on KotlinClientCodegen produce the expected result, as you can see in this test:

    @Test
    public void complexPropertyInPetstoreTest() {
        final OpenAPI openAPI = new OpenAPIParser().readLocation("src/test/resources/2_0/petstore.yaml", null, new ParseOptions()).getOpenAPI();

        Schema schema = ModelUtils.getSchema(openAPI, "Pet");
        final DefaultCodegen codegen = new KotlinClientCodegen();
        final CodegenModel cm = codegen.fromModel("sample", schema);

        Assert.assertEquals(cm.vars.size(), 6);
        Optional<CodegenProperty> findCategoryProperty = cm.vars.stream().filter(v -> "category".equals(v.getName())).findFirst();
        Assert.assertTrue(findCategoryProperty.isPresent());
        CodegenProperty categoryProperty = findCategoryProperty.get();
        Assert.assertEquals(categoryProperty.baseName, "category");
        Assert.assertEquals(categoryProperty.datatype, "Category");
        Assert.assertEquals(categoryProperty.complexType, "Category");
        Assert.assertEquals(categoryProperty.name, "category");
        Assert.assertEquals(categoryProperty.baseType, "Category");
        Assert.assertFalse(categoryProperty.required);
        Assert.assertTrue(categoryProperty.isNotContainer);
    }

(the test as no real value because it is the same check than org.openapitools.codegen.kotlin.KotlinClientCodegenModelTest.complexPropertyTest())

---

I will continue my investigations.

[eriktim]

Thanks for the great analysis, @jimschubert !
I initially ran into this for Elm and could reproduce it for TypeScript-Angular and Kotlin, hence indeed it affects multiple generators.

[jimschubert]

@jmini If you can't track it down, I'll be happy to help when I next have a chance. I enjoy a good bug hunt.

Also, I didn't feel like I lost time sure to my analysis, only in my silly mistake to check the wrong file. Rather that running bisect and finding the source in 10-15 min, it took me around an hour to run, go "That's not right", run again twice with modifications, then find my error and run successfully once more.

[jmini]

I have found why the test and the real run are different:

The call in the test (all most all tests):

        final CodegenModel cm = codegen.fromModel("sample", schema);

Should be:

        final CodegenModel cm = codegen.fromModel("sample", schema, openAPI.getComponents().getSchemas());

Then I have also the problem in the UT.

---

I will bring a solution, but this requires some work to do it correctly. We need to propagate the openAPI instance in multiple locations, so this requires some work.

Do you want me to revert the changes of #45, so that the broken stuff is fixed again?

[jmini]

I have opened #82 to revert the changes introduced by #45 (short term solution)

I am working on the change described in #83 to fix our unit tests and to re-implement #45 in a better way.

[tomplus]

I can confirm that python-* is also affected. Thanks for fixing it.

@@ -35,7 +34,7 @@ class Pet(object):
     """
     openapi_types = {
         'id': 'int',
-        'category': 'Category',
+        'category': 'object',
         'name': 'str',
         'photo_urls': 'list[str]',
         'tags': 'list[Tag]',

[jmini]

Fixed by merge of #82

[jimschubert]

@jmini That's some excellent sleuthing

Looks like we need a way to standardize the inputs, so the fromModel with two parameters is deprecated or removed? I haven't looked recently, but from memory I don't think there's a way to get the allDefinitions when not provided... so it'll probably take a little refactoring.

[jmini]

@jimschubert: I have just pushed #90 as a first step.