feathers icon indicating copy to clipboard operation
feathers copied to clipboard

result.total does not account for params.pipeline

Open xsl777 opened this issue 2 years ago • 8 comments

Steps to reproduce

pre.37, db: MongoDB

  1. Set custom pipeline in a service find before hook like

(context) => { context.params.pipeline = [{ $match: { foo: 'bar' } }] return context }

  1. Call find() with empty query

The result.data array is correct result.total should be equal to the number of db records where foo is equal to 'bar', but it's equal to the total number of records as if no custom pipeline specified

xsl777 avatar Feb 14 '23 13:02 xsl777

@xsl777, thanks for reporting. This definitely sounds like a bug, but not the one that you might think. Pagination shouldn't work at all when using the aggregation framework queries.

The aggregation framework can return any type of response, even non-countable and non-page-able ones. So there's not really a reliable way for us to calculate the number of documents. Some queries will return a single document (not in an array) containing a single primitive, and it doesn't even have to match your schema.

So if you're getting a response back that includes pagination, the short-term fix is for us to disable pagination for aggregation framework queries. The long-term fix is to look at the feasibility of performing some query analysis on the pipeline stages to see if we can enable pagination. That will take some research, which is why it's long-term.

marshallswain avatar Feb 24 '23 21:02 marshallswain

This also brings up a good point that I should specify this in the docs.

marshallswain avatar Feb 24 '23 21:02 marshallswain

We might be able to enable pagination whenever the $feathers stage is used as a first step. We'd also need to look at a way to look ahead in the pipeline operators and disable pagination when other pipeline conditions would break the pagination, though.

The $feathers stage is handled here: https://github.com/feathersjs/feathers/blob/dove/packages/mongodb/src/adapter.ts#L138-L158

marshallswain avatar Feb 24 '23 21:02 marshallswain

Here's the code causing the bug: https://github.com/feathersjs/feathers/blob/dove/packages/mongodb/src/adapter.ts#L243-L255.

I'm going to see if I can figure out a good way to auto-enable pagination in a reliable way, because I'd love for it to work as you described.

marshallswain avatar Feb 24 '23 21:02 marshallswain

I supposed this line could be updated with the $match params for simple requests. We could only automatically enable pagination for some of the aggregation stages:

  • $match
  • $skip
  • $sort
  • $unset
  • $set
  • $lookup
  • $geoNear
  • $fill

Also, there could probably only be one $match stage.

marshallswain avatar Feb 24 '23 21:02 marshallswain

i have same problem

find(params?: ServiceParams & {
    paginate?: PaginationOptions;
  }): Promise<Paginated<MediaAd>>;
  find(params?: ServiceParams & {
    paginate: false;
  }): Promise<MediaAd[]>;
  async find(params?: any): Promise<Paginated<MediaAd> | MediaAd[]> {


    const rsp = await super.find({
      ...params,
    })

    if (rsp.total && params && Array.isArray(params.pipeline)) {

      const model = await this.getModel(params);
      const count = await model.aggregate([
        ...params.pipeline,
        {
          $count: "mediaManagerId"
        },
        {
          $project:{
            count:'$mediaManagerId'
          }
        }
      ])
      const rows = await count.toArray()
      rsp.total = rows[0]['count']
    }

    return rsp;

  }

Hareis avatar Jun 03 '23 03:06 Hareis

I think the solution here is to remove total from pipeline requests. A separate request for total would need to be run.

marshallswain avatar Jun 07 '23 18:06 marshallswain