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

Introducing OpenAPI Generator #3

Merged
merged 9 commits into from Feb 16, 2022
Merged

Introducing OpenAPI Generator #3

merged 9 commits into from Feb 16, 2022

Conversation

ricardozanini
Copy link
Member

@ricardozanini ricardozanini commented Feb 8, 2022

Minimal use case to add OpenAPI Rest Client generation for Quarkus.

Signed-off-by: Ricardo Zanini <zanini@redhat.com>
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
deployment/pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Added some comments. Looks good overall

Signed-off-by: Ricardo Zanini <zanini@redhat.com>
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
@ricardozanini ricardozanini self-assigned this Feb 9, 2022
@ricardozanini ricardozanini added the enhancement New feature or request label Feb 9, 2022
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
@gastaldi
Copy link
Member

gastaldi commented Feb 9, 2022

Does it integrate with the Quarkus OpenAPI extension? Eg. Can you generate a client based on the output of http://localhost:8089/openapi?

@ricardozanini
Copy link
Member Author

It does not integrate automatically but can be a feature in the roadmap. :)

If what you're asking is to use as an input a remote reference like https://petstore.swagger.io/v2/swagger.json, it can't be done now, but I'm willing to implement since my project will use it. See: https://github.com/serverlessworkflow/specification/blob/0.8.x/specification.md#using-functions-for-restful-service-invocations

restClients.produce(new GeneratedOpenApiRestClientBuildItem(classInfo));
hasGeneratedFiles = true;
} else if (classInfo.name().packagePrefix().equals(spec.modelPackage)) {
models.produce(new GeneratedOpenApiModelBuildItem(classInfo));

Choose a reason for hiding this comment

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

maybe I'm misreading it, but wouldn't it make slightly more sense to have only one package name defined and created a name for the other by appending .model, or just use one package for both types of generated things?
It would simplify the config for users

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to mimic the exact configuration of the underlying library, but I agree that having only one property would simplify things for users. Let's do it.

Choose a reason for hiding this comment

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

btw, are the build items produced here used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, but they will soon. I was experimenting with some things and decided to let them here for now.

Signed-off-by: Ricardo Zanini <zanini@redhat.com>
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
# since the file name has a space, we use the URI representation of this space here to not break the properties file
quarkus.openapi-generator.spec."open%20weather.yaml".base-package=org.acme.openapi.weather

Choose a reason for hiding this comment

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

maybe we could allow "open weather.yaml" and encode for the user it as it's done for paths?

Copy link
Member Author

Choose a reason for hiding this comment

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

@michalszynkiewicz I had problems adding properties with spaces in the keys. It won't get read correctly. That's why I kept the encoding here to show how end-users should do this.

EDIT: actually, the IDE complains about the formatting style only. One should either add \\uu20u ASCII code representation of the space or use just encode via URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

btw, we have a nice wrong message for them as well:

The config property quarkus.openapi-generator.spec."open%20weather.yaml".base-package is required but it could not be found in any config source

Signed-off-by: Ricardo Zanini <zanini@redhat.com>
@FroMage
Copy link

FroMage commented Feb 14, 2022

Is this the same PR I reviewed in quarkus core a while back?

@ricardozanini
Copy link
Member Author

More or less the same thing, yes. I want to release a first version of the extension supporting a simple use case (sync REST) and adding more as we go and receive feedback.

@ricardozanini ricardozanini merged commit f9dc616 into main Feb 16, 2022
@ricardozanini ricardozanini deleted the introduce-extension branch February 16, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants