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] generate model if type != object if enums/validations exist #2757

Conversation

spacether
Copy link
Contributor

@spacether spacether commented Apr 28, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied 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)

Description of the PR

This PR makes it so models whose type != "object" are generated in python if the model has validations or enums. Those models are then used for serialization and deserialization when communicating with an API.

Why is this necessary?

  • If we do not add this generation of non-object model types, when deserializing a response where type="str" with regex validations and enums invalid server values can be be ingested and used by the client. Our current python client does not store any of the validation or enum info. It just stores the response as type string. From now on, it will store the response type as the model class name. That model class will store and the validations and enums and use them when serializing + deserializing.
  • The purpose of this diff is to work towards completion of the "additionalProperties support, Option2" feature in the python client [Python] add additionalProperties support, Option2 #2049
    • This PR lays the ground work for a future PR where types in python can be described with classes rather than a string description of a class.

Description of Updates:

  • models whose type != object with validations and/or enums are now generated in the python client
  • an Endpoint class has been created in the api_client to store api parameters inside each endpoint in a way that can be accessed by a user.
  • the interface to access enums and validations has been made the same for models and endpoints
    • enums can be accessed by
      • api_name.endpoint_name.allowable_values[('var_name',)] = {'ENUM_VAL': 'enum_val'}
      • model_class.allowable_values[('var_name',)] = {'ENUM_VAL': 'enum_val'}
    • validations can be accessed by
      • api_name.endpoint_name.validations[('var_name',)] = {'inclusive_maximum': 100}

      • model_class.validations[('var_name',)] = {'inclusive_maximum': 100}

  • there are now 2 types of python models:
    • ModeSimple based models which are ones where type != "object" which have validations and/or enums
    • ModelNormal based models which are ones where type == "object"

Note: merging this PR will close this issue: #1991

@spacether spacether changed the title Issue 1991 folds non object models into models Python generate models if type != object if enums and/or validations exist Apr 28, 2019
@spacether spacether changed the title Python generate models if type != object if enums and/or validations exist Python generate model if type != object if enums and/or validations exist Apr 28, 2019
@spacether spacether changed the title Python generate model if type != object if enums and/or validations exist Python client generate model if type != object if enums and/or validations exist Apr 28, 2019
@spacether spacether force-pushed the issue_1991_folds_non_object_models_into_models branch from 5ad7737 to 8541d2f Compare April 28, 2019 22:03
@spacether
Copy link
Contributor Author

spacether commented May 7, 2019

All tests pass, ready for final review @wing328
@ivan-gomes @mverderese once this PR is approved there are only two more PRs to go to finish the AdditionalProperties, Option 2 #2049
Those diffs should take a lot less time because I have already written that code.

@spacether spacether force-pushed the issue_1991_folds_non_object_models_into_models branch from 6052c0e to fdfc6dd Compare May 7, 2019 16:31
@wing328
Copy link
Member

wing328 commented May 9, 2019

@spacether thanks for the PR. Let me review later today

@spacether spacether force-pushed the issue_1991_folds_non_object_models_into_models branch from aa6ae98 to 49e4124 Compare May 9, 2019 17:04
@spacether
Copy link
Contributor Author

The travis ci failure is because the Cat and Dog models somehow invoked two templates to generate instances of ModelNormal. I will look into this. This is a new problem and was not happening before my 49e4124 rebase.

@spacether
Copy link
Contributor Author

@wing328 I fixed the bug. All tests now pass. This PR is ready for review.

@spacether
Copy link
Contributor Author

Hi @wing328
I know that you have been very busy with a new release of openapi-generator.
When you have a chance can you review this?
Tests have been green for a week and I can't move forward until this PR is merged.
Completely understand if you are swamped though. Thank you!

@wing328
Copy link
Member

wing328 commented May 17, 2019

@spacether Yup, very busy as usual. Thanks again for the PR and keeping it up-to-date. I'll review over the weekend and let you know if I've any question.

@wing328
Copy link
Member

wing328 commented May 17, 2019

cc @OpenAPITools/generator-core-team since the change covers default codegen as well.

Copy link
Contributor

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

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

I'm not familiar with Python but reviewed the changes on DefaultCodegen.

