Bug in RRuleIteratorImpl.advanceTo() when advancing before or at the dtStart?

[Alexandre Normand]

Hi,
I started using rfc-2445 to expand recurring events. I'm usually
only interested in a specific date range so I'm using advanceTo() for
that. I'm having issues when the advanceTo() is given a date before or
exactly at the dtStart. In those cases, the advanceTo would generate a
new date time instance even though the one generated at creation would
actually be the correct pendingUtc time. Generating a new instance
does actually skip the dtStart instance.

So either:
1. Before calling advanceTo, I would check if the DateTime is after
the dtStart.
2. Fix advanceTo to return the pendingUtc if it's already after or at
the advanceTo DateTime.

I made the fix to RRuleIteratorImpl (see fix below) but if it's not
something that going to make it to the official code-base, then I'm
going to do solution 1 in my code.

What do you guys think?

Index: src/com/google/ical/iter/RRuleIteratorImpl.java
===================================================================
--- src/com/google/ical/iter/RRuleIteratorImpl.java (revision 19)
+++ src/com/google/ical/iter/RRuleIteratorImpl.java Wed Jan 13
12:43:43 EST 2010
@@ -168,57 +168,60 @@
if (dateLocal.compareTo(this.builder_.toDate()) < 0) {
return;
}
+ if (this.pendingUtc_ == null || dateUtc.compareTo
(this.pendingUtc_) > 0)
+ {
- this.pendingUtc_ = null;
+ this.pendingUtc_ = null;

- try {
- if (this.canShortcutAdvance_) {
- // skip years before date.year
- if (this.builder_.year < dateLocal.year()) {
- do {
- if (!this.yearGenerator_.generate(this.builder_)) {
- this.done_ = true;
- return;
- }
- } while (this.builder_.year < dateLocal.year());
- while (!this.monthGenerator_.generate(this.builder_)) {
- if (!this.yearGenerator_.generate(this.builder_)) {
- this.done_ = true;
- return;
- }
- }
- }
- // skip months before date.year/date.month
- while (this.builder_.year == dateLocal.year()
- && this.builder_.month < dateLocal.month()) {
- while (!this.monthGenerator_.generate(this.builder_)) {
- // if there are more years available fetch one
- if (!this.yearGenerator_.generate(this.builder_)) {
- // otherwise the recurrence is exhausted
- this.done_ = true;
- return;
- }
- }
- }
- }
+ try {
+ if (this.canShortcutAdvance_) {
+ // skip years before date.year
+ if (this.builder_.year < dateLocal.year()) {
+ do {
+ if (!this.yearGenerator_.generate(this.builder_)) {
+ this.done_ = true;
+ return;
+ }
+ } while (this.builder_.year < dateLocal.year());
+ while (!this.monthGenerator_.generate(this.builder_)) {
+ if (!this.yearGenerator_.generate(this.builder_)) {
+ this.done_ = true;
+ return;
+ }
+ }
+ }
+ // skip months before date.year/date.month
+ while (this.builder_.year == dateLocal.year()
+ && this.builder_.month < dateLocal.month()) {
+ while (!this.monthGenerator_.generate(this.builder_)) {
+ // if there are more years available fetch one
+ if (!this.yearGenerator_.generate(this.builder_)) {
+ // otherwise the recurrence is exhausted
+ this.done_ = true;
+ return;
+ }
+ }
+ }
+ }

- // consume any remaining instances
- while (!this.done_) {
- DateValue dUtc = this.generateInstance();
- if (null == dUtc) {
- this.done_ = true;
- } else {
- if (!this.condition_.apply(dUtc)) {
- this.done_ = true;
- } else if (dUtc.compareTo(dateUtc) >= 0) {
- this.pendingUtc_ = dUtc;
- break;
- }
- }
- }
- } catch (Generator.IteratorShortCircuitingException ex) {
- this.done_ = true;
- }
- }
+ // consume any remaining instances
+ while (!this.done_) {
+ DateValue dUtc = this.generateInstance();
+ if (null == dUtc) {
+ this.done_ = true;
+ } else {
+ if (!this.condition_.apply(dUtc)) {
+ this.done_ = true;
+ } else if (dUtc.compareTo(dateUtc) >= 0) {
+ this.pendingUtc_ = dUtc;
+ break;
+ }
+ }
+ }
+ } catch (Generator.IteratorShortCircuitingException ex) {
+ this.done_ = true;
+ }
+ }
+ }

