Add option to preserve zero decimals #831

trbogart · 2022-11-22T23:01:29Z

Pull Request Checklist

Have you read through the contributor guidelines?
Have you squashed your commits?
Have you added copyright headers to new files?
Have you updated the documentation?

Updated documentation in code

Have you added tests for any changed functionality?

Fixes

Fixes #107 by adding a configuration option to preserve zero decimal numbers from being rounded to a whole number (for example, 1.0 will not be converted to 1).

In particular, we want to use this to prevent ElasticSearch from creating a long dynamic mapping for a field that should be a double. It's not really a bug (as described in that issue), so the old behavior is preserved by default.

My first approach was to serialize this in our code base. That works, but isn't ideal. It required copying a fair bit of play-json code to override just a single line of code. Also, our code serializes Json in dozens of places, all of which had to be changed to use the new methods.

Purpose

This adds an option to preserve a .0 for decimal numbers that are rounded to a whole number (e.g. 1.0 to 1). This distinction is important in some situations, such as ES dynamic mapping or KairosDB schema creation.

Background Context

It might be harmless to do the new behavior by default, but the original issue did mention a preference for opting in to this behavior, which is reasonable.

References

Are there any relevant issues / PRs / mailing lists discussions?
#107

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

trbogart · 2022-11-22T23:05:39Z

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

@@ -69,6 +69,16 @@ private[jackson] class JsValueSerializer(parserSettings: JsonParserSettings) ext
  import com.fasterxml.jackson.databind.node.BigIntegerNode
  import com.fasterxml.jackson.databind.node.DecimalNode

+  private def stripTrailingZeros(bigDec: JBigDec): JBigDec = {


This is the core change.

gmethvin · 2022-11-22T23:36:54Z

@cchantep Thoughts? The general approach seems reasonable to me.

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

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

trbogart · 2022-11-23T00:27:51Z

I'm not sure why mima is still failing. I'll have to come back to that next week.

gmethvin

MiMa is failing because you've changed the case class constructor to have 3 arguments rather than 2, which also affects the apply and copy methods generated by the case class.

Arguably it was a bad idea to make JsonParserSettings a case class in #191, as it makes it inconvenient to add new settings to the case class without breaking compatibility. We can still technically work around this by manually adding copy, apply and this methods with the old arguments, but it ends up being pretty ugly, and we will have to do this again every time we add another field to the case class.

So a few options are:

Add in the extra methods for binary compatibility—these could be made private[json] so they are only visible publicly in the binary representation.
Option 1, plus we make both the constructor and copy method private, so in the future when adding config we only need to worry about the apply method.
Switch to using a (preferably sealed) trait for configuration, deprecate the old case class config, and create a new way to construct instance of the trait for config.
Wait to introduce this breaking change in the next "major" version of play-json—which I think would be 2.10.0, and is coming up pretty soon.

Option 2 seems like the best balance between amount of work now and extensibility in the future.

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

play-json/jvm/src/test/scala/play/api/libs/json/JsonSpec.scala

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

trbogart · 2022-11-29T23:58:57Z

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

+ * Copyright (C) 2009-2021 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package play.api.libs.json


Renamed from JsonParserSettings.

I ended up using sealed traits, which I called JsonConfig, BigDecimalParseConfig, and BigDecimalSerializerConfig.

trbogart · 2022-11-29T23:59:30Z

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

+    bigDecimalSerializerConfig: BigDecimalSerializerConfig
+) extends JsonConfig
+
+@deprecated("Use BigDecimalParseConfig instead", "2.10.0")


What should I use for the 2nd argument of these deprecated annotations?

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

play-json/jvm/src/test/scala/play/api/libs/json/JsonSpec.scala

trbogart · 2022-11-30T00:10:53Z

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

 * val jsValue = mapper.readValue("""{"foo":"bar"}""", classOf[JsValue])
 * }}}
 */
-sealed class PlayJsonModule(parserSettings: JsonParserSettings)


