-
Notifications
You must be signed in to change notification settings - Fork 117
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
Conversation
.../main/java/com/fasterxml/jackson/datatype/jsr310/deser/key/ZonedDateTimeKeyDeserializer.java
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
public void Instant_style_can_be_deserialized() throws JsonProcessingException { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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 Looking forward to getting this improvement! |
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. |
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.
I spent some time investigating the test failure, and it is down to a bug in the way Java 8 parses the string. 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. |
@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 |
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
and
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. |
Ok, now all I need is the CLA and we are good to go! (I did see your question; I hope answer is acceptable) |
ZonedDateTimeKeyDeserializer
CLA received, can proceed. |
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.