Skip to content

Fix JaxRS-Jersey template for files upload #2570

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

Merged
merged 6 commits into from
Apr 5, 2019
Merged

Conversation

Zomzog
Copy link
Contributor

@Zomzog Zomzog commented Apr 1, 2019

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, ./bin/openapi3/{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: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Remove specific File management from template to align it with the service.

fix #2474

@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03)

@karismann
Copy link
Contributor

thanks for you PR but with this change, it seem like there is a regression with type file in OAS2 :

      parameters:
        - in: formData
          name: jarFile
          description: Jar File to upload
          required: true
          type: file
    public Response addPet(
            @FormDataParam("jarFile") InputStream jarFileInputStream,
            @FormDataParam("jarFile") FormDataContentDisposition jarFileDetail
,@Context SecurityContext securityContext)
    throws NotFoundException {
        return delegate.addPet(jarFile, securityContext);
    }

you can refer to swagger-api/swagger-codegen#3300

@@ -76,7 +76,7 @@ public class {{classname}} {
{{/hasMore}}{{/responses}} })
public Response {{nickname}}({{#allParams}}{{>queryParams}}{{>pathParams}}{{>headerParams}}{{>bodyParams}}{{>formParams}},{{/allParams}}@Context SecurityContext securityContext)
throws NotFoundException {
return delegate.{{nickname}}({{#allParams}}{{#isFile}}{{paramName}}InputStream, {{paramName}}Detail{{/isFile}}{{^isFile}}{{paramName}}{{/isFile}},{{/allParams}}securityContext);
return delegate.{{nickname}}({{#allParams}}{{paramName}}, {{/allParams}}securityContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

actually the real problem is the missing isFormParam in this condition. the formParams.mustache who generate an InputStream and an FormDataContentDisposition variables are conditionned with the isFormParam so this line should be fixed with :

return delegate.{{nickname}}({{#allParams}}{{#isFormParam}}{{#isFile}}{{paramName}}InputStream, {{paramName}}Detail{{/isFile}}{{/isFormParam}}{{^isFile}}{{paramName}}{{/isFile}},{{/allParams}}securityContext);

to solve the issue

Copy link
Contributor Author

@Zomzog Zomzog Apr 2, 2019

Choose a reason for hiding this comment

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

I've changed the template with something inspired by your solution.

I've tried it with the issue's sample and this one :

  /Plugin:
    post:
      summary: uploads an image
      description: ''
      operationId: uploadFile
      requestBody:
        content:
          multipart/form-data:
            schema:
              type: object
              properties:
                additionalMetadata:
                  description: Additional data to pass to server
                  type: string
                file:
                  description: file to upload
                  type: string
                  format: binary

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks . can you please push your new commit to your branch "2474" to update the PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be useful. Done.

Copy link
Contributor

@karismann karismann left a comment

Choose a reason for hiding this comment

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

LGTM
circle-ci error doesn't seem to be caused by this change
can you please edit the PR description and add "fix #2474" to auto-close the related issue when PR will be merged ? thanks

@wing328
Copy link
Member

wing328 commented Apr 5, 2019

Please update the samples:

     @GET
     @Path("/logout")
@@ -161,6 +161,6 @@ public class UserApi  {
 ,@ApiParam(value = "Updated user object" ,required=true) @NotNull @Valid User user
 ,@Context SecurityContext securityContext)
     throws NotFoundException {
-        return delegate.updateUser(username,user,securityContext);
+        return delegate.updateUser(username, user, securityContext);
     }
 }
Perform git status
On branch Zomzog-2474
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   samples/server/petstore/jaxrs-jersey/src/gen/java/org/openapitools/api/AnotherFakeApi.java
	modified:   samples/server/petstore/jaxrs-jersey/src/gen/java/org/openapitools/api/FakeApi.java
	modified:   samples/server/petstore/jaxrs-jersey/src/gen/java/org/openapitools/api/FakeClassnameTestApi.java
	modified:   samples/server/petstore/jaxrs-jersey/src/gen/java/org/openapitools/api/PetApi.java
	modified:   samples/server/petstore/jaxrs-jersey/src/gen/java/org/openapitools/api/StoreApi.java
	modified:   samples/server/petstore/jaxrs-jersey/src/gen/java/org/openapitools/api/UserApi.java

Tested via https://circleci.com/gh/OpenAPITools/openapi-generator/5930#tests/containers/2

@Zomzog
Copy link
Contributor Author

Zomzog commented Apr 5, 2019

I've updated java-jaxrs-petstore-server-all.sh to run also this sample and run it.

@wing328
Copy link
Member

wing328 commented Apr 5, 2019

CircleCI test passed via https://circleci.com/gh/OpenAPITools/openapi-generator/5932

@wing328 wing328 merged commit 42a9368 into OpenAPITools:master Apr 5, 2019
@wing328 wing328 added this to the 4.0.0 milestone Apr 5, 2019
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.

[BUG] [Java | JaxRS-Jersey] Bad file upload code generation
3 participants