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

[Play Framework] Update the bean validation to use version 2.0. #8354

Merged
merged 2 commits into from
Jan 23, 2021

Conversation

JFCote
Copy link
Member

@JFCote JFCote commented Jan 6, 2021

For a reason I don't know, it was not working anymore with version 1 of the bean validation. I think it was related that the POJO was modified globally to use jakarta annotation and these doesn't work anymore with version 1.

So instead of trying to fix it, I updated to version 2 which is the latest and used by many other generators. I tested it with my own very huge and complicated project and everything seems to work but please review this carefully because I'm not a specialist of bean validation.

PR 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.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @nmuesch (2021/01)

…now, it was not working anymore with version 1.

@JsonProperty("tags")
private List<Tag> tags = null;
@Valid
private List<Tag> tags = null;
Copy link
Member

@wing328 wing328 Jan 7, 2021

Choose a reason for hiding this comment

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

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 tried but either I was adding an unnecessary empty line or removing the 2 space indentation.
I will give it another try!

Copy link
Member

Choose a reason for hiding this comment

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

I can give it a try tomorrow as I'm pretty "experience" in mustache spacing.

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 just tried like 10 different ways I could think of and there is always a problem... If you find anything, please let me know!

Copy link
Member Author

@JFCote JFCote Jan 7, 2021

Choose a reason for hiding this comment

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

The problem is that the beanValidation partial file only contains "ifs". Most of the time, none of the "ifs" are true so when mustache renders the partials, it just see the file's \n which then creates an empty line for no reason. I searched for an option in the mustache library that could disable this but haven't found one so far.

Copy link
Member

Choose a reason for hiding this comment

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

Pushed 05bbacd. Still have a line break after annotation.

To fix that, need to remove all new line in the bean validation mustache template.

What about showing a message at the end of code generation to advise users using a code formatter?

Copy link
Member Author

Choose a reason for hiding this comment

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

@wing328 Not sure I understand your last sentence about the code formater.

Meanwhile, I had an idea during the weekend. I need to test it. The problem we have with the "partials" beanValidation is because sometime it doesn't have anything and it then just adds the line break.

I was thinking of always having something at the end of the beanValidation, something like that: **javaplayframeworkdummynode**

Then, in the post processing, there must be a way to search for this string + \n and to delete it. I think it can work. That is a strange workaround but Mustache templating is strange too, so we must adapt ;)

I'll give it a try today or tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

@wing328 Not sure I understand your last sentence about the code formater.

I mean recommending users to use something like https://github.com/jhipster/prettier-java to auto-format the code, which fixes all those indentation issues, extra spaces, etc in the output.

Copy link
Member Author

@JFCote JFCote Jan 20, 2021

Choose a reason for hiding this comment

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

@wing328 I checked other generator like Spring (on which Play Framework generator was based) and they have the same problem, even worse. See this for example: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/server/petstore/springboot-beanvalidation/src/main/java/org/openapitools/model/Animal.java#L42

So I think that with your fix, having only one \n is not that bad after all.

So if you agree, you can merge this PR.

@wing328
Copy link
Member

wing328 commented Jan 23, 2021

CI failure already fixed in the master.

@wing328 wing328 merged commit 030b75b into master Jan 23, 2021
@wing328 wing328 deleted the fix-ebean-validation-play-framework branch January 23, 2021 02:58
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