Codecs for Java TemporalAmount #43

cchantep · 2017-02-25T15:16:33Z

Support Java Time Duration (see #40 ) and Period, either as number (by default of millis of Duration or of days for Period), or as ISO string representation.

gmethvin · 2017-02-26T01:48:20Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+  private def javaDurationNumberReads(unit: TemporalUnit, esuffix: String) =
+    Reads[JDuration] {
+      case JsNumber(n) if !n.ulp.isWhole =>
+        JsError(s"error.invalid.$esuffix")


error.expected.integer? Or we could support fractional values and just round to the nearest nanosecond.

I prefer to have an error key related to the duration case.

I don't think rounding should happen there. The temporal unit is specified and a corresponding integer should be found, or the validation indicates there is a misusage.

Can we make it an error key that suggests an issue with the number being an integer? error.invalid.duration-int?

Also, numbers like 10.0 that have trailing zeros should still be considered integers by the validation. You might want to do a stripTrailingZeros.

Also, you probably don't need the esuffix anymore since this is only used for Duration.

scala> BigDecimal(10.0D).isWhole res0: Boolean = true scala> BigDecimal("10.0").isWhole res1: Boolean = true

gmethvin · 2017-02-26T02:28:42Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+    Reads[Period] { js =>
+      underlying.reads(js).flatMap { duration =>
+        try {
+          JsSuccess(Period.ofDays(duration.toDays.toInt))


If you are reading a Period, you'd want to get a period using the unit you requested. And with Periods it's important what the units are because a period could correspond to a different duration based on when it starts on the calendar (due to leap seconds/days, daylight saving time, etc.).

Actually, it probably makes sense to only support temporal units supported by Period.

gmethvin · 2017-02-26T02:47:54Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+    case JsString(repr) => try {
+      JsSuccess(JDuration.parse(repr))
+    } catch {
+      case _: Throwable => JsError("error.invalid.duration")


Let's only catch DateTimeParseException here, and for the other parse calls.

I don't see reason not to catch any exception there, even if for now only this one is supposed to happen.

Duration.parse is part of the Java standard library, so it's probably safe to assume that if it only says @throws DateTimeParseException, that's the only thing you're expected to handle. Any other kind of exception likely has nothing to do with whether the duration is valid. I would prefer those exceptions be passed up to the caller so they can inspect the exception and figure out what to do.

gmethvin · 2017-02-26T02:50:05Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+        try {
+          JsSuccess(Period.ofDays(duration.toDays.toInt))
+        } catch {
+          case e: Throwable => JsError("error.invalid.period")


I don't think any exception here would indicate an invalid period, so the whole try-catch can be removed.

As for { Future, Try }.{ map, flatMap }, JsResult should be safe. An exception while parsing period should be represented with the JsResult not letting the exception bubbling up.

The point is there is no general mechanism about that in the JsResult combinators.

An exception while parsing period should be represented with the JsResult not letting the exception bubbling up.

I disagree. JsResult is not a Try. JsError really only has the capacity to represent JSON parsing and validation errors, and I'm pretty sure that's what users expect of it. By presenting any exception as if it were invalid input, you're potentially masking other more dangerous errors.

gmethvin · 2017-02-26T02:59:32Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+  }
+
+  /** Deserializer of Java Period from a number of days. */
+  val javaPeriodDaysReads: Reads[Period] =


How about:

object JavaPeriodReads { val Days = javaPeriodNumberReads(ChronoUnit.DAYS) val Months = javaPeriodNumberReads(ChronoUnit.MONTHS) val Years = javaPeriodNumberReads(ChronoUnit.YEARS) }

gmethvin · 2017-02-26T03:21:12Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvWrites.scala

+
+  /** Serializer of Java Period as a number of days. */
+  val javaPeriodDaysWrites: Writes[Period] =
+    Writes[Period] { d => JsNumber(d.getDays) }


getDays returns only the days field in the Period. I'd think you'd want to convert the months and years to days and include those at well. You could use Duration.from(period).toDays.

It would also be useful to have constants for years and months like I suggested for the Reads

Indeed there is no stable way to write a Period as number (Duration.from(aPeriod) is not supported as Period is an estimated duration).

Yeah, I think we can just avoid writing periods as numbers.

gmethvin · 2017-02-26T23:15:37Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+  private def javaDurationNumberReads(unit: TemporalUnit, esuffix: String) =
+    Reads[JDuration] {
+      case JsNumber(n) if !n.ulp.isWhole =>
+        JsError(s"error.invalid.$esuffix")


Can we make it an error key that suggests an issue with the number being an integer? error.invalid.duration-int?

Also, numbers like 10.0 that have trailing zeros should still be considered integers by the validation. You might want to do a stripTrailingZeros.

gmethvin · 2017-02-27T01:19:55Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+      case JsNumber(n) if !n.ulp.isWhole =>
+        JsError(s"error.invalid.$esuffix")
+
+      case JsNumber(n) => JsSuccess(JDuration.of(n.toLong, unit))


You should probably return an error if the number isn't representable as long. BigDecimal#toLong will start providing nonsense values if the number is outside the range of a long, for example

scala> val longValue = BigDecimal("1E20").toLong longValue: Long = 7766279631452241920

scala> BigDecimal("1E20").isValidLong res3: Boolean = false

So it seems like we should be using .isValidLong instead of .ulp.isWhole.

gmethvin · 2017-02-27T01:20:53Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+  private def javaDurationNumberReads(unit: TemporalUnit, esuffix: String) =
+    Reads[JDuration] {
+      case JsNumber(n) if !n.ulp.isWhole =>
+        JsError(s"error.invalid.$esuffix")


Also, you probably don't need the esuffix anymore since this is only used for Duration.

gmethvin · 2017-02-27T01:49:08Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+    case JsString(repr) => try {
+      JsSuccess(JDuration.parse(repr))
+    } catch {
+      case _: Throwable => JsError("error.invalid.duration")


Duration.parse is part of the Java standard library, so it's probably safe to assume that if it only says @throws DateTimeParseException, that's the only thing you're expected to handle. Any other kind of exception likely has nothing to do with whether the duration is valid. I would prefer those exceptions be passed up to the caller so they can inspect the exception and figure out what to do.

gmethvin · 2017-02-27T02:39:21Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+    }
+
+  /** Deserializer of Java Period from a number (integer) of days. */
+  val javaPeriodDaysReads: Reads[Period] =


If we're not going to have a Writes for this maybe we should skip the Reads too? This is basically equivalent to implicitly[Reads[Int]].map(Period.ofDays(_)).

gmethvin · 2017-02-27T20:24:57Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+  private def javaPeriodNumberReads(f: Int => Period): Reads[Period] =
+    Reads[Period] {
+      case JsNumber(n) if !n.isValidInt =>
+        JsError(s"error.invalid.period")


Another note about message keys: I would expect the error to be different if I'm reading a number versus if I'm reading as a period string. I'd assume error.invalid.period would correspond to a message like "Invalid period", which is not enough information for me to understand why this is failing.

cchantep · 2017-03-03T18:35:51Z

Updated

gmethvin · 2017-03-03T22:17:20Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+        JsError("error.invalid.longDuration")
+
+      case JsNumber(n) => JsSuccess(JDuration.of(n.toLong, unit))
+      case _ => JsError("error.expected.duration")


error.expected.longDuration?

gmethvin · 2017-03-03T22:29:00Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+    case JsString(repr) => try {
+      JsSuccess(JDuration.parse(repr))
+    } catch {
+      case _: Exception => JsError("error.invalid.duration")


Should be case _: DateTimeParseException since that's the only exception that indicates an invalid duration.

Either we catch nothing there, having to ensure there is a generic mechanism making JsResult safe (for me no exception must be raised from), or for now Reads such as this one should catch everything.

I would consider it unsafe to catch an unexpected exception and map it to what looks like a validation error. This hides the true cause of the error from the caller.

If code like this throws an exception we don't already know about, it's likely because of either a bug or another serious issue on the server. I don't believe these things are our job to handle. JsResult is not a generic error handling mechanism; it's specifically for JSON validation. If you want to handle other exceptions, you should use a Try. With JsResult.toTry, you can even do:

val result: Try[Foo] = Try(Json.fromJson[Foo](json)).flatMap(JsResult.toTry)

If we are trying to make the API safer, I would start with methods like Json.parse, and make sure we at least document the exceptions thrown if the JSON is invalid. Then we can start thinking about how to provide better ways for handling exceptions in general. But I think that is best done at a high level rather than being the responsibility of each individual Reads.

Ok let this aside from this PR.

Will try to draft something about separately.

gmethvin · 2017-03-03T22:29:05Z

play-json/jvm/src/main/scala/play/api/libs/json/EnvReads.scala

+    case JsString(repr) => try {
+      JsSuccess(Period.parse(repr))
+    } catch {
+      case _: Exception => JsError("error.invalid.stringPeriod")


DateTimeParseException

cchantep added the type:improvement label Feb 25, 2017

gmethvin requested changes Feb 26, 2017

View reviewed changes

gmethvin reviewed Feb 27, 2017

View reviewed changes

gmethvin requested changes Feb 27, 2017

View reviewed changes

gmethvin reviewed Feb 27, 2017

View reviewed changes

cchantep force-pushed the feature/43_java_temporalamount branch from 94f11dd to e46f683 Compare March 2, 2017 21:28

Codecs for Java TemporalAmount

3177f1d

cchantep force-pushed the feature/43_java_temporalamount branch from e46f683 to 3177f1d Compare March 3, 2017 18:36

gmethvin requested changes Mar 3, 2017

View reviewed changes

cchantep added 2 commits March 4, 2017 11:37

Update EnvReads.scala

d926277

Update EnvReads.scala

8f033cd

gmethvin approved these changes Mar 4, 2017

View reviewed changes

gmethvin merged commit 85ebb2b into playframework:master Mar 4, 2017

cchantep deleted the feature/43_java_temporalamount branch March 4, 2017 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Codecs for Java TemporalAmount #43

Codecs for Java TemporalAmount #43

cchantep commented Feb 25, 2017

gmethvin Feb 26, 2017

cchantep Feb 26, 2017

gmethvin Feb 26, 2017 •

edited

Loading

gmethvin Feb 27, 2017

cchantep Feb 27, 2017

gmethvin Feb 26, 2017

gmethvin Feb 26, 2017

cchantep Feb 26, 2017

gmethvin Feb 27, 2017

gmethvin Feb 26, 2017

cchantep Feb 26, 2017

gmethvin Feb 27, 2017

gmethvin Feb 26, 2017

gmethvin Feb 26, 2017

cchantep Feb 26, 2017

gmethvin Feb 27, 2017

gmethvin Feb 26, 2017 •

edited

Loading

gmethvin Feb 27, 2017

cchantep Feb 27, 2017

gmethvin Feb 27, 2017 •

edited

Loading

gmethvin Feb 27, 2017

gmethvin Feb 27, 2017

gmethvin Feb 27, 2017

cchantep Feb 27, 2017

gmethvin Feb 27, 2017

cchantep commented Mar 3, 2017

gmethvin Mar 3, 2017

cchantep Mar 4, 2017

gmethvin Mar 3, 2017

cchantep Mar 3, 2017

gmethvin Mar 4, 2017 •

edited

Loading

cchantep Mar 4, 2017

gmethvin Mar 3, 2017

Codecs for Java TemporalAmount #43

Codecs for Java TemporalAmount #43

Conversation

cchantep commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Feb 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Feb 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchantep commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Feb 26, 2017 •

edited

Loading

gmethvin Feb 26, 2017 •

edited

Loading

gmethvin Feb 27, 2017 •

edited

Loading

gmethvin Mar 4, 2017 •

edited

Loading