OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.63k stars 6.53k forks source link

[BUG][python] Cannot instantiate class without required read-only parameters for POST endpoint #9296

Closed gbmarc1 closed 3 years ago

gbmarc1 commented 3 years ago

Bug Report Checklist

@spacether

Python generator

Description

I have a POST endpoint and the request body and response body use the same schema definition as below. Dataset has multiple readOnly parameters that must be present in the response, but should not be provided in the request.

I can instantiate the Dataset model providing None to the parameters that are readOnly and without verifying the model's input (_check_type=False). However, post request still fails (404 bad request) because params are still present with None values.

openapi-generator version

5.1.1

OpenAPI declaration file content or url
paths:
  /datasets:
    post:
      operationId: v1_inference_datasets_post
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Dataset'
        description: The dataset to be registered
        required: true
      responses:
        '201':
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Dataset'
          description: Confirmation of dataset registration
      summary: Register a dataset
      tags:
        - datasets

components:
  schemas:
    Dataset:
      additionalProperties: false
      example:
        statusLastChange: 2000-01-23T04:56:07+00:00
        modifiedOn: 2000-01-23T04:56:07+00:00
        name: name
        inactiveOn: 2000-01-23T04:56:07+00:00
        description: description
        mediaType: text/plain
        id: 0
        createdOn: 2000-01-23T04:56:07+00:00
        status: active
      properties:
        id:
          description: ID of the dataset
          readOnly: true
          type: integer
        name:
          description: User-defined name of the dataset
          maxLength: 128
          type: string
        description:
          description: User-defined description of the dataset
          maxLength: 256
          type: string
        mediaType:
          description: Data type of the dataset. All data must be of the same type.
          enum:
            - text/plain
          type: string
        createdOn:
          description: Creation time of the dataset
          format: date-time
          readOnly: true
          type: string
        modifiedOn:
          description: Modification time of the dataset
          format: date-time
          readOnly: true
          type: string
        inactiveOn:
          description: |
            Time when the dataset will become inactive. Make sure to retrieve all data and job results
            before this time.
          format: date-time
          readOnly: true
          type: string
        status:
          description: |
            Status of the dataset:
              * `active`: The dataset can be modified (including the data it contains).
              * `committed`: The dataset can no longer be
                 modified (including the data it contains) but jobs can be submitted.
              * `inactive`: The dataset can no longer be modified (including the
                data it contains) and jobs cannot be submitted. Once a dataset is inactive,
                it becomes a candidate for deletion meaning it will soon be deleted from the system.
          enum:
            - active
            - committed
            - inactive
          readOnly: true
          type: string
        statusLastChange:
          description: Time when the status last changed
          format: date-time
          readOnly: true
          type: string
      required:
        - createdOn
        - description
        - id
        - inactiveOn
        - mediaType
        - modifiedOn
        - name
        - status
        - statusLastChange
      type: object
Generation Details

generate -i openapi/openapi.yaml -g "python" --package-name ex_ai_hub_api_client2 -p packageVersion=0.2.0

Steps to reproduce
Related issues/PRs
Suggest a fix

I suggest the following 3 modifications:

  1. Modify the signature of __init__ for readOnly=True parameters from
    Dataset:
    @convert_js_args_to_python_args
    def __init__(self, id, name, description, created_on, modified_on, inactive_on, status, status_last_change, *args, **kwargs):

    to

    Dataset:
    @convert_js_args_to_python_args
    def __init__(self, id, name, description, created_on=None, modified_on=None, inactive_on=None, status=None, status_last_change=None, *args, **kwargs):
  1. Modify the checks accordingly to support null values when instantiating a class
  2. Do not add parameter to body payload for readOnly=True and nullable=False parameters when value is None .
spacether commented 3 years ago

None should not be used as a default value because any property required or optional may or may not be Nullable.

gbmarc1 commented 3 years ago

None should not be used as a default value because any property required or optional may or may not be Nullable.

What do you think of a new class? Such as Empty

class Empty:
    pass

@convert_js_args_to_python_args
def __init__(self, name, description, id=Empty(), created_on=Empty(), modified_on=Empty(), inactive_on=Empty(), status=Empty(), status_last_change=Empty(), *args, **kwargs):
spacether commented 3 years ago

Including a placeholder value like that could work but

readOnly functionality is dependent on the context in which the payload is instantiated.

That wording means that instantiating the models in different contexts needs different method signatures.

So my suggestion would be to break out a separate method to instantiate classes in the two contexts.

We already use the parameter spec_property_naming to indicate that the code is in the context of from_server

gbmarc1 commented 3 years ago

I think you are right. The init constructor should be used for the to_server context and another constructor for the from_server. I was looking for another feature "deserialization" in the models to convert a dict to an instance of the model. I suggest to name the method "from_dict". A bit like this:

https://github.com/OpenAPITools/openapi-generator/blob/22950fa2b2babba2315f2c48da3644b60083d7b2/modules/openapi-generator/src/main/resources/python-flask/model.mustache#L64

spacether commented 3 years ago

Model instantiation is not only done with dictionaries. We also have array models and primitive model. Primitive models are made if there are validation or enum constraints. So we are instantiating models from openapi data, not just dicts.

Some examples:

We need all of those model types because endpoints can return refed component schemas for:

If we did not make the list and primitive model types, then we would leave out the enum and validation constraints.

gbmarc1 commented 3 years ago

ok sounds a good plan. I can try to implement that next week

spacether commented 3 years ago

Thank you

gbmarc1 commented 3 years ago

Could I have a reference that explain what is the difference between normal/composed/simple models?

spacether commented 3 years ago

ModelNormal models are classes that store object schemas ModelSimple models are classes that store int/str/list/date/datetime schemas and they are only generated if there is a validation or enum constraint or if the model is a list model ModelComposed models are classes that store composed schemas Also described in code in the class definitions: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/openapi3/client/petstore/python/petstore_api/model_utils.py#L288

Composed schemas are not always of type object. They can be any type. For example oneOf different array definitions.

spacether commented 3 years ago

Adding the classification Enhancement: Feature because the feature readOnly parameters has not yet been implemented in python

gbmarc1 commented 3 years ago

Solved in #9409