05bit / peewee-async

Asynchronous interface for peewee ORM powered by asyncio
http://peewee-async-lib.readthedocs.io
MIT License
733 stars 100 forks source link

Async ForeignKeys #15

Open mrbox opened 8 years ago

mrbox commented 8 years ago

At the moment if you have model

class Model(peewee.Model):
    other_model = peewee.ForeignKey(OrderModel)

using it like this

model_instance.other_model

will trigger sync query.

I've already have this working so this issue is just to keep contribution in mind ;)

rudyryk commented 8 years ago

I doubt if we need a special interface for that, at this time we may just join related fields with main query. This is actually preferred practice for both sync and async code.

It may be ok to have one as last resort, but most times it's just better to add join() calls.

mrbox commented 8 years ago

I found it extremely unconvinient while developing- that's why I've created AFK- and also AsyncGenericForeignKey. It probably could be improved but you'll see when I commit it.

rudyryk commented 8 years ago

Here starts mixing of concepts, which I strongly would like to avoid.

To be clear: peewee layer is responsible for constructing queries. Async layer is responsible for executing queries.

When you introduce AsyncGenericForeignKey you're adding execution logic to models definition. This is absolutely ok if it does fit your needs, but for peewee-async project I think we need an extremely clean and simple design. The tradeoff is more verbose code for some cases.

rudyryk commented 8 years ago

Async layer is responsible for executing queries.

Considering that, I'd think of adding get_related() function in addition to get_object(). Or may be get_object() should perform related object lookup if first argument is relation. Just draft ideas.

mrbox commented 8 years ago

I disagree- peewee-async is Asynchronous interface for peewee ORM which mean for me that it should replace execution layer(as you pointed) and allow code to be compatible with peewee as much as possible. Which mean for me that I should be able to use subobject.object construction without bigger issues(I come from djagno world where it's very common construction and verbose just enough)- and replacing it by join and filter is for me big issue as it makes code readability worse. Of course, I can create a project on top of peewee-async to keep AFK/AGFK/whatever I'll find needeed but I think that it will create too big fragmentation of projects.

About When you introduce AsyncGenericForeignKey you're adding execution logic to models definition - already GenericForeignKey introduces execution logic to models and that's why it's placed in playhouse which is separate module but shipped with peewee.

rudyryk commented 8 years ago

Well :) I have to disagree here :) Models define data structure, GenericForeignKey in Django defines generic relation - model A has relation to arbitrary model X, I'm ok with that. And what is asynchronous field? :) Databases can't have sync columns as well as async columns.

rudyryk commented 8 years ago

For example Django have FileField for arbitrary files and extended ImageField for image files, and that's also all about data types, not about how request is performed.

rudyryk commented 8 years ago

I believe there's a way to add some sugar for simplifying user's code, but I'd leave models definition as is. As models can be reused in other apps like Flask-Admin etc. which is synchronous.

rudyryk commented 8 years ago

replacing it by join and filter is for me big issue as it makes code readability worse

I've solved it by providing single shortcut in my models like that:

class Page(Model):
   user = ForeignKeyField(User)
   # ...

   @classmethod
   def extended(cls):
      return self.select(Page, User).join(User)

and just call Page.extended() instead of Page.select()

Async calls by nature are very close to network API calls with JSON interface, where you just get objects IDs and have to make separate calls for fetching relations.

mrbox commented 8 years ago

Well, ok then- I'll close this issue and maybe make separate repo for extensions sometime later. I understand your reasoning but for me it's creating traps for yourself- if you once forget about calling .extended() and then you try to fetch page.user, you're stuck with sync IO which kills performance. With AsyncField it'll blow up in your face because you'll get error that coroutine doesn't have attribue foo ;)

rudyryk commented 8 years ago

To prevent sync requests there's a special flag database.allow_sync = False or context manager sync_unwanted(). They are designed specially for such cases. So if you don't expect sync query but it happens, then exception will raise.

rudyryk commented 8 years ago

