Fix DateFormat time zone is not restored and add Test. #2549

Carpe-Wang · 2023-11-20T13:00:10Z

Purpose

Fix DateFormat time zone is not restored after parsing. Fixes #2547

Description

Before attempting to parse the date string, save the current timezone for each DateFormat object. Use a finally block to ensure that the original timezone is restored for each DateFormat object.

Checklist

New code follows the Google Java Style Guide
This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
If necessary, new public API validates arguments, for example rejects null
New public API has Javadoc
- Javadoc uses @since $next-version$
  ( $next-version$ is a special placeholder which is automatically replaced during release)
If necessary, new unit tests have been added
- Assertions in unit tests use Truth, see existing tests
- No JUnit 3 features are used (such as extending class TestCase)
- If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
mvn clean verify javadoc:jar passes without errors

Carpe-Wang · 2023-11-20T13:24:40Z

I just fetch and pull the new code, and I use mvn clean test -X. there are some detailed error information as followed.

[ERROR] /Users/carpewang/googleDeveloper/gson/gson/src/main/java/module-info.java:[29,40] module not found: com.google.errorprone.annotations
[ERROR] Cannot access com.google.gson.module-info
[ERROR] Unable to resolve module
[ERROR] -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project gson: Compilation failure

And i git clone the gson' master branch code and use mvn clean test -X command, there a same error.
I try my best to solve the error, but I can't. could you help me in your spare time?

Marcono1234 · 2023-11-20T22:59:39Z

Which JDK and Maven version are you using? Could you please provide the output of mvn --version?

Carpe-Wang · 2023-11-21T01:06:49Z

Which JDK and Maven version are you using? Could you please provide the output of mvn --version?

Thank you for your reply. there is output of mvn --version

carpewang@bogon gson % mvn --version
Apache Maven 3.9.5 (57804ffe001d7215b5e7bcb531cf83df38f93546)
Maven home: /Users/carpewang/Desktop/apache-maven-3.9.5
Java version: 21.0.1, vendor: Oracle Corporation, runtime: /Users/carpewang/Library/Java/JavaVirtualMachines/openjdk-21.0.1/Contents/Home
Default locale: zh_CN_#Hans, platform encoding: UTF-8
OS name: "mac os x", version: "11.7.4", arch: "x86_64", family: "mac"

Marcono1234 · 2023-11-21T18:20:43Z

It looks like you are using JDK 21; could you please try JDK 17 instead? Gson currently does not support building with JDK 21, see #2501.

Carpe-Wang · 2023-11-22T12:51:32Z

It looks like you are using JDK 21; could you please try JDK 17 instead? Gson currently does not support building with JDK 21, see #2501.

ok, thank you for your help, I solve the error, and I am trying to do the picture which you mentioned before

and add the test.

…eAdapter

Carpe-Wang · 2023-11-22T14:57:47Z

It looks like you are using JDK 21; could you please try JDK 17 instead? Gson currently does not support building with JDK 21, see #2501.

ok, thank you for your help, I solve the error, and I am trying to do the picture which you mentioned before

and add the test.

Hello, I had finished the SqlTimeTypeAdapter and SqlDateTypeAdapter. Please review the code.

Marcono1234

Thanks a lot! I have added a few small review comments but generally at least to me this PR looks good.

Marcono1234 · 2023-11-22T21:17:58Z

gson/src/main/java/com/google/gson/internal/sql/SqlDateTypeAdapter.java

+        TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone
        utilDate = format.parse(s);
+        format.setTimeZone(originalTimeZone); // Restore the original time zone after parsing


Please use try-finally here as well, to be safe (in case time zone was already changed despite an exception being thrown):

Suggested change

TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone

utilDate = format.parse(s);

format.setTimeZone(originalTimeZone); // Restore the original time zone after parsing

TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone

try {

utilDate = format.parse(s);

} finally {

format.setTimeZone(originalTimeZone); // Restore the original time zone after parsing

}

(you might to have to reformat the code above)

Marcono1234 · 2023-11-22T21:18:36Z

