dmfs / lib-recur

A recurrence processor for Java
Apache License 2.0
199 stars 47 forks source link

Day skip on WEKLY frequency with BYHOUR property set #97

Closed annashumylo closed 3 years ago

annashumylo commented 3 years ago

Hi all, During some tests I found a weird behavior with the rule creation. Maybe I'm using it wrong, can someone please verify it? Here is an expected behavior:

  Mo Tu We Th Fr Sa Su
               1  2  3
   4  5  6  7  8  9 10
  11 12 13 14 15 16 17
  18 19 20 21 22 23 24
  25 26 27 28 29 30 31

I'm creating a weekly rrule where interval is 2, count is 2 and selected weekday is Monday. Start of the rule is Wed Dec 30 2020 00:00:00 CET time. I'm expecting to get two values: 2021-01-04 and 2021-01-18 and this is working fine.

RecurrenceRule recurrenceRule2 = new RecurrenceRule(Freq.WEEKLY);
recurrenceRule2.setCount(2);
recurrenceRule2.setInterval(2);
recurrenceRule2.setByDayPart(Collections.singletonList(new WeekdayNum(1, Weekday.MO)));
RecurrenceRuleIterator iterator2 = recurrenceRule2.iterator(1609282800000L, TimeZone.getTimeZone("Europe/Paris"));

When I'm adding BYHOUR part it skips 01.04 and jumps right to 01.11. Second date is then 01.25:

RecurrenceRule recurrenceRule = new RecurrenceRule(Freq.WEEKLY);
recurrenceRule.setCount(2);
recurrenceRule.setByPart(RecurrenceRule.Part.BYHOUR, 13);
recurrenceRule.setInterval(2);
recurrenceRule.setByDayPart(Collections.singletonList(new WeekdayNum(1, Weekday.MO)));
RecurrenceRuleIterator iterator = recurrenceRule.iterator(1609282800000L, TimeZone.getTimeZone("Europe/Paris"));

The complete broken test is here in case you need it:

       @Test
    public void test() throws InvalidRecurrenceRuleException {
        RecurrenceRule recurrenceRule = new RecurrenceRule(Freq.WEEKLY);
        recurrenceRule.setCount(2);
        recurrenceRule.setByPart(RecurrenceRule.Part.BYHOUR, 13);
        recurrenceRule.setInterval(2);
        recurrenceRule.setByDayPart(Collections.singletonList(new WeekdayNum(1, Weekday.MO)));
        RecurrenceRuleIterator iterator = recurrenceRule.iterator(1609282800000L, TimeZone.getTimeZone("Europe/Paris"));

        DateTime mon4 = iterator.nextDateTime();
        DateTime mon18 = iterator.nextDateTime();

        Assertions.assertEquals(DateTime.parse(TimeZone.getTimeZone("Europe/Paris"), "20210104T130000"), mon4);
        Assertions.assertEquals(DateTime.parse(TimeZone.getTimeZone("Europe/Paris"), "20210118T130000"), mon18);
    }

Thanks in advance!

dmfs commented 3 years ago

thanks for the report. Looks indeed like something is broken here. I'll check that.

dmfs commented 3 years ago

In general it's better to use a start date which is in sync with the rule. In your case the start date is on a Wednesday, which is not part of the rule. The result is actually ambiguous and you could argue for both cases to be correct, Jan 4th and Jan 11th.

So both versions seem reasonable. I agree that specifying an hour should not make a difference though. I guess that's due to the fact that a different WEEKLY expansion code path is executed in both cases. If there is no BYHOUR part, the code takes a fast path for WEEKLY date generation, with the BYHOUR part it takes the generic path. Apparently they both come to different conclusions in terms of which instance should be the first one.

I'm actually leaning towards the second interpretation, in which Jan 11th is the first instance. This follows the idea that Dec 30th is the "0th" instance and the next one should be (about) 2 weeks later, and certainly not in the next week thereafter.

One more comment: I probably need to improve the documentation here, when being used with WEEKLY rules, the number in WeekdayNum should always be 0.

dmfs commented 3 years ago

Which version of the library are you using? It seems I'm getting a consistent result with and without a BYHOUR part.

        assertThat(new RecurrenceRule("FREQ=WEEKLY;INTERVAL=2;BYDAY=MO;COUNT=2"),
            is(validRule(DateTime.parse("20201230T000000"),
                walking(),
                instances(are(onWeekDay(MO))),
                startingWith("20210111T000000", "20210125T000000"))));

        assertThat(new RecurrenceRule("FREQ=WEEKLY;INTERVAL=2;BYDAY=MO;BYHOUR=13;COUNT=2"),
            is(validRule(DateTime.parse("20201230T000000"),
                walking(),
                instances(are(onWeekDay(MO))),
                startingWith("20210111T130000", "20210125T130000"))));

Same with your code snippet (except that the current library version ignores WeekdayNums with a non-zero value for WEEKLY generation, which then results in an error because no instances are generated).

The latest version is 0.12.2, please retry with this version.

annashumylo commented 3 years ago

Hi, thanks for your reply, I updated a version and now the output is consistent for both (with and without time). After your explanation I see that you have a point here, so I'll close this issue, have a nice day!