Skip to content
This repository was archived by the owner on Sep 2, 2025. It is now read-only.

Remove GroupBy for Find Count queries#152

Closed
gmercey wants to merge 9 commits intofeathersjs-ecosystem:masterfrom
gmercey:master
Closed

Remove GroupBy for Find Count queries#152
gmercey wants to merge 9 commits intofeathersjs-ecosystem:masterfrom
gmercey:master

Conversation

@gmercey
Copy link
Contributor

@gmercey gmercey commented May 25, 2021

An update for PR #150 as it wasn't clearing groupBy introduced by $modify queries

@gmercey
Copy link
Contributor Author

gmercey commented May 25, 2021

I fixed the tests params.modifierFiltersResults=true applies count from modify query and params.modifierFiltersResults=undefined applies count from modify query as the query response for both tests was as follow :

{
  total: 1,
  limit: 2,
  skip: 0,
  data: [
    Company {
      id: 41,
      name: 'Google',
      ceo: 1,
      size: 'medium',
      jsonObject: null,
      jsonArray: null,
      jsonbObject: null,
      jsonbArray: null,
      employees: [Array]
    },
    Company {
      id: 42,
      name: 'Apple',
      ceo: null,
      size: 'large',
      jsonObject: null,
      jsonArray: null,
      jsonbObject: null,
      jsonbArray: null,
      employees: [Array]
    }
  ]
}

Clearing the groupBy for the count query fixes the issue

@dekelev
Copy link
Member

dekelev commented May 26, 2021

Hi @alex-all3dp , can you please review this PR? it may break the behavior you've added here.

@gmercey I'm reverting PR #150 until this PR is resolved.

@alex-all3dp
Copy link
Contributor

alex-all3dp commented May 27, 2021

@dekelev @gmercey
The tests that were changed in this PR are actually the ones I added to ensure backwards compatibility for the previous behaviour, where the total was the count result of the the applied GROUP BY clause.
If this changes now, it may be breaking expected behaviour for other users?

Regardless of this, I tested this branch with my setup and it breaks some find queries with modifiers again:

"DBError: select count("id") as "total", "TableName"."id" from "TableName" where (("TableName"."id" = $1)) - column "TableName.id" must appear in the GROUP BY clause or be used in an aggregate function

@gmercey
Copy link
Contributor Author

gmercey commented May 27, 2021

@alex-all3dp with which DB Engine do you get the error? Maybe we need to have different behaviour depending on the engine used?

@alex-all3dp
Copy link
Contributor

@alex-all3dp with which DB Engine do you get the error? Maybe we need to have different behaviour depending on the engine used?

@gmercey PostgreSQL

@gmercey
Copy link
Contributor Author

gmercey commented May 27, 2021

We could wrap the clear('groupBy) around if (params.modifierFiltersResults !== false) to accommodate? Because the example you are giving is not a proper count query for pagination as it returns a non-aggregated column whereas the count query should return 1 value

@alex-all3dp
Copy link
Contributor

We could wrap the clear('groupBy) around if (params.modifierFiltersResults !== false) to accommodate? Because the example you are giving is not a proper count query for pagination as it returns a non-aggregated column whereas the count query should return 1 value

I guess that could work :)

@gmercey
Copy link
Contributor Author

gmercey commented May 27, 2021

@dekelev @alex-all3dp PR updated

@alex-all3dp
Copy link
Contributor

@gmercey The if clause does not fix the issue. My bad, I didn't look closely enough the first time.

So also without the if clause my find queries with modifiers still work as expected. The problem I am having on this branch is that service.get with a.modifier (GET /entity/:id?$modify=myModifier) throws the error message. Any ideas?

@gmercey
Copy link
Contributor Author

gmercey commented May 28, 2021

@alex-all3dp is it failing because the service.get is a wrapper for _find :

  _get (id, params = {}) {
    // merge user query with the 'id' to get
    const findQuery = Object.assign({}, { $and: [] }, params.query);
    findQuery.$and.push(this.getIdsQuery(id)); // BUG will fail with composite primary key because table name will be missing

    return this._find(Object.assign({}, params, { query: findQuery }))
      .then(page => {
        const data = page.data || page;

        if (data.length !== 1) {
          throw new errors.NotFound(`No record found for id '${id}'`);
        }

        return data[0];
      });
  }

What we should do in this case, is to pass paginate=false as a param because we don't care about pagination in that case.

@dekelev dekelev closed this Nov 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants