8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR #13031

justin-curtis-lu · 2023-03-14T22:16:45Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR

Reviewers

Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13031/head:pull/13031
$ git checkout pull/13031

Update a local copy of the PR:
$ git checkout pull/13031
$ git pull https://git.openjdk.org/jdk.git pull/13031/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13031

View PR using the GUI difftool:
$ git pr show -t 13031

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13031.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-03-14T22:17:48Z

👋 Welcome back jlu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-14T22:21:10Z

@justin-curtis-lu The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-03-14T22:25:20Z

Webrevs

naotoj

Hi Justin,
Thanks for the fix. Still reviewing the changes, but here are some comments I have noticed:

naotoj · 2023-03-15T19:47:24Z

src/java.base/share/classes/java/util/GregorianCalendar.java

+                    // Make sure that the (potential) minimum week has the
+                    // current DAY_OF_WEEK
+                    if (newWeekOfYear == 1 && isInvalidWeek1()) {
+                        newWeekOfYear+=1;


is +1 always correct? Does it work when the amount is negative?

When a week is rolled down to 1 (and it does not have a valid day_of_week), the existing code would appropriately adjust min, so that the new week set would be 52. However, yes, it should only be incremented if amount is positive, but there does not need to be a case for when amount is negative.

naotoj · 2023-03-15T19:50:46Z

src/java.base/share/classes/java/util/GregorianCalendar.java

+        // Calculate the DAY_OF_WEEK for Jan 1 of the current YEAR
+        long jan1Fd =  gcal.getFixedDate(internalGet(YEAR), 1, 1, null);
+        int jan1Dow = BaseCalendar.getDayOfWeekFromFixedDate(jan1Fd);
+        int daysInFirstWeek;
+        if (getFirstDayOfWeek() <= jan1Dow) {
+            // Add wrap around days
+            daysInFirstWeek = (7 - jan1Dow) + getFirstDayOfWeek();
+        } else {
+            daysInFirstWeek = getFirstDayOfWeek() - jan1Dow;
+        }
+        // If the week is minimum, check if the DAY_OF_WEEK does not exist
+        return isMinWeek(daysInFirstWeek) &&
+                dayNotInMinWeek(internalGet(DAY_OF_WEEK), jan1Dow, getFirstDayOfWeek() - 1);
+    }
+
+    /**
+     * Determines if the specified amount of days can make up
+     * a valid minimum week.
+     */
+    private boolean isMinWeek (int days) {
+        return days >= getMinimalDaysInFirstWeek();
+    }
+
+    /**
+     * Determines if the specified day exists in the minimum week.
+     * For example, dayNotInMinWeek(4, 6, 3) returns false since Wednesday
+     * is not between the minimum week given by [Friday, Saturday,
+     * Sunday, Monday, Tuesday].
+     */
+    private boolean dayNotInMinWeek (int day, int startDay, int endDay) {
+        if (endDay >= startDay) {
+            // dayNotInMinWeek(2, 3, 6), check that 2 is
+            // not between 3 4 5 6
+            return !(day >= startDay && day <= endDay);
+        } else {
+            // dayNotInMinWeek(4, 6, 3), check that 4 is
+            // not between 6 7 1 2 3
+            return !(day >= startDay || day <= endDay);
+        }
+    }
+


Are these GregorianCalendar specific? What about other calendars?

Good point, these are probably applicable to the other calendars, will take another look there.

naotoj · 2023-03-15T19:53:07Z

test/jdk/java/util/Calendar/RollToMinWeek.java

+/*
+ * @test
+ * @bug 8225641
+ * @summary Test the behavior of Calendar.roll(WEEK_OF_YEAR) when the week
+ * is rolled into a minimal week 1
+ * @run junit RollToMinWeek
+ */


Have you considered adding test cases into test/jdk/java/util/Calendar/CalendarTestScripts, instead of creating a single-purpose test case?

Will take a look at the existing .cts files and see if it would be advantageous to add test cases there