gson/src/main/java/com/google/gson/internal/sql/SqlTimeTypeAdapter.java

+        TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone
        Date date = format.parse(s);
+        format.setTimeZone(originalTimeZone); // Restore the original time zone


Please use try-finally here as well:

Suggested change

TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone

Date date = format.parse(s);

format.setTimeZone(originalTimeZone); // Restore the original time zone

TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone

try {

date = format.parse(s);

} finally {

format.setTimeZone(originalTimeZone); // Restore the original time zone after parsing

}

(you might to have to reformat the code above)

gson/src/test/java/com/google/gson/internal/bind/DefaultDateTypeAdapterTest.java

Marcono1234 · 2023-11-22T21:26:07Z

gson/src/test/java/com/google/gson/internal/bind/DefaultDateTypeAdapterTest.java

+    Date deserializedDate = gson.fromJson("\"1970-01-01 00:00 PST\"", Date.class);
+
+    // Serialize the deserialized date object again
+    String jsonAfterDeserialization = gson.toJson(deserializedDate);


Maybe this should rather call toJson(originalDate); otherwise if deserializedDate is incorrect due to a bug we would get confusing failures here.

Marcono1234 · 2023-11-22T21:27:28Z

gson/src/test/java/com/google/gson/internal/bind/DefaultDateTypeAdapterTest.java

+    assertEquals("\"1970-01-01 00:00 UTC\"", json);
+
+    // Deserialize a date string with the PST timezone
+    Date deserializedDate = gson.fromJson("\"1970-01-01 00:00 PST\"", Date.class);


Maybe add an assertion here that the date was parsed correctly, I assume it would be this:

Suggested change

Date deserializedDate = gson.fromJson("\"1970-01-01 00:00 PST\"", Date.class);

Date deserializedDate = gson.fromJson("\"1970-01-01 00:00 PST\"", Date.class);

assertThat(deserializedDate).isEqualTo(new Date(28800000));

Marcono1234 · 2023-11-22T21:30:08Z

gson/src/test/java/com/google/gson/internal/bind/DefaultDateTypeAdapterTest.java

+    String jsonAfterDeserialization = gson.toJson(deserializedDate);
+    // The expectation is that the date, after deserialization, when serialized again should still
+    // be in the UTC timezone
+    assertEquals("\"1970-01-01 08:00 UTC\"", jsonAfterDeserialization);


Please use Truth instead of the JUnit methods here; that will leads to better failure messages and makes the assertion easier to read:

Suggested change

assertEquals("\"1970-01-01 08:00 UTC\"", jsonAfterDeserialization);

assertThat(jsonAfterDeserialization).isEqualTo("\"1970-01-01 08:00 UTC\"");

Marcono1234 · 2023-11-22T21:39:19Z

One more thing: Could you please edit the description to say "Fixes #2547" instead of "About issue is #2547".
GitHub will then link this PR to the issue (shown on the right under "Successfully merging this pull request may close these issues"), and once this PR is merged will automatically close the issue as well.

Just in case you didn't know this, you can edit the description of the PR by clicking the "..." at the top right:

Carpe-Wang · 2023-11-23T00:26:16Z

Thanks a lot! I have added a few small review comments but generally at least to me this PR looks good.

thanks I had change the pr description, and add try finally in DefaultDateTypeAdapterTest, SqlTimeTypeAdapter, and SqlDateTypeAdapter. But I meet some troubles when I want change testGsonDateFormat in DefaultDateTypeAdapterTest. Gson BUILD FAILURE because of

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:testCompile (default-testCompile) on project gson: Compilation failure
[ERROR] Warnings found, but -Werror was specified
[ERROR] 
[ERROR] -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:testCompile (default-testCompile) on project gson: Compilation failure
Warnings found, but -Werror was specified

Marcono1234 · 2023-11-24T20:32:42Z

gson/src/main/java/com/google/gson/internal/sql/SqlDateTypeAdapter.java

