result.total does not account for params.pipeline

[xsl777]

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
}

2. 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

[marshallswain]

@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]

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

[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]

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]

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.

[Hareis]

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;

  }

[marshallswain]

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

[alidplus]

The actual bug is happening here: https://github.com/feathersjs/feathers/blob/dove/packages/mongodb/src/adapter.ts#L314

We call this.countDocuments(params) to count documents regardless of whether pipeline exists or not, while we call getData to retrieve documents for a single page, While the getData branches for the logic of pipeline existence.

On the other side, the countDocuments method tries to count all of aggregated documents by passing undefined to $limit. while the aggregateRaw methods gets the default limit value by calling makeFeathersPipeline -> filterQuery -> getLimit.
Then, it gets 10 documents and returns the array.length as total!

I think the ideal solution is to use a similar branching like getData to count the total document with considering the pipeline:

    const getTotal = () => {
      return params.pipeline ? this.countAggregate(params) : this.countDocuments(params)
    }
    ...
    const [data, total] = await Promise.all([getData(), getTotal(params)])

the we can remove the if (params.pipeline) {... block from the countDocuments and implement the countAggregate like this:

  async countAggregate(params: ServiceParams) {
    const model = await this.getModel(params)
    const pipeline = params.pipeline || []
    const index = pipeline.findIndex((stage: Document) => stage.$feathers)
    const before = index >= 0 ? pipeline.slice(0, index) : []
    const after = index >= 0 ? pipeline.slice(index + 1) : pipeline

    const res = await model.aggregate([...before, ...after, { $count: 'count' }], params.mongodb).then((result) => result.toArray())
    return res?.[0]?.count ?? 0
  }

[daffl]

I believe this is fixed now via #3541