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

Async ForeignKeys #15

Open
mrbox opened this issue Feb 5, 2016 · 22 comments
Open

Async ForeignKeys #15

mrbox opened this issue Feb 5, 2016 · 22 comments

Comments

@mrbox
Copy link
Contributor

mrbox commented Feb 5, 2016

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

rudyryk commented Feb 6, 2016

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

mrbox commented Feb 6, 2016

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

rudyryk commented Feb 6, 2016

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

rudyryk commented Feb 6, 2016

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

mrbox commented Feb 6, 2016

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

rudyryk commented Feb 8, 2016

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

rudyryk commented Feb 8, 2016

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

rudyryk commented Feb 8, 2016

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

rudyryk commented Feb 8, 2016

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

mrbox commented Feb 15, 2016

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

rudyryk commented Feb 16, 2016

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

rudyryk commented Feb 16, 2016

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

mrbox commented Feb 16, 2016

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- #19 ;)

@rudyryk
Copy link
Member

rudyryk commented Feb 16, 2016

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

mrbox commented Feb 16, 2016

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

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

rudyryk commented Oct 30, 2016

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

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

rudyryk commented Oct 31, 2016

It might be a worthwhile addition to the docs.

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

@anjianshi
Copy link

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

rudyryk commented Aug 8, 2018

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

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

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

5 participants