Do not blindly delete duplicate schedules (#389)

* Do not blindly delete duplicate schedules

Because there are no unique constraints on schedule models (except for
SolarSchedule), schedules tables can contain duplicates, those
duplicate schedules should not be blindly deleted when encountered
because they can be linked to existing tasks and would cause the tasks
to be also deleted (on delete cascade).

Instead we now just return the first duplicate found, just to avoid
creating further duplicates.

This fixes issue #322.

* Fixed flake8 warnings

* More robust tests

* Removed MultipleObjectsReturned block

- removed MultipleObjectsReturned code block for solar schedules as
  unique_togther constraint safely prevents duplicates
- removed related test method

* DRY

Author: izimobil

django_celery_beat/models.py

@@ -117,13 +117,13 @@ def from_schedule(cls, schedule):
         spec = {'event': schedule.event,
                 'latitude': schedule.lat,
                 'longitude': schedule.lon}
+
+        # we do not check for MultipleObjectsReturned exception here because
+        # the unique_together constraint safely prevents from duplicates
         try:
             return cls.objects.get(**spec)
         except cls.DoesNotExist:
             return cls(**spec)
-        except MultipleObjectsReturned:
-            cls.objects.filter(**spec).delete()
-            return cls(**spec)
 
     def __str__(self):
         return '{0} ({1}, {2})'.format(
@@ -183,8 +183,7 @@ def from_schedule(cls, schedule, period=SECONDS):
         except cls.DoesNotExist:
             return cls(every=every, period=period)
         except MultipleObjectsReturned:
-            cls.objects.filter(every=every, period=period).delete()
-            return cls(every=every, period=period)
+            return cls.objects.filter(every=every, period=period).first()
 
     def __str__(self):
         readable_period = None
@@ -236,8 +235,7 @@ def from_schedule(cls, schedule):
         except cls.DoesNotExist:
             return cls(**spec)
         except MultipleObjectsReturned:
-            cls.objects.filter(**spec).delete()
-            return cls(**spec)
+            return cls.objects.filter(**spec).first()
 
 
 class CrontabSchedule(models.Model):
@@ -350,8 +348,7 @@ def from_schedule(cls, schedule):
         except cls.DoesNotExist:
             return cls(**spec)
         except MultipleObjectsReturned:
-            cls.objects.filter(**spec).delete()
-            return cls(**spec)
+            return cls.objects.filter(**spec).first()
 
 
 class PeriodicTasks(models.Model):

t/unit/test_models.py

@@ -7,13 +7,17 @@
 from django.db.migrations.autodetector import MigrationAutodetector
 from django.db.migrations.loader import MigrationLoader
 from django.db.migrations.questioner import NonInteractiveMigrationQuestioner
+from django.utils import timezone
 
 import timezone_field
 
 from django_celery_beat import migrations as beat_migrations
 from django_celery_beat.models import (
     crontab_schedule_celery_timezone,
     SolarSchedule,
+    CrontabSchedule,
+    ClockedSchedule,
+    IntervalSchedule,
 )
 
 
@@ -63,7 +67,22 @@ def test_models_match_migrations(self):
             msg='Model changes exist that do not have a migration')
 
 
-class CrontabScheduleTestCase(TestCase):
+class TestDuplicatesMixin:
+    def _test_duplicate_schedules(self, cls, kwargs, schedule=None):
+        sched1 = cls.objects.create(**kwargs)
+        cls.objects.create(**kwargs)
+        self.assertEqual(cls.objects.filter(**kwargs).count(), 2)
+        # try to create a duplicate schedule from a celery schedule
+        if schedule is None:
+            schedule = sched1.schedule
+        sched3 = cls.from_schedule(schedule)
+        # the schedule should be the first of the 2 previous duplicates
+        self.assertEqual(sched3, sched1)
+        # and the duplicates should not be deleted !
+        self.assertEqual(cls.objects.filter(**kwargs).count(), 2)
+
+
+class CrontabScheduleTestCase(TestCase, TestDuplicatesMixin):
     FIRST_VALID_TIMEZONE = timezone_field.\
         TimeZoneField.default_choices[0][0].zone
 
@@ -74,6 +93,19 @@ def test_default_timezone_without_settings_config(self):
     def test_default_timezone_with_settings_config(self):
         assert crontab_schedule_celery_timezone() == self.FIRST_VALID_TIMEZONE
 
+    def test_duplicate_schedules(self):
+        # See: https://github.com/celery/django-celery-beat/issues/322
+        kwargs = {
+            "minute": "*",
+            "hour": "4",
+            "day_of_week": "*",
+            "day_of_month": "*",
+            "month_of_year": "*",
+            "day_of_week": "*",
+        }
+        schedule = schedules.crontab(hour="4")
+        self._test_duplicate_schedules(CrontabSchedule, kwargs, schedule)
+
 
 class SolarScheduleTestCase(TestCase):
     EVENT_CHOICES = SolarSchedule._meta.get_field("event").choices
@@ -99,3 +131,18 @@ def test_celery_solar_schedules_included_as_event_choices(self):
 
         for event_choice in event_choices_values:
             assert event_choice in schedules.solar._all_events
+
+
+class IntervalScheduleTestCase(TestCase, TestDuplicatesMixin):
+
+    def test_duplicate_schedules(self):
+        kwargs = {'every': 1, 'period': IntervalSchedule.SECONDS}
+        schedule = schedules.schedule(run_every=1.0)
+        self._test_duplicate_schedules(IntervalSchedule, kwargs, schedule)
+
+
+class ClockedScheduleTestCase(TestCase, TestDuplicatesMixin):
+
+    def test_duplicate_schedules(self):
+        kwargs = {'clocked_time': timezone.now()}
+        self._test_duplicate_schedules(ClockedSchedule, kwargs)