jazzband / django-push-notifications

Send push notifications to mobile devices through GCM or APNS in Django.
MIT License
2.23k stars 605 forks source link

Use django polymorphic #393

Open antwan opened 7 years ago

antwan commented 7 years ago

I think this project is a good candidate to implement multi-table inheritance between different service providers, so it would be easier to select all user devices within one single queryset.

I'm using the good django-polymorphic package on several projects and it's based on Content Types and Multi table inheritance internally with some improvements, ie automatically get the right class, reduce number of queries, etc.

With such architecture we could use simple things like this: user.devices.send_message('Hello') user.devices.all() -> <Queryset [<APNSDevice>, <GCMDevice>, <GCMDevice>]>

I'm willing to develop PR for it, but only if there is interest/appetence and if it's likely to be accepted at some point. So my question : What do you think ?

matthewh commented 7 years ago

I agree with you that adding support for sending messages across different device types is very useful. +1 for that idea. I strongly disagree that using polymorphic models is the best answer. I think we can accomplish this without introducing a dependency on django-polymorphic though it may not be as convenient as user.devices.all().

Each platform supports a slew of platform specific options. If we are going to implement this, how do we account for those?

One possibility would be to expose a dict for each platform to specify options: user.devices.send_message('Hello', apns={'category': 'hello'}, gcm={'priority': high'}, wns={...})

jleclanche commented 7 years ago

Heh that's funny, I was thinking about it just yesterday actually.

I'm +1 on it, but django polymorphic models only support 1.10 at the moment, waiting on the 1.11 compatible release first.

antwan commented 7 years ago

@matthewh Unfortunately if you want per-class specific methods (ie use the FCM send_message() for FCM devices, and APNS send_message() for APNS devices), you need to do things that django-polymorphic does (use content_type to retrieve the type, and do additional requests, at least one per platform).

I'm also strongly against adding more dependencies in a project, but it's clearly a textbook case for polymorphic here. It is a minimal package, and having raw multi-table inheritance would require rewriting what's in there anyway.

I like the per-platform option in queryset, let's see how to design it the best way possible.

@jleclanche I will investigate in what's required before implementing anything.

matthewh commented 7 years ago

@Antwan86 I have used django-polymorphic in the past and my general experience has been, if you think django-polymorphic solves your problem, you are solving the wrong problem.

Let's take a step back and look at why we have a model for each platform.

In the master branch each Device model defines two fields (the rest are inherited from Device):

So, we make device_id for FCM/GCM be a UUID (already a PR for that). We make APNS registration_id a TextField like GCM and WNS and optionally enforce the maxlength 200 in clean().

Finally, add a column for device_type (allowed values are: gcm, fcm, apns, wns). This has the side effect of encapsulating the functionality of cloud_message_type as well.

With those 3 changes we avoid an external dependency and get the fastest possible lookup performance because there are no inner joins. Enabling the types of queries you proposed.

user.devices.send_message('Hello')
user.devices.all() -> <Queryset [<Device type=apns>, <Device type=gcm>, <Device type=gcm>]>
antwan commented 7 years ago

If the child models are strictly identical, Django won't create additional multi-table inheritance, and there won't be additional JOIN calls when using polymorphic. The performance will be exactly the same than the solution you proposed, the type being stored in the polymorphic_ctype_id column.

But anyway, I agree avoiding external dependencies is good, I just think keeping separate models is a wrong approach as many developers will need to send notifications to devices independently of their type.