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.

Sign up for GitHub

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

[Python] Changes to the templates for test code of python client. #4514

Merged
merged 1 commit into from
Jan 22, 2020
Merged

[Python] Changes to the templates for test code of python client. #4514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
178 changes: 178 additions & 0 deletions ...enapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,184 @@ public String toRegularExpression(String pattern) {
return addRegularExpressionDelimiter(pattern);
}

@Override
public String toExampleValue(Schema schema) {
return toExampleValueRecursive(schema, new ArrayList<String>(), 5);
}

private String toExampleValueRecursive(Schema schema, List<String> included_schemas, int indentation) {
String indentation_string = "";
for (int i=0 ; i< indentation ; i++) indentation_string += " ";
String example = super.toExampleValue(schema);

// correct "true"s into "True"s, since super.toExampleValue uses "toString()" on Java booleans
if (ModelUtils.isBooleanSchema(schema) && null!=example) {
if ("false".equalsIgnoreCase(example)) example = "False";
else example = "True";
}

// correct "&#39;"s into "'"s after toString()
if (ModelUtils.isStringSchema(schema) && schema.getDefault() != null) {
example = (String) schema.getDefault();
}

if (StringUtils.isNotBlank(example) && !"null".equals(example)) {
if (ModelUtils.isStringSchema(schema)) {
example = "'" + example + "'";
}
return example;
}

if (schema.getEnum() != null && !schema.getEnum().isEmpty()) {
// Enum case:
example = schema.getEnum().get(0).toString();
if (ModelUtils.isStringSchema(schema)) {
example = "'" + escapeText(example) + "'";
}
if (null == example)
LOGGER.warn("Empty enum. Cannot built an example!");

return example;
} else if (null != schema.get$ref()) {
// $ref case:
Map<String, Schema> allDefinitions = ModelUtils.getSchemas(this.openAPI);
String ref = ModelUtils.getSimpleRef(schema.get$ref());
if (allDefinitions != null) {
Schema refSchema = allDefinitions.get(ref);
if (null == refSchema) {
return "None";
} else {
String refTitle = refSchema.getTitle();
if (StringUtils.isBlank(refTitle) || "null".equals(refTitle)) {
refSchema.setTitle(ref);
}
if (StringUtils.isNotBlank(schema.getTitle()) && !"null".equals(schema.getTitle())) {
included_schemas.add(schema.getTitle());
}
return toExampleValueRecursive(refSchema, included_schemas, indentation);
}
} else {
LOGGER.warn("allDefinitions not defined in toExampleValue!\n");
}
}
if (ModelUtils.isDateSchema(schema)) {
example = "datetime.datetime.strptime('1975-12-30', '%Y-%m-%d').date()";
return example;
} else if (ModelUtils.isDateTimeSchema(schema)) {
example = "datetime.datetime.strptime('2013-10-20 19:20:30.00', '%Y-%m-%d %H:%M:%S.%f')";
return example;
} else if (ModelUtils.isBinarySchema(schema)) {
example = "bytes(b'blah')";
return example;
} else if (ModelUtils.isByteArraySchema(schema)) {
example = "YQ==";
} else if (ModelUtils.isStringSchema(schema)) {
// a BigDecimal:
if ("Number".equalsIgnoreCase(schema.getFormat())) {return "1";}
if (StringUtils.isNotBlank(schema.getPattern())) return "'a'"; // I cheat here, since it would be too complicated to generate a string from a regexp
int len = 0;
if (null != schema.getMinLength()) len = schema.getMinLength().intValue();
if (len < 1) len = 1;
example = "";
for (int i=0;i<len;i++) example += i;
} else if (ModelUtils.isIntegerSchema(schema)) {
if (schema.getMinimum() != null)
example = schema.getMinimum().toString();
else
example = "56";
} else if (ModelUtils.isNumberSchema(schema)) {
if (schema.getMinimum() != null)
example = schema.getMinimum().toString();
else
example = "1.337";
} else if (ModelUtils.isBooleanSchema(schema)) {
example = "True";
} else if (ModelUtils.isArraySchema(schema)) {
if (StringUtils.isNotBlank(schema.getTitle()) && !"null".equals(schema.getTitle())) {
included_schemas.add(schema.getTitle());
}
ArraySchema arrayschema = (ArraySchema) schema;
example = "[\n" + indentation_string + toExampleValueRecursive(arrayschema.getItems(), included_schemas, indentation+1) + "\n" + indentation_string + "]";
} else if (ModelUtils.isMapSchema(schema)) {
if (StringUtils.isNotBlank(schema.getTitle()) && !"null".equals(schema.getTitle())) {
included_schemas.add(schema.getTitle());
}
Object additionalObject = schema.getAdditionalProperties();
if (additionalObject instanceof Schema) {
Schema additional = (Schema) additionalObject;
String the_key = "'key'";
if (additional.getEnum() != null && !additional.getEnum().isEmpty()) {
the_key = additional.getEnum().get(0).toString();
if (ModelUtils.isStringSchema(additional)) {
the_key = "'" + escapeText(the_key) + "'";
}
}
example = "{\n" + indentation_string + the_key + " : " + toExampleValueRecursive(additional, included_schemas, indentation+1) + "\n" + indentation_string + "}";
} else {
example = "{ }";
}
} else if (ModelUtils.isObjectSchema(schema)) {
if (StringUtils.isBlank(schema.getTitle())) {
example = "None";
return example;
}

// I remove any property that is a discriminator, since it is not well supported by the python generator
String toExclude = null;
if (schema.getDiscriminator()!=null) {
toExclude = schema.getDiscriminator().getPropertyName();
}

example = packageName + ".models." + underscore(schema.getTitle())+"."+schema.getTitle()+"(";

// if required only:
// List<String> reqs = schema.getRequired();

// if required and optionals
List<String> reqs = new ArrayList<String>();
for (Object toAdd : schema.getProperties().keySet())
reqs.add((String)toAdd);

Map<String, Schema> properties = schema.getProperties();
Set<String> propkeys = null;
if (properties != null) propkeys = properties.keySet();
if (toExclude != null && reqs.contains(toExclude)) {
reqs.remove(toExclude);
}
for (String toRemove : included_schemas) {
if (reqs.contains(toRemove)) {
reqs.remove(toRemove);
}
}
if (StringUtils.isNotBlank(schema.getTitle()) && !"null".equals(schema.getTitle())) {
included_schemas.add(schema.getTitle());
}
if (null != schema.getRequired()) for (Object toAdd : schema.getRequired()) {
reqs.add((String)toAdd);
}
if (null!=propkeys) for (String propname : propkeys) {
Schema schema2 = properties.get(propname);
if (reqs.contains(propname)) {
String refTitle = schema2.getTitle();
if (StringUtils.isBlank(refTitle) || "null".equals(refTitle)) {
schema2.setTitle(propname);
}
example += "\n" + indentation_string + underscore(propname) + " = "+
toExampleValueRecursive(schema2, included_schemas, indentation+1)+", ";
}
}
example +=")";
} else {
LOGGER.warn("Type " + schema.getType() + " not handled properly in toExampleValue");
}

if (ModelUtils.isStringSchema(schema)) {
example = "'" + escapeText(example) + "'";
}

return example;
}

