Wrong total if params.knex is set

If params.knex is set, params.query is bypassed, and the knex query is executed insted. However, for the count query (ie total), the params.query is taken into account (instead of params.knex), and results in a wrong total.

I think this is inconsistant, shouldn't params.knex, if set, should be used to compute the count query?

That makes sense. Can you try if it works changing https://github.com/feathersjs/feathers-knex/blob/master/src/index.js#L167 to

let countQuery = (params.knex || this.db(params)).count(`${this.id} as total`);

No, that does not work. params.knex can be a complicated request, wheres this.db(params)) is just select * from table. The count on params.knex is not working.

What is .count returning for a custom query? If that doesn't work I don't see how we can support total at all in that case.

Ok, you should precise that pagination (total, limit, skip) is not supported when params.knex is set.
I close the issue.

I'd leave it open. We have to check if the query is customized and skip the count. Do you have an example customized query where the count is wrong?

Here is an example.

The hook code:

return async function (hook) {
  const { $search } = hook.params.query;

  if ($search) {
    // TODO total is wrong
    Object.assign(hook.params, { query: _.omit(hook.params.query, '$search') });

    const query = this.createQuery({ query: hook.params.query });

    query.where(function () {
      this
        .where('firstname', 'like', `%${$search}%`)
        .orWhere('lastname', 'like', `%${$search}%`);
    });

    Object.assign(hook.params, { knex: query });
  }

  return hook;
};

The result:

{
    "total": 1001,
    "limit": 10,
    "skip": 0,
    "data": []
}

I haven't tried it yet, but shouldn't params.knex.count(${this.id} as total); return the correct total in this case?

Not really, as $search is not in the query params anymore and you can't really add it later for count since you no longer have the info. I suggest we do something like:

  _find (params, count, getFilter = filter) {
    const { filters } = getFilter(params.query || {});
    const q = params.knex || this.createQuery(params);
    // define q2 before $limit or $skip
    const q2 = q.clone();

    // Handle $limit
    if (filters.$limit) {
      q.limit(filters.$limit);
    }

    // Handle $skip
    if (filters.$skip) {
      q.offset(filters.$skip);
    }

    let executeQuery = total => {
      return q.then(data => {
        return {
          total: parseInt(total, 10),
          limit: filters.$limit,
          skip: filters.$skip || 0,
          data
        };
      });
    };

    if (filters.$limit === 0) {
      executeQuery = total => {
        return Promise.resolve({
          total: parseInt(total, 10),
          limit: filters.$limit,
          skip: filters.$skip || 0,
          data: []
        });
      };
    }

    if (count) {
      let countQuery = this.db(params).count(`${this.id} as total`);
      // use the exact same query as subquery and count
      countQuery.from(q2.as(`${this.table}`));

      return countQuery.then(count => count[0].total).then(executeQuery);
    }

    return executeQuery().catch(errorHandler);
  }

Just ran into this also. If I add a simple where clause to the query in a Before hook (as recommended by the readme) the total property returned is still the total number of records, not the number of records that would be returned by the filtered query.

I'm using this as a simple case:

    find: [
      function(context) {
        const query = this.createQuery({query: context.params.query});
        query.where('details', 'LIKE',  '%this%');
        context.params.knex = query;
        return context;
      }
    ],

response:

{
    "total": 5,
    "limit": 10,
    "skip": 0,
    "data": [{
        "id": 4,
        "details": "This is a workorder",
    }]
}

Here's what I've found.

I need this to work:

class CustomKnexService extends knexService {
  createQuery (params = {}) {
    const { pipe, interest } =  pickBy(params.query, jsonKeys);
    const q = super.createQuery({
      query: omit(params.query, jsonKeys),
    });

    if (pipe !== 'all') {
         // custom stuff here
    }
    return q;
  }

But pagination breaks, unless you also overide _find:

    if (count) {
      console.log('countQuery', omit(params.query, [...jsonKeys, '$sort']))
      let countQuery = params.knex || this.createQuery({
        query: omit(params.query, ['$sort']),
      }).clearSelect().count(`${this.id} as total`);

      this.knexify(countQuery, omit(params.query, ['$sort']));

      return countQuery.then(count => count[0].total).then(executeQuery);
    }

omit(params.query, ['$sort']), and clearSelect() being imporant, omit being from lodash.

Why doesn't count use params.knex || this.createQuery ? That makes getting a count for a slightly customized query into a nightmare.

No reason. A pull request to change that would be very welcome.

Fix count query when using params.knex

Fix count query when using params.knex (#145)

Hi guys,

I still get a wrong total when I combine a sub query with a $sort:

const { checkContext } = require('feathers-hooks-common');
const { omit } = require('lodash');

module.exports = () => {
  // eslint-disable-line no-unused-vars
  return async context => {
    checkContext(context, 'before');

    const knex = context.app.get('knexClient');
    const { $search } = context.params.query;

    if ($search) {
      Object.assign(context.params, {
        query: omit(context.params.query, '$search')
      });

      const query = context.service.createQuery({
        query: context.params.query
      });

      const subQuery = knex
        .select('id')
        .from('customers')
        .where('firstName', 'ilike', `%${$search}%`);

      query
        .where('customerId', 'in', subQuery)
        .groupBy('orders.id', 'orders.code', 'orders.totalInCents');

      Object.assign(context.params, { knex: query });
    }

    return context;
  };
};

I get a total of 1 while there are 5 documents. My request is: localhost:3030/orders?$sort[totalInCents]=-1&$search=roel

Navigation Menu

Repository navigation

Wrong total if params.knex is set #121

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Wrong total if params.knex is set #121

Description

Activity

daffl commented on Oct 6, 2017

sylvainlap commented on Oct 18, 2017

daffl commented on Oct 18, 2017

sylvainlap commented on Oct 19, 2017

daffl commented on Oct 19, 2017

sylvainlap commented on Oct 20, 2017

daffl commented on Oct 20, 2017

paflopes commented on Nov 2, 2017

green3g commented on Jan 23, 2018

carcinocron commented on Feb 2, 2018

daffl commented on Feb 2, 2018

roelvan commented on May 3, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions