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.

Sign up for GitHub

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

Jump to bottom

Fixed signals can not connect to OneToOneField (#572) #573

Merged
merged 9 commits into from
Oct 13, 2022
Merged

Fixed signals can not connect to OneToOneField (#572) #573

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b99abe3
Fixed signals can not connect to OneToOneField (#572)
954-Ivory Aug 21, 2022
127fe2e
Remove unused import.
954-Ivory Aug 21, 2022
82d3979
Changed `PeriodicTasks.pre_save` and `PeriodicTasks.pre_delete` in `P…
954-Ivory Aug 22, 2022
bc790e6
Update django_celery_beat/models.py
auvipy Aug 24, 2022
df24abd
Append the unit-test
954-Ivory Oct 13, 2022
8dd73d4
Correction of notes
954-Ivory Oct 13, 2022
95763dd
Append test-case for: O2O-rel instance be deleted
954-Ivory Oct 13, 2022
709c5c3
Add the docs
954-Ivory Oct 13, 2022
a3c8aa2
Format code according to `pycodestyle`
954-Ivory Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions django_celery_beat/apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,7 @@ class BeatConfig(AppConfig):
label = 'django_celery_beat'
verbose_name = _('Periodic Tasks')
default_auto_field = 'django.db.models.AutoField'

def ready(self):
from .signals import signals_connect
signals_connect()
21 changes: 0 additions & 21 deletions django_celery_beat/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
from django.core.exceptions import MultipleObjectsReturned, ValidationError
from django.core.validators import MaxValueValidator, MinValueValidator
from django.db import models
from django.db.models import signals
from django.utils.translation import gettext_lazy as _

from . import managers, validators
Expand Down Expand Up @@ -619,23 +618,3 @@ def schedule(self):
return self.solar.schedule
if self.clocked:
return self.clocked.schedule


signals.pre_delete.connect(PeriodicTasks.changed, sender=PeriodicTask)
signals.pre_save.connect(PeriodicTasks.changed, sender=PeriodicTask)
signals.pre_delete.connect(
PeriodicTasks.update_changed, sender=IntervalSchedule)
signals.post_save.connect(
PeriodicTasks.update_changed, sender=IntervalSchedule)
signals.post_delete.connect(
PeriodicTasks.update_changed, sender=CrontabSchedule)
signals.post_save.connect(
PeriodicTasks.update_changed, sender=CrontabSchedule)
signals.post_delete.connect(
PeriodicTasks.update_changed, sender=SolarSchedule)
signals.post_save.connect(
PeriodicTasks.update_changed, sender=SolarSchedule)
signals.post_delete.connect(
PeriodicTasks.update_changed, sender=ClockedSchedule)
signals.post_save.connect(
PeriodicTasks.update_changed, sender=ClockedSchedule)
41 changes: 41 additions & 0 deletions django_celery_beat/signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
def signals_connect():
from django.db.models import signals, OneToOneRel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OneToOneRel is a private API as far as I know, can you provide a testt case of examples of what you were actually trying to acheive?

Copy link
Contributor Author

@954-Ivory 954-Ivory Aug 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test case:

# admin.py
class MyPeriodicTaskInline(admin.StackedInline):
    model = MyPeriodicTask
    # ...
    # I want to render a `StackedInline` of `MyPeriodicTask` in `OtherModelAdmin` with my scene..
    # But it's not important.

class OtherModelAdmin(admin.ModelAdmin):
    model = OtherModel
    inlines = (
        MyPeriodicTaskInline,
    )
# models.py
class MyPeriodicTask(PeriodicTask):
    other_model = models.ForeignKey(
        'OtherModel',
        null=True,
        on_delete=models.CASCADE
    )

    def save(self, *args, **kwargs):
        self.kwargs = json.dumps('a_field_key',self.other_model.a_field)
        super().save(*args, **kwargs)
        # The bug is in it: 
        # This `save()` will call the `save_base()`.
        # But in `save_base()`, it will set `pre_save.sender = MyPeriodicTask`, not `PeriodicTask`.
        # So, the `PeriodicTask` can not receive signal with
        # [signals.pre_delete.connect(PeriodicTasks.changed, sender=PeriodicTask)](https://github.com/celery/django-celery-beat/blob/d4b97af62889527582194f3932e013ad972a88b3/django_celery_beat/models.py#L625)
        # [signals.pre_save.connect(PeriodicTasks.changed, sender=PeriodicTask)](https://github.com/celery/django-celery-beat/blob/d4b97af62889527582194f3932e013ad972a88b3/django_celery_beat/models.py#L625)
# query
my_periodic_task = MyPeriodicTask.first()
my_periodic_task.enabled = not my_periodic_task.enabled
my_periodic_task.save()
# the `PeriodicTasks.changed` will not be call.

Even without not overwrite save(), the pre_save.sender still be MyPeriodicTask.__class__.
Because in MyPeriodicTask, the self is MyPeriodicTask.
And Django use self.class to set sender.
delete() is the same, look this.

Copy link
Contributor Author

@954-Ivory 954-Ivory Aug 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two way to fix it.

  1. Don't use signal, call PeriodicTasks.changed() in PeriodicTask.save() and PeriodicTask.delete().
  2. Like this PR, use reverse related to get all OneToOneField, then get thier __class__, connect to the signal.

Because OneToOneRel is a definite type for judge reverse related which is OnetoOneField, so I use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would prefer first approach

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add the tests as well

Copy link
Contributor Author

@954-Ivory 954-Ivory Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add the tests as well

Yep, but I got the problem like this:
https://stackoverflow.com/questions/502916/django-how-to-create-a-model-dynamically-just-for-testing
I'm trying to solve it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that related to this patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that related to this patch?

Yep, I solved it.
Because I need to create the model in testing.

from .models import ClockedSchedule, PeriodicTask, PeriodicTasks, IntervalSchedule, CrontabSchedule, SolarSchedule

def o2o_discover():
"""
Description: Discover the `OneToOneField`, and connect their signals to `PeriodicTasks.changed`.

Issues: Signals can not connect to OneToOneField.
https://github.com/celery/django-celery-beat/issues/572

Note: The inheritance relationship introduces links between the child model and each of its
parents (via an automatically-created OneToOneField).
https://docs.djangoproject.com/en/stable/topics/db/models/#multi-table-inheritance
"""
related_objects = PeriodicTask._meta.related_objects
for obj in related_objects:
if isinstance(obj, OneToOneRel):
sender_class = obj.related_model
signals.pre_delete.connect(PeriodicTasks.changed, sender=sender_class)
signals.pre_save.connect(PeriodicTasks.changed, sender=sender_class)

o2o_discover()
signals.pre_delete.connect(PeriodicTasks.changed, sender=PeriodicTask)
signals.pre_save.connect(PeriodicTasks.changed, sender=PeriodicTask)
signals.pre_delete.connect(
PeriodicTasks.update_changed, sender=IntervalSchedule)
signals.post_save.connect(
PeriodicTasks.update_changed, sender=IntervalSchedule)
signals.post_delete.connect(
PeriodicTasks.update_changed, sender=CrontabSchedule)
signals.post_save.connect(
PeriodicTasks.update_changed, sender=CrontabSchedule)
signals.post_delete.connect(
PeriodicTasks.update_changed, sender=SolarSchedule)
signals.post_save.connect(
PeriodicTasks.update_changed, sender=SolarSchedule)
signals.post_delete.connect(
PeriodicTasks.update_changed, sender=ClockedSchedule)
signals.post_save.connect(
PeriodicTasks.update_changed, sender=ClockedSchedule)