openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

Fix #17472 Skip validation annotations for response and fix validation annotations for list arguments

Open jpfinne opened this issue 1 year ago • 8 comments

Since [#4947] [java]: adds support for validation of primitives in arrays, the generated java code can be uncompilable.

See [BUG] generated code cannot be compiled (Java 21) #17472 [BUG][SPRING] Webflux minItems generates incompatible validation annotation #17564

The issue is in the regex in postProcessResponseWithProperty() property.dataType = property.dataType.replaceAll("(?:(?i)@[a-z0-9]+\s)*+", "");

The regex only strips simple @Valid but fails with @Size(min=10).
So gives List<@Size(min=10)String> is converted into List<(min=10)String> instead of List<String>. It seems too complex to fix the regex especially for @Pattern.

This pull request takes a different approach. It does not generate the bean validation for response so there is not need to clean the response in the post processor.

PR checklist

  • [x] Read the contribution guidelines.
  • [x] 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.
  • [x] 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.
  • [x] File the PR against the 7.3.0
  • [x] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328 you did extensive review on the code of @Aliaksie.
Can you review this pull request?

@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08) Please review

jpfinne avatar Jan 28 '24 20:01 jpfinne

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

wing328 avatar Jan 29 '24 05:01 wing328

A test would be great

MelleD avatar Jan 29 '24 06:01 MelleD

@MelleD Test added. @wing328 GitHub account set (I hope so)

jpfinne avatar Jan 29 '24 08:01 jpfinne

@jpfinne Thanks for the tests :).

I think a few more test cases are necessary for good coverage. Take a look at the other BeanValidation test, maybe you can expand that too. There are a lot of combinations.

e.g.

  • what happens to a Set
  • What happens if the list has a Dto class
  • What about other validations regex etc.

MelleD avatar Jan 29 '24 11:01 MelleD

@MelleD a bit overkill when knowing the implementation. But let's imagine we want blackbox testing, if it can speed up the approval and the merge...

I've tested your 3 uses cases:

  • what happens to a Set
  • What happens if the list has a Dto class
  • What about other validations regex etc.

ps: There is no space between @Valid and Size in the generated @Valid@Size(min = 5) . This is valid java code! Something already present in 7.1.0 I've added JavaFileAssert.fileContainsPattern so that the test stays valid for @Valid@Size(min = 5) and for @Valid @Size(min = 5) .

jpfinne avatar Jan 29 '24 14:01 jpfinne

Also fix #17647 and #17709

jpfinne avatar Feb 01 '24 18:02 jpfinne

@martin-mfg The contract is updated

jpfinne avatar Feb 02 '24 16:02 jpfinne

a bit overkill when knowing the implementation.

Why? There is already a test and file with some combination etc. Why you don’t reuse it?

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/bugs/issue_4947.yaml

MelleD avatar Feb 02 '24 17:02 MelleD

can you please resolve the merge conflicts when you've time?

wing328 avatar Mar 13 '24 07:03 wing328