naotoj · 2023-03-15T19:57:43Z

test/jdk/java/util/Calendar/RollToMinWeek.java

+    private static Stream<Arguments> calendarProvider() {
+        return Stream.of(
+                // Test a variety of rolls that previously produced incorrect results
+                Arguments.of(buildCalendar(27, 11, 2020, Locale.ENGLISH),


The first day of week depends on the region, not the language. In fact, I would prefer explicitly specifying it via a locale like en-US-u-fw-mon, and testing all the weekdays.

Updated the testing

naotoj · 2023-03-15T19:58:29Z

test/jdk/java/util/Calendar/RollToMinWeek.java

+    }
+
+    private static Calendar buildCalendar(int day, int month, int year, Locale locale) {
+        Calendar calendar = Calendar.getInstance(locale);


If the fix is GregorianCalendar specific, should it check the type?

Yes, I have changed it so that it is intentionally set as Gregorian, and also tested for being Gregorian.

justin-curtis-lu · 2023-03-17T19:16:49Z

Although this issue applies to Gregorian Calendar, it appears it may also occur with a Japanese Calendar under the right circumstances.

{$$$ Failed: Rolled: "Sunday, 29 December 1" by 1 week, expecting: "Sunday, 5 May 1", but got: "Saturday, 5 January 31"}

The Reiwa era begins on May 1st, 2019. Rolling the last Sunday of 2019 by 1 week, one would expect the resultant date to be the first Sunday of the Reiwai period (the 5th of May, 2019). However, a completely wrong date is returned, and the calendar actually rolls back an era into Heisei.

Looking into it further, as well as the other calendars

justin-curtis-lu · 2023-03-24T18:38:29Z

The test currently tests 343 combinations for rolling from the end of the year into week 1

Before patch:
[ JUnit Tests: found 343, started 343, succeeded 308, failed 35, aborted 0, skipped 0]

After patch:
[ JUnit Tests: found 343, started 343, succeeded 343, failed 0, aborted 0, skipped 0]

naotoj

Looks good. Mostly cosmetic comments follow

naotoj · 2023-03-27T20:43:48Z

src/java.base/share/classes/java/util/GregorianCalendar.java

+                    // rolling up into week 1, as the existing checks
+                    // sufficiently handle rolling down into week 1.
+                    if (newWeekOfYear == 1 && (isInvalidWeek1())) {
+                        if (amount > 0) {


Can be folded into the above if

naotoj · 2023-03-27T20:44:40Z

src/java.base/share/classes/java/util/GregorianCalendar.java

+                    // current DAY_OF_WEEK. Only make a check for
+                    // rolling up into week 1, as the existing checks
+                    // sufficiently handle rolling down into week 1.
+                    if (newWeekOfYear == 1 && (isInvalidWeek1())) {


Extra parens around isInvalidWeek1()

naotoj · 2023-03-27T20:50:45Z

src/java.base/share/classes/java/util/GregorianCalendar.java

+    private boolean isMinWeek (int days) {
+        return days >= getMinimalDaysInFirstWeek();
+    }


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

naotoj · 2023-03-27T20:52:56Z

src/java.base/share/classes/java/util/GregorianCalendar.java

+    private boolean dayNotInMinWeek (int day, int startDay, int endDay) {
+        if (endDay >= startDay) {
+            // dayNotInMinWeek(2, 3, 6), check that 2 is
+            // not between 3 4 5 6
+            return !(day >= startDay && day <= endDay);
+        } else {
+            // dayNotInMinWeek(4, 6, 3), check that 4 is
+            // not between 6 7 1 2 3
+            return !(day >= startDay || day <= endDay);
+        }


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

naotoj · 2023-03-27T22:12:07Z

test/jdk/java/util/Calendar/RollToMinWeek.java

+ * @test
+ * @bug 8225641
+ * @summary Test the behavior of Calendar.roll(WEEK_OF_YEAR) when the last week
+ * is rolled up into a minimal week 1 of the same year


minimal week 1 may be unclear.

naotoj · 2023-03-27T22:12:20Z

test/jdk/java/util/Calendar/RollToMinWeek.java

+
+/**
+ * Test to validate the behavior of Calendar.roll(WEEK_OF_YEAR, +1)
+ * 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.

naotoj · 2023-03-27T22:21:04Z

test/jdk/java/util/Calendar/RollToMinWeek.java

+    @MethodSource("rollUpCalProvider")
+    public void rollUpTest(Calendar calendar, String[] validDates){
+        if (calendar instanceof GregorianCalendar) {
+            testRoll(calendar, validDates, +1);


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

naotoj · 2023-03-27T22:27:38Z

test/jdk/java/util/Calendar/RollToMinWeek.java

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


11 may better be replaced with Calendar.DECEMBER.

naotoj · 2023-03-27T22:35:01Z

test/jdk/java/util/Calendar/RollToMinWeek.java

+        Calendar.Builder calBuilder = new Builder();
+        calBuilder.setCalendarType(type);
+        calBuilder.setWeekDefinition(firstDayOfWeek, minimumWeekLength);
+        calBuilder.setDate(year, month, dayOfMonth);
+        return calBuilder.build();
+    }


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();

…ith Gregorian for this test

naotoj · 2023-03-29T23:14:17Z

src/java.base/share/classes/java/util/GregorianCalendar.java

+     * Sunday, Monday, Tuesday].
+     */
+    private boolean dayInMinWeek (int day, int startDay, int endDay) {
+        endDay = endDay == 0


Why endDay can be 0? If the arguments are Calendar.XXXDAY, should they be between 1 to 7? In other words, why is the above calling site doing getFirstDayOfWeek() - 1?
I'd add some range checks for the input values.

Right. I have changed it so the method throws an exception if the start and end days are not valid DAY_OF_WEEK values.

And I have moved the ternary of switching 0 to 7 outside the method accordingly.

naotoj · 2023-03-29T23:14:52Z

test/jdk/java/util/Calendar/RollFromLastToFirstWeek.java

+ * week is incremented to prevent this.
+ */
+public class RollFromLastToFirstWeek {
+    static Calendar.Builder GREGORIAN_BUILDER = new Builder()


Nit: Calendar. can be omitted (and can be private static final)

naotoj · 2023-03-30T23:20:52Z

src/java.base/share/classes/java/util/GregorianCalendar.java

+            throw new IllegalArgumentException("Start day or end day is not " +
+                    "a valid day of the week");


Sorry, I take my previous comment back. I think we can simply rely on the return value from getFirstDayOfWeek() as it is well-checked, so no need to check the input here. Otherwise the thrown IAE woudl have to end up in add()/roll() public methods, which cannot be spec'ed with these internal arguments.

No worries, that makes sense. Removed the exception.

…nWeek

naotoj

LGTM

openjdk · 2023-03-31T17:25:03Z

@justin-curtis-lu This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR

Reviewed-by: naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 413 new commits pushed to the master branch:

a7546b3: 8304996: Add missing HandleMarks
ac898e9: 8303278: Imprecise bottom type of ExtractB/UB
a1a9ec6: 8297286: runtime/vthread tests crashing after JDK-8296324
c6bd489: 8305206: Add @SPEC tags in java.base/java.* (part 1)
ccbb0e8: 8303798: REDO - Remove fdlibm C sources
9ce5fdc: 8305421: Work around JDK-8305420 in CDSJDITest.java
b062b1b: 8304743: Compile_lock and SystemDictionary updates
df819cf: 8304945: StringBuilder and StringBuffer should implement Appendable explicitly
312bbe7: 8305485: Problemlist runtime/Thread/TestAlwaysPreTouchStacks.java
50e31e0: 8305442: (bf) Direct and view implementations of CharBuffer.toString(int, int) do not need to catch SIOBE
... and 403 more: https://git.openjdk.org/jdk/compare/f3abc4063de658418283aee1f552c4b4976e5211...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@naotoj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

justin-curtis-lu · 2023-04-04T05:53:40Z

/integrate

openjdk · 2023-04-04T05:54:29Z

@justin-curtis-lu
Your change (at version 06b9038) is now ready to be sponsored by a Committer.

naotoj · 2023-04-04T20:59:09Z

/sponsor

openjdk · 2023-04-04T21:01:40Z

Going to push as commit a324fa2.
Since your change was applied there have been 430 commits pushed to the master branch:

3399fbf: 8305602: ProblemList java/lang/invoke/lambda/LogGeneratedClassesTest.java
94a05e0: 8305599: (fc) Temporarily problem-list java/nio/channels/{AsyncCloseAndInterrupt.java, FileChannel/Transfer.java}
7c65048: 8305343: BigDecimal.fractionOnly() erroneously returns true for large scale value
dd59471: 8304846: Provide a shared utility to dump generated classes defined via Lookup API
2ee4245: 8305509: C1 fails "assert(k != nullptr) failed: illegal use of unloaded klass"
7baccd9: 8303260: (fc) FileChannel::transferFrom should support position > size()
3ef834f: 8298619: java/io/File/GetXSpace.java is failing
c594119: 8305511: Remove ignore from com/sun/jdi/PopAndInvokeTest.java
62bd2eb: 8303147: [s390x] fast & slow debug builds are broken
f9ed825: 8303082: [AIX] Missing C++ name demangling with XLClang++
... and 420 more: https://git.openjdk.org/jdk/compare/f3abc4063de658418283aee1f552c4b4976e5211...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-04T21:02:07Z

@naotoj @justin-curtis-lu Pushed as commit a324fa2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

justin-curtis-lu added 2 commits March 14, 2023 12:46

Bug fix to GregCal with accompanying test

d95da46

method positioning

2238748

openjdk bot added the rfr Pull request is ready for review label Mar 14, 2023

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Mar 14, 2023

naotoj reviewed Mar 15, 2023

View reviewed changes

justin-curtis-lu added 3 commits March 24, 2023 10:52

Add safety check that amount is positive

ceb0f1e

Test all possible min week and first day of week combos for bugged date

fe2f92f

Expand testing to all end of year days that roll into week 1

a844bb9

Remove max week methods accidentally comitted

f837268

naotoj reviewed Mar 27, 2023

View reviewed changes

justin-curtis-lu added 5 commits March 28, 2023 10:51

Reuse Calendar.Builder

d6c08f7

Pass in amount as 1 directly

6cde8df

Add type to static declaration of Cal.Builder, since only concerned w…

6aca4a2

…ith Gregorian for this test

Rename test, clarify test documentation

1e46343

Impl cleanup, add Saturday end day conditional

60d0621

naotoj reviewed Mar 29, 2023

View reviewed changes

justin-curtis-lu added 2 commits March 30, 2023 11:07

Tweak static cal builder

17b2e75

dayInMinWeek should range check input and throw exception

7c9291a

naotoj reviewed Mar 30, 2023

View reviewed changes

getFirstDayInWeek() is well checked, so need for exception in dayInMi…

06b9038

…nWeek

naotoj approved these changes Mar 31, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 31, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 4, 2023

openjdk bot added the integrated Pull request has been integrated label Apr 4, 2023

openjdk bot closed this Apr 4, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 4, 2023

		throw new IllegalArgumentException("Start day or end day is not " +
		"a valid day of the week");

8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR #13031

8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR #13031

Uh oh!

Conversation

justin-curtis-lu commented Mar 14, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 14, 2023

Uh oh!

openjdk bot commented Mar 14, 2023

Uh oh!

mlbridge bot commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justin-curtis-lu commented Mar 17, 2023

Uh oh!

justin-curtis-lu commented Mar 24, 2023

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

justin-curtis-lu commented Mar 14, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 14, 2023 •

edited

Loading

openjdk bot commented Mar 31, 2023 •

edited

Loading