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

Add converters for java.time.LocalDate, java.time.LocalDateTime, java.time.LocalTime and java.time.ZonedDate #82

Closed
wants to merge 1 commit into from

Conversation

@mcimbora
Copy link
Contributor

commented Jan 13, 2017

No description provided.

@coveralls

This comment has been minimized.

Copy link

commented Jan 13, 2017

Coverage Status

Coverage decreased (-0.008%) to 79.552% when pulling b707f1a on mcimbora:java_time_converters into a04f343 on x-stream:master.

@mcimbora

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

Do not integrate yet, as this needs some changes. LocalTime.toString() omits seconds/nanoseconds if they're null, which is optional by the ISO 8601 specification. It makes sense to conform to http://www.w3schools.com/xml/schema_dtypes_date.asp which requires all components of hh:mm:ss to be included in the output.

@ge0ffrey

This comment has been minimized.

Copy link

commented Jan 13, 2017

This will fix #75

@joehni

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

To me, this looks perfect! For the XML types you would have to separate anyway between the 3 types date, time and datetime. Thanks!

@ge0ffrey

This comment has been minimized.

Copy link

commented Jan 15, 2017

@mcimbora Could you also add the OffsetDateTime converter too? Not just ES needs them: optaplanner-benchmark replaced a java.util.Date with OffsetDateTime, and this is ugly in the persisted xml:

  <startingTimestamp xStreamId="42" resolves-to="java.time.Ser">
    <byte>10</byte>
    <int>2017</int>
    <byte>1</byte>
    <byte>15</byte>
    <byte>13</byte>
    <byte>29</byte>
    <byte>5</byte>
    <int>396000000</int>
    <byte>4</byte>
  </startingTimestamp>

Btw, I found out the big difference between OffsetDateTime and ZonedDateTime: in databases and files (including xml files), always use OffsetDateTime, because timezone rules change (which corrupts ZonedDateTime). In UI's, use ZonedDateTime (so potentially also in REST services, so there still needs to be an xstream converter).

@joehni Can we also register these java.time converters to XStream to be used by default (so no special config is needed)? This kinda does break backwards compatibility (see xml file above that won't parse afterwards) ... but I think it's the right thing to do (not in a hotfix version, but maybe in a minor version?). Update: looks like this change is already doing that. I still prefer it like that, but you'll want to be aware of the implication...

@mcimbora mcimbora force-pushed the mcimbora:java_time_converters branch from b707f1a to faa6250 Jan 16, 2017
@coveralls

This comment has been minimized.

Copy link

commented Jan 16, 2017

Coverage Status

Coverage decreased (-0.08%) to 79.476% when pulling faa6250 on mcimbora:java_time_converters into a04f343 on x-stream:master.

@mcimbora mcimbora force-pushed the mcimbora:java_time_converters branch 2 times, most recently from 5496b18 to a1d00f3 Jan 16, 2017
….time.LocalTime and java.time.ZonedDate
@mcimbora mcimbora force-pushed the mcimbora:java_time_converters branch from a1d00f3 to bd3a150 Jan 16, 2017
@coveralls

This comment has been minimized.

Copy link

commented Jan 16, 2017

Coverage Status

Coverage decreased (-0.08%) to 79.476% when pulling a1d00f3 on mcimbora:java_time_converters into a04f343 on x-stream:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 16, 2017

Coverage Status

Coverage decreased (-0.08%) to 79.476% when pulling bd3a150 on mcimbora:java_time_converters into a04f343 on x-stream:master.

@mcimbora

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2017

@joehni Thanks, I updated the PR. Added OffsetDateTimeConverter and made sure the output format of time remains consistent (hh:mm:ss). Nanos are displayed optionally (when the information is present in the time object).

@coveralls

This comment has been minimized.

Copy link

commented Jan 16, 2017

Coverage Status

Coverage decreased (-0.08%) to 79.476% when pulling bd3a150 on mcimbora:java_time_converters into a04f343 on x-stream:master.

@Override
public Object fromString(String str) {
try {
return LocalDateTime.parse(str);

This comment has been minimized.

Copy link
@ge0ffrey

ge0ffrey Jan 16, 2017

This should use the FORMATTER instance too.

This comment has been minimized.

Copy link
@mcimbora

mcimbora Jan 16, 2017

Author Contributor

@ge0ffrey Coffee time? :-)

This comment has been minimized.

Copy link
@mcimbora

mcimbora Jan 16, 2017

Author Contributor

@ge0ffrey Thinking about it a bit I disagree. Both 11:30 and 11:30:00 are valid time inputs. I don't see a reason to throw away this flexibility.

This comment has been minimized.

Copy link
@ge0ffrey

ge0ffrey Jan 16, 2017

I see your point - and it's good point and this isn't a major problem either way - but I 'd argue:

  • Read and write should be in sync. If read allows more than write writes, it means that reading an XML file and writing the same content again can change the output. That's a bit of pain for some use cases (think xml solver fragments in optaplanner benchmark's report).
  • It should fail fast on non-compliant XSD dates.
  • If it doesn't fail fast, it's like browsers not fail fasting on invalid HTML but every browser just rendering it differently. By fail fasting the users would write valid HTML.

That being said, I 'd say:

  • Consistently with the other converter inside xstream is most important, @joehni can tell it - and in the end it's his call anyway :)
  • I'd argue "when in doubt, leave it out" (Josh Bloch quoth about API design): when users request this smartness, it can always be added later. When we add this smartness now, it can never be removed later (even when almost no one uses it and it prevents it from doing other useful smartness).
@Override
public Object fromString(String str) {
try {
return LocalTime.parse(str);

This comment has been minimized.

Copy link
@ge0ffrey

ge0ffrey Jan 16, 2017

same here, use LocalTime.parse(str, FORMATTER)


static {
FORMATTER = new DateTimeFormatterBuilder()
.appendPattern("uuuu-MM-dd'T'HH:mm:ss")

This comment has been minimized.

Copy link
@mcimbora

mcimbora Jan 16, 2017

Author Contributor

@ge0ffrey I don't get the remark, the offset is appended 2 lines below in the builder.

This comment has been minimized.

Copy link
@ge0ffrey

ge0ffrey Jan 16, 2017

My mistake, sorry, ignore this comment :)

@joehni joehni self-assigned this Jan 25, 2017
@joehni joehni added the enhancement label Jan 25, 2017
@joehni joehni added this to the 1.4.x milestone Jan 25, 2017
@joehni

This comment has been minimized.

Copy link
Member

commented Jan 27, 2017

Applied. Thanks!

@joehni joehni closed this Jan 27, 2017
@ge0ffrey

This comment has been minimized.

Copy link

commented Jan 28, 2017

Thanks Jörg!
If reasonable, we look forward to an XStream so we can upgrade and get rid of our copies of these converters before our users start using them.

@joehni joehni modified the milestones: 1.4.x, 1.4.10 Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.