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

Support more formats in ZonedDateTimeKeyDeserializer #305

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

caluml
Copy link
Contributor

@caluml caluml commented Mar 19, 2024

ZonedDateTimeKeyDeserializer was only able to deserialize ZonedDateTimes that matched the DateTimeFormatter.ISO_OFFSET_DATE_TIME format.

This PR removes that restriction, allowing for more formats, such as

  • 2015-07-24T12:23:34.184Z[UTC]
  • 2015-07-24T13:23:34.184+01:00[Europe/London]
  • 2015-07-24T12:23:34.184+02:00

to be used as keys.

}

@Test
public void Instant_style_can_be_deserialized() throws JsonProcessingException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you change this to throws Exception -- just so it's easier to merge into master branch (Jackson 3.0) where JsonProcessingException is replaced by StreamReadException.
(and same for other instances)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cowtowncoder
Copy link
Member

Ok this looks good, makes sense & could go in 2.17(.1) patch since API does not change.

But one process thing before I can merge PR: if we haven't yet received CLA from you (it only needs to be sent once, one is good for all contributions), we'd need it from here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

the usual way is to print, fill & sign, scan/photo, email to cla at fasterxml dot com.
Once we receive it I can proceed merging this.

Looking forward to getting this improvement!

@cowtowncoder
Copy link
Member

Looks like there's a test failure for JDK 8 for some reason (off-by-hour value -- Daylight Savings Time problem?)

@caluml
Copy link
Contributor Author

caluml commented Mar 22, 2024

Looks like there's a test failure for JDK 8 for some reason (off-by-hour value -- Daylight Savings Time problem?)

This is a weird one. I downloaded jdk8u402-b06 and I get the same problem locally, so it's nothing to do with the build hosts.
I tried changing the dates to 2015-01-24 in the tests (to avoid any DST issues), but the tests still fail.
I will continue investigating.

This is because Java 8 parses the supplied string differently to
later Java versions.
Specifying timezones either through environment variables, command-
line, or within the JVM didn't make any difference.
@caluml
Copy link
Contributor Author

caluml commented Mar 22, 2024

I spent some time investigating the test failure, and it is down to a bug in the way Java 8 parses the string.
https://bugs.openjdk.org/browse/JDK-8066982

I tried specifying timezones on the command line, in environment variables, to Surefire in the pom, and within the Java code itself, but these don't make any difference.
I tried setting the date to 2015-01-24 (so it wouldn't be within any DST range) and that didn't work either.
So unfortunately, the only answer to get the tests passing is to use a different test for Java 8, which feels nasty.

@cowtowncoder
Copy link
Member

@caluml that's nasty indeed. :-(

I think that if there was an easy enough way to exclude the test on JDK 8, I'd be ok with that (along with comment on why it is @Disabled or so)?
(I'm sure this is doable, would just need to google for a reasonably clean mechanism, I don't think there's annotation).

@caluml
Copy link
Contributor Author

caluml commented Mar 22, 2024

Yes, it's not ideal, is it?

But the logic to run one test on Java 8, and the other test on non-Java 8 is already in there - see

String javaVersion = System.getProperty("java.version");
assumeFalse(javaVersion.startsWith("1.8"));

and

String javaVersion = System.getProperty("java.version");
assumeTrue(javaVersion.startsWith("1.8"));

https://javadoc.io/static/junit/junit/4.13.2/org/junit/Assume.html#assumeTrue(boolean)

It should run and pass on Java 8, 11, 17 and 21 now.

@cowtowncoder cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Mar 23, 2024
@cowtowncoder
Copy link
Member

Ok, now all I need is the CLA and we are good to go! (I did see your question; I hope answer is acceptable)

@cowtowncoder cowtowncoder changed the title Support more formats in ZonedDateTimeKeyDeserializer Support more formats in ZonedDateTimeKeyDeserializer Mar 23, 2024
@cowtowncoder cowtowncoder removed the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Mar 27, 2024
@cowtowncoder
Copy link
Member

CLA received, can proceed.

@cowtowncoder cowtowncoder merged commit 9e1ba80 into FasterXML:2.17 Mar 27, 2024
4 checks passed
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.

2 participants