$modify queries broken in v5.5.1

[alex-all3dp]

Updating to 5.5.1 with objection 2.2.0 and pg 8.2.1 results in the following error message for GET requests with $modify query:

"select count(distinct("tableName"."columnName", "tableName"."columnName")) as "total", "tableName"."columnName" from "tableName" - column "tableName.columnName" must appear in the GROUP BY clause or be used in an aggregate function"

the modifier looks something like this:

static get modifiers() {
    return {
      includeRef: (builder: QueryBuilder<TableName>) =>
        builder
          .withGraphFetched('reference')
          .modifyGraph('reference', (refBuilder: QueryBuilder<RefTable>) =>
            refBuilder.select('columnName')
          ),
    }
  }

Does the modifier need to be adapted in any way? It would be odd in my eyes, given that the update is declared as a patch release and should not break existing implementations like this?

[nakedgun]

Getting the same problem with pg/5.5.1 (caused by the fix for #98). For me, I have a modifier which uses a selectRaw statement (long,lat) to compute a new column (distance), which now errors out with:

column \"posts.lat\" must appear in the GROUP BY clause or be used in an aggregate function",

If I update the modifier to select(['id','long','lat'] and groupBy(['id,'long','lat], it will work. But as soon as you pass another field to the query, e.g:

await app.service('offices').find({
          query: {
            $modify: ['maxDistance', 37.45845, -122.16324, 100],
            isOpen: true
          }
        })

You'll get this error: index.js@L436

{
  "name": "GeneralError",
  "message": "Cannot read property 'total' of undefined",
  "code": 500,
  "className": "general-error",
  "errors": {}
}

[dekelev]

Thanks for the report. I was able to reproduce the issue and added this test: $modify and paginate with no results.

I've released v5.5.2 to address this by simply adding a check on the count query result before reading the total out of it.

Let me know if it doesn't solve your issue.

[nakedgun]

Thanks for the update @dekelev . Unfortunately I cannot get this to work still - the total is still incorrect. Here's a simplified version of the repro (5.5.2/pg):

const { raw } = require('objection');
// model.js
table.increments('id');
table.integer('someNumericColumn').notNullable();
// modifiers
    return {
      test(builder) {
        builder.select(
          raw(
            'someNumericColumn / 50 as test'
          )
        );
      },

const res = await app.service('offices').find({
          query: {
            $modify: ['test']
          }
        })

returns:

{
  "name": "GeneralError",
  "message": "select count(\"offices\".\"id\") as \"total\", someNumericColumn / 50 as test from \"offices\" - column \"offices.someNumericColumn\" must appear in the GROUP BY clause or be used in an aggregate function",
  "code": 500,
  "className": "general-error",
  "errors": {}
}

Alright - add the groupBy statement to the modifier as it wants:

test(builder) {
        builder.select(
          raw(
            'someNumericColumn / 50 as test'
          )
        ).groupBy('test')
      },

Result:

{
  "total": 1, <-- wrong
  "limit": 10,
  "skip": 0,
  "data": [
    {
      "test": 0.7743260192871094
    },
    {
      "test": 0.6283399963378906
    },
    {
      "test": 0.8328759765625
    },
    {
      "test": 0.6338959884643555
    },
    {
      "test": 0.8557260131835938
    },
    {
      "test": 0.6644419860839844
    },
    {
      "test": 0.84716796875
    },
    {
      "test": 0.7741739654541016
    },
    {
      "test": 0.8791100311279297
    },
    {
      "test": 0.6312220001220703
    }
  ]
}

[alex-all3dp]

I still have the same error as message before (left my code unchanged, didn't add any groupBy clause)

[dekelev]

@alex-all3dp Please share you full example

[alex-all3dp]

@dekelev here is one of the simpler examples that currently fails:

GET /ModelName?$modify=includeRef

Error:

"select count("ModelName"."id") as "total", "ModelName"."id" from "ModelName" - column "ModelName.id" must appear in the GROUP BY clause or be used in an aggregate function"

Model file:

class ModelName extends Model {

id: string,

static get modifiers() {
    return {
      includeRef: (builder: QueryBuilder<ModelName>) =>
        builder.withGraphFetched('ref'),
    }
  }

  static get relationMappings() {
    return {
      ref: {
         relation: Model.ManyToManyRelation,
        modelClass: 'Ref',
        join: {
          from: 'ModelName.id',
          through: {
            from: 'JoinTable.modelNameId',
            to: 'JoinTable.refId',
          },
          to: 'Ref.id',
        },
    }
}

[dekelev]

Thanks guys! so it seems that the issue is because we now run the ObjectionJS modify method with the count queries and as a result, if groupBy & select are used inside a modifier method, queries will fail since we add the ID fields in the select automatically and it doesn't work with sql_mode=only_full_group_by.

[nakedgun]

Hey @dekelev is there any plan to fix this? I actually see two issues here:

The first issue: must appear in the GROUP BY clause or be used in an aggregate function is understandable - but fixed by adding a groupBy statement referencing the table's PK, for example.

The second issue, which happens when you add the GROUP BY statement, is that the wrong total is returned. That's because COUNT + GROUP BY is counting the number of rows within each group, not the total number of rows returned. One solution here is to put everything into a subquery and count the result of that.

An example of what I mean:

// existing approach - find() with a modifier using groupBy returns incorrect count
// under the hood, this is basically the SQL which is being run
select count("offices"."id") as "total"
from "offices" 
group by "id"

total
1
1
1
1
1
// total will be 1 (expected: 5)

// subquery method - works
select count(*) as "total" from (select count("offices"."id") as "total"
from "offices" 
group by "id") groups

total 
5

This is similar to this issue here: feathersjs-ecosystem/feathers-knex#192

I'm not a DB expert but if this is a valid approach could we expose this as a service option to utilize subqueries for counts?

[dekelev]

@nakedgun The count issue that you're describing is mentioned in the ObjectionJS docs here & here.

Regarding the error on fields missing from the groupBy statement - I do plan to check for an automated solution, because I'm not even sure that you can workaround the issue manually using the $select query operator.

I do not have much free time these days, so I'm keeping this issue opened until I'll be available for this or will get a relevant PR.

[dekelev]

I've added basic support for modifiers with groupBy in the strict-group-by branch.

Can you please check this with your use-cases?

Notice that when you use $eager or $joinEager, ObjectionJS will automatically add the id column to the SELECT columns list. I'm not sure why it does that, but when only_full_group_by restriction is enabled, the only workaround is to add the id column to the groupBy columns within the modifier or to use $joinRelation without $eager if you don't need the relations' columns in return.

[alex-all3dp]

@dekelev thank you for your effort and continued support on this.
Unfortunately I still receive errors with the setup described above:
"select count("Table"."id") as "total", "Table"."id" from "Table" where "Table"."id" = $1 - column "Table.id" must appear in the GROUP BY clause or be used in an aggregate function"