/** calculates and stored the next date in this recurrence. */
private void fetchNext() {

[Mike Samuel]

Cool. Do you have a testcase that exercises this bug?

2010/1/13 Alexandre Normand <alexandre.bre...@gmail.com>:

> --
> You received this message because you are subscribed to the Google Groups "google-rfc-2445" group.
> To post to this group, send email to google-...@googlegroups.com.
> To unsubscribe from this group, send email to google-rfc-24...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/google-rfc-2445?hl=en.
>
>
>
>

[Alexandre Normand]

It took some time because my testcase was for my code that uses
rfc2445. Here's a testcase added to RRuleIteratorImplTest.java (fails
on the current code base and succeed with the fix in my original
message):

Index: test/com/google/ical/iter/RRuleIteratorImplTest.java
===================================================================
--- test/com/google/ical/iter/RRuleIteratorImplTest.java (revision 19)
+++ test/com/google/ical/iter/RRuleIteratorImplTest.java Wed Jan 13
17:49:02 EST 2010
@@ -898,6 +898,13 @@
"",
IcalParseUtil.parseDateValue("25000101"));

+ // advancing right to the start
+ runRecurrenceIteratorTest(
+
"RRULE:FREQ=YEARLY;INTERVAL=1;BYMONTHDAY=10;BYMONTH=1;COUNT=3",
+ IcalParseUtil.parseDateValue("20100110T140000"), 3,
+ "20100110T140000,20110110T140000,20120110T140000",
+ IcalParseUtil.parseDateValue("20100110T140000"));
+
// TODO(msamuel): check advancement of more examples
}

Thank you,

On Jan 13, 4:46 pm, Mike Samuel <mikesam...@gmail.com> wrote:
> Cool.  Do you have a testcase that exercises this bug?
>

> 2010/1/13 Alexandre Normand <alexandre.bressani.norm...@gmail.com>:

[Mike Samuel]

Great. Thanks. I should get a chance to try patching these two
changes in tomorrow.

2010/1/13 Alexandre Normand <alexandre.bre...@gmail.com>:

[Alexandre Normand]

Great, thanks you Mike. And do let me know if you think this should be
done better.

On Jan 13, 8:53 pm, Mike Samuel <mikesam...@gmail.com> wrote:
> Great.  Thanks.  I should get a chance to try patching these two
> changes in tomorrow.
>

> 2010/1/13 Alexandre Normand <alexandre.bressani.norm...@gmail.com>:

[Mike Samuel]

I applied your patch at
http://code.google.com/p/google-rfc-2445/source/detail?r=30

The operative portion is at
http://code.google.com/p/google-rfc-2445/source/diff?spec=svn30&r=30&format=side&path=/trunk/src/com/google/ical/iter/RRuleIteratorImpl.java

and I modified one of the test primitives to make sure that advancing
to the start date is a noop for all the existing tests:

http://code.google.com/p/google-rfc-2445/source/diff?spec=svn30&r=30&format=side&path=/trunk/test/com/google/ical/iter/RRuleIteratorImplTest.java

2010/1/13 Alexandre Normand <alexandre.bre...@gmail.com>:

[Alexandre Normand]

Thanks Mike, your changes to the test are much better than the single
test case I had added.

Cheers,

On Jan 18, 6:00 pm, Mike Samuel <mikesam...@gmail.com> wrote:
> I applied your patch athttp://code.google.com/p/google-rfc-2445/source/detail?r=30
>
> The operative portion is athttp://code.google.com/p/google-rfc-2445/source/diff?spec=svn30&r=30&...

>
> and I modified one of the test primitives to make sure that advancing
> to the start date is a noop for all the existing tests:
>

> http://code.google.com/p/google-rfc-2445/source/diff?spec=svn30&r=30&...
>
> 2010/1/13 Alexandre Normand <alexandre.bressani.norm...@gmail.com>: