[PYTHON] Setting default value for Required variables

[the-akhil-nair]

Problem:
In the following PR:
#8802

The requiredVars have been moved out of the composed schema __init__ and _from_openapi_data signature because the required vars were probably defined in the composed oneOf/anyOf/allOf schemas and this schema does not know about them.
We understand that each schema should be validated separately and required vars of a schema should not be present in composed schema, but while doing the validation of input payload in allOf schema the schema is expecting those requiredVars
to be set and sent in the input payload itself.

For Example, consider the following snippet from the specification file:

schemas:
    Dog:
      allOf:
        - $ref: '#/components/schemas/Mammals'
        - type: object
          properties:
            className:
              enum:
                - 'Dog'
              x-enum-as-string: true
              default: 'Dog'
              type: string
            breed:
              type: string
            legs:
               $ref: '#/components/schemas/Legs'
    Cat:
      allOf:
        - $ref: '#/components/schemas/Mammals'
        - type: object
          properties:
            className:
              enum:
                - 'Cat'
              x-enum-as-string: true
              default: 'Cat'
              type: string
            declawed:
              type: boolean
            legs:
               $ref: '#/components/schemas/Legs'
    Legs:
        type: object
        required:
        - legs
        properties:
            legs:
              enum:
              - '2'
              - '4'
              default: '4'
              x-enum-as-string: true
            name:
               type: string
    Mammals:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          required:
            - className
          properties:
            className:
              enum:
                - 'Dog'
                - 'Cat'
              x-enum-as-string: true
              type: string
    Animal:
      type: object
      required:
        - className
      properties:
        className:
          enum:
            - 'Dog'
            - 'Cat'
          x-enum-as-string: true
          type: string
        color:
          type: string
          default: red
        run:
           type: boolean
           default: True
           readOnly: true

As we can see in the above case Dog class contain the Mammals class as part of allOf composed schema.
Now if we will not specify the className in Dog class as payload and when the payload will be validated against Mammals class and Animal class, Mammal class will throw an error.

Code used to test:

from openapi_client.model.dog import Dog
from openapi_client.model.legs import Legs


def define_legs():
    return Legs(legs="4")


def dog():
    legs = define_legs()

    dog_instance = Dog(color="Black")

    dog_instance.breed = "Bulldog"

    dog_instance.legs = legs

dog()

Error Captured:

Traceback (most recent call last):
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 1782, in get_allof_instances
    allof_instance = allof_class(**model_args, **constant_args)
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 45, in wrapped_init
    return fn(_self, *args, **kwargs)
