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] Fix the post processing of enums #18566

Merged
merged 1 commit into from May 11, 2024

Conversation

0xMattijs
Copy link
Contributor

@0xMattijs 0xMattijs commented May 3, 2024

description

This fixes the issue described in #16560 of a problem with the Python family of converters that when processing enums, none of them substitutes the enum var extensions.

When processing the following schema:

openapi: 3.0.0
info:
  version: 1.0.0
  title: Weather API
paths: {}

components:
  schemas:
    WeatherType:
      type: integer
      format: int32
      enum:
        - 42
        - 18
        - 56
      x-enum-descriptions:
        - 'Blue sky'
        - 'Slightly overcast'
        - 'Take an umbrella with you'
      x-enum-varnames:
        - Sunny
        - Cloudy
        - Rainy

The generator would yield:

from datetime import date, datetime  # noqa: F401

from typing import List, Dict  # noqa: F401

from openapi_server.models.base_model import Model
from openapi_server import util


class WeatherType(Model):
    """NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).

    Do not edit the class manually.
    """

    """
    allowed enum values
    """
    NUMBER_42 = 42
    NUMBER_18 = 18
    NUMBER_56 = 56
    def __init__(self):  # noqa: E501
        """WeatherType - a model defined in OpenAPI

        """
        self.openapi_types = {
        }

        self.attribute_map = {
        }

    @classmethod
    def from_dict(cls, dikt) -> 'WeatherType':
        """Returns the dict as a model

        :param dikt: A dict.
        :type: dict
        :return: The WeatherType of this WeatherType.  # noqa: E501
        :rtype: WeatherType
        """
        return util.deserialize_model(dikt, cls)

The variables have the default NUMBER_x substitutions.

problem

The problem is caused by the fact that the enum var extension substitutions done bypostProcessModelsEnum at the beginning of postProcessModelsMap are overwritten at a later point in that same method.

The overwriting of the variable names occurs here:

https://github.com/OpenAPITools/openapi-generator/blob/296a6ac5163b226015511b4bea42b78f0d2966f3/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractPythonCodegen.java#L986C10-L998C14

Note: the proposed solution mentioned in #16560 of calling postProcessModelsEnum as part of postProcessModels seems ineffective, as the override postProcessModels is never called by the generator.

proposed solution

Add a condition to test if x-enum-varnames exist before performing the default NUMBER_x substition.

A previous attempt to fix the issue by moving postProcessModelsEnum to the end of the method lead to a complication with string enum types, ending up with variable names ending up enclosed in double quotes.

With the proposed modification the generated is as expected:

class WeatherType(Model):
    """NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).

    Do not edit the class manually.
    """

    """
    allowed enum values
    """
    Sunny = 42
    Cloudy = 18
    Rainy = 56
    def __init__(self):  # noqa: E501
        """WeatherType - a model defined in OpenAPI

        """
        self.openapi_types = {
        }

        self.attribute_map = {
        }

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 response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    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 by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@0xMattijs
Copy link
Contributor Author

0xMattijs commented May 3, 2024

It turns out there are still some complications with this PR, as moving the postProcessModelsEnum will cause the generator to emit faulty variable names for string enums:

E.g.:

 class OuterEnum(str, Enum):
     """
     allowed enum values
     """

    'placed' = 'placed'
    'approved' = 'approved'
    'delivered' = 'delivered'

Expected:

class OuterEnum(str, Enum):
     """
     allowed enum values
     """
     PLACED = 'placed'
     APPROVED = 'approved'
     DELIVERED = 'delivered'

@0xMattijs 0xMattijs marked this pull request as draft May 3, 2024 19:38
@0xMattijs
Copy link
Contributor Author

Since the desired enum post processor is run before all other transformations in the Python generator, a working approach is to prevent the enum transformation for int enums to run if the x-enum-varnames vendorExtensions is present.

@0xMattijs 0xMattijs marked this pull request as ready for review May 3, 2024 21:38
@0xMattijs 0xMattijs changed the title Fix the post processing of enums in the Python generator [Python] Fix the post processing of enums May 5, 2024
@wing328
Copy link
Member

wing328 commented May 6, 2024

thanks for the PR. can you please add a test spec to modules/openapi-generator/src/test/resources/3_0/python/petstore-with-fake-endpoints-models-for-testing.yaml ?

@0xMattijs
Copy link
Contributor Author

0xMattijs commented May 6, 2024

@wing328 I have added the definitions below to petstore-with-fake-endpoints-models-for-testing.yaml and I have executed bin/generate-samples.sh.

Is there anything else I need to do, like defining the expected results somewhere?

        enum_number_vendor_ext:
          type: integer
          format: int32
          enum:
            - 42
            - 18
            - 56
          x-enum-descriptions:
            - 'Description for 42'
            - 'Description for 18'
            - 'Description for 56'
          x-enum-varnames:
            - FortyTwo
            - Eigtheen
            - FiftySix
        enum_string_vendor_ext:
          type: string
          enum:
            - FOO
            - Bar
            - baz
          x-enum-descriptions:
            - 'Description for FOO'
            - 'Description for Bar'
            - 'Description for baz'
          x-enum-varnames:
            - FOOVar
            - BarVar
            - bazVar

samples/yaml/weather.yaml Outdated Show resolved Hide resolved
…t uses the proper variable namesfrom x-enum-varnames

Remove sample
@wing328 wing328 merged commit 365fcd3 into OpenAPITools:master May 11, 2024
34 checks passed
@wing328
Copy link
Member

wing328 commented May 11, 2024

thanks for the PR, which has been merged.

have a nice weekend

renatomameli pushed a commit to renatomameli/openapi-generator that referenced this pull request May 17, 2024
…t uses the proper variable namesfrom x-enum-varnames (OpenAPITools#18566)

Remove sample
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.

None yet

2 participants