validating the date format, add test case, since NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException. #2529

Carpe-Wang · 2023-11-06T15:06:38Z

validating the date format, add test case and since NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException.

Purpose

Throw exception if public GsonBuilder setDateFormat(String pattern) input pattern is invalid. [Relate issue](Change default Date serialization to ISO 8601 format #2472)
NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException in public class ISO8601Utils

Checklist

New code follows the Google Java Style Guide
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

…NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException.

google-cla · 2023-11-06T15:06:42Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

eamonnmcmanus

This looks great, thanks! Could you add a test, for example in DefaultTypeAdaptersTest, that fails with the current code and passes with this change? You could use assertThrows: with the current code the test will fail because no exception will be thrown.

eamonnmcmanus · 2023-11-06T16:14:11Z

gson/src/test/java/com/google/gson/functional/DefaultTypeAdaptersTest.java

+      builder.setDateFormat(invalidPattern);
+    });
+  }
+  @Test


Just one tiny thing: could you add a blank line before each of these new test methods?

OK, I am sorry about that.

Carpe-Wang · 2023-11-06T16:37:30Z

This looks great, thanks! Could you add a test, for example in DefaultTypeAdaptersTest, that fails with the current code and passes with this change? You could use assertThrows: with the current code the test will fail because no exception will be thrown.

Please check again to ensure it meets the requirements.

eamonnmcmanus

Thanks! I am just checking this against Google's internal tests to see if they show up anything unexpected. If not, I'll go ahead and merge this.

Marcono1234 · 2023-11-06T22:12:29Z

Should the new tests maybe be moved to GsonBuilderTest instead since they seem to only test GsonBuilder functionality?

Also, could you please change the title of this PR, there is an "Edit" button at the top right of the GitHub UI¹. The main change here seems to be about validating the date format, and issue 2472 might not be that closely related that it is worth mentioning it in the final commit message after merge. What do you think about something like this as title (based on what you wrote in the description)?

Throw exception if GsonBuilder.setDateFormat input pattern is invalid

Or alternatively, @eamonnmcmanus could you please adjust the message when merging? ↩

Marcono1234 · 2023-11-06T22:19:02Z

gson/src/main/java/com/google/gson/GsonBuilder.java

@@ -595,7 +596,12 @@ public GsonBuilder disableHtmlEscaping() {
   */
  @CanIgnoreReturnValue
  public GsonBuilder setDateFormat(String pattern) {
-    // TODO(Joel): Make this fail fast if it is an invalid date format
+    try {


It appears null might actually be a valid pattern value to clear any custom pattern previously set for the builder. The changes here would not support that anymore.

@eamonnmcmanus, should null be supported?

Apparently no unit test is covering that yet, might be good to add one (or I can add one in a separate PR if desired).

if pattern is null, here will throw null point exception.

Carpe-Wang · 2023-11-07T13:44:30Z

Should the new tests maybe be moved to GsonBuilderTest instead since they seem to only test GsonBuilder functionality?

Also, could you please change the title of this PR, there is an "Edit" button at the top right of the GitHub UI1. The main change here seems to be about validating the date format, and issue 2472 might not be that closely related that it is worth mentioning it in the final commit message after merge. What do you think about something like this as title (based on what you wrote in the description)?

Throw exception if GsonBuilder.setDateFormat input pattern is invalid

Footnotes

Or alternatively, @eamonnmcmanus could you please adjust the message when merging? ↩

I had change the title. Please check again to ensure it meets the requirements. In addition I'm not sure if I need to move the test cases to GsonBuilderTest, and I see a conflict with a small note that says "Only those with write access to this repository can merge pull requests." Do I need to resolve this conflict by myself?

eamonnmcmanus · 2023-11-07T16:33:44Z

I see a conflict with a small note that says "Only those with write access to this repository can merge pull requests." Do I need to resolve this conflict by myself?

Yes, you should resolve the conflict. I will merge the PR when it is ready.

I don't have a strong opinion on where the new tests should be. We may move them later. On the other hand, I agree with @Marcono1234 that we probably shouldn't change the behaviour for setDateFormat(null). So the call to new SimpleDateFormat should be guarded by a != null check.

gson/src/main/java/com/google/gson/internal/bind/util/ISO8601Utils.java

+                    offset += 1;
+                }
+                // second and milliseconds can be optional
+                if (date.length() > offset) {


Carpe-Wang · 2023-11-07T16:56:43Z

gson/src/main/java/com/google/gson/internal/bind/util/ISO8601Utils.java


-      if (hasT) {
+            if (!hasT && (date.length() <= offset)) {


I am trying to solve that

Carpe-Wang · 2023-11-08T02:23:19Z

I close the pr, because I make error when I solve conflict. I make a new pr 2538. Sorry for that.

issue 2472 throw exception if it is an invalid date format and since …

11281be

…NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException.

eamonnmcmanus requested changes Nov 6, 2023

View reviewed changes

Carpe-Wang and others added 4 commits November 6, 2023 23:51

add test

04c04d2

Merge branch 'main' into main

48e6e99

add test

3bd2e14

Merge remote-tracking branch 'origin/main'

75278c8

eamonnmcmanus reviewed Nov 6, 2023

View reviewed changes

add a blank line before each of test methods

26e04d0

eamonnmcmanus approved these changes Nov 6, 2023

View reviewed changes

Marcono1234 reviewed Nov 6, 2023

View reviewed changes

Carpe-Wang changed the title ~~issue 2472 throw exception if it is an invalid date format and since …~~ validating the date format, add test case, since NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException. Nov 7, 2023

Carpe-Wang and others added 2 commits November 8, 2023 00:41

add if pattern != null

8ac0ffb

Merge branch 'main' into main

1a671b2

github-advanced-security bot found potential problems Nov 7, 2023

View reviewed changes

Carpe-Wang closed this Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validating the date format, add test case, since NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException. #2529

validating the date format, add test case, since NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException. #2529

Carpe-Wang commented Nov 6, 2023 •

edited

Loading

google-cla bot commented Nov 6, 2023

eamonnmcmanus left a comment

eamonnmcmanus Nov 6, 2023

Carpe-Wang Nov 6, 2023

Carpe-Wang commented Nov 6, 2023

eamonnmcmanus left a comment

Marcono1234 commented Nov 6, 2023

Marcono1234 Nov 6, 2023

Carpe-Wang Nov 7, 2023

Carpe-Wang commented Nov 7, 2023

Footnotes

eamonnmcmanus commented Nov 7, 2023

Carpe-Wang Nov 7, 2023

Carpe-Wang commented Nov 8, 2023

validating the date format, add test case, since NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException. #2529

validating the date format, add test case, since NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException. #2529

Conversation

Carpe-Wang commented Nov 6, 2023 • edited Loading

Purpose

Checklist

google-cla bot commented Nov 6, 2023

eamonnmcmanus left a comment

Choose a reason for hiding this comment

eamonnmcmanus Nov 6, 2023

Choose a reason for hiding this comment

Carpe-Wang Nov 6, 2023

Choose a reason for hiding this comment

Carpe-Wang commented Nov 6, 2023

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Marcono1234 commented Nov 6, 2023

Footnotes

Marcono1234 Nov 6, 2023

Choose a reason for hiding this comment

Carpe-Wang Nov 7, 2023

Choose a reason for hiding this comment

Carpe-Wang commented Nov 7, 2023

Footnotes

eamonnmcmanus commented Nov 7, 2023

Carpe-Wang Nov 7, 2023

Choose a reason for hiding this comment

Carpe-Wang commented Nov 8, 2023

Carpe-Wang commented Nov 6, 2023 •

edited

Loading