[BUG][python] Support named arrays

[jirikuncar]

The model for named array is not generated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @arun-nalla (2019/11) @spacether (2019/11)

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[spacether]

My investigation shows that this line:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientExperimentalCodegen.java#L420
Is preventing these models from being generated
because the array model does not have any validations or enums.
A couple of solutions that come to mind here are:

1. In the java layer define the Python data_type in this case as [outer_enum.OuterEnum]. This is probably the simpler solution to get working. This solution would not generate the array_of_enums.ArrayOfEnums model file.
2. uncomment the suppression of these models and ensure that these models are generated correctly. When I do this is see that the openapi_types data in the produced models is incorrect. Also the produced models are subclases of the ModelNormal class when they should be subclasses of the ModelSimple class

ModelSimple is the correct base class to use for option 2 because ModelSimple midels are used to represent models with one value and no named property. So ints, arrays, strings etc.

What do you think about each of these possible solutions?

[jirikuncar]

I have added FarmApi with /farm/animals endpoint returning array of Animals.

[jirikuncar]

@spacether what is the intended use for Model(Simple|Normal) with lists?

[jirikuncar]

@spacether it should all be fixed now. I have run the sample generator locally but it is failing on CI. I don't know why 😞

[jimschubert]

@spacether looks like all your comments have been addressed here, is this ok to merge?
We can fix merge conflicts and regenerate via separate PR if you have no further concerns.

[spacether]

I will need a couple of days to dig in to this. It looks like the PR broke data types of unrelated samples so no I don't think that we should merge yet.

[jirikuncar]

PR broke data types of unrelated samples

@spacether can you be more specific? The changes for additionalProperties seem to come from master. I will try to regenerate samples again.

[spacether]

The changes that I mentioned are the additionalProperties ones. If they came from master wouldn't rebasing on master make them disappear?

[spacether]

Array models are making a model that aliases the array type, so I think that this should be true.
Long term my goal is to have the ModelSimple classes inherrit from their base classes like str int etc.
Why do we need to set this to False for ArrayModels? Historically generateAliasAsModel applied only to Map and Array models in most generators. In python-experimental we apply it to those AND all primitives that they have validations and enums. Standard processing in the generators was to describe all these primitive models as their simple types like str, list. But if those models had validations and that data was lost in that process. So I added code to preserve that information and make object type models which preserve the validation information which would have been lost. That code hoists the model information into a required value variable.

After I did that the CodegenModel class was updated to include validation information which previously had been missing. So in summary this code here is an older hack to preserve information. If we can use the existing ArrayModels as is that is preferable. It should define its datatype somewhere inside the CodegenModel and that should be used to set the ModelSimple data type in the python class for our made up value parameter.

We can do this by doing a check of the model type when we assign the openapi_types in ModelSimple.
We can check the data that is available by running the generator with a debugmodels flag.
In my comment here I explain how to do that.

[jirikuncar]

I am not aware of all the corner cases in the code. I was trying to make it work for my use-case and not break existing tests. Feel free to propose code changes.

[jirikuncar]

I hope I have found a sufficient fix in b2ca2f7.

[spacether]

What is the dataType in this CodegenModel.dataType?
This model having vars of length 1 shows that my older hack is being used and that we have hoisted model information into a new value variable.
How about we keep the CodegenModel as-is (no hoisting) and use the information from one of these sources?

• CodegenModel.dataType
• CodegenModel.arrayModelType
  We can see all of the properties that are available by looking at that class here

We can debug the CodegenModels and see the information in them by running the generate command with the -DdebugModels=true as shown here

How about in the classvars template here
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/python/python-experimental/model_templates/classvars.mustache#L115
Doing a check to see if the model is an arrayModel
and setting the datatype accordingly?
It would look like:

{{#isArrayModel}}
            'value': ({{{dataType}}},),  # noqa: E501
{{/isArrayModel}}
{{^isArrayModel}}
{{#requiredVars}}
            '{{name}}': ({{{dataType}}},),  # noqa: E501
{{/requiredVars}}
{{#optionalVars}}
            '{{name}}': ({{{dataType}}},),  # noqa: E501
{{/optionalVars}}
{{/isArrayModel}}

We would also need to do a similar change in
method_init_shared
To:

• not loop over required an optional args if we have an array model, if we are that case, just set value as a required positional arg
• else use our existing looping over variables
• in the docstring if we are an arraymodel set value as a required positional arg

Our model doc mustache template would need a similar change to grab the data from dataType rather than vars.

@jirikuncar
What do you think about making these changes?
If you don't want to make them, we can keep using the existing code/(my older solution) where we shovel the model data into a value variable.

[jirikuncar]

I would prefer to keep the current code. It looks like too many changes and {{#isArrayModel}} guards are required for templates.

[spacether]

Ah, understood. That's fine.

[jirikuncar]

The changes that I mentioned are the additionalProperties ones. If they came from master wouldn't rebasing on master make them disappear?

It should but there is some problem with the sample generation. I will continue updating my branch until it resolves.

[jirikuncar]

@jimschubert I have run the ./bin/openapi3/python-experimental-petstore.sh and also tried ./bin/utils/ensure-up-to-date --batch but the generated code for additional properties is still there.

[jirikuncar]

@spacether can we go with this solution for a time being?
@jimschubert the CI is passing 🎉

[spacether]

Thank you for your hard work on this. This looks great!