piccolo-orm / piccolo

A fast, user friendly ORM and query builder which supports asyncio.
https://piccolo-orm.com/
MIT License
1.45k stars 91 forks source link

Reverse relations fetching #1036

Open Forceres opened 4 months ago

Forceres commented 4 months ago

FEATURE REQUEST: Is it a problem to add reverse relation fetching (conceptual or technical)? It would be nice to see something like that:

   class Student(Table, tablename="students"):
       name = Varchar(length=64)
       teacher_id = ForeignKey(Teacher)

   class Teacher(Table, tablename="teachers"):
       name = Varchar(length=64)

   await Teacher.objects(Teacher.all_related())
sinisaos commented 4 months ago

@Forceres Here is an old reverse lookup PR that never took off. Here is also workaround to get same result with existing code.

Forceres commented 4 months ago

Why old PR was never took off?

Forceres commented 4 months ago

@Forceres Here is an old reverse lookup PR that never took off. Here is also workaround to get same result with existing code.

That workaround is too ineffective

sinisaos commented 4 months ago

@Forceres That PR was never properly reviewed by the Piccolo author. I don't remember what the reasons were for that (bad API design or something else). Everything worked as expected at the time PR was made. Everything should work now also, but I'm not sure if it does after all the changes that have been made in Piccolo.

Forceres commented 4 months ago

@Forceres That PR was never properly reviewed by the Piccolo author. I don't remember what the reasons were for that (bad API design or something else). Everything worked as expected at the time PR was made. Everything should work now also, but I'm not sure if it does after all the changes that have been made in Piccolo.

I don't see any reasons for it to be declined, the only one solution without that PR is to use raw queries builder (written from zero to hero)

sinisaos commented 4 months ago

I don't see any reasons for it to be declined, the only one solution with that PR is to use raw queries builder (written from zero to hero)

I agree with you, but you have to wait @dantownsend for an explanation. That PR is pretty similar to how the Piccolo M2M was built.

Forceres commented 4 months ago

I don't see any reasons for it to be declined, the only one solution with that PR is to use raw queries builder (written from zero to hero)

I agree with you, but you have to wait @dantownsend for an explanation. That PR is pretty similar to how the Piccolo M2M was built.

Someone has already tried to wait for him) But, anyway, there is no choice, forced to wait

Forceres commented 4 months ago

Modern ORM, which doesn't have basic functionality like reverse relations

dantownsend commented 4 months ago

I know it's frustrating when PRs don't get merged in, but it's just a matter of resources and time - I generally have to prioritise stuff I need for client work, as that's what pays my bills. Loads of my own PRs haven't been merged in either, because unless I'm 100% happy with it, and willing to support a feature in the future, I'm hesitant about merging it in.

I do come back to old PRs - so if they haven't been merged they're not 'dead'.

But I agree, this would be a very nice feature to have. I generally work around it with something like this:

teacher = await Teacher.objects().get(name="Alice")
students = await Student.objects().where(Student.teacher == teacher)
Forceres commented 4 months ago

I know it's frustrating when PRs don't get merged in, but it's just a matter of resources and time - I generally have to prioritise stuff I need for client work, as that's what pays my bills. Loads of my own PRs haven't been merged in either, because unless I'm 100% happy with it, and willing to support a feature in the future, I'm hesitant about merging it in.

I do come back to old PRs - so if they haven't been merged they're not 'dead'.

But I agree, this would be a very nice feature to have. I generally work around it with something like this:

teacher = await Teacher.objects().get(name="Alice")
students = await Student.objects().where(Student.teacher == teacher)

Got you. Hope you find time to resolve it. That approach has one problem - it is 2 queries instead of one with joins

Forceres commented 4 months ago

I know it's frustrating when PRs don't get merged in, but it's just a matter of resources and time - I generally have to prioritise stuff I need for client work, as that's what pays my bills. Loads of my own PRs haven't been merged in either, because unless I'm 100% happy with it, and willing to support a feature in the future, I'm hesitant about merging it in.

I do come back to old PRs - so if they haven't been merged they're not 'dead'.

But I agree, this would be a very nice feature to have. I generally work around it with something like this:

teacher = await Teacher.objects().get(name="Alice")
students = await Student.objects().where(Student.teacher == teacher)

Any news?