This is the only public usage of JsonParserSettings. I deprecated this and replaced it with PlayJsonMapperModule (I'm open to naming suggestions).

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

trbogart · 2022-12-05T20:07:09Z

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

  import java.math.BigInteger
  import java.math.{ BigDecimal => JBigDec }

  import com.fasterxml.jackson.databind.node.BigIntegerNode
  import com.fasterxml.jackson.databind.node.DecimalNode

+  private def stripTrailingZeros(bigDec: JBigDec): JBigDec = {
+    val stripped = bigDec.stripTrailingZeros
+    if (jsonConfig.bigDecimalSerializerConfig.preserveZeroDecimal && bigDec.scale > 0 && stripped.scale <= 0) {


stripTrailingZeros can drop the scale below 0 (e.g. "10.0" will end up with a scale of -1, which needs to be corrected to 1). I also added some additional unit tests to cover this case.

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

gmethvin · 2022-12-12T22:32:49Z

@trbogart thanks for the configuration changes. @cchantep what do you think about this approach?

trbogart · 2023-01-03T22:42:08Z

@gmethvin and @cchantep, this is ready for review again.

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

cchantep · 2023-01-04T09:13:25Z

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala

-  private lazy val mapper = (new ObjectMapper).registerModule(new PlayJsonModule(JsonParserSettings.settings))
+  val defaultInstance: JacksonJson = JacksonJson(JsonConfig.settings)
+
+  private val ref: AtomicReference[JacksonJson] = new AtomicReference[JacksonJson](defaultInstance)


So it add overhead for all runtime whereas it's only useful for testing?
Couldn't the internal set logic and its synchronization be only in tests?

I agree if this is only for tests it probably doesn't need to be atomic. This is an internal API so we can control how it's used.

Also wondering if we could keep JacksonJson an object and just have a setConfig in there?

I'll remove the atomic. I kept the case class as a convenience to initialize the mapper and factory together.

Having the case class also means that any given operations will use a consistent set of mapper and factory, so there's not really a need for synchronization. Running tests concurrently could still give unexpected results (since it relies on static state), but the atomic reference didn't actually help with that anyway.

Co-authored-by: Cédric Chantepie <cchantep@users.noreply.github.com>

trbogart · 2023-01-05T04:58:24Z

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

+ */
+sealed trait BigDecimalParseConfig {
+
+  /**


I added more documentation for the configuration

gmethvin · 2023-01-12T23:21:14Z

@cchantep do you have any other concerns or are we good to merge this?

trbogart · 2023-01-12T23:35:18Z

Actually, there is another thing before merging. I added some deprecated annotations, and I wasn't sure what to use for the "since" field. I used "2.10.0", but I'm not sure if that is correct.

Also, what is the timeline for getting this released?

Thanks!

@gmethvin and @cchantep

mkurz · 2023-01-12T23:37:49Z

Also, what is the timeline for getting this released?

I didn't follow this conversations, however if @gmethvin and @cchantep merge this I can take over and cut a new release afterwards.

gmethvin · 2023-01-12T23:50:14Z

I think we want to backport this to 2.9.x, so deprecating in 2.9.4 would make sense? I believe play-json 2.9.x is usable with Play 2.8?

mkurz · 2023-01-13T00:15:07Z

I think we want to backport this to 2.9.x, so deprecating in 2.9.4 would make sense? I believe play-json 2.9.x is usable with Play 2.8?

Right now Play 2.8 ships with play-json 2.8.x, however I think it should be possible to manually upgrade to play-json 2.9.x, I think some people did that already, not sure though.

Co-authored-by: Cédric Chantepie <cchantep@users.noreply.github.com>

trbogart · 2023-01-19T21:29:39Z

2.8.x backport PR: #840
2.9.x backport PR: #839

trbogart commented Nov 22, 2022

View reviewed changes

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala Outdated Show resolved Hide resolved

trbogart commented Nov 22, 2022

View reviewed changes

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala Show resolved Hide resolved

trbogart commented Nov 22, 2022

View reviewed changes

trbogart force-pushed the preserveZeroDecimal branch from 38b3e53 to b4d0b00 Compare November 22, 2022 23:10

Add option to preserve zero decimals

892ad8c

trbogart force-pushed the preserveZeroDecimal branch from b4d0b00 to 892ad8c Compare November 22, 2022 23:11

trbogart mentioned this pull request Nov 22, 2022

Double values with 0 fraction written without fraction #107

Closed

Richard Bogart added 3 commits November 22, 2022 15:41

scalafmt

1917857

JDK 11 binary compatibility check

1694da9

revert last commit

bfdf86c

cchantep requested changes Nov 22, 2022

View reviewed changes

Richard Bogart added 3 commits November 22, 2022 16:10

fix binary compatibility, add tests for zero

8b17d32

cleanup imports, use atomic ref

6f3c176

remove import

2a9615e

restore import

8840aae

gmethvin reviewed Nov 23, 2022

View reviewed changes

play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala Outdated Show resolved Hide resolved

cchantep reviewed Nov 26, 2022

View reviewed changes

fix binary compatibility, formatting

932f02d

trbogart commented Nov 29, 2022

View reviewed changes

Richard Bogart added 5 commits November 29, 2022 16:02

rename constants for consistency

a3189c5

fix formatting

004edea

fix bug stripping zeros for powers of ten, add tests

f731f20

formatting

6d4ac29

explicitly throw exception from try-finally

894ba30

trbogart requested a review from cchantep December 5, 2022 23:17

trbogart commented Dec 5, 2022

View reviewed changes

trbogart removed the request for review from cchantep January 3, 2023 22:41

trbogart requested a review from gmethvin January 3, 2023 22:41

cchantep reviewed Jan 4, 2023

View reviewed changes

Richard Bogart and others added 3 commits January 4, 2023 20:33

remove atomic reference, document configuration

1c4f325

Update play-json/jvm/src/main/scala/play/api/libs/json/JsonConfig.scala

f024b1b

Co-authored-by: Cédric Chantepie <cchantep@users.noreply.github.com>

fix build, rename method

b64a605

trbogart commented Jan 5, 2023

View reviewed changes

gmethvin approved these changes Jan 5, 2023

View reviewed changes

formatting

5ad9f53

trbogart requested a review from cchantep January 12, 2023 23:07

update deprecated since values

ae7912f

cchantep approved these changes Jan 13, 2023

View reviewed changes

gmethvin merged commit 4a8a215 into playframework:main Jan 17, 2023

trbogart pushed a commit to trbogart/play-json that referenced this pull request Jan 17, 2023

Add option to preserve zero decimals (playframework#831)

481e661

Co-authored-by: Cédric Chantepie <cchantep@users.noreply.github.com>

This was referenced Jan 17, 2023

Preserve zero decimal (backport to 2.9.x) #839

Merged

Preserve zero decimal (backport to 2.8.x) #840

Closed

trbogart mentioned this pull request Jan 27, 2023

Allow setting configuration programmatically #843

Closed

5 tasks

brunoballekens mentioned this pull request Jan 30, 2023

Update play-json to 2.9.4 waylayio/kairosdb-scala#122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to preserve zero decimals #831

Add option to preserve zero decimals #831

trbogart commented Nov 22, 2022 •

edited

Loading

trbogart Nov 22, 2022

gmethvin commented Nov 22, 2022

trbogart commented Nov 23, 2022

gmethvin left a comment

trbogart Nov 29, 2022

trbogart Nov 29, 2022

trbogart Nov 30, 2022

trbogart Dec 5, 2022

gmethvin commented Dec 12, 2022

trbogart commented Jan 3, 2023

cchantep Jan 4, 2023

gmethvin Jan 5, 2023

trbogart Jan 5, 2023

trbogart Jan 5, 2023

trbogart Jan 5, 2023

gmethvin commented Jan 12, 2023

trbogart commented Jan 12, 2023

mkurz commented Jan 12, 2023

gmethvin commented Jan 12, 2023

mkurz commented Jan 13, 2023

trbogart commented Jan 19, 2023

Add option to preserve zero decimals #831

Add option to preserve zero decimals #831

Conversation

trbogart commented Nov 22, 2022 • edited Loading

Pull Request Checklist

Fixes

Purpose

Background Context

References

Choose a reason for hiding this comment

gmethvin commented Nov 22, 2022

trbogart commented Nov 23, 2022

gmethvin left a 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 commented Dec 12, 2022

trbogart commented Jan 3, 2023

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 commented Jan 12, 2023

trbogart commented Jan 12, 2023

mkurz commented Jan 12, 2023

gmethvin commented Jan 12, 2023

mkurz commented Jan 13, 2023

trbogart commented Jan 19, 2023

trbogart commented Nov 22, 2022 •

edited

Loading