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

Bad Pageable description in Page<DumbBuzModel> description #1215

Closed
dtrouillet opened this issue Jul 12, 2021 · 11 comments
Closed

Bad Pageable description in Page<DumbBuzModel> description #1215

dtrouillet opened this issue Jul 12, 2021 · 11 comments
Labels
enhancement New feature or request

Comments

@dtrouillet
Copy link

dtrouillet commented Jul 12, 2021

Describe the bug
When I use this library

        <dependency>
            <groupId>org.springdoc</groupId>
            <artifactId>springdoc-openapi-ui</artifactId>
            <version>1.5.9</version>
        </dependency>
        <dependency>
            <groupId>org.springdoc</groupId>
            <artifactId>springdoc-openapi-data-rest</artifactId>
            <version>1.5.9</version>
        </dependency>

And this code :

    @GetMapping(value = "/search", produces = MediaType.APPLICATION_JSON_VALUE)
    @PreAuthorize("hasAnyRole('ROLE_USER','ROLE_ADMIN')")
    public Page<ApplicationSimpleDto> findApplication(@ParameterObject Pageable pageable, @QuerydslPredicate(root = Application.class) Predicate predicate) {
        LOGGER.debug("REST request to find Application - Read");
        return applicationService.findApplication(predicate, PaginationUtil.generatePageRequestOrDefault(pageable))
                .map(application -> mapper.map(application, ApplicationSimpleDto.class));
    }

the result is this schema :

 "content": [
    {
      "id": 34,
      "name": "test",
      "code": "ASD",
      "dateFichierPattern": "ddMMyyyy",
      "dateParametrePattern": "yyyyMMdd"
    },
    {
      "id": 36,
      "name": "test",
      "code": "AAA",
      "dateFichierPattern": "ddMMyyyy",
      "dateParametrePattern": "yyyyMMdd"
    },
    {
      "id": 135,
      "name": "Faya.fr",
      "code": "FA0",
      "dateFichierPattern": "yyyyMMdd",
      "dateParametrePattern": "yyyyMMdd"
    }
  ],
  "pageable": {
    "sort": {
      "sorted": false,
      "unsorted": true,
      "empty": true
    },
    "offset": 0,
    "pageNumber": 0,
    "pageSize": 20,
    "unpaged": false,
    "paged": true
  },
  "last": true,
  "totalPages": 1,
  "totalElements": 3,
  "size": 20,
  "number": 0,
  "sort": {
    "sorted": false,
    "unsorted": true,
    "empty": true
  },
  "first": true,
  "numberOfElements": 3,
  "empty": false
}

but in openapi spec generated it's this schemas (look at pageable description) :

{
  "totalPages": 0,
  "totalElements": 0,
  "size": 0,
  "content": [
    {
      "id": 0,
      "name": "string",
      "code": "string",
      "dateFichierPattern": "string",
      "dateParametrePattern": "string"
    }
  ],
  "number": 0,
  "sort": {
    "sorted": true,
    "unsorted": true,
    "empty": true
  },
  "first": true,
  "last": true,
  "numberOfElements": 0,
  "pageable": {
    "page": 0,
    "size": 1,
    "sort": [
      "string"
    ]
  },
  "empty": true
}

If I do not use springdoc-openapi-data-rest I don't have this trouble but I have no support for Pageable in parameters.

To Reproduce
Steps to reproduce the behavior:
You can use this repo : https://github.com/informatique-cdc/ebad/tree/feature/openapi_security and I had openapi description in attached file (is a yaml file).
openapi.txt

Expected behavior
The good behaviour is to have this :

{
  "totalPages": 0,
  "totalElements": 0,
  "size": 0,
  "content": [
    {
      "id": 0,
      "name": "string",
      "code": "string",
      "dateFichierPattern": "string",
      "dateParametrePattern": "string"
    }
  ],
  "number": 0,
  "sort": {
    "sorted": true,
    "unsorted": true,
    "empty": true
  },
  "first": true,
  "last": true,
  "numberOfElements": 0,
  "pageable": {
    "sort": {
      "sorted": false,
      "unsorted": true,
      "empty": true
    },
    "offset": 0,
    "pageNumber": 0,
    "pageSize": 20,
    "unpaged": false,
    "paged": true
  },
  "empty": true
}

Thanks a lot for your work :)

@bnasslahsen
Copy link
Contributor

@dtrouillet,

This is your generated Pageable object:

  Pageable:
    type: object
    properties:
      page:
        minimum: 0
        type: integer
        format: int32
      size:
        minimum: 1
        type: integer
        format: int32
      sort:
        type: array
        items:
          type: string

Are you expecting anything different ?

@dtrouillet
Copy link
Author

Hi @bnasslahsen ,

Yes I expect this for the response :

Pageable:
      type: object
      properties:
        sort:
          $ref: '#/components/schemas/Sort'
        pageNumber:
          type: integer
          format: int32
        pageSize:
          type: integer
          format: int32
        paged:
          type: boolean
        unpaged:
          type: boolean
        offset:
          type: integer
          format: int64
    Sort:
      type: object
      properties:
        unsorted:
          type: boolean
        sorted:
          type: boolean
        empty:
          type: boolean

@bnasslahsen
Copy link
Contributor

