Skip to content

Add Spring pagination with vendor extension x-spring-paginated#3357

Closed
diyfr wants to merge 20 commits intoswagger-api:masterfrom
diyfr:SpringPagination
Closed

Add Spring pagination with vendor extension x-spring-paginated#3357
diyfr wants to merge 20 commits intoswagger-api:masterfrom
diyfr:SpringPagination

Conversation

@diyfr
Copy link
Contributor

@diyfr diyfr commented Jul 13, 2016

Fix [https://github.com//issues/3353](https://github.com/swagger-api/swagger-codegen/issues/3353]

x-spring-paginated

Add Spring pagination to API. Transform array of object to Spring Page [http://docs.spring.io/spring-data/commons/docs/current/api/org/springframework/data/domain/Page.html], with pagination options : size, page, sort

x-spring-paginated: boolean

Generate ApiImplicitParams and Pageable object to method.

in petstore.json

  "/pet/findByStatus": {
      "get": {
        "tags": [
          "pet"
        ],
        "summary": "Finds Pets by status",
        "description": "Multiple status values can be provided with comma seperated strings",
        "operationId": "findPetsByStatus",
        "x-spring-paginated": true, /* HERE */
        "produces": [
          "application/json",
          "application/xml"
        ],
        "parameters": [ .............

generate in PetApi.java

@ApiOperation(value = "Finds Pets by status", notes = "Multiple status values can be provided with comma seperated strings", response = Pet.class, responseContainer = "List", authorizations = {
        @Authorization(value = "petstore_auth", scopes = {
            @AuthorizationScope(scope = "write:pets", description = "modify pets in your account"),
            @AuthorizationScope(scope = "read:pets", description = "read your pets")
            })
    }, tags={ "pet",  })
    @ApiImplicitParams({
        @ApiImplicitParam(name = "page", dataType = "integer", paramType = "query",
                value = "Results page you want to retrieve (0..N)"),
        @ApiImplicitParam(name = "size", dataType = "integer", paramType = "query",
                value = "Number of records per page."),
        @ApiImplicitParam(name = "sort", allowMultiple = true, dataType = "string", paramType = "query",
                value = "Sorting criteria in the format: property(,asc|desc). " +
                        "Default sort order is ascending. " +
                        "Multiple sort criteria are supported.")
    }) 
    @ApiResponses(value = { 
        @ApiResponse(code = 200, message = "successful operation", response = Pet.class),
        @ApiResponse(code = 400, message = "Invalid status value", response = Pet.class) })
    @RequestMapping(value = "/pet/findByStatus",
        produces = { "application/json", "application/xml" }, 
        method = RequestMethod.GET)
    ResponseEntity<List<Pet>> findPetsByStatus(@ApiParam(value = "Status values that need to be considered for filter", defaultValue = "available") @RequestParam(value = "status", required = false, defaultValue="available") List<String> status, @ApiIgnore final Pageable pageable);

@cbornet I modified SpringCodeGen.java >>
application.properties generate swagger.properties for springmvc profil replaced by application.mustache.
and removed application.properties

</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-data-jpa</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Include spring-data-commons instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbornet I think it is easier to change application.properties than change pom.xml.
If you need persistence (what project do not need) it will change pom for recommender dependency and application.properties for add datasource.

Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of projects don't use JPA !! Think about NoSQL, Mongo, Cassandra, ...

Copy link
Contributor

Choose a reason for hiding this comment

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

It's less intrusive to let the user add spring-boot-starter-data-jpa if he needs JPA and it will not cause any problem to have spring-data-commons in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok @cbornet
Next action :

Change dependency to spring-data-common
SpringFox plugin parameter
But I have to leave , I could not go back to the code before mid-August

Copy link
Contributor

Choose a reason for hiding this comment

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

Will you at least have access to your gh account ?
In that case I can PR to your fork and you will just have to merge.

Copy link
Contributor Author

@diyfr diyfr Jul 13, 2016

Choose a reason for hiding this comment

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

@cbornet via mobile only . no position with git client
Otherwise we demand the integration and adapts after you ?

TODO (for me :) ) update Wiki page for vendor extension...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbornet if you want you can to directly modify the branch of my git repository (access contributor)

@diyfr
Copy link
Contributor Author

diyfr commented Jul 13, 2016

@wing328 The build failed with ruby implementation ?

@wing328
Copy link
Contributor

wing328 commented Jul 13, 2016

@diyfr I've restarted the job and all tests have passed.

@diyfr
Copy link
Contributor Author

diyfr commented Jul 13, 2016

@wing328 I see with @cbornet for some changes

@@ -1,5 +1,6 @@
package io.swagger.api;

@javax.annotation.Generated(value = "class io.swagger.codegen.languages.SpringCodegen", date = "2016-07-13T09:45:49.128+02:00")
Copy link
Contributor

Choose a reason for hiding this comment

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

generate the samples with hideGenerationTimestamps=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where specifying this ?

@diyfr diyfr changed the title Add Spring pagination with vendor extension x-spring-pageable Add Spring pagination with vendor extension x-spring-paginated Jul 13, 2016
@wing328
Copy link
Contributor

wing328 commented Jul 13, 2016

Some of the commits (as shown in the Commits tab) are not linked to your Github account, which means this PR won't count as your contribution in https://github.com/swagger-api/swagger-codegen/graphs/contributors.

Let me know if you need help fixing it.

Ref: http://stackoverflow.com/questions/3042437/change-commit-author-at-one-specific-commit

@@ -1,3 +1,4 @@
#Spring MVC swagger.properties
springfox.documentation.swagger.v2.path=/api-docs
server.contextPath={{^contextPath}}/{{/contextPath}}{{#contextPath}}{{contextPath}}{{/contextPath}}
Copy link
Contributor

@cbornet cbornet Jul 13, 2016

Choose a reason for hiding this comment

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

server.contextPath is not used by spring-mvc. Must be removed

@diyfr
Copy link
Contributor Author

diyfr commented Jul 13, 2016

oups @wing328 my git took the overall configuration instead of the project

@wing328
Copy link
Contributor

wing328 commented Jul 19, 2016

@diyfr Another way to fix the commit authorship is to add that email address as secondary email in your Github account.

@wing328 wing328 modified the milestones: v2.2.1, v2.3.0 Aug 8, 2016
@diyfr
Copy link
Contributor Author

diyfr commented Aug 8, 2016

@wing328 I just got back from holidays. commit authorship is fixed.
I plan to further modify the solution by adding a plugin springfox as proposed @cbornet
But it will not be immediate.

@wing328 wing328 modified the milestones: v2.2.2, v2.3.0 Aug 8, 2016
@wing328
Copy link
Contributor

wing328 commented Aug 8, 2016

@diyfr I hope you'd a great vacation.

Please take your time.

My understanding is that this PR is not a breaking change. Please let me know if that's not the case.

@diyfr
Copy link
Contributor Author

diyfr commented Aug 19, 2016

@cbornet
Before making a delivery
This classes are never used or we have not implemented :

  • ApiOriginFilter ->we have org.springframework.web.bind.annotation.CrossOrigin ?
  • ApiException, NotFoundException :
    -> Generated code is wrong. example in PetStore(simple) different schema for responses
  /pets:  
......    
    responses:
        '200':  
          description: pet response  
          schema:  
            $ref: '#/definitions/pet'  
        default:  
          description: unexpected error  
          schema:  
            $ref: '#/definitions/errorModel'  

-> Generated annotation :

@ApiResponse(code = 200, message = "pet response", response = Pet.class),
@ApiResponse(code = 200, message = "unexpected error", response = Pet.class) })

There are several ways to treat it . Which you advise ?

@diyfr
Copy link
Contributor Author

diyfr commented Aug 22, 2016

For ApiException change extends Exception to RuntimeException.
Now you can use ApiException without declaring thows in signature method.
For example :

throw new ApiException(000,"Can't send action code , number invalid or external service is unavailable");

And for get the code in the JsonResponse need to add a custom ErrorAttribute

   @Bean
    public ErrorAttributes errorAttributes() {
        return new DefaultErrorAttributes() {

            @Override
            public Map<String, Object> getErrorAttributes(
                    RequestAttributes requestAttributes,
                    boolean includeStackTrace) {
                Map<String, Object> errorAttributes = super.getErrorAttributes(requestAttributes, includeStackTrace);
                Throwable error = getError(requestAttributes);
                if (error instanceof ApiException) {
                    errorAttributes.put("errorCode", ((ApiException)error).getErrorCode());
                }
                return errorAttributes;
            }

        };
    }

@wing328 wing328 closed this Aug 22, 2016
@wing328 wing328 reopened this Aug 22, 2016
@cbornet
Copy link
Contributor

cbornet commented Aug 22, 2016

@diyfr I think the error management should be done in a distinct PR. This one is already too big with a messy commit history (maybe you could squash the commits).
Otherwise it is a good idea.
I believe the ApiException class was inherited from the JAX-RS generator so it can be adapted for Spring. But I don't know if the swagger-codegen core handles error models : there is nothing in swagger spec telling that 'default' response is an error.
As for the CORS filter, the cleaner way would probably be to override addCorsMappings() in a WebMvcConfigurerAdapter bean as decribed here. Just ensure that it also works with good old Spring (without boot).

consumes = { {{#consumes}}"{{{mediaType}}}"{{#hasMore}}, {{/hasMore}}{{/consumes}} },{{/hasConsumes}}{{/singleContentTypes}}
method = RequestMethod.{{httpMethod}})
{{#java8}}default {{/java8}}{{#async}}{{^java8}}Callable{{/java8}}{{#java8}}CompletableFuture<{{/java8}}{{/async}}ResponseEntity<{{>returnTypes}}>{{#async}}>{{/async}} {{operationId}}({{#allParams}}{{>queryParams}}{{>pathParams}}{{>headerParams}}{{>bodyParams}}{{>formParams}}{{#hasMore}},{{/hasMore}}{{/allParams}}){{^java8}};{{/java8}}{{#java8}} {
{{#java8}}default {{/java8}}{{#async}}{{^java8}}Callable{{/java8}}{{#java8}}CompletableFuture<{{/java8}}{{/async}}ResponseEntity<{{>returnTypes}}>{{#async}}>{{/async}} {{operationId}}({{#allParams}}{{>queryParams}}{{>pathParams}}{{>headerParams}}{{>bodyParams}}{{>formParams}}{{#hasMore}},{{/hasMore}}{{/allParams}}{{#vendorExtensions.x-spring-paginated}}, @ApiIgnore final Pageable pageable{{/vendorExtensions.x-spring-paginated}}){{^java8}};{{/java8}}{{#java8}} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use ApiIgnore but configure .ignoredParameterTypes(Pageable.class) in the docket (only if not using the plugin)

@wing328
Copy link
Contributor

wing328 commented Aug 22, 2016

Agreed with @cbornet to put other features in a separate PR.

(I'll squash the commits when merging this PR)

@diyfr
Copy link
Contributor Author

diyfr commented Aug 22, 2016

I thought to close this pull request.
I want making one that corrects the differents hotfix mentioned on this PR.
And then make a new one for the extension x-paginated, because there are too many commit with the authors problems. I'd suggest something cleaner

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.

3 participants