[695] Fix date deserialization for date-only patterns#696
[695] Fix date deserialization for date-only patterns#696jamezp merged 3 commits intoeclipse-ee4j:masterfrom
Conversation
KyleAure
left a comment
KyleAure
left a comment
There was a problem hiding this comment.
I think this is a good solution to the issue and handles many more time types, but it effectively makes the config property jsonb.zero-time-defaulting obsolete.
I'm a bit new to this project so I am not sure how this has been handled in the past, but it would be a good idea to either: 1. remove this property or 2. make it a no-op and document it. Unless there are any other use cases where you think it would useful?
|
Thank you @KyleAure. I left that property alone for compatibility, but you're absolutely right that it doesn't make much sense to keep it. Maybe we should formally deprecate with the reason and make it a no-op. |
|
@KyleAure I'm considering leaving that off from this fix. When I do that I get some failures locally and I'm a little concerned others might be using this kind of pattern: I see some other issues too, but this would require changing the test. It looks like the failure is on: @JsonbDateFormat(value = "yyyy.MM.dd")
private LocalDateTime localDateTime;Which is admittedly a bit odd, but it works now and I'm hesitant to change too much. That said, if you feel this is worth looking at I think we could probably solve some of these issues without the setting. I just think it will end up being a bigger change. |
|
I'm temporarily making this a draft as we shouldn't be using the default locale when a locale is not specified. We should be using UTC per the specification. However, that breaks tests given it uses midnight for the start of day and ends up subtracting an hour making the date go back to the previous day. I just need to look into it and just haven't had the chance yet. |
jamezp
force-pushed
the
issue695-date-format
branch
from
8b68cdf to
300556b
Compare
DateDeserializer previously required both date and time components, causing failures with patterns like yyyy-MM-dd. Now uses parseBest() to handle ZonedDateTime, LocalDateTime, LocalDate, and YearMonth inputs. Signed-off-by: James R. Perkins <jperkins@ibm.com>
|
@KyleAure If you wouldn't mind giving this another review. The issue we found was this was failing with I tested against the following timezone to ensure we wouldn't have an issue:
|
jamezp
force-pushed
the
issue695-date-format
branch
from
300556b to
ad5720c
Compare
KyleAure
left a comment
KyleAure
left a comment
There was a problem hiding this comment.
Looks good, I appreciate the new tests you wrote.
Just a few suggestions on documentation.
src/main/java/org/eclipse/yasson/internal/deserializer/types/DateDeserializer.java
Outdated
Show resolved
Hide resolved
| * <b>Important:</b> Date objects created from date-only patterns represent midnight UTC, which may display as a | ||
| * different calendar day when viewed in local timezone. For date values where preserving the local calendar date is | ||
| * critical, use {@link java.sql.Date} or better {@link java.time.LocalDate} instead. |
There was a problem hiding this comment.
I think this information is better suited to be put either in the JSON-B Javadoc for JsonbDateFormat or somewhere in Section 3.5 of the spec, otherwise I do not expect any user of Yasson to be looking at the javadoc for this class.
There was a problem hiding this comment.
Yeah, I completely agree. I need to file an issue there. I mostly put it here for others looking at why it's done like this.
src/main/java/org/eclipse/yasson/internal/deserializer/types/DateDeserializer.java
Outdated
Show resolved
Hide resolved
…ateDeserializer.java Co-authored-by: Kyle Aure <KyleJAure@gmail.com>
…ateDeserializer.java Co-authored-by: Kyle Aure <KyleJAure@gmail.com>
2 participants
DateDeserializer previously required both date and time components, causing failures with patterns like yyyy-MM-dd. Now uses parseBest() to handle ZonedDateTime, LocalDateTime, LocalDate, and YearMonth inputs.
resolves #695