bnasslahsen commented Jul 15, 2021

@dtrouillet,

You want a different description for the same object Pageable, between the request and response.

  • You can get rid off springdoc-openapi-data-rest.
  • And then use @PageableAsQueryParam annotation for your parameter or directly swagger-annotations if you want more control over the descriptions.

This is a sample code, that you can adapt.

@GetMapping(value = "/search", produces = MediaType.APPLICATION_JSON_VALUE)
@PageableAsQueryParam
public Page<ApplicationSimpleDto> findApplication(@Parameter(hidden = true) Pageable pageable) {
	return ...;
}

@dtrouillet
Copy link
Author

Thank you for your answer.
But in my project, I need to user springdoc-apenapi-data-rest for Predicate usage :/

@dtrouillet
Copy link
Author

My local solution is to exclude SpringDocDataRestConfiguration.class with

@SpringBootApplication(exclude = SpringDocDataRestConfiguration.class)

And add a similar class without

	static {
		getConfig().replaceWithClass(org.springframework.data.domain.Pageable.class, Pageable.class)
				.replaceWithClass(org.springframework.data.domain.PageRequest.class, Pageable.class);
	}

@bnasslahsen
Copy link
Contributor

i see this workaround works fine.
Or alternatively, you can disable the replacement explicitly:

@Configuration
class OpenAPIConfig {

	@EventListener(ContextRefreshedEvent.class)
	void contextRefreshedEvent() {
		SpringDocUtils.getConfig().disableReplacement(org.springframework.data.domain.Pageable.class);
	}

}

@dtrouillet
Copy link
Author

It work as well. Thank you 👍🏼

@little-pinecone
Copy link

This issue should be reopened, as the bug persists and the workaround is ugly (and basically disables springdoc-openapi-data-rest functionality).

Springdoc-openapi-data-rest incorrectly assumes that replacing the schema for Pageable for both the request and response is a good solution, leaving the OpenApi docs in a broken state (just differently broken). If someone uses these docs to generate client code (e.g. with openapi-generator-maven-plugin) the resulting code will be broken as the Pageable in a Spring Boot response will look completely different than the one in the docs.

Perhaps, after replacing Pageable, we could then replace Page with a custom class that itself uses a different class for its Pageable? Would that be possible, given the fact that it's using generics?

In the meantime, I think it's a good idea to mention the workaround in the Springdoc documentation.

@bnasslahsen
Copy link
Contributor

bnasslahsen commented Aug 28, 2021

@little-pinecone,

Replacing Page, might have sense. But will add custom code that will be tightly coupled to Page of spring-data-rest.
Even we suppose Page is a stable object, but if there is any new attribute. springdoc-openapi will have to impact it code itself.

We have already introduced the custom Pageable replacement. But the less we have this kind of objects (tightly coupled to other libraries) and the better we maintain springdoc-openapi.

Meanwhile, if we find out any better alternatives. will update this ticket.

Anyway, if you are feeling the replacement of Page will bring as added value, please feel free to propose a PR.

@daniel-frak
Copy link

@bnasslahsen springdoc-openapi-data-rest is already tightly coupled to spring-data-commons, which itself contains Page. Pageable and Page are also already tightly coupled in Spring due to one relying on the other (and, to my knowledge, have been stable for a while now) - I don't think coupling is an issue here.

In fact, what is proposed here is fixing a bug in springdoc-openapi-data-rest due to the fact that Springdoc is ignoring this coupling. The module is unusable right now without a fix (or workaround) if one uses Page with the endpoints (and I can't really think of a scenario where one would use Pageable and not also use Page).

I would propose a PR, but I'm not sure how to go about replacing Page, as it relies on generics and thus my attempts so far have ended in failure. Could you (or someone) point us at how to achieve this?

In the meantime, I do think it's wise to reopen this issue, as the docs are currently incorrectly claiming that the support for Pageable of spring-data-commons is available, whereas only support for Pageable requests is provided, with support for Pageable responses (which will logically always be within Page) is nonexistent:
https://springdoc.org/#spring-data-rest-support
https://springdoc.org/#how-can-i-map-pageable-spring-date-commons-object-to-correct-url-parameter-in-swagger-ui

As an aside, Springfox does not have this issue. It would be great to have feature parity in this aspect.

@bnasslahsen
Copy link
Contributor

bnasslahsen commented Aug 28, 2021

@daniel-frak,

I am not sure, you are understanding what i have explained here:
Duplicating code, is worse than just referencing a dependency and reusing it's capabilities.
Sometimes, you have to make pragmatic choices and accept duplication. But it doesn't mean you apply this choice everywhere in your design which can make things worse...

For the consistency of combining both Pageable and Page, you have many workarounds listed here. They just work fine. They are not ideal, but as i mentioned earlier if we find out any better alternatives. we will update this ticket.

If you are looking for handling generics replacement, you can see WebFluxSupportConverter.

@bnasslahsen bnasslahsen added the enhancement New feature or request label Jan 9, 2022
@springdoc springdoc locked as resolved and limited conversation to collaborators Mar 9, 2022
@springdoc springdoc deleted a comment from rhubarb Mar 10, 2023
@springdoc springdoc deleted a comment from little-pinecone Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants