avoid reparsing numbers when serializing#1074
Conversation
61f53e8 to
995ddc0
Compare
995ddc0 to
c399caf
Compare
b46b958 to
d97f4dd
Compare
play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala
Outdated
Show resolved
Hide resolved
play-json/jvm/src/main/scala/play/api/libs/json/jackson/StringBasedNumericNode.scala
Outdated
Show resolved
Hide resolved
play-json/jvm/src/main/scala/play/api/libs/json/jackson/JacksonJson.scala
Outdated
Show resolved
Hide resolved
|
Unit test required |
@cchantep I think the existing tests do a good job of testing this code change. |
ccdfc2e to
41444b1
Compare
| json.writeTree(new DecimalNode(new JBigDec(raw))) | ||
| json match { | ||
| case _: TokenBuffer => | ||
| json.writeTree(new BigIntegerNode(new BigInteger(raw))) |
There was a problem hiding this comment.
I have a change in jackson 2.18 which means this line can be changed to something that avoids the BigInteger parsing.
There was a problem hiding this comment.
@pjfanning I merged the test (#1077) and also the main branch is on Jackson 2.19.1 now thanks to your pull request. Can you please now update this line with what you mentioned (as we are now on Jackson 2.18+)? thanks!
| java.math.BigDecimal.valueOf(Double.MaxValue).add(java.math.BigDecimal.valueOf(1)).toString, | ||
| java.math.BigDecimal.valueOf(Double.MinValue).add(java.math.BigDecimal.valueOf(-1)).toString | ||
| ) | ||
| numStrings.map { numString => |
There was a problem hiding this comment.
some of the numbers are serialized with mathematically equivalent values but not exactly the same text represeenation - examples include numbers with + signs (which are not necessarily included in the serialized text)
There was a problem hiding this comment.
fails with this when forced to check the strings are equal as opposed to the numbers are equal
[info] + Serialize JsNumbers with integers correctly (14 ms)
[error] x Serialize JsNumbers with decimal points correctly (10 ms)
[error] '3.4028235E+38' != '3.4028235E38' (JsonSpec.scala:513)
[error] x Serialize JsNumbers with e notation correctly (10 ms)
[error] '1.23456789012345679012345679E+999' != '1.23456789012345679012345679e999' (JsonSpec.scala:526)
This also happens with #1077 which has these tests but not the main source change
| "1.23456789012345679012345679e-999", | ||
| "-1.23456789012345679012345679e-999" | ||
| ) | ||
| numStrings.map { numString => |
b62bfb5 to
bfa15b2
Compare
bfa15b2 to
e6d7170
Compare
|
@mkurz I can make a small adjustment in this PR because newer jackson releases have a new method that I added so this PR can use it |
Update StringBasedNumericNode.scala rework Update JacksonJson.scala add containsEOrDot requested changes to containsEOrDot remove StringBasedNumericNode imports remove new function add test add int serialization test Update JsonSpec.scala Update JsonSpec.scala
haha, I just commented in the code ;) yes please go ahead (I guess it will be case tb: TokenBuffer =>
tb.writeNumber(raw, true)because I was looking at your fork already ;) |
e6d7170 to
9c00caf
Compare
I pushed that change. |
|
@pjfanning Looking at your fork, do you also want to add PRs for the rest?
? |
Alternative to #1073
This change avoids creating NumericNodes and parsing strings to numbers which ultimately have to be turned back to strings to write them.
I've had to leave the BigIntegerNode usage in there. Removing it breaks some tests. I've debugged and it is down to how writeNumber(String) is implemented in jackson-databind TokenBuffer. We would need to change this class in jackson-databind to fix this (and that is not likely to happen as it will probably cause other issues). Basically, writeNumber(String) is assumed to be a non-integer in TokenBuffer.