Do not blindly delete duplicate schedules

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)