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

[Python-experimental] JSON schema 'null' type should be modeled as 'none_type' #6121

Merged
merged 61 commits into from
May 13, 2020

Conversation

sebastien-rosset
Copy link
Contributor

@sebastien-rosset sebastien-rosset commented Apr 30, 2020

It looks like recently something changed because the JSON schema null type is now causing problems. I'm not sure what specific commit caused this problem. The getTypeString function in PythonClientExperimentalCodegen.java is incorrectly returning null when the input Schema is the Null type.

For example, with the following schema, the generated python-experimental code has syntax errors.

Foo:
  oneOf:
    - type: 'null'
    - '#/components/schemas/Bar'

Without this PR, the following import is incorrectly generated:

try:
    from openapi_client.models import null
except ImportError:
    null = sys.modules[
        'openapi_client.models.null']

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.

@sebastien-rosset
Copy link
Contributor Author

@spacether , I'm not sure if I should handle the OAS null type in the getTypeString function (as proposed in this PR), or by adding the "null" type in the typeMapping field.

@sebastien-rosset sebastien-rosset marked this pull request as draft April 30, 2020 20:28
@spacether
Copy link
Contributor

I think that we need to add the type mapping but I have not used that before. For Python-experimental we should point it to our already defined none_type.

@sebastien-rosset
Copy link
Contributor Author

I think that we need to add the type mapping but I have not used that before. For Python-experimental we should point it to our already defined none_type.

ok, sure, let me try with typeMappings.

@sebastien-rosset sebastien-rosset marked this pull request as ready for review April 30, 2020 22:09
Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Please add a python test showing deserializing a None working correctly.

@sebastien-rosset
Copy link
Contributor Author

Please add a python test showing deserializing a None working correctly.

Added unit test

@sebastien-rosset sebastien-rosset changed the title [Python-experimental] JSON schema 'null' type should be modeled as 'none_type' [Python-experimental][go-experimental] JSON schema 'null' type should be modeled as 'none_type' May 2, 2020
Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

These python-experimental updates look good. Thank you for this PR.

@spacether
Copy link
Contributor

spacether commented May 7, 2020

@sebastien-rosset we need the CI tests to pass before we can merge this.
Travis says:

E     File "/home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/python-experimental/petstore_api/model_utils.py", line 137
E       oneof_anyof_classes = cls._composed_schemas.get('oneOf', ()) +
E                                                                    ^
E   SyntaxError: invalid syntax

It might need parens around that statement because it is multiline, like:

oneof_anyof_classes = (first_part +
  second_part)

Can you fix it?

@spacether
Copy link
Contributor

spacether commented May 8, 2020

@sebastien-rosset to fix this next CI-error you need to update this line:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/python/python-experimental/model_templates/model_simple.mustache#L14
to:

_composed_schemas = {}

or change the oneof_anyof_classes assignment in __new__ to only assign a value if bool(cls._composed_schemas) == True which is equivalent to:

oneof_anyof_classes = ()
if cls._composed_schemas:
  oneof_anyof_classes = (cls._composed_schemas.get('oneOf', ()) +
                                cls._composed_schemas.get('anyOf', ()))

@sebastien-rosset
Copy link
Contributor Author

@sebastien-rosset to fix this next CI-error you need to update this line:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/python/python-experimental/model_templates/model_simple.mustache#L14
to:

_composed_schemas = {}

or change the oneof_anyof_classes assignment in __new__ to only assign a value if bool(cls._composed_schemas) == True which is equivalent to:

oneof_anyof_classes = ()
if cls._composed_schemas:
  oneof_anyof_classes = (cls._composed_schemas.get('oneOf', ()) +
                                cls._composed_schemas.get('anyOf', ()))

I have some something like that here: https://github.com/OpenAPITools/openapi-generator/pull/5809/files#diff-cc72ac40056827e3a103f9b4f56059cfR70

I mentioned #5809 should be merged before this PR.

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. The python-experimental updates look good to me.
Waiting for tests to pass.

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.

3 participants