[Jaxrs-cxf-cdi] Add beanvalidation annotations updated #4615

jfiala · 2017-01-21T13:03:22Z

PR checklist

Read the contribution guildelines.
Ran the shell/batch 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)
Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

add beanvalidation annotation support to jaxrs-cxf-cdi
for details see #4091

This is a re-creation of the PR #4507 to have a clean comparison view.

…gger-codegen into jaxrs_cxf_cdi_beanval

…r-api#4091

wing328 · 2017-01-22T09:43:25Z

cc @nickcmaynard

nickcmaynard · 2017-01-22T10:07:00Z

modules/swagger-codegen/src/main/resources/JavaJaxRS/cxf-cdi/api.mustache

        @ApiResponse(code = {{{code}}}, message = "{{{message}}}", response = {{{returnType}}}.class{{#returnContainer}}, responseContainer = "{{{returnContainer}}}"{{/returnContainer}}){{#hasMore}},{{/hasMore}}{{/responses}} })
    public Response {{nickname}}({{#allParams}}{{>queryParams}}{{>pathParams}}{{>headerParams}}{{>bodyParams}}{{>formParams}}{{#hasMore}}, {{/hasMore}}{{/allParams}}) {
-    	return delegate.{{nickname}}({{#allParams}}{{#isFile}}inputStream, fileDetail{{/isFile}}{{^isFile}}{{paramName}}{{/isFile}}, {{/allParams}}securityContext);
+    	return delegate.{{nickname}}({{#allParams}}{{#isFile}}fileInputStream, fileDetail{{/isFile}}{{^isFile}}{{paramName}}{{/isFile}}, {{/allParams}}securityContext);


Good point, though don't we mean {{paramName}}InputStream?

without this change there currently is a compile error in PetApi.java, as the parameter is declared with name fileInputStream instead of inputStream

@Multipart(value = "file", required = false) InputStream fileInputStream

So, the fix is to modify the returned parameter from inputStream to fileInputStream.
Of course also the parameter name in the method signature could be changed, pls let me know if you prefer it the other way.

Yes, because

{{#isFile}} @Multipart(value = "{{paramName}}"{{^required}}, required = false{{/required}}) InputStream {{paramName}}InputStream, @Multipart(value = "{{paramName}}" {{^required}}, required = false{{/required}}) Attachment {{paramName}}Detail{{/isFile}}

The parameter name is generated by {{paramName}}InputStream. fileInputStream is the result of that. So shouldn't we generate here, too?

Yes, thx I'll fix that.

thx for the hint, now {{paramName}}InputStream is used

nickcmaynard · 2017-01-22T10:09:20Z

modules/swagger-codegen/src/main/resources/JavaJaxRS/cxf-cdi/beanValidation.mustache

@@ -0,0 +1,53 @@
+{{#required}}


This, beanValidationPathParams.mustache, and beanValidationQueryParams.mustache are very similar. I understand the arguments for avoiding reuse across language generators, but I simply don't agree that needs to be the case within a language generator. I think these three files need refactoring to remove the duplication.

OK, the only difference is that PathParams are always required, so they don't need a check for NotNull, I commented this in beanValidationPathParams.mustache. So we could extract all other annotation checks into a beanValidationCommonParams.mustache.

Regarding cross-language templates using lambdas this is a nice feature of Mustache, but we need green light from @fehguy /@wing328 for cross-language template reuse.

Sounds good, within the language.

OK, I'll change that.

thx for the hint, the templates are now reused in beanValidationParams.mustache.

nickcmaynard · 2017-01-22T10:09:37Z

modules/swagger-codegen/src/main/resources/JavaJaxRS/cxf-cdi/enumClass.mustache

@@ -1,16 +1,33 @@
-import javax.xml.bind.annotation.XmlEnum;
-import javax.xml.bind.annotation.XmlType;
+@XmlType(name="{{datatypeWithEnum}}")


Why do we have enum changes in here?

@nickcmaynard With the old version enumClass.mustache there are compile errors, as the generated sample files were out of date. Pls simply try with the old version and then you see the compilation errors.

My point is that this is a simple case of one PR, two issues. We should get into the habit of one PR, one issue.

I'm absolutely with you (if the samples are up to date and compiling).

nickcmaynard · 2017-01-22T10:13:17Z

Bunch of code review comments above. Also, still seeing jaxrs-spec sample regeneration...

jfiala · 2017-01-22T12:19:09Z

@nickcmaynard Responded to all review comments, pls let me know if I should adjust the templates in my branch or if you prefer to do a PR on my repo etc.

jfiala · 2017-01-23T19:47:24Z

@nickcmaynard fixed all review comments, pls review :).

jfiala · 2017-01-23T20:33:34Z

The consolidation of beanValidationParams.mustache should be backported to all other languages, created #4636 as reminder.

nickcmaynard · 2017-01-24T18:37:25Z

modules/swagger-codegen/src/main/resources/JavaJaxRS/cxf-cdi/api.mustache

        @ApiResponse(code = {{{code}}}, message = "{{{message}}}", response = {{{returnType}}}.class{{#returnContainer}}, responseContainer = "{{{returnContainer}}}"{{/returnContainer}}){{#hasMore}},{{/hasMore}}{{/responses}} })
    public Response {{nickname}}({{#allParams}}{{>queryParams}}{{>pathParams}}{{>headerParams}}{{>bodyParams}}{{>formParams}}{{#hasMore}}, {{/hasMore}}{{/allParams}}) {
-    	return delegate.{{nickname}}({{#allParams}}{{#isFile}}inputStream, fileDetail{{/isFile}}{{^isFile}}{{paramName}}{{/isFile}}, {{/allParams}}securityContext);
+    	return delegate.{{nickname}}({{#allParams}}{{#isFile}}{{paramName}}InputStream, fileDetail{{/isFile}}{{^isFile}}{{paramName}}{{/isFile}}, {{/allParams}}securityContext);


Sorry - just spotted that by the same logic, we probably also want {{paramName}}Detail!

fixed - added {{paramName}}Detail

nickcmaynard · 2017-01-24T18:39:22Z

modules/swagger-codegen/src/main/resources/JavaJaxRS/cxf-cdi/beanValidation.mustache

@@ -0,0 +1,53 @@
+{{#required}}


Can beanValidation.mustache also bring in {{>beanValidationParams}}? There looks like there's more common ground here...

the difference here is that in beanValidation.mustache every annotation gets a new line whereas in the params template all annotation share the same line. So I fear we have to keep them separate.

OK - because this template is used for method annotations, you're adding newlines. However, to my knowledge, there's no Java syntax requirement for newlines on method/property annotations - indeed, much like parameter annotations, newlines are about style, rather than syntax.

So - syntactically I think this would be OK without the newlines. What do you think?

I've also noticed you've got some clever integer/decimal handling in here that's missing in beanValidationParams.mustache- should that be ported?

I'm not sure if its better to drop the newlines in favor of having one beanValidationCore.mustache.
Currently they are all duplicated with newlines (beanValidation.mustache) and all in one line (beanValidationParams.mustache).
Let's have a second opinion before I change something here - @wing328 can you give us a thought?

From a beauty of generated code perspective I'd favor the current solution at the cost of having to maintain both. We could add a mustache comment to remind the maintainers. In fact the bigger effort is having to maintain beanValidation*.mustache across all Java languages, but that's a different story...

nickcmaynard · 2017-01-24T18:40:13Z

modules/swagger-codegen/src/main/resources/JavaJaxRS/cxf-cdi/pom.mustache

            <artifactId>swagger-annotations</artifactId>
            <version>[1.5.3,2)</version>
        </dependency>
+


This section incorrectly indented

you probably mean the beanvalidation dependency, this is fixed now.

nickcmaynard · 2017-01-24T18:41:58Z

@jfiala added a couple more review comments - apologies. Additionally, I'm still seeing sample updates for the jaxrs-spec generator, which is unexpected.

jfiala · 2017-01-25T20:59:52Z

@nickcmaynard updated according to your comments and reset the jaxrs-spec samples to master.

jfiala · 2017-02-04T14:01:06Z

I refined the handling of DecimalMin/DecimalMax to correctly use Min/Max for Integer+Long and DecimalMin/Max for all others (it would be great to have a flag for non-decimal types in CodegenProperty) and added it to both beanValidation.mustache + beanValidationParams.mustache.

@wing328 Pls advise if we should merge the two beanValidation template files at the cost of losing the newlines for the ApiModels (the beanvalidation annotations would then always be a one-liner for params and models, but maybe that's even better for readability...).

jfiala · 2017-02-05T15:13:56Z

@wing328 can you pls take a look here and merge this into master, then I we can conclude the beanvalidation (PR #4635).

wing328 · 2017-02-05T16:53:58Z

Pls advise if we should merge the two beanValidation template files at the cost of losing the newlines for the ApiModels (the beanvalidation annotations would then always be a one-liner for params and models, but maybe that's even better for readability...).

I would suggest we go with merging the 2 template files for better readability and see if the community has any feedback.

wing328 · 2017-02-05T16:55:55Z

@jfiala thanks for the PR, which has been merged into master.

@nickcmaynard thanks for reviewing the change.

jfiala · 2017-03-12T21:43:46Z

Beanvalidation annotations fully implemented for jaxrs-cxf-cdi

) * add check for hideGenerationTimestamp swagger-api#4091 * update generated sample with no generated timestamps swagger-api#4091 * add beanvalidation to jaxrs-cxf-cdi swagger-api#4091 * add beanvalidation to jaxrs-cxf-cdi swagger-api#4091 * update crlf * replace tabs * add check for hideGenerationTimestamp swagger-api#4091 * update generated sample with no generated timestamps swagger-api#4091 * add beanvalidation to jaxrs-cxf-cdi swagger-api#4091 * add beanvalidation to jaxrs-cxf-cdi swagger-api#4091 * update crlf * replace tabs * re-generate samples after rebasing swagger-api#4091 * fix handling of inner enum templates swagger-api#4091 * fix InputStream/Multipart imports and fileInputStream variable swagger-api#4091 * fix paramName for files swagger-api#4091 * consolidate beanValidationParams swagger-api#4091 * add paramNameDetail swagger-api#4615 * fix indentation and regenerate samples swagger-api#4615 * reset samples jaxrs-spec to master * update generated samples * adapt Min/Max/DecimalMin/DecimalMax handling for int/long/else * add ModelApiResponse

jfiala and others added 17 commits January 6, 2017 17:35

add check for hideGenerationTimestamp swagger-api#4091

00664bc

update generated sample with no generated timestamps swagger-api#4091

7df73e3

add beanvalidation to jaxrs-cxf-cdi swagger-api#4091

9f01c58

add beanvalidation to jaxrs-cxf-cdi swagger-api#4091

14bb432

update crlf

04b1df0

replace tabs

2f42133

Merge branch 'master' into jaxrs_cxf_cdi_beanval

d08661a

add check for hideGenerationTimestamp swagger-api#4091

7c29f9e

update generated sample with no generated timestamps swagger-api#4091

84e3a80

add beanvalidation to jaxrs-cxf-cdi swagger-api#4091

59caedd

add beanvalidation to jaxrs-cxf-cdi swagger-api#4091

4cc5b0b

update crlf

bff8231

replace tabs

7ab67ae

Merge branch 'jaxrs_cxf_cdi_beanval' of https://github.com/jfiala/swa…

98b657d

…gger-codegen into jaxrs_cxf_cdi_beanval

re-generate samples after rebasing swagger-api#4091

35e0c0e

fix handling of inner enum templates swagger-api#4091

4b301f7

fix InputStream/Multipart imports and fileInputStream variable swagge…

df67581

…r-api#4091

jfiala mentioned this pull request Jan 21, 2017

[Jaxrs-cxf-cdi] Add beanvalidation annotations #4507

Closed

3 tasks

wing328 added the Server: Java label Jan 22, 2017

wing328 added this to the v2.2.2 milestone Jan 22, 2017

wing328 added the Enhancement: Feature label Jan 22, 2017

nickcmaynard reviewed Jan 22, 2017

View reviewed changes

jfiala added 2 commits January 23, 2017 20:41

fix paramName for files swagger-api#4091

f8487aa

consolidate beanValidationParams swagger-api#4091

c3669c4

jfiala mentioned this pull request Jan 23, 2017

Backport beanValidationParams.mustache to all other languages #4636

Closed

nickcmaynard reviewed Jan 24, 2017

View reviewed changes

jfiala added 3 commits January 25, 2017 21:47

add paramNameDetail swagger-api#4615

9b4c327

fix indentation and regenerate samples swagger-api#4615

66156fc

reset samples jaxrs-spec to master

c87e93f

jfiala added 3 commits February 4, 2017 14:42

update generated samples

cab2cbe

adapt Min/Max/DecimalMin/DecimalMax handling for int/long/else

9941dea

add ModelApiResponse

e755583

jfiala mentioned this pull request Feb 4, 2017

[Java*] Refine handling of beanvalidation (parameters + decimals) #4711

Closed

8 tasks

wing328 merged commit 38c8796 into swagger-api:master Feb 5, 2017

jfiala deleted the jaxrs_cxf_cdi_beanval branch February 5, 2017 16:59

jfiala mentioned this pull request Feb 5, 2017

[Java*] Merge beanValidation/beanValidationParams.mustache #4719

Closed

7 tasks

[Jaxrs-cxf-cdi] Add beanvalidation annotations updated #4615

[Jaxrs-cxf-cdi] Add beanvalidation annotations updated #4615

Uh oh!

Conversation

jfiala commented Jan 21, 2017 • edited by wing328 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

wing328 commented Jan 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfiala Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickcmaynard commented Jan 22, 2017

Uh oh!

jfiala commented Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfiala commented Jan 23, 2017

Uh oh!

jfiala commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfiala Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickcmaynard commented Jan 24, 2017

Uh oh!

jfiala commented Jan 25, 2017

Uh oh!

jfiala commented Feb 4, 2017

Uh oh!

jfiala commented Feb 5, 2017

Uh oh!

wing328 commented Feb 5, 2017

Uh oh!

wing328 commented Feb 5, 2017

Uh oh!

jfiala commented Jan 21, 2017 •

edited by wing328

Loading

jfiala Jan 23, 2017 •

edited

Loading

jfiala commented Jan 22, 2017 •

edited

Loading

jfiala Jan 26, 2017 •

edited

Loading