@Override
public void setParameterExampleValue(CodegenParameter p) {
String example;
Expand Down
28 changes: 24 additions & 4 deletions modules/openapi-generator/src/main/resources/python/model_test.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
from __future__ import absolute_import

import unittest
import datetime
Copy link
Contributor

@spacether spacether Nov 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Please delete some of the existing model tests and have your template generate new examples. How about replacing the below tests:

  • samples/client/petstore/python/test/test_animal.py (discriminator model)
  • samples/client/petstore/python/test/test_cat.py (composed allof model)
  • samples/client/petstore/python/test/test_enum_class.py (model with enums)
  • samples/client/petstore/python/test/test_format_test.py (normal model with many parameter types)

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 will have no time for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

The request up above will take very little time.
To do it one would run:

rm samples/client/petstore/python/test/test_animal.py
rm samples/client/petstore/python/test/test_cat.py
rm samples/client/petstore/python/test/test_enum_class.py
rm samples/client/petstore/python/test/test_format_test.py
bin/python-petstore.sh.sh
# turn on the test server
docker run -d -e SWAGGER_HOST=http://petstore.swagger.io -e SWAGGER_BASE_PATH=/v2 -p 80:8080 swaggerapi/petstore
# test the updated client
cd samples/client/petstore/python
# run the tests
make test

That should take around a minute max.

These api_test and model_test updates will impact all of the tests that are auto generated when the users run the generator.
This pull request does not yet include any tests of this new functionality.
Tests will need to be added showing that both the new model tests and the api client tests produce code which runs and passes tests.
If these tests were omitted this update could breaking the tests produced in the python client. If this is too much work for this PR the model_test update and the api_test update can be broken up in to different PRs.

I am hearing that you are busy. We can wait until you have time to add this test verification. It is very important that new changes do not break the existing tests, so we can go at whatever speed works for you .

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing that we can do is look at the content of model_test.mustache in other generators and see how they handle instantiating parameters that have model values. Maybe they have some cool tricks that we can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! I tried to remove and re-create all the existing samples/client/petstore/python/test/*, and the hardest tests to manage are exactly the ones with discriminators, since python and python-experimental produce different signatures. For example, for petstore/models/Cat.py:
python: def __init__(self, declawed=None, local_vars_configuration=None):
python-experimental: def __init__(self, class_name, _check_type=True, _from_server=False, _path_to_item=(), _configuration=None, **kwargs):

The funny thing is that class_name (used in the discriminator) is NOT in the old generator, and it is required (it has no default) in the python-experimental generator.

The only solution I see is to develop a model_test.moustache for the old python, and a model_test_experimental.moustache for the python-experimental.

Any suggestions on alternative solutions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I mean when generating the code. For example:

  Tiger:
    type: object
    required:
      - teeth
      - cat
    properties:
      teeth:
        type: string
      cat:
        $ref: '#/definitions/Cat'
  Cat:
    type: object
    required:
      - fur
    properties:
      fur:
        type: string
      animal:
        $ref: '#/definitions/Animal'
  Animal:
    type: object
    required:
      - color
    properties:
      color:
        type: string

What do you do, when generating the make_instance for Tiger? The options are to generate:

            return Tiger(
                teeth = 'many', 
                cat = openapi_client.models.category.Cat(
                fur = 'short',
                animal = openapi_client.models.category.Animal(
                color = 'black and yellor',
            )))

or

            return Tiger(
                teeth = 'many', 
                cat = openapi_client.models.category.Cat(
                fur = 'short',
            ))

Ideally, you would generate the first when include_optional is True and the second then include_optional is False. Anyway, the code for the Cat is generated by toExampleValue, which does NOT know what include_optional is. Thus, internal properties are either generated ALWAYS with all optional parameters, or NEVER with optional parameters.

And now, here is the bomb:

  TagList:
    type: object
    required:
      - name
    properties:
      name:
        type: string
      nextTag:
        $ref: '#/definitions/TagList'

It is a linked list, and it should have an empty nextTag sooner or later. Anyway, if we always comprise optional parameters in toExampleValue, we will have infinite recursion.
So, we should NEVER add optional parameters at toExampleValue level. Do you agree?

Copy link
Contributor

@spacether spacether Jan 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So I see two solutions to these problems.
For the problem of needing to pick whether optional is required or not
Create a string property in CodegenProperty called exampleOptional. In it store the example where include_optional is true. In example store the example where include_optional is false

For the problem of a model with params or items of type model, you can include a list of Schemas or dataTypes that you are inspecting in your toExample function. Every time you call it recursively, add a schema. If we are going to add a schema that we have already added, add it then return, which will end our recursive calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first solution implies changing the data model, and I prefer to leave it to the next developer.

I have implement the second solution.

Copy link
Contributor

@spacether spacether Jan 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am hearing that you don't want to edit the CodegenProperty.java class.
For the first solution we can support deep include_optional=True without changing the data model.
This would work by storing the exampleOptional string value in a property that we are not using in CodegenProperty.
Why not store that value in the parameter.unescapedDescription property and add a comment describing what we are storing in it?
If this is something that you still don't want to do that's fine, just let me know. I just want you to know that there is another viable solution here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I would prefer to have this PR accepted, then do something for the api, then I could come back to this issue - after discussing it with you in a chat.


{{#models}}
{{#model}}
import {{packageName}}
from {{modelPackage}}.{{classFilename}} import {{classname}} # noqa: E501
from {{packageName}}.rest import ApiException


class Test{{classname}}(unittest.TestCase):
"""{{classname}} unit test stubs"""

Expand All @@ -22,11 +22,31 @@ class Test{{classname}}(unittest.TestCase):
def tearDown(self):
pass

def make_instance(self, include_optional):
"""Test {{classname}}
include_option is a boolean, when False only required
params are included, when True both required and
optional params are included """
# model = {{packageName}}.models.{{classFilename}}.{{classname}}() # noqa: E501
if include_optional :
return {{classname}}(
{{#vars}}
{{name}} = {{#example}}{{{.}}}{{/example}}{{^example}}None{{/example}}{{#hasMore}}, {{/hasMore}}
{{/vars}}
)
else :
return {{classname}}(
{{#vars}}
{{#required}}
{{name}} = {{#example}}{{{.}}}{{/example}}{{^example}}None{{/example}},
{{/required}}
{{/vars}}
)

def test{{classname}}(self):
"""Test {{classname}}"""
# FIXME: construct object with mandatory attributes with example values
# model = {{packageName}}.models.{{classFilename}}.{{classname}}() # noqa: E501
pass
inst_req_only = self.make_instance(include_optional=False)
inst_req_and_optional = self.make_instance(include_optional=True)

{{/model}}
{{/models}}
Expand Down
25 changes: 20 additions & 5 deletions samples/client/petstore/python/test/test_animal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@

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

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


from __future__ import absolute_import

import unittest
import datetime

import petstore_api
from petstore_api.models.animal import Animal # noqa: E501
from petstore_api.rest import ApiException


class TestAnimal(unittest.TestCase):
"""Animal unit test stubs"""

Expand All @@ -28,11 +28,26 @@ def setUp(self):
def tearDown(self):
pass

def make_instance(self, include_optional):
"""Test Animal
include_option is a boolean, when False only required
params are included, when True both required and
optional params are included """
# model = petstore_api.models.animal.Animal() # noqa: E501
if include_optional :
return Animal(
class_name = '0',
color = 'red'
)
else :
return Animal(
class_name = '0',
)

def testAnimal(self):
"""Test Animal"""
# FIXME: construct object with mandatory attributes with example values
# model = petstore_api.models.animal.Animal() # noqa: E501
pass
inst_req_only = self.make_instance(include_optional=False)
inst_req_and_optional = self.make_instance(include_optional=True)


if __name__ == '__main__':
Expand Down
23 changes: 18 additions & 5 deletions samples/client/petstore/python/test/test_cat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@

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

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


from __future__ import absolute_import

import unittest
import datetime

import petstore_api
from petstore_api.models.cat import Cat # noqa: E501
from petstore_api.rest import ApiException


class TestCat(unittest.TestCase):
"""Cat unit test stubs"""

Expand All @@ -28,11 +28,24 @@ def setUp(self):
def tearDown(self):
pass

def make_instance(self, include_optional):
"""Test Cat
include_option is a boolean, when False only required
params are included, when True both required and
optional params are included """
# model = petstore_api.models.cat.Cat() # noqa: E501
if include_optional :
return Cat(
declawed = True
)
else :
return Cat(
)

def testCat(self):
"""Test Cat"""
# FIXME: construct object with mandatory attributes with example values
# model = petstore_api.models.cat.Cat() # noqa: E501
pass
inst_req_only = self.make_instance(include_optional=False)
inst_req_and_optional = self.make_instance(include_optional=True)


if __name__ == '__main__':
Expand Down
25 changes: 20 additions & 5 deletions samples/client/petstore/python/test/test_category.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@

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

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


from __future__ import absolute_import

import unittest
import datetime

import petstore_api
from petstore_api.models.category import Category # noqa: E501
from petstore_api.rest import ApiException


class TestCategory(unittest.TestCase):
"""Category unit test stubs"""

Expand All @@ -28,11 +28,26 @@ def setUp(self):
def tearDown(self):
pass

def make_instance(self, include_optional):
"""Test Category
include_option is a boolean, when False only required
params are included, when True both required and
optional params are included """
# model = petstore_api.models.category.Category() # noqa: E501
if include_optional :
return Category(
id = 56,
name = 'default-name'
)
else :
return Category(
name = 'default-name',
)

def testCategory(self):
"""Test Category"""
# FIXME: construct object with mandatory attributes with example values
# model = petstore_api.models.category.Category() # noqa: E501
pass
inst_req_only = self.make_instance(include_optional=False)
inst_req_and_optional = self.make_instance(include_optional=True)


if __name__ == '__main__':
Expand Down
Loading