tortoise / tortoise-orm

Familiar asyncio ORM for python, built with relations in mind
https://tortoise.github.io
Apache License 2.0
4.55k stars 374 forks source link

Annotate on recursive relation giving error on PostgreSQL #333

Closed mojimi closed 4 years ago

mojimi commented 4 years ago

Hello, I'm getting this PG error while trying to do a count on a recursive relationship.

tortoise.exceptions.OperationalError: column "task.estimated_start_date" must appear in the GROUP BY clause or be used in an aggregate function

class Task(BaseModel):
    mother = fields.ForeignKeyField('models.Task', null=True, related_name="children")
#Some code later...
tasks = await Task.filter(mother=None).annotate(children_count=Count("children"))

Am I doing something wrong?

What I'm trying to achieve with my query, is a hierarchical view of the tasks, first I'm getting the "outer" ones, and then checking if they have any children so then I will display a dropdown arrow that will asynchronous load the children and so on.

grigi commented 4 years ago

That code looks right to me. :thinking: Oh, I think I know what the error is about. The sub-task (lets call it task2, as in INNER JOIN task as task2) is being Counted as a whole, whereas you probably intend to count the instances, so we should do a Count("children.id")?

I think this is going to be a common mistake, so we should consider auto-fixing the aggregation to always refer to <field>.pk if field is a relation?

mojimi commented 4 years ago

I'm not versed enough in the project's source code to opinate, I just copied the example on the docs.

Using Count("children.id") gave me a key error, is there any temporary fix I can use?

grigi commented 4 years ago

Did that not work? Argh. So my guess is wrong. I'll look at this in the coming days, I have to go sleep now. Possibly annotation is not using the same subquery resolver as filter? @lntuition Is this one of the things you mentioned as a motivation for the F & Q refactor?

mojimi commented 4 years ago

@grigi no problem! sleep well

For now I'll just pre-calculate the child_count

grigi commented 4 years ago

Built a test for this, got the same error:

await Employee.filter(name="Root").annotate(num_team_members=Count("team_members"))

The SQL generated looks like:

SELECT "employee"."manager_id","employee"."name","employee"."id",COUNT("employee2"."id") "num_team_members"
FROM "employee"
LEFT OUTER JOIN "employee" "employee2" ON "employee2"."id"="employee2"."manager_id"
WHERE "employee2"."name"='Root'
GROUP BY "employee2"."id"

:rofl: The last bit is all confused, instead of differentiating between employee and employee2 everything now has the 2... This is almost identical to the bug we had with filtering on a self-referential entry.

grigi commented 4 years ago

Released v0.16.3 & v0.15.21 with this fix. Please test

mojimi commented 4 years ago

Released v0.16.3 & v0.15.21 with this fix. Please test

Thanks for the quick response, I still got another error though (on 0.16.3)

await Task.filter(mother=None).annotate(children_count=Count("children"))

image

grigi commented 4 years ago

!! Is this the same code? That is one of those errors that should not be possible, hence my confusion. I suspect a type confusion somewhere...

Could you please provide extra context re that error please?

mojimi commented 4 years ago

!! Is this the same code? That is one of those errors that should not be possible, hence my confusion. I suspect a type confusion somewhere...

Could you please provide extra context re that error please?

Ah I have a hunch, my dependencies were all outdated, lemme try that

mojimi commented 4 years ago

@grigi Yep, just updated dependencies and it works now, thanks for the very quick fix!

mojimi commented 4 years ago

@grigi Ah... I did find out an issue though...

When I do

mother_task = Task()
await mother_task.save()
child = Task(mother=mother_task)
await child.save()
#...
children = await Task.filter(mother=mother_task).annotate(children_count=Count("children"))

print(children[0].children_count) #Gives 1
print(await children[0].fetch_related('children')) #Gives none

children_count is set as 1 even though a task has no children

Do you think this could be an error on my part?

grigi commented 4 years ago

Ok, I confirmed it in a local test.

grigi commented 4 years ago

It should be fixed in next release. You can test out develop doing a pip install https://github.com/tortoise/tortoise-orm/archive/develop.zip

grigi commented 4 years ago

Release 0.16.4 os out with the fix, please confirm.

mojimi commented 4 years ago

Fixed