$modify queries broken in v5.5.1
alex-all3dp opened this issue · 37 comments
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?
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": {}
}
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.
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
}
]
}
I still have the same error as message before (left my code unchanged, didn't add any groupBy clause)
@alex-all3dp Please share you full example
@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',
},
}
}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.
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?
@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.
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.
@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"
@alex-all3dp I can't see any groupBy in your example. did you add it somewhere?
@dekelev no I didn't. it would seem odd to me to be required to add a groupBy as part of a simple modifier like the one from my example? Especially given that it worked as expected before and after an update in the minor version range, the implementation breaks. Is the requirement for using groupBy documented anywhere?
There's no requirement to add groupBy, but your error clearly complain about wrong use of GROUP BY, because PG & MySQL in latest versions, has a strict requirements that all non-aggregated columns in the SELECT will be part of the GROUP BY.
DISTINCT also acts as GROUP BY, so it's possible that distinct runs somewhere in the lib and is causing this error.
The lib runs DISTINCT only when $joinRelation is used or when the model have compound PK.
I don't see you used $joinRelation anywhere and I also don't see a compound PK in your model.
Anyway, I'll try to reproduce the issue on my side based on the examples you shared so far and proceed from there.
So it seems that COUNT by itself, which is an aggregate function, now requires that non-aggregated columns will not be included in the SELECT. COUNT basically runs GROUP BY under the hood.
The issue is that ObjectionJS itself is adding the extra id column, a non-aggregated column, to the SELECT.
Adding groupBy wherever count or countDistinct is used will workaround the issue, but will also break the total pagination field.
Using $eager instead of running withGraphFetched in a modifier, which is essentially the same thing, does not cause any error. This is a weird difference in behavior for the same action, but it seems to relate to some internal ObjectionJS behavior.
You will be able to workaround your issue by using $eager or by calling groupBy on the ModelName.id column together with the withGraphFetched call.
@alex-all3dp @nakedgun Let me know if you have any more suggestions or more use-cases that doesn't work with the strict-group-by branch.
If anything goes well, I'll release a new version during next week based on that branch.
Hey @dekelev . Thanks for your continued work on this! I checked out the strict-group-by branch with my api. Same essential problem though for me, e.g:
// model.js
test(builder) {
builder.select(
raw(
'someNumericColumn / 50 as test'
)
).groupBy('id') <-- required, or we error: "GROUP BY clause or be used in an aggregate function at character..."
},await app.service('offices').find({
query: {
$modify: ['test']
}
})
{
"total": 1, <-- wrong
"limit": 10,
"skip": 0,
"data": [
// correct (omitted)
]
}With this, no error, however res.total is wrong. I don't see how I can work around this issue when using a modifier. A more generic solution here might be to expose some public methods to build or customize the queries feathers-objection uses internally, e.g. Feathers-knex has createQuery() which is useful - since in theory, this could be used in a before hook to customize the count query and modify it for special use cases. For example, for my situation, I'd modify the count query to run in a subquery which fixes my issue. I don't believe this kind of workflow is possible with feathers-objection - though correct me if I'm wrong.
In the mean time, I have put in a hack which simulates this kind of developer workflow. I have a hook which detects the use of this modifier in find({}). When it does, I clone the original query and set the limit to something very high (i.e. 99999). I then take the number of results returned by this query (i.e. data.length) and monkey patch that to the actual count of the user's query. It's not efficient but I needed something to unblock my situation.
In a perfect world this would all be seamless, but failing that, I guess having some tools to customize the count query whilst making use of feathers-objection internals would be useful, since I suspect there might always be edge cases here. You know the library better than I do though - I'm not sure the best way to expose this, but that's my two cents. Hope it helps!
I have encountered the same issue and I wasn't willing to give up the cleanliness of the modifiers. So, as a workaround I added a hook to the relevant service that queries for the total.
I created a hook called getTotal and put it in the after --> find hook.
const getTotal = (model) => async (ctx) => {
const results = await model.query().page(1, 1);
ctx.result.total = results.total;
return ctx;
};
https://vincit.github.io/objection.js/api/query-builder/other-methods.html#page
ObjectionJS automatically adds id columns to the SELECT in some cases and then strip it out before returning the result.
This behavior seems to be the result of this issue and was released in v0.7.4.
The branch I've opened will only detect groupBy and use its columns in the SELECT instead of id columns or just *.
It's a very basic support for groupBy that will also work with these databases that has only_full_group_by restriction, like MySQL & PostgreSQL.
The service class can also be extended if needed to add any custom workarounds.
Any PR that can solve the issue without breaking any behavior is welcome.
Changes were released in v5.7
Thanks!
@dekelev I still struggle with the fact that a patch release introduces a breaking change that would require all $modify queries to add a groupBy in order to work.
The issue was introduced with the added countQuery in
feathers-objection/src/index.js
Lines 451 to 453 in 248fe0c
What exactly did this fix? The
total count works as expected, even without this line. Removing the block, fixes my issue.Any chance to get this change reverted?
In this issue, for example, using $modify with where id = 1 breaks the count.
I see. Well, I will just add the required groupBy clauses then I guess ;)
@dekelev I now use groupBy('id') in my modifiers but now encounter the other issue described in this thread, where the total count is 1.
I tried using eager instead of withGraphFetched as suggested above but without any luck. Is there a known solution for this issue? From the answers it seems that there isn't or am I missing something?
@dekelev I looked into these but if you refer to the custom hooks, that just seems overly complicated to me in order to work around a bug. I reverted back to 5.5.0 because it serves my current purpose best.
In general I still think the patch to 5.5.1 could have been handled differently, without introducing breaking changes. My suggestion would be to only enable the behaviour in
feathers-objection/src/index.js
Lines 451 to 453 in 248fe0c
Of course if there were a solution that fixes the issue in a way without breaking other queries that would be preferable to the configuration setting. But as it stands, v5.5.1 and above seem broken to me in regards to handling
$modify queries.Just want to add that I still love the library and greatly appreciate all the work an effort you put into it :)
@dekelev Has there been any resolution on this issue?
@egluhbegovic @dekelev I just played around with v7.0.0 and the issue still persists unfortunately.
@dekelev I looked into this a bit more in detail and I think I understand the issue better than before.
Am I correct in saying that the change in v5.5.1 fixes the wrong total when modifiers are used which filter the result set?
If yes, it could make sense to have the choice to not apply countQuery() to the $modify query, in order to avoid the issue with groupBy and the total in objection.js.
Would you be open for a PR that introduces a new optional parameter modifierFiltersResults that could be used for that purpose?
Suggested change:
if (query && query.$modify && params.modifierFiltersResults !== false) {
this.modifyQuery(countQuery, query.$modify);
}This check would not break any implementations which rely on the current implementation but would allow to bypass applying the count to the $modify query, for instances where we know that the total is not affected by the modifier.
For instance the parameter could be added via a find hook, similar to the other workaround mentioned here but it would not rely on querying all results for a 2nd time in order to get the previous count.
Let me know what you think. Also if you don't have the time but would be open for a PR, I'd be happy to draft one up.
Best
Alex
Thanks @alex-all3dp , I will check this out.
@alex-all3dp, I'm not sure I understand the issue you're having that led you to a solution that prevents running modifyQuery on the count query, which leads to invalid total value.
If pagination is not needed, then you can simply set params.paginate to false in your service call and avoid the count query in the first place.
@dekelev Well, the total is incorrectly calculated by the count for modifyQuery, if the modifier uses groupBy. In that case it always returns 1as thetotal`, as discussed in this thread.
So the new flag is mainly a workaround for the issues in objection.js / knex.js that you referenced. It allows to disable the countQuery being applied to the modifyQuery, which we can safely do if we are certain that the modifier does not filter the result set (e.g. by using where).
If the modifier simply enriches the result set via eager loading, the total of the original query will be correct and there is no need to apply count to the modifyQuery. So it would allow users who run into this issue to disable the behaviour that results in a wrong total for modifiers with groupBy if (and only if) they are certain that the respective modifier does not change the total of the original query.
Does that make sense? Unfortunately I can't simply disable pagination,.because it is required for our use case.
The alternative would be to run another count query in a hook, as suggested in #102 (comment), but in that case we would need to run an additional query for every request. With the optional "workaround parameter" we would achieve the same thing without the need for an additional query.
Looking forward to your feedback!
Best
Alex
@alex-all3dp I see your point now, but the tests you've added doesn't seems to reflect your use-case. Can you check them out please? For example, I would expect to see total=data.length in the modifierFiltersResults: false test.
Also, please add another test with params without the modifierFiltersResults operator, to see that nothing breaks. thanks!
@dekelev I was also unhappy this part of the PR, as it as a bit odd to rely on a test that intentionally checks for the incorrect result.
I intended to add a test for exactly the behavior that breaks in our use case (.withGraphFetched().groupBy('id'). But the issue with the count breaking for groupBy usage does not occur with SQLite, so the test would not really assert that the operator fixes the actual issue.
All "old" tests are without the modifierFiltersResults, so it already shows that nothing breaks, right?
I will look into improving the PR a bit with regards to the test suite. Thank your for the feedback!
@dekelev Makes sense. I updated the PR by using the new withRelationAndGroupBy modifier in the tests that I added (incl. one where modifierFiltersResults is undefined.
Actually the issue was reproducable on SQLite, so I did not add Postgres to the description. Let me know if this is ok for you.
Thanks @alex-all3dp, it looks great! I'll release it tomorrow.