Using inc=True and dtstart with between() incorrectly always includes the dtstart time

[Lidor7]

Creating a weekly Monday recurrence. Then getting all the occurrences between 2015-08-02 (Sunday) and 2015-08-11 (Tuesday) should return 2015-08-03 (Monday) and 2015-08-10 (Monday).

However, if I use the inc=True option to make start/end times inclusive and also use the dtstart option to specify the start date (lets me find occurrences in the past), it always includes the dtstart value as one of the occurrences.

Here is the code (version 1.2.0) demonstrating the bug:

recurrence.Recurrence(rrules=[recurrence.Rule(recurrence.WEEKLY, byday=recurrence.MONDAY)]).between(datetime.datetime(2015, 8, 2), datetime.datetime(2015, 8, 11), inc=True, dtstart=datetime.datetime(2015, 8, 2))

This results in:

[datetime.datetime(2015, 8, 2, 0, 0), datetime.datetime(2015, 8, 3, 0, 0), datetime.datetime(2015, 8, 10, 0, 0)]

Expected:

[datetime.datetime(2015, 8, 3, 0, 0), datetime.datetime(2015, 8, 10, 0, 0)]

[dominicrodger]

Hi @Lidor7 - thanks for filing this - I appreciate you taking the time to report it :)

This one's a little difficult to fix without breaking other people's code, since I presume other people might have worked around it by just dropping the first element.

Are you sure it only happens if inc=True is set? If so, perhaps we could drop support for that keyword (perhaps add a new keyword inclusive, which behaves correctly, and deprecate inc over a couple of versions or something).

I'd love to get rid of this bug, but I'm anxious not to break existing code if I can help it.

Anyone else have any thoughts on this?

[Lidor7]

@dominicrodger Yes, it only happens when inc=True. When I drop that parameter it behaves as expected.

I'm glad that you're thinking about not breaking other people's code, but I don't believe dropping the first element is really a correct workaround. In my example the dtstart date isn't an occurrence. But imagine the case where the dtstart date is actually an occurrence. You can't blindly drop the first element because you don't know if it's included because of the bug or because it's actually an occurrence. My workaround was to simply not use the inc parameter and just expanding my range by 1 day on both sides.

Because of this, I think fixing the bug outright may be fine.

On a side note, it's probably not possible to make this change at this point, but I find inclusive instead of exclusive being the default behavior more intuitive.

[dominicrodger]

