llazzaro / django-scheduler

A calendaring app for Django.
BSD 3-Clause "New" or "Revised" License
1.27k stars 391 forks source link

Cancelled Occurrence replaced by a new Event/Occurrence causes occurrences_after to fail #562

Open dwasyl opened 3 months ago

dwasyl commented 3 months ago

Where there is an Event with a cancelled Occurrence, but another Event has been scheduled in place of the original at the same time, it causes EventListManager.occurrences_after() to fail at next_occurrence = heapq.heappop(occurrences)[0] with an '<' not supported between instances of 'generator' and 'generator' error.

Part of the issue seems to be that EventListManager.occurrences_after() doesn't employ the same logic as Event.get_occurrences() to work persisted occurrences into the generator set.

For example, in a case where an Event has only a single Occurrence and that Occurrence is cancelled, how should that be treated here? In either case, it causes occurrences_after() to fail which isn't the correct behaviour.

The use of OccurrenceReplacer happens at the end, but it has already failed due to match Occurrences being added to the heap.

dwasyl commented 3 months ago

Continuing to investigate my own issue, the problem is linked to the heaps PriorityQueue implmentation as described https://docs.python.org/3/library/heapq.html#priority-queue-implementation-notes

Based on some notes from here: https://stackoverflow.com/questions/56998340/heapq-cant-handle-tuples-having-same-priority-if-the-item-is-not-comparable, the simplest would be to add an occurrent/event # to the tuple that gets added to the heap, however, it would be difficult to find that again based on the use of heapreplace in schedule/utils.py:

            try:
                next_occurrence = heapq.heapreplace(
                    occurrences, (next(generator), generator)
                )[0]

Potentially just added the Event.pk to the tuple would deal with the issue as a single Event likely wouldn't get a duplicate Occurrence, the is easily replicable when doing the heapreplace.

Thoughts?

dwasyl commented 3 months ago

For the record, having posted this the solutions became easier to solve, the EventListManager.occurrences_after() needed some tweaking to add an occurrence count to the heap tuple to that the generators don't get compared.

On a very simple basis:

    def occurrences_after(self, after=None):
        """
        It is often useful to know what the next occurrence is given a list of
        events.  This function produces a generator that yields the
        the most recent occurrence after the date ``after`` from any of the
        events in ``self.events``
        """
        from schedule.models import Occurrence

        if after is None:
            after = timezone.now()
        occ_replacer = OccurrenceReplacer(
            Occurrence.objects.filter(event__in=self.events)
        )
        generators = [
            event._occurrences_after_generator(after) for event in self.events
        ]
        occurrences = []

        occ_count = 1
        for generator in generators:
            try:
                heapq.heappush(occurrences, (next(generator), occ_count, generator))
            except StopIteration:
                pass
            occ_count += 1

        while occurrences:
            generator = occurrences[0][2]
            occ_count = occurrences[0][1]

            try:
                next_occurrence = heapq.heapreplace(
                    occurrences, (next(generator), occ_count, generator)
                )[0]
            except StopIteration:
                next_occurrence = heapq.heappop(occurrences)[0]
            yield occ_replacer.get_occurrence(next_occurrence)

The only additions being the three occ_count lines. Perhaps I'll work this into a PR, if you are still looking at updating/making releases @llazzaro ?