And I'm sure for now, there's no need to rewrite everything to async, just no need, btw - great reading on topic: http://techspot.zzzeek.org/2015/02/15/asynchronous-python-and-databases/ Flask-Admin is an example of useful and awesome project, many will continue to use it, and it just doesn't need to be async. That's main reason to have sync-compatible interface and have async just as extension.

mrbox commented 8 years ago

I completely see your point and I agree that it's not necessary to use async FKs- but for my case I need more convenience so it's quite useful- and I don't use Flask-Admin so can safely include some deeper async concepts ;) But- there is something good waiting for your eyes- https://github.com/05bit/peewee-async/pull/19 ;)

rudyryk commented 8 years ago

Thanks a lot for PR! :) I have some vision on cleaning up some things, I hope to do that soon and release an update.

mrbox commented 8 years ago

Not a problem- I had to solve it because I can't use any other connection than pooled in my project and I really need transactions ;) I didn't have time to create one base class for AsyncConnection classes and I would find it pretty useful to keep some interface

jackfischer commented 8 years ago

at this time we may just join related fields with main query.

@rudyryk, would you be able to give an example of what this looks like? I cannot find anything related to it in the documentation :(

rudyryk commented 8 years ago

@jackfischer Hello Jack!

I mean something like that:

database = peewee.Proxy()
objects = peewee_async.Manager(database)

class User(peewee.Model):
    username = orm.CharField(max_length=100, unique=True)
    # ...
    class Meta:
        database = database

class Post(peewee.Model):
    user = orm.ForeignKeyField(
        User, index=True, related_name='posts', on_delete='CASCADE')
    # ...
    class Meta:
        database = database

async def get_post(self, *args, **kwargs):
    """Get post by lookup.
    Example:
        post = await get_post(id=1)
    """
    query = Post.select(Post, User).join(User).switch(Post)
    try:
        obj = await objects.get(query, *args, **kwargs)
    except Post.DoesNotExist:
        obj = None
    return obj

Is't not simple part in peewee but you can see more examples for .join() queries in docs: http://docs.peewee-orm.com/en/latest/peewee/querying.html#joining-tables

Also take a look at .alias(), it may be helpful if you have multiple foreign keys for one model: http://docs.peewee-orm.com/en/latest/peewee/querying.html#using-model-aliases

jackfischer commented 8 years ago

Thank you! I'm new to peewee and peewee async so this example is helpful. It might be a worthwhile addition to the docs. Thank you for all your work

rudyryk commented 8 years ago

It might be a worthwhile addition to the docs.

Sure, I agree! I think of extending the docs.

anjianshi commented 7 years ago

I have implemented get_related() method, maybe someone can use it.

import peewee_async

class ExtManager(peewee_async.Manager):
    async def get_related(self, instance, related_name, single_backref=False):
        """
        return related instance for foreign key relationship
        return query for backref or return related instance if single_backref is True
        """
        model_cls = type(instance)
        related_field = getattr(model_cls, related_name)

        if isinstance(related_field, ReverseRelationDescriptor):
            return await self._get_backrefs(instance, related_name, single_backref)
        else:
            return await self._get_foreign_key_target(instance, related_name)

    async def _get_foreign_key_target(self, instance, field_name):
        foreign_key_value = getattr(instance, field_name + "_id")

        model_cls = type(instance)
        foreign_key_field = getattr(model_cls, field_name)
        target_cls = foreign_key_field.rel_model
        target_field = foreign_key_field.to_field

        return await self.get(target_cls, target_field == foreign_key_value)

    async def _get_backrefs(self, instance, related_name, single_backref=False):
        query = getattr(instance, related_name)
        instances = await self.execute(query)

        if single_backref:
            for instance in instances:
                return instance
            raise query.model_class.DoesNotExist
        else:
            return instances
rudyryk commented 6 years ago

Hi @anjianshi, sorry for the late response, I didn't have much time to work on the project recently. I like the approach. Are you currently using it? Could that be adopted to new peewee, I mean peewee 3.5 and later.

evstegneych commented 3 years ago

Hello. Is there an implementation at the moment? How do I use .join ()?