Kaltsoon / sequelize-cursor-pagination

➡️ Cursor-based pagination queries for Sequelize models
86 stars 27 forks source link

Using "group" option #63

Closed AlexTurdean closed 1 year ago

AlexTurdean commented 1 year ago

Using group_by makes the totalCount and hasNextPage to not work properly. Could you extract group_by option from the count query?

Looking at the code: modelClass.count(totalCountQueryOptions) this would not return a number if you are using postgres and group_by

AlexTurdean commented 1 year ago

I added a wrapper for the count:

 // This is to fix totalCount in case you use "group" option (SQL group by) when using sequelize-cursor-pagination
  if (result.totalCount?.[0]?.count) {
    result.totalCount = result.totalCount.length;
  }

But now I realised that it also messes with flags. Could you fix this please?

Kaltsoon commented 1 year ago

Thanks for reporting this. Could you open a pull request?

AlexTurdean commented 1 year ago

Sure. The result right now for mode.count() when you pass a group by is something like this: [ { count: 2 }, { count: 10 }, { count: 7 }, ... ]. I can't think of a easy way to fix it at sql level. I'll try to use the length of the array as a replacement, which should work great for all the usescases.

AlexTurdean commented 1 year ago

I don't have permission to push. I'll show you what I did in the meantime:

const [instances, totalCount, cursorCount] = await Promise.all([
    model.findAll(paginationQueryOptions),
    (async () => {
        const totalCount = await model.count(totalCountQueryOptions);
        if (typeof totalCount === "number") {
            return totalCount;
        }
        return totalCount.length;
    })(),
    (async () => {
        const cursorCount = await model.count(cursorCountQueryOptions);
        if (typeof cursorCount === "number") {
            return cursorCount;
        }
        return cursorCount.length;
    })(),
]);
Kaltsoon commented 1 year ago

Could you share some details about what kind of query you are working on? Perhaps there's some reasonable workaround? If you could give a concrete test case and an expected outcome it would help a lot.

AlexTurdean commented 1 year ago

I am using the group option. I want to search for all the items in a database with related associations. I have to use group to not have duplicates

AlexTurdean commented 1 year ago

totalCount, cursorCount basically fail if you use group option in your query

AlexTurdean commented 1 year ago

We will need to use group for multiple queries. Your code expects that the response from count is a number, when it's actually an array.

Kaltsoon commented 1 year ago

Ok, I'll start working on this. Thanks for the suggestions!

AlexTurdean commented 1 year ago

Thank you! :D

Kaltsoon commented 1 year ago

Support for the group by is now available in version 3.4.0. Could you try it out and see if it suits your needs? My test case is a bit naive so if it is not working properly, could you provide a better test case?

AlexTurdean commented 1 year ago

I just tested it, seems to work just fine. Also took a look at the code, totally agree. I will let my team know about this, and if they find any issues I will let you know.

Thanks you! Really appreciate this, keep up your awesome work!

Kaltsoon commented 1 year ago

Excellent!