Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ISO8601Utils parser to default to UTC timezone when the value is not provided #2480

Conversation

HyperspaceAlmanac
Copy link

@HyperspaceAlmanac HyperspaceAlmanac commented Aug 26, 2023

Purpose

This is to create a fix for #1511 which is marked as a bug. Currently the parser defaults to the local timezone when the value is not provided, but the issue marked as a bug specifies that it should instead parse the time using UTC timezone.

Description

This is a fix for #1511

There is existing unit test that checks the parsed time against local timezone. I updated that unit test to instead compare against time generated using UTC time zone.

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

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Sep 3, 2023

Thanks a lot for you pull request!

I am not quite sure what the correct behavior here would be. On one hand relying implicitly on system default values (such as the default timezone here) is most often not ideal and can lead to unexpected behavior and bugs. But on the other hand maybe in this case using the default timezone can be desired in some cases:

If we try to compare this with the java.time API, then

  • the pattern yyyy-MM-dd is equivalent to java.time.LocalDate
  • java.util.Date is equivalent to java.time.Instant

For java.time conversion from LocalDate to Instant explicitly requires specifying a timezone.

Since here for Gson the conversion happens implicitly, I guess the desired behavior might be application-specific, and there might be cases where the default timezone is desired.
This is only my personal opinion on this; and I don't have any specific examples at the moment for either of those cases.

Sorry if the "bug" label on the GitHub issue you mentioned gave a false impression. I added the label back then while trying to clean up stale and resolved GitHub issues. Similar to new GitHub issues which automatically get the label based on which issue type the reporter selects, it is mainly intended to indicate that the reporter considers this a bug, but not necessarily that this is "acknowledged" and will definitely be fixed.

@eamonnmcmanus, what do you think?

As side note, the unit test which had to be modified here was apparently introduced by an external contributor in #1638, so it might just have matched the actual behavior back then, without making any statement about whether that behavior is desired.

@Marcono1234
Copy link
Collaborator

In FasterXML/jackson-databind#816 where this date format was requested for Jackson (and where this Gson class is coming from), the reporter explicitly said:

ISO8601Utils.parse() should be modified to accept a date-only string (YYYY-MM-DD) with no time zone and return the value as a date corresponding to midnight on that day in the local (default) time zone.

So it seems there are indeed cases where the default time zone is desired.

@Marcono1234
Copy link
Collaborator

@eamonnmcmanus what do you think?

@eamonnmcmanus
Copy link
Member

The state of date/time parsing in Gson is indeed not great. One of the reasons it's not great is that we can't easily change things without breaking deployments that were depending on the previous behaviour. That's the case here: people almost certainly are depending on the way this works currently. Changing it could cause things to fail in very subtle ways. We could conceivably have some sort of non-default configuration option to switch to UTC, but we can't just switch unconditionally.

I think it would be worth undertaking a significant effort to improve the date/time situation. Just fixing this one thing in isolation isn't going to help much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants