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

[Java] allow setting boolean getter (is, has, get) in templates #7344

Merged
merged 9 commits into from
Jan 13, 2018

Conversation

wing328
Copy link
Contributor

@wing328 wing328 commented Jan 8, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

To fix #7261

If this fix looks good, I'll apply the same change to other mustache templates:

modules/swagger-codegen/src/main/resources//MSF4J/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaJaxRS/spec/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaJaxRS/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaJaxRS/resteasy/eap/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaJaxRS/resteasy/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaJaxRS/cxf-cdi/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaJaxRS/cxf/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaSpring/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaInflector/pojo.mustache
modules/swagger-codegen/src/main/resources//Java/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaPlayFramework/pojo.mustache
modules/swagger-codegen/src/main/resources//undertow/pojo.mustache
modules/swagger-codegen/src/main/resources//JavaVertXServer/pojo.mustache
modules/swagger-codegen/src/main/resources//java-pkmst/pojo.mustache

@@ -118,9 +118,16 @@ public class {{classname}} {{#parent}}extends {{{parent}}} {{/parent}}{{#parcela
{{#vendorExtensions.extraAnnotation}}
{{{vendorExtensions.extraAnnotation}}}
{{/vendorExtensions.extraAnnotation}}
{{#isBoolean}}
Copy link
Contributor

Choose a reason for hiding this comment

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

@wing328 I don't understand where you could configure to use either getMyBooleanProperty() or isMyBooleanProperty().
Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can use the -t option with customized templates as follows to have getMyBooleanProperty:

  {{#isBoolean}}
  public {{{datatypeWithEnum}}} get{{getter}}() {
    return {{name}};
  }
  {{/isBoolean}}
  {{^isBoolean}}
  public {{{datatypeWithEnum}}} {{getter}}() {
    return {{name}};
  }
  {{/isBoolean}}

Copy link
Contributor

@macjohnny macjohnny Jan 9, 2018

Choose a reason for hiding this comment

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

don't you think it would be easier to have a configuration option to choose between the two possibilities?
as a developer I wouldn't want to have to copy and customize a swagger mustache template to simply configure a standard-variant. Customizing requires additional efforts when e.g. upgrading.
Moreover, using is...() with a non-primitive Boolean instead of a primitive boolean is not specified and can thus lead to discussions concerning support in e.g. data mappers like here: https://jira.spring.io/browse/SPR-9482

Copy link
Contributor

Choose a reason for hiding this comment

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

@cbornet what is your opinion on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't you think it would be easier to have a configuration option to choose between the two possibilities?

Agreed that adding an option is one way but personally I try to avoid too many options as there're already quite a lot of options for Java client generator already.

Besides is, get, another possible choice is has.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wing328 trying to keep the number of options as small as possible is a good approach. however, as the option in this case would allow using standard-behavior of data mappers, (which used to work before), I consider it worth adding instead of requiring customizations.
Personally, I think it is one of the strengths of swagger codegen to be able to generate standard-code from a swagger-file and directly use it without customizations.
And using the default data mapper of Spring Boot definitely is such a standard-setup.

@svenhuber
Copy link

Since this is standard behavior in Spring Boot I would prefer having an option too. If you don't need it, the additional option will do no harm to users.

@wing328
Copy link
Contributor Author

wing328 commented Jan 11, 2018

Thanks for the feedback.

I'll merge this into master so that we can release v2.3.1.

I'll create another task to track the enhancement adding another CLI option (booleanGetterPrefix) for Java generator.

William

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.

[Java] Bug: boolean getter used for non-primitive Boolean
3 participants