airflow-client-python icon indicating copy to clipboard operation
airflow-client-python copied to clipboard

[openapi] wrong use of $ref and nullable

Open 11010cy opened this issue 4 years ago • 1 comments

[description] file: airflow/airflow/api_connexion/openapi/v1.yaml

As seen in the file above, the following usages are used in many places:

# line 2661
dag_run_timeout:
              nullable: true
              $ref: '#/components/schemas/TimeDelta'

# line 2498
sla_miss:
          $ref: '#/components/schemas/SLAMiss'
          nullable: true

Obviously the expectation is to be able to make a variable nullable while using $ref.

But the generated code is not as expected. https://github.com/apache/airflow-client-python/blob/master/airflow_client/client/model/dag_detail.py

# line 132
@cached_property
    def openapi_types():
        """
        This must be a method because a model may have properties that are
        of type self, this must run after the class is loaded

        Returns
            openapi_types (dict): The key is attribute name
                and the value is attribute type.
        """
        lazy_import()
        return {
           ...
            'dag_run_timeout': (TimeDelta,),  # noqa: E501
           ...
        }

[expected]

return {
           ...
            'dag_run_timeout': (TimeDelta, none_type, ),  # noqa: E501
           ...
        }

[Suggest a fix]

Per the openapi spec, properties adjacent to refs are ignored: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#reference-object This object cannot be extended with additional properties and any properties added SHALL be ignored. except for summary and description.

The right schema could be:

dag_run_timeout:
 oneOf:
  - type: null
  - $ref: '#/components/schemas/TimeDelta'

The generated code will be like:

return {
           ...
             'dag_run_timeout': (bool, date, datetime, dict, float, int, list, str, none_type,),  # noqa: E501
           ...
        }

Of course, some bugs due to nullable can be fixed, but the readability of the generated model will be worse. I am still working on that.

[related] #20 #27

11010cy avatar Jan 21 '22 09:01 11010cy

[Suggest a fix] As referenced in https://github.com/OpenAPITools/openapi-generator/issues/11352, there's bug in inline composed schemas.

So the right schema could be:

...
dag_run_timeout:
              $ref: '#/components/schemas/NullableTimeDelta'
...

...
    TimeDelta:
      description: Time delta
      type: object
      required:
        - __type
        - days
        - seconds
        - microseconds
      properties:
        __type: {type: string}
        days: {type: integer}
        seconds: {type: integer}
        microseconds: {type: integer}

    NullableTimeDelta:
      description: |
        nullable timeDelta. marks a timeDelta object to be nullable.
      oneOf:
        - type: null
        - $ref: '#/components/schemas/TimeDelta'
...

It works fine for me.

11010cy avatar Jan 25 '22 02:01 11010cy

May be related: https://github.com/apache/airflow-client-python/issues/50

csm10495 avatar Oct 31 '22 20:10 csm10495

This was solved here https://github.com/apache/airflow/pull/32887/files.

We should add a pre-commit hook to prevent such wrong usage of the nullable property in the OpenAPI spec.

More info here about other ways tried to fix the issue. (allOf, anyOf, NullRef object etc.)

pierrejeambrun avatar Aug 24 '23 12:08 pierrejeambrun