@spacether spacether force-pushed the issue_1991_folds_non_object_models_into_models branch from c3d6bd2 to c427de6 Compare May 19, 2019 21:51
@wing328 wing328 added this to the 4.0.1 milestone May 20, 2019
@spacether
Copy link
Contributor Author

I still need to tweak the python generator so it handles the below models better:
AnimalFarm (stop generating this if the child parameter does not have enums or validations)
StringBooleanMap (what type is this when the spec omits setting the type?)

@wing328
Copy link
Member

wing328 commented May 21, 2019

    StringBooleanMap:
      additionalProperties:
        type: boolean

If generateAliasAsModel is enabled, this becomes a model extending dictionary/map, e.g.

public class StringBooleanMap extends HashMap<String, Boolean>  {

Ref: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/server/petstore/jaxrs/jersey2/src/gen/java/org/openapitools/model/StringBooleanMap.java#L26

If generateAliasAsModel is disabled, this simply becomes an alias to a map of boolean.

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#model-with-mapdictionary-properties

type: object (by default) is omitted.

The above explanation applies to AnimalFarm as well:

    AnimalFarm:
      type: array
      items:
        $ref: '#/components/schemas/Animal'

If you have any questions about "alias", please let me know and I'll try my best to explain.

@spacether
Copy link
Contributor Author

Thanks for that explanation. I think that StringBooleanMap is being generated correctly in python.

@spacether
Copy link
Contributor Author

spacether commented May 23, 2019

Using the command line argument to stop generation of alias models requires removing files from 29 generator sample folders. I am working through them.

@wing328
Copy link
Member

wing328 commented May 23, 2019

@spacether you mean this change will generate additional files and require the switch (generateAliasAsModel) to stop those files from being generated?

@spacether
Copy link
Contributor Author

spacether commented May 23, 2019

@wing328 nope. So far this is file removal.
most generators had previously generated alias models docs and tests. My update removes the models and no longer generates the docs and tests so I need to delete the docs and tests manually.
On a separate note, it looks like Ruby and some Java generators are generating alias models for the first time. I will investigate them.
Update: I fixed it so Ruby and others aren't incorrectly generating those alias models in commit 8f12836

@spacether spacether force-pushed the issue_1991_folds_non_object_models_into_models branch 2 times, most recently from 8f12836 to 03bd6ed Compare May 26, 2019 21:00
@spacether
Copy link
Contributor Author

spacether commented May 27, 2019

@wing328 all better and ready for reviews.
All tests now pass.
The Python generator now sets the existing generate alias as models to true and only generates the alias models if they have enums or validations.

@spacether spacether force-pushed the issue_1991_folds_non_object_models_into_models branch from 7bbd5aa to f886098 Compare September 20, 2019 17:07
protected void handleMethodResponse(Operation operation,
Map<String, Schema> schemas,
CodegenOperation op,
ApiResponse methodResponse) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this code was moved and not changed in any way

Copy link
Member

Choose a reason for hiding this comment

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

Yup, noticed that too.

if (modelTemplate != null && modelTemplate.containsKey("model")) {
CodegenModel m = (CodegenModel) modelTemplate.get("model");
if (m.isAlias) {
if (m.isAlias && !ModelUtils.isGenerateAliasAsModel()) {
Copy link
Member

Choose a reason for hiding this comment

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

@spacether without && !ModelUtils.isGenerateAliasAsModel() it won't work in your case right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right

Copy link
Member

Choose a reason for hiding this comment

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

FYI. I've filed #3951 as && !ModelUtils.isGenerateAliasAsModel() impacts all other generators as well.

@spacether spacether force-pushed the issue_1991_folds_non_object_models_into_models branch from f886098 to 762bc06 Compare September 20, 2019 17:21
@spacether
Copy link
Contributor Author

@wing328 all feedback has been addressed. This PR is ready for another review.

@wing328
Copy link
Member

wing328 commented Sep 24, 2019

Tested the generateAliasAsModel option and worked as expected.

@wing328 wing328 merged commit 252c3e5 into OpenAPITools:master Sep 24, 2019
@spacether spacether deleted the issue_1991_folds_non_object_models_into_models branch September 24, 2019 18:33
jimschubert added a commit that referenced this pull request Sep 30, 2019
* master: (207 commits)
  Add missing enum processing in C++ codegen, already present for Qt5 (#3986)
  [C++] [Pistache] Removed deprecated warnings (#3985)
  [C++][Pistache] Simplified model template (#3417)
  add go oas3 petstore to ensure up-to-date (#3979)
  replace gitter with slack in the doc (#3977)
  Fix wrong variable name in LessThan and LessThanOrEqual asserts (#3971)
  #3957 - Removed hardcoded baseUrl (#3964)
  Regenerate go openapi3 samples (#3975)
  [rust] Make it easier to test rust client generator (#3543)
  Fix issue3635 (#3948)
  add gradle repository (#3867)
  [java] allow to use setArtifactVersion() programmatically (#3907)
  Add a link to DevRelCon SF 2019 (#3961)
  Add a link to a medium blog post (#3960)
  update maven-compiler-plugin version (#3956)
  fix generateAliasAsModels in default generator (#3951)
  Implement BigDecimal to Decimal in swift4 for currency data as type=string format=number (#3910)
  Add F# Functions server generator (#3933)
  [python-experimental] generate model if type != object if enums/validations exist (#2757)
  [scala] add [date-time] field to codegen unit test (#3939)
  ...
Jesse0Michael pushed a commit to Jesse0Michael/openapi-generator that referenced this pull request Oct 3, 2019
…ations exist (OpenAPITools#2757)

* Python-experimental adds model_utils module, refactors python api class

* Fixes python-experimental so the sample sare generated in the petstore_api folder

* FIxes python samples tests

* Updates python and python-experimental tests

* Fixes python-experimental tests

* Adds newlines back to python templates + samples

* Reverts files with newline tweaks back to master branch versions

* Fixes indentation errors in python-experimental api_client

* Removes unused files

* Python files now generated in correct folders

* Adds logging when the user tries to set generateAliasAsModel in python-experimental

* Fixes typo
@hleb-albau
Copy link

wow, why not to make this behavior tweakable?

Also, how client users should import enums right now?
using api_name.endpoint_name.allowable_values[('var_name',)] seems too agnly

@spacether
Copy link
Contributor Author

spacether commented Jul 27, 2020

What way do you want it to be tweakable?
Producing classes for these non object non array types that include validation is required for our composed schemas.
If we have:

StringWithValidation:
  - type: string
  - validation: some regex here
ComposedSchema:
  oneOf
  - StringWithValidation

Then we need to produce the StringWithValidation model to contain the StringWithValidation validation logic.

For now, about your enums question, yes using api_name.endpoint_name.allowable_values[('var_name',)] is the correct way to do it at this time.
We have a ticket to improve enum handling in python-experimental at #7058

@hleb-albau
Copy link

I just want to mention enums part of PR. Also, right now enum client serialization is broken and produce {"value":"x"} json object, instead of raw "x".

Also, in some cases you want to have behavior described there #6828 (as does current generator). Can't find property, how to disable enums value check while initializing class during de-serialization on python-experimental

@spacether
Copy link
Contributor Author

spacether commented Jul 27, 2020

Enum serialization issue

Hi, can you create a new issue describing how enums are not working with a sample spec?
We have an endpoint test here showing that when we send an enum we send the raw value.

Enum checking allowed values

Right now there is no way to disable checking allowed values per:


Why would you want to disable it? What if the server sends an invalid value because your client spec document is out of date?

Enum Constants

Also, in some cases you want to have behavior described there #6828 (as does current generator). Can't find property, how to disable enums value check while initializing class during de-serialization on python-experimental

Would your needs for a static enum be met with Enum classes like:

class StatusValue(str, enum.Enum):
  APPROVED = "approved"
  IN_REVIEW = "in-review"

where the value that you send or receive would be StatusValue.APPROVED and would serialize to a string

@hleb-albau
Copy link

where the value that you send or receive would be StatusValue.APPROVED and would serialize to a string

I think my problem do not related to enums directly, but it is lack of static constants description in openapi. Thus, if I have integer field, that have set of 100 predefined values, i want to have:

  • defined values with description in my client
  • in models use raw integer, to have ability to de-serialize response even with new value types, that I am not depend on.

currently, for java client, i use 2 cycle generation. on first run, I produce from modified enum template file with constants. On a second run I use type-mappings to replaces openapi enum model with primitive values.

btw, will try to provide full same of wrong enum serialization tomorrow

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.

4 participants