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

Support Python codegen for the OpenAPI backend #21316

Merged
merged 21 commits into from
Dec 22, 2024

Conversation

grihabor
Copy link
Contributor

No description provided.

```python
openapi_document(
    name="java",
    source="document.json",
    skip_python=True,
)

openapi_document(
    name="python",
    source="document.json",
    skip_java=True,
    python_generator_name="python",
    python_additional_properties={
        "packageName": "version_service_client",
        "projectName": "version-service-client",
    },
)
```
@huonw huonw requested review from jyggen and alonsodomin August 18, 2024 05:15
@alonsodomin alonsodomin added category:new feature backend: Python Python backend-related issues backend: OpenAPI OpenAPI backend-related issues labels Aug 19, 2024
Copy link
Contributor

@alonsodomin alonsodomin left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, just some suggestions around preventing the end users to shoot themselves in the foot in case they input the wrong generator name. The final implementation doesn't need to look exactly like that, just throwing some ideas to the wall and see what sticks.

class OpenApiPythonGeneratorNameField(StringField):
alias = "python_generator_name"
required = False
help = "Python generator name"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the potential for having an abstract OpenApiGeneratorNameField that is common to all codegen implementations and then extended in each of them narrowing down the possible values while providing also a default.

Something like:

´src/python/pants/backend/openapi/codegen/extra_fields.py`:

class OpenApiGeneratorNameField(StringField):
    required = False
    default: ClassVar[str]
    value: str

src/python/pants/backend/openapi/codegen/python/extra_fields.py:

class PythonOpenApiGeneratorNameField(OpenApiGeneratorNameField):
    alias = "python_generator_name"
    default = "python"
    valid_choices = ("python", "python-pydantic-v1")

Similar thing for the Java codegen as apparently there are new (beta) generators available.

The reason I'm suggesting this is because nothing is that like it's been implemented, nothing is stopping an end user for typing python_generator_name="rust" or simply misspelling the value, which would lead to an execution error that may be hard to decipher.

This has the obvious consequence of having to update the list of valid choices for each codegen when new ones are available, which can be a PITA.

A way to prevent having to update our sources would be to instead have a validation rule that obtains the list of all supported generators from the tool, the command openapi-generator list -s should return such a list in a comma-separated list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing is stopping an end user for typing python_generator_name="rust"

I don't think this is the problem we need to solve. Similarly, one could use python_sources(sources=["*.rs"]), we don't want to validate such a thing. Moreover openapi-generator list doesn't tell us which language it generates, which makes sense because the generator can actually generate multiple different kinds of files, like .py, .txt, .yaml. Anyway, the validation will create more problems than it will solve.

simply misspelling the value

Yes, this is a good idea, we can use openapi-generator list for simple set validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added validation with OpenAPIGeneratorNames

Copy link
Contributor

@alonsodomin alonsodomin Dec 21, 2024

Choose a reason for hiding this comment

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

Moreover openapi-generator list doesn't tell us which language it generates

I rather take it more like the generator targets a language + tooling, it does generate support files that are not in the target language, this is because it aims at generating a self-contained buildable product. And this isn't 100% correct either as it also can generate docs and Apache httpd configuration files so I understand that hardcoding an assumption into their naming convention could be problematic. Anyway, at least validating that the chosen name is an accepted one before running the tool gives a better UX.

openapi_document(
name="petstore",
source="petstore_spec.yaml",
python_generator_name="python",
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are giving freedom to the end user to choose the generator, may be worth having an additional test in which the generator being used is python-pydantic-v1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test for python-fastapi

class OpenAPIGeneratorType(Enum):
JAVA = "java"


Copy link
Contributor

Choose a reason for hiding this comment

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

If following the previously suggested approach of having a validation rule for the generator name. This could be replaced by a mapping that gets cached:

@dataclass(frozen=True)
class OpenAPIGeneratorNameMapping:
    mapping: FrozenDict[str, tuple[str, ...]]

    def generators_for(self, lang: str) -> tuple[str, ...]:
          valid_choices = self.mapping.get(lang)
          if valid_choices is None:
              raise ValueError(f"Unsupported language generator: {lang}")
          return valid_choices

    def all_languages(self) -> tuple[str, ...]:
       # return a tuple with the supported languages

    def all_generator(self) -> tuple[str, ...]:
       # return a tuple with all the generator names.

async get_openapi_generator_name_mapping(openapi_generator: OpenAPIGenerator) -> OpenAPIGeneratorNameMapping:
    # Run the JVM process for the generator passing arguments `list -s`
    # Parse CSL result from stdout. Generator names are always prefixed by the language name, e.i.: `java`, `java-helicon-client`, `python`, etc.
    # https://openapi-generator.tech/docs/generators

Then the OpenAPIGeneratorNameMapping could be used as the basis for a validation of the generator_name field at different stages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added list of names OpenAPIGeneratorNames, mapping to languages might create more problems

@huonw
Copy link
Contributor

huonw commented Sep 11, 2024

We've just branched for 2.23, so merging this pull request now will come out in 2.24, please move the release notes updates to docs/notes/2.24.x.md. Thank you!

Copy link
Contributor Author

@grihabor grihabor left a comment

Choose a reason for hiding this comment

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

@alonsodomin Thank you for review! Ready for the second round

class OpenAPIGeneratorType(Enum):
JAVA = "java"


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added list of names OpenAPIGeneratorNames, mapping to languages might create more problems

openapi_document(
name="petstore",
source="petstore_spec.yaml",
python_generator_name="python",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test for python-fastapi

class OpenApiPythonGeneratorNameField(StringField):
alias = "python_generator_name"
required = False
help = "Python generator name"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added validation with OpenAPIGeneratorNames

Copy link
Contributor

@alonsodomin alonsodomin left a comment

Choose a reason for hiding this comment

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

LGTM now, I still believe that the python_generator_name extra field would provide a better UX if it had the python value by default (I'm assuming here that the vast majority of users will use the default generator)

@grihabor
Copy link
Contributor Author

Thank you for review! Shall we merge it?

I'm assuming here that the vast majority of users will use the default generator

I'm actually using openapi to generate a server stub for tests in my repo :) the default one is for client

@alonsodomin
Copy link
Contributor

yeah, once it's been approved, consider it safe for merging

@grihabor
Copy link
Contributor Author

I don't have write permissions. Hey @huonw, could you merge it please? Thank you

@alonsodomin alonsodomin merged commit 0e0fcdd into pantsbuild:main Dec 22, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: OpenAPI OpenAPI backend-related issues backend: Python Python backend-related issues category:new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants