Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverse relations fetching #1036

Open
Forceres opened this issue Jun 25, 2024 · 11 comments
Open

Reverse relations fetching #1036

Forceres opened this issue Jun 25, 2024 · 11 comments

Comments

@Forceres
Copy link

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
Copy link
Member

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

@Forceres
Copy link
Author

Forceres commented Jun 25, 2024

Why old PR was never took off?

@Forceres
Copy link
Author

@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
Copy link
Member

@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
Copy link
Author

Forceres commented Jun 25, 2024

@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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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

@dantownsend
Copy link
Member

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
Copy link
Author

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
Copy link
Author

Forceres commented Jul 4, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants