[Discussion] Improvement on functionality of nestjs-query

[Saeid-Za]

Hello. I've been using this amazing library for a couple of weeks now, and it has been really satisfying to see productivity raising up, and I wanted to thank the author and contributors of this project for that, you've created a useful tool, so keep doing what you are doing ! Now, regarding this issue, after using nestjs-query in one of our projects, I saw some opportunities that would support more common use cases that one would encounter. so here are some suggestions:

[SOLVED] 1. enableTotalCount for offset-based pagination

There is a common use case for dashboard web applications to use tables with offset-based pagination. but these tables would need the total amount of rows to provide full functionality. here are some suggestions on how query would be like:

{
  todoItems {
    totalCount
    queries {
      id
      title
      completed
      created
      updated
    }
  }
}

Note the queries sub selection. There were some discussions around this topic at #345, to use aggregation instead, but nesjs-query is already supporting totalCount field on cursor based pagination, and adding this feature to offset-based, would be a good addition.

2. Merging Cursor based & offset based pagination

As i was reading documentation for Pagination on nestjs-query, I saw a lot of similarities between Offset Based Paging and Offset Based Cursor and the differences were:

• Offset Based Paging supports offset parameter, a useful parameter to jump between rows, Offset Based Cursor does not.
• Offset Based Cursor returns Connection instead of Array, that would support filtering on sub-queries, a handful feature.

Cursor is already using offset to return results so, there would be no performance issue to expose offset. In the merged version, I don't see any need for cursor and paginationInfo, so maybe remove them?

And there is a Key-Based Cursor that is the "true" Cursor based pagination, that has no jump support, only supporting next and previous page. as the merged version differs from Key-Based version in interface, i would recommend splitting them into two separate modes ( like how offset-based and cursor-based are different today ) Solving this issue first, would eliminate the need to fix previous issue. Related issues : #327

3. Adding Nested Relation Filter support.

This is an important feature to have for supporting medium to large applications with nested filters. there is currently no support for this feature and typeorm itself does not provide such functionality. This would be a great addition to nestjs-query. Consider these relationships between tables: User 1 - 1 Address User 1 - n todo Address 1 - 1 City

This query, should be possible:

{
  todoItems(filter: { author: { address: {city : {name: {like : "%don%"}} } } }) {
    edges {
      node {
        id
        title
      }
    }
  }
}

This could also be added to Aggregation Queries.

{
  todoItemAggregate(filter: { author: { address: {city : {id : {eq : "1"}}} } }) {
    count {
      id
    }
  }
}

This would also mean that user should provide Filter Complexity to limit the filtering depth

[SOLVED] 4. Adding Sensitivity support to filters.

there could be an option to provide case-sensitivity support for filtering string values. Here is an example inspired by how prisma would expose this config:

{
  todoItems(filter: { title: { eq: "my todo", mode: "insensitive" } }) {
    edges {
      node {
        id
        title
      }
    }
  }
}

I didn't wanted to create several feature issues, so i gathered them all in one proposal, I will add some more items as I encounter more complex use cases. I did make some patch or workarounds as I was developing my server, so I can help with Implementation. I would love to hear some thoughts, Thanks !

[sebargarcia]

+1 for suggestions 1, 3, and 4. For suggestion 2 there are differences between pagination strategies so I suggest keep it as it is now

[Saeid-Za]

@sebargarcia Thanks for the Support ! Regarding suggestion 2, if we are going to keep them as they are, offset parameter for pagination should be added to Offset Based Cursor to support jump functionality and "jumping" isn't supported on Key Based Cursor pagination, which i don't know if could be compatible in one interface (cursor and paginationInfo aren't really needed in Offset Based Cursor).

[imnotjames]

I had tested this in TypeORM - nested relation filtering seems to work with the small test I wrote.

See https://github.com/typeorm/typeorm/issues/2707

[doug-martin]

Hi @Saeid-Za

Thank you for the detailed feature requests. At first glance I think 1 and 2 could be solved by exanpding the offset based paging to have a more useful structure similar to what you suggested.

{
  todoItems {
    totalCount
    nodes {
      id
      title
      completed
      created
      updated
    }
  }
}

I chose nodes to keep inline with the graph nomenclature.

For the second part of number 2, the offset based paging provides hidden benefits that are used by nestjs-query for relations namely recursive connections. I think by making the OFFSET strategy a little more robust we can keep the cursor implementations opaque while providing the functionality you are looking with the OFFSET paging strategy? The other benefit I see (and we have used this strategy internally) is to start off without a defined keyset and moving to it once we have a good keyset in place (our use case is a little more complex) but it allowed us to make a seamless switch once we we're in the position to do so. Thoughts?

For item 3 I'll have to do more digging to see whats possible with TypeOrm (thank you @imnotjames I'll take a look at that issue) and Sequelize it should be possible but I won't make any promises at the moment. I try to keep the persistence query services mainly as adapters, if the underlying ORM does not support it I dont want to put it in the adapter as that is bordering on ORM/QueryBuilder territory. Feel free to issue a PR if you want to.

For number 4 you should be able to acheive this through an iLike so { title: {iLike: 'my todo'}}. Or am I missing something?

[Saeid-Za]

Hi @doug-martin, thanks for commenting your thoughts ! For FR number 1, as you stated, nodes would be a better name.

For FR number 2, I completely agree on the fact that with current Cursor implementation, developers could progressively adapt Key Based pagination, and I think this is a really nice feature to have, so we would need to keep paginationInfo and cursor sub-queries and after, before arguments. The only drawback with current implementation I see is that Offset Based Cursor with OFFSET argumnet, would be a better version of Offset Based Paging, because it supports everything that the other one supports and would also support filtering on sub-queries by returning ConnectionType. IMO, Having one strategy containing another one would confuse the developer, if the only benefit to OFFSET strategy is internal, we should keep it internal and not expose it. I'd love to hear your thought on this.

For FR number 3, I understand that you wouldn't agree on bordering ORM scope, but considering the crucial benefit, I think it would be really useful to have it implemented. Having first level filter in relations is indeed useful but far from real-world web applications. I did some patch-work to make it work with TypeORM, should I make a PR for that? It should be a solid starting point.

For FR number 4, That is exactly what I meant on first comment, Thanks !

[dschewchenko]

I'm agree, that 3th FR is really important.

[hrmcdonald]

FR 1 here would be really awesome. Tacking on an aggregation query works fine, but when paginating this is so often needed that it would be a lot easier if we could include it in the same query response like it is now possible to do with cursor pagination.

[doug-martin]

@hrmcdonald enableTotalCount for offset based connections was added in the latest release! You can check out the migration guide in the docs to see how to use it as it was a breaking change.

https://doug-martin.github.io/nestjs-query/docs/migration-guides/v0.22.x-to-v0.23.x