Excellent point about blindly dropping the first element - it looks like I reached that conclusion a while back, and applied the same workaround (https://github.com/dominicrodger/kanisa/blob/master/kanisa/models/diary.py#L121), presumably for the same reason.

OK - I'm happy for this to be fixed, and we'll just add something in the release notes to warn people about the change - we can do this in 1.3.0.

I agree about inclusive being a more sensible default, and sadly I agree that changing it is probably problematic at this point (it's doable, but I don't think it's worth the effort).

Do you want to take a look at fixing the way inc=True behaves?

[Lidor7]

I can give it a go. I can't guarantee any particular timeline, though. Feel free to bug me about it each week to remind me.

[pytrumpeter]

Hi there,

First off, thanks so much for your work on bringing this project back to life. I was finding the idea of doing this myself daunting to say the least.

I too have run into this issue. A few days ago I started working on adapting this into my django app alongside FullCalendar, and noticed that the "phantom" date was showing up. I however am getting this even though I'm not using inc=True.

I believe it is related to two lines here (https://github.com/django-recurrence/django-recurrence/blob/master/recurrence/base.py#L555). I commented those out in my project and am now getting the result I want, but I've yet to determine if there is an adverse effect.

[dominicrodger]

@trpt4him - interesting, those lines do look odd to me. I'm having trouble reconciling your comment about it happening regardless of the value of inc with @Lidor7's comment about it only occurring when inc=True. I wonder if we could add a test for this scenario, and see how it behaves.

[Lidor7]

@dominicrodger You're right. It seems like there are cases (if not all cases) where inc=True isn't necessary to reproduce the same bug. Take a look at this example (first code snippet only works with dates in the future, so update accordingly).

This recurrence behaves as expected. Between 4/24 and 4/27 there is a Tuesday occurrence on 4/26.

>>> recurrence.Recurrence(rrules=[recurrence.Rule(recurrence.WEEKLY, byday=recurrence.TUESDAY)]).between(datetime.datetime(2016, 4, 24), datetime.datetime(2016, 4, 27))
[datetime.datetime(2016, 4, 26, 16, 51, 42)]

But when you add in dtstart, it adds in an extra occurrence -- the same date as dstart (this snippet should reproduce even with dates in the past):

>>> recurrence.Recurrence(rrules=[recurrence.Rule(recurrence.WEEKLY, byday=recurrence.TUESDAY)]).between(datetime.datetime(2016, 4, 24), datetime.datetime(2016, 4, 27), dtstart=datetime.datetime(2016, 4, 25))
[datetime.datetime(2016, 4, 25, 0, 0), datetime.datetime(2016, 4, 26, 0, 0)]

There is no good workaround for this scenario that I can think of, so it seems like a pretty serious bug.

[denisiko]

I think I figured out why the described behaviour sometimes occurs only with inc=True, and sometimes without.

As @trpt4him already mentioned, the core problem lies in the ruleset creation function where dtstart is added to the set. This is clearly false because between() params should never enrich the ruleset, as they only define a range of recurrences that are constructed with given rules.

The two examples given by @Lidor7 both trigger this behaviour, where the first only behaves like that when inc=True and the latter always shows the behaviour regardless of inc. This is because in the first example dtstart is equivalent to after (1. between param) so inc=True includes this date:

recurrence.Recurrence(rrules=[recurrence.Rule(recurrence.WEEKLY, byday=recurrence.MONDAY)]).between(datetime.datetime(2015, 8, 2), datetime.datetime(2015, 8, 11), inc=True, dtstart=datetime.datetime(2015, 8, 2))

results in

[datetime.datetime(2015, 8, 2, 0, 0), datetime.datetime(2015, 8, 3, 0, 0), datetime.datetime(2015, 8, 10, 0, 0)]

In the second example dtstart is a date that lies in between after and before so it gets displayed no matter what value inc has. Consider the following:

recurrence.Recurrence(rrules=[recurrence.Rule(recurrence.WEEKLY, byday=recurrence.TUESDAY)]).between(datetime.datetime(2016, 4, 24), datetime.datetime(2016, 4, 27), dtstart=datetime.datetime(2016, 4, 24))

results in

[datetime.datetime(2016, 4, 26, 0, 0)]

but

recurrence.Recurrence(rrules=[recurrence.Rule(recurrence.WEEKLY, byday=recurrence.TUESDAY)]).between(datetime.datetime(2016, 4, 24), datetime.datetime(2016, 4, 27), inc=True, dtstart=datetime.datetime(2016, 4, 24))

results in

[datetime.datetime(2016, 4, 24, 0, 0), datetime.datetime(2016, 4, 26, 0, 0)]

You won't see this behaviour if your dtstart does not lie between after and before. So all in all @trpt4him's conclusion is right and @Lidor7's examples are just logic consequences.

I think what was meant by adding dtstart in the ruleset is that it should be interpreted like an explicit 'add date', but it doesn't make sense in my opinion and is not a reason to break the functionality of the inc param. Therefore I strongly suggest to remove these two lines (e.g. by pulling @douwevandermeij's 94e8e78) and test afterwards, also because it would enable #71 to be solved.

[denisiko]

@dominicrodger - What do you think? I could write a test for this issue to demonstrate the behaviour, but first of all it must be clear what results to expect:

The way I see it whenever dtstart lies between after and before (start and end times), it gets included in the results. No matter if inc is True or False. This is not a right behaviour in my opinion.

When dtstart is equivalent to either after or before, then it only finds its way into the result set if inc=True. This is clearly false because you want inc to only effect start and end times if they fall into the specified pattern.

So custom tests for this problem would fail if you expect the behaviour that I think should occur (namely that dtstart is nevery included, except if it falls into the pattern e.g. as a thursday when you search for thursdays).

[denisiko]

OK. As @jpulec pointed out the described behavior is totally consistent with the RFC 2445 specification:

The "DTSTART" property value, if specified, counts as the first occurrence.

So there is a clash between RFC 2445 and dateutil.rrule, where dtstart is not the first occurence. I suggested a solution for this in #90.

[dominicrodger]

Is this still relevant with the work being done in #90, or can we close this?

[solomonix]

I know this has been around for a while, but it was frustrating me to no end in a project I am working on. I only include dtstart in between() so that I can see recurrences in the past. Based on the observations of @denisiko, dtstart causes problems when either inc=True, or dtstart is between the after/before date parameters of between().

What I ended up doing was just making my calls to between() use a dtstart that is one day earlier than the "after" date, so that I can: (a) see events in the past; and (b) dtstart doesn't count as an occurrence.

To borrow @denisiko's example, when you use:

recurrence.Recurrence(rrules=[recurrence.Rule(recurrence.WEEKLY, byday=recurrence.TUESDAY)]).between(datetime.datetime(2016, 4, 24), datetime.datetime(2016, 4, 27), inc=True, dtstart=datetime.datetime(2016, 4, 24))

you get:

[datetime.datetime(2016, 4, 24, 0, 0), datetime.datetime(2016, 4, 26, 0, 0)]

but, with the approach I have taken, this call would be:

recurrence.Recurrence(rrules=[recurrence.Rule(recurrence.WEEKLY, byday=recurrence.TUESDAY)]).between(datetime.datetime(2016, 4, 24), datetime.datetime(2016, 4, 27), inc=True, dtstart=datetime.datetime(2016, 4, 23))

and I get:

[datetime.datetime(2016, 4, 26, 0, 0)]

as expected.

I hope this approach can help someone while a fix is worked out.

Otherwise, fantastic package guys! Truly a lifesaver for this particular project.

[denisiko]

Thx @solomonix for the workaround. But now with #93 merged there's no need for a workaround anymore. Just use include_dtstart=False in your RecurrenceField.

This feature is available since version 1.5.0 of django-recurrence. So the issue can be closed I think.