@@ -60,17 +60,18 @@ public java.sql.Date read(JsonReader in) throws IOException {
      return null;
    }
    String s = in.nextString();
+    TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone


Please move this and the setTimeZone call inside the synchronized (this) { ... } block. Type adapters must be thread-safe and with the current implementation there could be a race condition.

Or rewrite this so that the synchronized block covers the try; I guess that should be fine as well, e.g.:

synchronized (this) { TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone try { Date utilDate = format.parse(s); return new java.sql.Date(utilDate.getTime()); } catch (ParseException e) { throw new JsonSyntaxException("Failed parsing '" + s + "' as SQL Date; at path " + in.getPreviousPath(), e); } finally { format.setTimeZone(originalTimeZone); // Restore the original time zone after parsing } }

This is my mistake; I did not carefully read the code review you provided at the beginning.

Marcono1234 · 2023-11-24T20:33:13Z

gson/src/main/java/com/google/gson/internal/sql/SqlTimeTypeAdapter.java

@@ -61,16 +61,17 @@ public Time read(JsonReader in) throws IOException {
      return null;
    }
    String s = in.nextString();
+    TimeZone originalTimeZone = format.getTimeZone(); // Save the original time zone


Same as #2549 (comment)

Marcono1234 · 2023-11-24T20:39:40Z

But I meet some troubles when I want change testGsonDateFormat in DefaultDateTypeAdapterTest. Gson BUILD FAILURE because of
Warnings found, but -Werror was specified

You have to look further up in the Maven log output then; there should be one or more "[WARNING] ..." lines for the maven-compiler-plugin. Alternatively, you can also push the non-working code if you want so that we can see the error message here in the GitHub workflow log, and suggest how to solve it.

Carpe-Wang · 2023-11-25T10:26:56Z

Date deserializedDate = gson.

I solved it. Thank you for your patient replys. I believe that the suggestion in the image to use toJson(originalDate) is unnecessary because I have already performed toJson on both originalDate and deserializedDate, with corresponding assertions for each.

Marcono1234

I believe that the suggestion in the image to use toJson(originalDate) is unnecessary because I have already performed toJson on both originalDate and deserializedDate, with corresponding assertions for each.

Ok, that is fine for me now because you added assertThat(deserializedDate.getTime()).isEqualTo(...).

Just two more small things, see comments.

Marcono1234 · 2023-11-25T22:12:00Z

gson/src/test/java/com/google/gson/internal/bind/DefaultDateTypeAdapterTest.java

+      Date expectedDate = new Date(28800000); // This represents the original date in UTC
+      String expectedJson = gson.toJson(expectedDate);
+      assertThat(jsonAfterDeserialization).isEqualTo(expectedJson);


Please hardcode the expected JSON string here:

Suggested change

Date expectedDate = new Date(28800000); // This represents the original date in UTC

String expectedJson = gson.toJson(expectedDate);

assertThat(jsonAfterDeserialization).isEqualTo(expectedJson);

assertThat(jsonAfterDeserialization).isEqualTo("\"1970-01-01 08:00 UTC\"");

Otherwise your current code would fail to detect the bug (if you reverted your setTimeZone fix) because both gson.toJson(deserializedDate) and gson.toJson(expectedDate) would use PST as time zone, and would therefore both be wrong. But because both are wrong in the same way, the test would erroneously pass as successful.

Marcono1234 · 2023-11-25T22:12:39Z

gson/src/test/java/com/google/gson/internal/bind/DefaultDateTypeAdapterTest.java

+
+      // Serialize the date object
+      String json = gson.toJson(originalDate);
+      assertEquals("\"1970-01-01 00:00 UTC\"", json);


Please use Truth for assertions here:

Suggested change

assertEquals("\"1970-01-01 00:00 UTC\"", json);

assertThat(json).isEqualTo("\"1970-01-01 00:00 UTC\"");

…edback.

Carpe-Wang · 2023-11-26T02:39:13Z

I believe that the suggestion in the image to use toJson(originalDate) is unnecessary because I have already performed toJson on both originalDate and deserializedDate, with corresponding assertions for each.

Ok, that is fine for me now because you added assertThat(deserializedDate.getTime()).isEqualTo(...).

Just two more small things, see comments.

Thank you for your suggestions; they are helpful for improving my skills. I have made the changes according to your comments.

Marcono1234

Thanks for your work on this! The changes look good to me.
@eamonnmcmanus, could you please have a look?

Just one small note, but you don't have to change the code for that:
I thought a bit more about the gson.toJson(deserializedDate) in the test. This bothers me a bit because the point is that the time zone of the DateFormat is not restored. The current test code makes it look a bit as if Date internally stored a time zone as well (which it doesn't), and as if the DateFormat 'forces' usage of UTC even though the Date has PST as time zone (again, which is not actually the case).
That is why I think it would have been better to call gson.toJson(originalDate) a second time instead of gson.toJson(deserializedDate), because that makes it clearer that the issue here is with the DateFormat, and not with the deserialized Date.

Carpe-Wang · 2023-11-29T01:01:07Z

Thanks for your work on this! The changes look good to me. @eamonnmcmanus, could you please have a look?

Just one small note, but you don't have to change the code for that: I thought a bit more about the gson.toJson(deserializedDate) in the test. This bothers me a bit because the point is that the time zone of the DateFormat is not restored. The current test code makes it look a bit as if Date internally stored a time zone as well (which it doesn't), and as if the DateFormat 'forces' usage of UTC even though the Date has PST as time zone (again, which is not actually the case). That is why I think it would have been better to call gson.toJson(originalDate) a second time instead of gson.toJson(deserializedDate), because that makes it clearer that the issue here is with the DateFormat, and not with the deserialized Date.

Thank you for your patient guidance, which is meaningful for developing my skills and valuable to me.

Carpe-Wang · 2023-11-30T16:25:46Z

Hello @eamonnmcmanus, if there are any changes that you are still dissatisfied with, or if you think there are areas that can be improved, please let me know.

eamonnmcmanus

Thanks for doing this! And sorry it took me a while to get to it.

Fix DateFormat time zone is not restored and add Test.

46942b6

Carpe-Wang marked this pull request as draft November 20, 2023 13:03

Marcono1234 mentioned this pull request Nov 22, 2023

DateFormat time zone is not restored after parsing, affecting subsequent serialization #2547

Closed

Carpe-Wang added 2 commits November 22, 2023 20:44

delete the test and make sure mvn clean test -X can be SUCCESS

6ad06f9

delete the test and make sure mvn clean test -X can be SUCCESS

fb8c463

Carpe-Wang added 2 commits November 22, 2023 21:51

add test

fb850a5

restore the DateFormat time zone in SqlDateTypeAdapter and SqlTimeTyp…

7928fe4

…eAdapter

Carpe-Wang marked this pull request as ready for review November 22, 2023 14:56

Marcono1234 reviewed Nov 22, 2023

View reviewed changes

Carpe-Wang and others added 3 commits November 23, 2023 07:57

Merge branch 'main' into main

3da89da

Adjust the code according to the code review feedback.

db4ff5e

Adjust the code according to the code review feedback.

7b8e91a

Marcono1234 reviewed Nov 24, 2023

View reviewed changes

Carpe-Wang added 3 commits November 25, 2023 17:50

Adjust the code according to the code review feedback.

1687fb1

Adjust the Test

23bbbca

fix Werror error

3b7c7d0

Marcono1234 reviewed Nov 25, 2023

View reviewed changes

Adjust the DefaultDateTypeAdapterTest according to the code review fe…

a8a13e7

…edback.

Carpe-Wang requested a review from Marcono1234 November 26, 2023 09:01

Marcono1234 approved these changes Nov 26, 2023

View reviewed changes

Carpe-Wang added 2 commits November 28, 2023 08:59

Merge branch 'main' into main

49f0caf

Merge branch 'main' into main

edc10e1

Carpe-Wang added 3 commits December 3, 2023 07:52

Merge branch 'main' into main

72cc078

Merge branch 'main' into main

d3767a8

Merge branch 'main' into main

972900a

eamonnmcmanus approved these changes Dec 5, 2023

View reviewed changes

eamonnmcmanus merged commit 58d1a9f into google:main Dec 5, 2023
11 checks passed

stebbi36 mentioned this pull request Dec 5, 2023

Thanks for doing this! And sorry it took me a while to get to it. mapbox/mapbox-maps-ios#2071

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DateFormat time zone is not restored and add Test. #2549

Fix DateFormat time zone is not restored and add Test. #2549

Carpe-Wang commented Nov 20, 2023 •

edited

Carpe-Wang commented Nov 20, 2023 •

edited

Marcono1234 commented Nov 20, 2023

Carpe-Wang commented Nov 21, 2023

Marcono1234 commented Nov 21, 2023

Carpe-Wang commented Nov 22, 2023

Carpe-Wang commented Nov 22, 2023

Marcono1234 left a comment

Marcono1234 Nov 22, 2023

Marcono1234 Nov 22, 2023

Marcono1234 Nov 22, 2023

stebbi36 Dec 5, 2023

Marcono1234 Nov 22, 2023

Marcono1234 Nov 22, 2023

Marcono1234 commented Nov 22, 2023

Carpe-Wang commented Nov 23, 2023

Marcono1234 Nov 24, 2023 •

edited

Carpe-Wang Nov 25, 2023

Marcono1234 Nov 24, 2023 •

edited

Marcono1234 commented Nov 24, 2023

Carpe-Wang commented Nov 25, 2023

Marcono1234 left a comment

Marcono1234 Nov 25, 2023

Marcono1234 Nov 25, 2023

Carpe-Wang commented Nov 26, 2023

Marcono1234 left a comment •

edited

Carpe-Wang commented Nov 29, 2023 •

edited

Carpe-Wang commented Nov 30, 2023

eamonnmcmanus left a comment

	Date deserializedDate = gson.fromJson("\"1970-01-01 00:00 PST\"", Date.class);
	Date deserializedDate = gson.fromJson("\"1970-01-01 00:00 PST\"", Date.class);
	assertThat(deserializedDate).isEqualTo(new Date(28800000));

	assertEquals("\"1970-01-01 08:00 UTC\"", jsonAfterDeserialization);
	assertThat(jsonAfterDeserialization).isEqualTo("\"1970-01-01 08:00 UTC\"");

	assertEquals("\"1970-01-01 00:00 UTC\"", json);
	assertThat(json).isEqualTo("\"1970-01-01 00:00 UTC\"");

Fix DateFormat time zone is not restored and add Test. #2549

Fix DateFormat time zone is not restored and add Test. #2549

Conversation

Carpe-Wang commented Nov 20, 2023 • edited

Purpose

Description

Checklist

Carpe-Wang commented Nov 20, 2023 • edited

Marcono1234 commented Nov 20, 2023

Carpe-Wang commented Nov 21, 2023

Marcono1234 commented Nov 21, 2023

Carpe-Wang commented Nov 22, 2023

Carpe-Wang commented Nov 22, 2023

Marcono1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 commented Nov 22, 2023

Carpe-Wang commented Nov 23, 2023

Marcono1234 Nov 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 Nov 24, 2023 • edited

Choose a reason for hiding this comment

Marcono1234 commented Nov 24, 2023

Carpe-Wang commented Nov 25, 2023

Marcono1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carpe-Wang commented Nov 26, 2023

Marcono1234 left a comment • edited

Choose a reason for hiding this comment

Carpe-Wang commented Nov 29, 2023 • edited

Carpe-Wang commented Nov 30, 2023

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Carpe-Wang commented Nov 20, 2023 •

edited

Carpe-Wang commented Nov 20, 2023 •

edited

Marcono1234 Nov 24, 2023 •

edited

Marcono1234 Nov 24, 2023 •

edited

Marcono1234 left a comment •

edited

Carpe-Wang commented Nov 29, 2023 •

edited