-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Self refrencing joins with aggregate_rows #606
Conversation
Can you provide a test-case that shows the broken behavior? |
from peewee import *
from unittest import TestCase
db = SqliteDatabase(':memory:')
class SomethingThatReferencesItself(Model):
name = CharField()
bro = ForeignKeyField('self', null=True, related_name='bros')
class Meta:
database = db
class EagerLoadSelfReferencingJoinPleaseYesOkay(TestCase):
@classmethod
def setUpClass(cls):
SomethingThatReferencesItself.create_table()
bob = SomethingThatReferencesItself.create(name='bob')
SomethingThatReferencesItself.create(name='joe', bro=bob)
SomethingThatReferencesItself.create(name='rofl guy', bro=bob)
def test_get_aggregate_bro(self):
Bro = SomethingThatReferencesItself.alias()
joe = (SomethingThatReferencesItself
.select(SomethingThatReferencesItself, Bro)
.join(Bro, on=(SomethingThatReferencesItself.bro == Bro.id).alias('bro'))
.where(SomethingThatReferencesItself.name == 'joe')
.aggregate_rows()
.get())
self.assertIn('bro', joe._obj_cache)
self.assertEqual(joe._obj_cache['bro'].name, 'bob') I'm using Model._obj_cache to verify that the data was eagerly loaded. Because ModelOptions.rel_for_model does not support joining via ModelAlias, this will not work. I'm not aware of a way to eagerly load a parent with its children using a self-referencing join, for example, loading the above bob with his bros, but you're the master so perhaps you'd know how to write/test that. |
Hmm, I think the fix for this may be more involved. I'm seeing some issues when self-references are queried using either |
Added support for self-joins in |
Awesome work! Looking forward to the next release! As you can tell, I use |
@arrowgamer not quite fixed yet, still need to get the |
@coleifer Yes, I saw your commit was only for |
So from e5ce2bd I think that the basic use-case should now work. Honestly due to the complexity of the |
So what would be the syntax of using Edit: Nevermind, took a look at your testcase =) Thanks! |
The example test case shows, but say you have class Category(Model):
name = CharField()
parent = ForeignKeyField('self', related_name='children') You would write: Child = Category.alias()
query = (Category
.select(Category, Child)
.join(Child, JOIN.LEFT_OUTER, on=(Category.id == Child.parent).alias('kids'))
.order_by(Category.id, Child.id)
.aggregate_rows())
for category in query:
print category.name
for child in category.kids:
print ' -', child.name A caveat is that you must specify an alias in the join condition. |
Thanks a bunch for the explanation! Once released, I'll report any issues if I find any, as I'm already using aggregated self-referencing joins. |
Hi again. I was trying to use aggregate_rows to eagerly load a model with a self referencing join, and I found that I could not do so unless the rel_for_model function was altered as per this pull request.
The reason is that rel_for_model doesn't appear to support joins via ModelAlias.