fix: document service find many pagination

[Marc-Roig]

What does it do?

We allowed page & pageSize parameters on find Many, but those params were not properly converted to the offset & limit database params.

This PR proposes a fix, which always converts pagination params to offset and limit, so both page & pagesize, and start & limit works on the findMany method, or any other that accepts pagination.

How to test

Go to the List View of the CMS, and the pages should load correctlly, 10 entities at a time

Fixes #20005

[Marc-Roig]

I am creating tests in the meantime

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
contributor-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 6, 2024 0:24am

[alexandrebodin]

I'm not sure I like the way you had to change the db layer to support this.

IMHO we should remove all notion of page & pageSize on the db layer.

For those page to limit/offset transforms I'm not sure we should do it here, the convertQueryParams is supposed to pass the same params it receives but formatted, this can have a huge impact on people using it and I'm not sure I see a good enough reason to do that here 🤔

[Marc-Roig]

IMHO we should remove all notion of page & pageSize on the db layer.

agree on this, this would mean removing the "findPage" method from the db layer, which atm I do not see it as a good idea

For those page to limit/offset transforms I'm not sure we should do it here, the convertQueryParams is supposed to pass the same params it receives but formatted, this can have a huge impact on people using it and I'm not sure I see a good enough reason to do that here 🤔

Alright :) , two solutions then:

• Transform page params to limit&offset in the doc service findMany pipe function
• Prevent users from using pagination params in the findMany

I would go for the first option, wdyt?

[alexandrebodin]

IMHO we should remove all notion of page & pageSize on the db layer.

agree on this, this would mean removing the "findPage" method from the db layer, which atm I do not see it as a good idea

For those page to limit/offset transforms I'm not sure we should do it here, the convertQueryParams is supposed to pass the same params it receives but formatted, this can have a huge impact on people using it and I'm not sure I see a good enough reason to do that here 🤔

Alright :) , two solutions then:

• Transform page params to limit&offset in the doc service findMany pipe function
• Prevent users from using pagination params in the findMany

I would go for the first option, wdyt?

How did we support it in the findMany of the entityService ? I think we can go with this, even if long term I'm not sure I would

[Marc-Roig]

How did we support it in the findMany of the entityService ? I think we can even if long term I'm not sure I would

We did not support it on the findMany, we had a findPage method

[alexandrebodin]

How did we support it in the findMany of the entityService ? I think we can even if long term I'm not sure I would

We did not support it on the findMany, we had a findPage method

I don't think we have enough bandwidth to just complete drop that and refactor everywhere else, let's support it in the findMany for convenience

[alexandrebodin]

How did we support it in the findMany of the entityService ? I think we can even if long term I'm not sure I would

We did not support it on the findMany, we had a findPage method

I don't think we have enough bandwidth to just complete drop that and refactor everywhere else, let's support it in the findMany for convenience. side note if we do support it I feel like there was no real point of dropping findPage 🤔

[Marc-Roig]

🤝 will start working on that

[alexandrebodin]

How did we support it in the findMany of the entityService ? I think we can even if long term I'm not sure I would

We did not support it on the findMany, we had a findPage method

I don't think we have enough bandwidth to just complete drop that and refactor everywhere else, let's support it in the findMany for convenience. side note if we do support it I feel like there was no real point of dropping findPage 🤔

[Marc-Roig]

No strong opinion, findPage did support both page&pageSize and start&limit, just that the response included the page count.

Keeping it would make the life a bit easier for users (specially migrating from v4), also would introduce a 4th "findX" method to document.

If findPage supported both start&limit I think it would make sense for the findmany to support those too.
And we can always add the findPage method again later on, wdyt?

[alexandrebodin]

No strong opinion, findPage did support both page&pageSize and start&limit, just that the response included the page count.

Keeping it would make the life a bit easier for users (specially migrating from v4), also would introduce a 4th "findX" method to document.

If findPage supported both start&limit I think it would make sense for the findmany to support those too. And we can always add the findPage method again later on, wdyt?

I don't think findMany should know what's a page at all that's my concern, if we added cursor based pagination we would not support it in findMany butexpect it to be converted to a limit & a filter on a column,

[Marc-Roig]

alright ! do you think we should reintroduce the findPage method in the doc service then?

[alexandrebodin]

The code looks good :+1

If you can add tests for both the CM & the content API that would be 🔥

[Marc-Roig]

@alexandrebodin created tests for the CM, and content api tests where skipped :hehe: everything should be covered.

Not checked, but do you think we need to work on the graphql part too?

[alexandrebodin]

@alexandrebodin created tests for the CM, and content api tests where skipped :hehe: everything should be covered.

Not checked, but do you think we need to work on the graphql part too?

Niceee all good then,

For graphql we use the utils but that one you didn't change so we should be good. orther checking if we have some tests that exist or not though

[Feranchz]

Did QA locally and works great 🥳