TypeError: __init__() missing 1 required positional argument: 'class_name'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 1782, in get_allof_instances
    allof_instance = allof_class(**model_args, **constant_args)
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 45, in wrapped_init
    return fn(_self, *args, **kwargs)
  File "E:\Dev\Required_Vars\animal\openapi_client\model\mammals.py", line 289, in __init__
    composed_info = validate_get_composed_info(
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 1988, in validate_get_composed_info
    allof_instances = get_allof_instances(self, model_args, constant_args)
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 1785, in get_allof_instances
    raise ApiValueError(
openapi_client.exceptions.ApiValueError: Invalid inputs given to generate an instance of 'Animal'. The input data was invalid for the allOf schema 'Animal' in the co
mposed schema 'Mammals'. Error=__init__() missing 1 required positional argument: 'class_name'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "E:\Dev\Required_Vars\animal\examples.py", line 18, in <module>
    dog()
  File "E:\Dev\Required_Vars\animal\examples.py", line 12, in dog
    dog_instance = Dog(color="Black")
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 45, in wrapped_init
    return fn(_self, *args, **kwargs)
  File "E:\Dev\Required_Vars\animal\openapi_client\model\dog.py", line 300, in __init__
    composed_info = validate_get_composed_info(
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 1988, in validate_get_composed_info
    allof_instances = get_allof_instances(self, model_args, constant_args)
  File "E:\Dev\Required_Vars\animal\openapi_client\model_utils.py", line 1785, in get_allof_instances
    raise ApiValueError(
openapi_client.exceptions.ApiValueError: Invalid inputs given to generate an instance of 'Mammals'. The input data was invalid for the allOf schema 'Mammals' in the
composed schema 'Dog'. Error=Invalid inputs given to generate an instance of 'Animal'. The input data was invalid for the allOf schema 'Animal' in the composed schem
a 'Mammals'. Error=__init__() missing 1 required positional argument: 'class_name'

And because of this now it will become the user's responsibility to add the requiredVars as a mandatory input in the payload.
That is user need to always specify the className in their payload like the following:

 dog_instance = Dog(class_name="Dog",
                       color="Black")

Since in our application end users may not be know these requiredVars always, the SDK failure creates an issue for us.

Solution:
The proposed solution is to add a configuration variable: setRequiredVars.
If it is set to true, then the older logic will be used. The older logic was used to set the default value of requiredVars in __init__ and _from_openapi_data as positional arguments.

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed responses from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator bypassing the relevant config(s) as an argument to the script, for example, ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master (5.3.0), 6.0.x
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
  @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) @wing328

[vvb]

@spacether - this has the fix for one of the issues we spoke about early on. Please review this when you get the time. @wing328 The CI failure does not look related to the change. Does OP need to do anything for this failure.

[wing328]

As discussed with @the-akhil-nair, the spec has been updated with new test cases to cover the change: 4ed0521

[spacether]

@the-akhil-nair
I have some questions and concerns.

1. Where is setRequiredVars a configuration or generator additional property input?
   I only see it as a void IJsonSchemaValidationProperties methos to set required properties.

2. Did you mean to use getHasRequired instead?
   Your change is breaking as it changes the interfaces and TypeErrors will be thrown if parameters are missing where different errors were thrown previously.

3. Please add or use an existing generator flag to turn on this behavior only when the flag is set to make this a non-breaking change.

4. What about this use case; can you make your update allow in None for these two models?

NullableObjectPropWithRequiredVars:
  type: object
  nullable: true
  properties:
    foo:
      type: string
  required:
  - foo
ComposedSchema:
  allof:
    $ref: componses/schemas/NullableObjectPropWithRequiredVars

In that case ComposedSchema would probably have a required property set, but we need to allow it to be omitted when None is passed in when making an instance of NullableObjectPropWithRequiredVars OR ComposedSchema
Your fix looks like it will break for that use case, can you make it work?

[the-akhil-nair]

@the-akhil-nair I have some questions and concerns.

1. Where is setRequiredVars a configuration or generator additional property input?
   I only see it as a void IJsonSchemaValidationProperties method to set required properties.

It is a configuration variable and not part of the generator property. To avoid confusion I have renamed it to initRequiredVars.

2. Did you mean to use getHasRequired instead?
   Your change is breaking as it changes the interfaces and TypeErrors will be thrown if parameters are missing where different errors were thrown previously.

This variable is not part of the generator code. It's not a setter function or setter variable. It's just a configuration variable that will be used to switch back to older code if the flag is turned on.

3. Please add or use an existing generator flag to turn on this behavior only when the flag is set to make this a non-breaking change.

This PR change will do the same. It's a flag that will turn on the behavior. If the flag is not mentioned in the config file or not used anywhere then the normal openAPI code will be used. Once the flag is turned on it will use the older openAPI code where the required variables will automatically be set.

[spacether]

@the-akhil-nair I have some questions and concerns.

1. Where is setRequiredVars a configuration or generator additional property input?
   I only see it as a void IJsonSchemaValidationProperties method to set required properties.

It is a configuration variable and not part of the generator property. To avoid confusion I have renamed it to initRequiredVars.

2. Did you mean to use getHasRequired instead?
   Your change is breaking as it changes the interfaces and TypeErrors will be thrown if parameters are missing where different errors were thrown previously.

This variable is not part of the generator code. It's not a setter function or setter variable. It's just a configuration variable that will be used to switch back to older code if the flag is turned on.

3. Please add or use an existing generator flag to turn on this behavior only when the flag is set to make this a non-breaking change.

This PR change will do the same. It's a flag that will turn on the behavior. If the flag is not mentioned in the config file or not used anywhere then the normal openAPI code will be used. Once the flag is turned on it will use the older openAPI code where the required variables will automatically be set.

How does one use initRequiredVars?
Do they do it by forking the generator and by setting the initRequiredVars boolean in the generateModels payload?
Can you add a test that shows how to use it?
That's why I suggested adding it as a generator additional property; so it is easier for everyone to use.
Thank you for answering questions 1-3, what about question 4?

[the-akhil-nair]

How does one use initRequiredVars? Do they do it by forking the generator and by setting the initRequiredVars boolean in the generateModels payload? Can you add a test that shows how to use it? That's why I suggested adding it as a generator additional property; so it is easier for everyone to use. Thank you for answering questions 1-3, what about question 4?

@spacether I have added new commit which will include initRequiredVars as generator additional property which will answer questions 1-3.
About question 4 we want your opinion on how we can handle such cases generically.

[spacether]

About question 4 we want your opinion on how we can handle such cases generically.

To handle it generically, you would need to check all allOf schemas and if all of them allow in null, then you need to allow in null at the composed schema level and make the required values at the composed schema optional. This is a headache and oneOf and anyOf and combinations of properties + oneOf/anyOf/allOf also cause cases like this.

That's why I separated out the variables in the way that openapi schema validation describes (only store the properties in the schema that they are defined in).

[spacether]

Can you rebase on master and resolve the merge conflicts again?

[spacether]

People probably aren't going to take time to read a PR to learn what it does. Can you fully describe what it does?
something like
When true required variables are included as positional arguments in init and from openapi data. Note: this can break some composition use cases. To learn more read PR blah.

[the-akhil-nair]

Changed the Description for additional property.
https://github.com/OpenAPITools/openapi-generator/pull/10809/files#diff-e1b4dad5dfdcaee3164087471224118ebb7958563501fec4171518883a8887c6R400

[spacether]

This looks great. Thank you for making those updates. Thank you for your PR.