RFR: 8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR [v4]

[Naoto Sato]

On Fri, 24 Mar 2023 18:48:41 GMT, Justin Lu <jlu at openjdk.org> wrote:

>> This PR fixes the bug which occurred when `Calendar.roll(WEEK_OF_YEAR)` rolled into a minimal first week with an invalid `WEEK_OF_YEAR` and `DAY_OF_WEEK` combo.
>> 
>> For example, Rolling _Monday, 30 December 2019_ by 1 week produced _Monday, 31 December 2018_, which is incorrect. This is because `WEEK_OF_YEAR` is rolled from 52 to 1, and the original `DAY_OF_WEEK` is 1. However, there is no Monday in week 1 of 2019. This is exposed when a future method calls `Calendar.complete()`, which eventually calculates a `fixedDate` with the invalid `WEEK_OF_YEAR` and `DAY_OF_WEEK` combo.
>> 
>> To prevent this, a check is added for rolls into week 1, which determines if the first week is minimal. If it is indeed minimal, then it is checked if  `DAY_OF_WEEK` exists in that week, if not, `WEEK_OF_YEAR` must be incremented by one.
>> 
>> After the fix, Rolling _Monday, 30 December 2019_ by 1 week produces _Monday, 7 January 2019_
>
> Justin Lu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove max week methods accidentally comitted

Looks good. Mostly cosmetic comments follow

src/java.base/share/classes/java/util/GregorianCalendar.java line 1315:

> 1313:                     // rolling up into week 1, as the existing checks
> 1314:                     // sufficiently handle rolling down into week 1.
> 1315:                     if (newWeekOfYear == 1 && (isInvalidWeek1())) {

Extra parens around `isInvalidWeek1()`

src/java.base/share/classes/java/util/GregorianCalendar.java line 1316:

> 1314:                     // sufficiently handle rolling down into week 1.
> 1315:                     if (newWeekOfYear == 1 && (isInvalidWeek1())) {
> 1316:                         if (amount > 0) {

Can be folded into the above `if`

src/java.base/share/classes/java/util/GregorianCalendar.java line 3015:

> 3013:     private boolean isMinWeek (int days) {
> 3014:         return days >= getMinimalDaysInFirstWeek();
> 3015:     }

Since this method is only used above once, maybe make the check inline?

src/java.base/share/classes/java/util/GregorianCalendar.java line 3032:

> 3030:             // not between 6 7 1 2 3
> 3031:             return !(day >= startDay || day <= endDay);
> 3032:         }

Could be more readable if the method does not have `Not`, i.e, `daysInMinWeek`? This would remove the negation in the return statements.

test/jdk/java/util/Calendar/RollToMinWeek.java line 28:

> 26:  * @bug 8225641
> 27:  * @summary Test the behavior of Calendar.roll(WEEK_OF_YEAR) when the last week
> 28:  * is rolled up into a minimal week 1 of the same year

`minimal week 1` may be unclear.

test/jdk/java/util/Calendar/RollToMinWeek.java line 43:

> 41: /**
> 42:  * Test to validate the behavior of Calendar.roll(WEEK_OF_YEAR, +1)
> 43:  * when rolling into a minimal week 1 from the max week. WEEK_OF_YEAR can

IIUC, `minimal` and `max` mean `first` and `last` respectively? I'd rather change those as it would confuse with `minimal days in the first week`.

test/jdk/java/util/Calendar/RollToMinWeek.java line 52:

> 50:     public void rollUpTest(Calendar calendar, String[] validDates){
> 51:         if (calendar instanceof GregorianCalendar) {
> 52:             testRoll(calendar, validDates, +1);

If the amount is fixed to `+1`, no need to pass it as an argument

test/jdk/java/util/Calendar/RollToMinWeek.java line 98:

> 96:                 for (int firstDay = 1; firstDay <= 7; firstDay++) {
> 97:                     calList.add(Arguments.of(buildCalendar("gregory", firstDay, weekLength,
> 98:                                     dayOfMonth, 11, 2019), validDates[date]));

`11` may better be replaced with `Calendar.DECEMBER`.

test/jdk/java/util/Calendar/RollToMinWeek.java line 114:

> 112:         calBuilder.setDate(year, month, dayOfMonth);
> 113:         return calBuilder.build();
> 114:     }

`Builder` instance can be reused. Also since Builder can method chain, you could write

static Calendar.Builder CAL_BUILDER = new Builder().setCalendarType(type);

return CAL_BUILDER
    .setWeekDefinition(...)
    .setDate(...)
    .build();

-------------

PR Review: https://git.openjdk.org/jdk/pull/13031#pullrequestreview-1359843143
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149772051
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149771259
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149777645
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149779427
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149838007
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149838132
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149843461
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149847360
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149851739