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][python] Support named arrays #6493

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -421,14 +421,12 @@ public Map<String, Object> postProcessAllModels(Map<String, Object> objs) {

Schema modelSchema = ModelUtils.getSchema(this.openAPI, cm.name);
CodegenProperty modelProperty = fromProperty("value", modelSchema);

if (cm.isEnum || cm.isAlias) {
if (!modelProperty.isEnum && !modelProperty.hasValidation) {
if (!modelProperty.isEnum && !modelProperty.hasValidation && !cm.isArrayModel) {
// remove these models because they are aliases and do not have any enums or validations
modelSchemasToRemove.put(cm.name, modelSchema);
}
} else if (cm.isArrayModel && !modelProperty.isEnum && !modelProperty.hasValidation) {
// remove any ArrayModels which lack validation and enums
modelSchemasToRemove.put(cm.name, modelSchema);
}
}
}
Expand Down Expand Up @@ -829,10 +827,10 @@ public CodegenModel fromModel(String name, Schema schema) {
result.unescapedDescription = simpleModelName(name);

// make non-object type models have one property so we can use it to store enums and validations
if (result.isAlias || result.isEnum) {
if (result.isAlias || result.isEnum || result.isArrayModel) {
Schema modelSchema = ModelUtils.getSchema(this.openAPI, result.name);
CodegenProperty modelProperty = fromProperty("value", modelSchema);
if (modelProperty.isEnum == true || modelProperty.hasValidation == true) {
if (modelProperty.isEnum == true || modelProperty.hasValidation == true || result.isArrayModel) {
// these models are non-object models with enums and/or validations
// add a single property to the model so we can have a way to access validations
result.isAlias = true;
Copy link
Contributor

@spacether spacether Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I have found a sufficient fix in b2ca2f7.

Expand All @@ -847,7 +845,6 @@ public CodegenModel fromModel(String name, Schema schema) {
postProcessModelProperty(result, prop);
}
}

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,12 @@ from {{packageName}}.model_utils import ( # noqa: F401
{{> python-experimental/model_templates/model_simple }}
{{/isAlias}}
{{^isAlias}}
{{#isArrayModel}}
{{> python-experimental/model_templates/model_simple }}
{{/isArrayModel}}
{{^isArrayModel}}
{{> python-experimental/model_templates/model_normal }}
{{/isArrayModel}}
{{/isAlias}}
{{/interfaces}}
{{#interfaces}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ public void arrayModelTest() {
Assert.assertEquals(cm.classname, "sample.Sample");
Assert.assertEquals(cm.classVarName, "sample");
Assert.assertEquals(cm.description, "an array model");
Assert.assertEquals(cm.vars.size(), 0);
Assert.assertEquals(cm.vars.size(), 1); // there is one value for Childer definition
Copy link
Contributor

@spacether spacether Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, understood. That's fine.

Assert.assertEquals(cm.parent, "list");
Assert.assertEquals(cm.imports.size(), 1);
Assert.assertEquals(Sets.intersection(cm.imports, Sets.newHashSet("children.Children")).size(), 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,19 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/HealthCheckResult'
/fake/array-of-enums:
get:
tags:
- fake
summary: Array of Enums
operationId: getArrayOfEnums
responses:
200:
description: Got named array of enums
content:
application/json:
schema:
$ref: '#/components/schemas/ArrayOfEnums'
servers:
- url: 'http://{server}.swagger.io:{port}/v2'
description: petstore server
Expand Down Expand Up @@ -2074,6 +2087,10 @@ components:
properties:
name:
type: string
ArrayOfEnums:
type: array
items:
$ref: '#/components/schemas/OuterEnum'
DateTimeTest:
type: string
default: '2010-01-01T10:10:10.000111+01:00'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ docs/AdditionalPropertiesNumber.md
docs/AdditionalPropertiesObject.md
docs/AdditionalPropertiesString.md
docs/Animal.md
docs/AnimalFarm.md
docs/AnotherFakeApi.md
docs/ApiResponse.md
docs/ArrayOfArrayOfNumberOnly.md
Expand Down Expand Up @@ -93,6 +94,7 @@ petstore_api/model/additional_properties_number.py
petstore_api/model/additional_properties_object.py
petstore_api/model/additional_properties_string.py
petstore_api/model/animal.py
petstore_api/model/animal_farm.py
petstore_api/model/api_response.py
petstore_api/model/array_of_array_of_number_only.py
petstore_api/model/array_of_number_only.py
Expand Down
1 change: 1 addition & 0 deletions samples/client/petstore/python-experimental/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ Class | Method | HTTP request | Description
- [additional_properties_object.AdditionalPropertiesObject](docs/AdditionalPropertiesObject.md)
- [additional_properties_string.AdditionalPropertiesString](docs/AdditionalPropertiesString.md)
- [animal.Animal](docs/Animal.md)
- [animal_farm.AnimalFarm](docs/AnimalFarm.md)
- [api_response.ApiResponse](docs/ApiResponse.md)
- [array_of_array_of_number_only.ArrayOfArrayOfNumberOnly](docs/ArrayOfArrayOfNumberOnly.md)
- [array_of_number_only.ArrayOfNumberOnly](docs/ArrayOfNumberOnly.md)
Expand Down
10 changes: 10 additions & 0 deletions samples/client/petstore/python-experimental/docs/AnimalFarm.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# animal_farm.AnimalFarm

## Properties
Name | Type | Description | Notes
------------ | ------------- | ------------- | -------------
**value** | [**[animal.Animal]**](Animal.md) | |

[[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md)


Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
# coding: utf-8

"""
OpenAPI Petstore

This spec is mainly for testing Petstore server and contains fake endpoints, models. Please do not use this for any other purpose. Special characters: \" \\ # noqa: E501

The version of the OpenAPI document: 1.0.0
Generated by: https://openapi-generator.tech
"""


from __future__ import absolute_import
import re # noqa: F401
import sys # noqa: F401

import six # noqa: F401
import nulltype # noqa: F401

from petstore_api.model_utils import ( # noqa: F401
ApiTypeError,
ModelComposed,
ModelNormal,
ModelSimple,
cached_property,
change_keys_js_to_python,
convert_js_args_to_python_args,
date,
datetime,
file_type,
int,
none_type,
str,
validate_get_composed_info,
)
try:
from petstore_api.model import animal
except ImportError:
animal = sys.modules[
'petstore_api.model.animal']


class AnimalFarm(ModelSimple):
"""NOTE: This class is auto generated by OpenAPI Generator.
Ref: https://openapi-generator.tech

Do not edit the class manually.

Attributes:
allowed_values (dict): The key is the tuple path to the attribute
and the for var_name this is (var_name,). The value is a dict
with a capitalized key describing the allowed value and an allowed
value. These dicts store the allowed enum values.
validations (dict): The key is the tuple path to the attribute
and the for var_name this is (var_name,). The value is a dict
that stores validations for max_length, min_length, max_items,
min_items, exclusive_maximum, inclusive_maximum, exclusive_minimum,
inclusive_minimum, and regex.
additional_properties_type (tuple): A tuple of classes accepted
as additional properties values.
"""

allowed_values = {
}

validations = {
}

additional_properties_type = None

_nullable = False

@cached_property
def openapi_types():
"""
This must be a class method so a model may have properties that are
of type self, this ensures that we don't create a cyclic import

Returns
openapi_types (dict): The key is attribute name
and the value is attribute type.
"""
return {
'value': ([animal.Animal],), # noqa: E501
}

@cached_property
def discriminator():
return None

_composed_schemas = None

required_properties = set([
'_data_store',
'_check_type',
'_spec_property_naming',
'_path_to_item',
'_configuration',
'_visited_composed_classes',
])

@convert_js_args_to_python_args
def __init__(self, value, *args, **kwargs): # noqa: E501
"""animal_farm.AnimalFarm - a model defined in OpenAPI

Args:
value ([animal.Animal]):

Keyword Args:
_check_type (bool): if True, values for parameters in openapi_types
will be type checked and a TypeError will be
raised if the wrong type is input.
Defaults to True
_path_to_item (tuple/list): This is a list of keys or values to
drill down to the model in received_data
when deserializing a response
_spec_property_naming (bool): True if the variable names in the input data
are serialized names, as specified in the OpenAPI document.
False if the variable names in the input data
are pythonic names, e.g. snake case (default)
_configuration (Configuration): the instance to use when
deserializing a file_type parameter.
If passed, type conversion is attempted
If omitted no type conversion is done.
_visited_composed_classes (tuple): This stores a tuple of
classes that we have traveled through so that
if we see that class again we will not use its
discriminator again.
When traveling through a discriminator, the
composed schema that is
is traveled through is added to this set.
For example if Animal has a discriminator
petType and we pass in "Dog", and the class Dog
allOf includes Animal, we move through Animal
once using the discriminator, and pick Dog.
Then in Dog, we will make an instance of the
Animal class but this time we won't travel
through its discriminator because we passed in
_visited_composed_classes = (Animal,)
"""

_check_type = kwargs.pop('_check_type', True)
_spec_property_naming = kwargs.pop('_spec_property_naming', False)
_path_to_item = kwargs.pop('_path_to_item', ())
_configuration = kwargs.pop('_configuration', None)
_visited_composed_classes = kwargs.pop('_visited_composed_classes', ())

if args:
raise ApiTypeError(
"Invalid positional arguments=%s passed to %s. Remove those invalid positional arguments." % (
args,
self.__class__.__name__,
),
path_to_item=_path_to_item,
valid_classes=(self.__class__,),
)

self._data_store = {}
self._check_type = _check_type
self._spec_property_naming = _spec_property_naming
self._path_to_item = _path_to_item
self._configuration = _configuration
self._visited_composed_classes = _visited_composed_classes + (self.__class__,)

self.value = value
for var_name, var_value in six.iteritems(kwargs):
if var_name not in self.attribute_map and \
self._configuration is not None and \
self._configuration.discard_unknown_keys and \
self.additional_properties_type is None:
# discard variable.
continue
setattr(self, var_name, var_value)
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from petstore_api.model.additional_properties_object import AdditionalPropertiesObject
from petstore_api.model.additional_properties_string import AdditionalPropertiesString
from petstore_api.model.animal import Animal
from petstore_api.model.animal_farm import AnimalFarm
from petstore_api.model.api_response import ApiResponse
from petstore_api.model.array_of_array_of_number_only import ArrayOfArrayOfNumberOnly
from petstore_api.model.array_of_number_only import ArrayOfNumberOnly
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# coding: utf-8

"""
OpenAPI Petstore

This spec is mainly for testing Petstore server and contains fake endpoints, models. Please do not use this for any other purpose. Special characters: \" \\ # noqa: E501

The version of the OpenAPI document: 1.0.0
Generated by: https://openapi-generator.tech
"""


from __future__ import absolute_import
import sys
import unittest

import petstore_api
try:
from petstore_api.model import animal
except ImportError:
animal = sys.modules[
'petstore_api.model.animal']
from petstore_api.model.animal_farm import AnimalFarm


class TestAnimalFarm(unittest.TestCase):
"""AnimalFarm unit test stubs"""

def setUp(self):
pass

def tearDown(self):
pass

def testAnimalFarm(self):
"""Test AnimalFarm"""
# FIXME: construct object with mandatory attributes with example values
# model = AnimalFarm() # noqa: E501
pass


if __name__ == '__main__':
unittest.main()
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ README.md
docs/AdditionalPropertiesClass.md
docs/Address.md
docs/Animal.md
docs/AnimalFarm.md
docs/AnotherFakeApi.md
docs/ApiResponse.md
docs/Apple.md
docs/AppleReq.md
docs/ArrayOfArrayOfNumberOnly.md
docs/ArrayOfEnums.md
docs/ArrayOfNumberOnly.md
docs/ArrayTest.md
docs/Banana.md
Expand Down Expand Up @@ -109,10 +111,12 @@ petstore_api/model/__init__.py
petstore_api/model/additional_properties_class.py
petstore_api/model/address.py
petstore_api/model/animal.py
petstore_api/model/animal_farm.py
petstore_api/model/api_response.py
petstore_api/model/apple.py
petstore_api/model/apple_req.py
petstore_api/model/array_of_array_of_number_only.py
petstore_api/model/array_of_enums.py
petstore_api/model/array_of_number_only.py
petstore_api/model/array_test.py
petstore_api/model/banana.py
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ Class | Method | HTTP request | Description
*FakeApi* | [**fake_outer_composite_serialize**](docs/FakeApi.md#fake_outer_composite_serialize) | **POST** /fake/outer/composite |
*FakeApi* | [**fake_outer_number_serialize**](docs/FakeApi.md#fake_outer_number_serialize) | **POST** /fake/outer/number |
*FakeApi* | [**fake_outer_string_serialize**](docs/FakeApi.md#fake_outer_string_serialize) | **POST** /fake/outer/string |
*FakeApi* | [**get_array_of_enums**](docs/FakeApi.md#get_array_of_enums) | **GET** /fake/array-of-enums | Array of Enums
*FakeApi* | [**test_body_with_file_schema**](docs/FakeApi.md#test_body_with_file_schema) | **PUT** /fake/body-with-file-schema |
*FakeApi* | [**test_body_with_query_params**](docs/FakeApi.md#test_body_with_query_params) | **PUT** /fake/body-with-query-params |
*FakeApi* | [**test_client_model**](docs/FakeApi.md#test_client_model) | **PATCH** /fake | To test \&quot;client\&quot; model
Expand Down Expand Up @@ -125,10 +126,12 @@ Class | Method | HTTP request | Description
- [additional_properties_class.AdditionalPropertiesClass](docs/AdditionalPropertiesClass.md)
- [address.Address](docs/Address.md)
- [animal.Animal](docs/Animal.md)
- [animal_farm.AnimalFarm](docs/AnimalFarm.md)
- [api_response.ApiResponse](docs/ApiResponse.md)
- [apple.Apple](docs/Apple.md)
- [apple_req.AppleReq](docs/AppleReq.md)
- [array_of_array_of_number_only.ArrayOfArrayOfNumberOnly](docs/ArrayOfArrayOfNumberOnly.md)
- [array_of_enums.ArrayOfEnums](docs/ArrayOfEnums.md)
- [array_of_number_only.ArrayOfNumberOnly](docs/ArrayOfNumberOnly.md)
- [array_test.ArrayTest](docs/ArrayTest.md)
- [banana.Banana](docs/Banana.md)
Expand Down
Loading