FasterXML / jackson-modules-java8

Set of support modules for Java 8 datatypes (Optionals, date/time) and features (parameter names)
Apache License 2.0
398 stars 116 forks source link

Support more formats in `ZonedDateTimeKeyDeserializer` #305

Closed caluml closed 5 months ago

caluml commented 5 months ago

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

to be used as keys.

cowtowncoder commented 5 months ago

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 commented 5 months ago

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

caluml commented 5 months ago

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.

caluml commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

CLA received, can proceed.