Fix numeric conversion to Long when serializing numbers as JsonElement tree#2814
Open
Marcono1234 wants to merge 2 commits intogoogle:mainfrom
Open
Fix numeric conversion to Long when serializing numbers as JsonElement tree#2814Marcono1234 wants to merge 2 commits intogoogle:mainfrom
Marcono1234 wants to merge 2 commits intogoogle:mainfrom
Conversation
Contributor
Author
|
Closed and reopend this PR to fix an intermittent test failure, see #2815. |
Marcono1234
commented
Feb 22, 2025
Comment on lines
+377
to
381
| } else if (value instanceof Long) { | ||
| out.value(value); | ||
| } else { | ||
| // Only perform unwrapping if numeric conversion is necessary | ||
| out.value(value.longValue()); |
Contributor
Author
There was a problem hiding this comment.
For correctness this value instanceof Long check is not needed; the previous code which always called out.value(value.longValue()) was fine as well. However, for JsonTreeWriter it would have unwrapped and then wrapped the value again. But on the other hand for JsonWriter value(long) might be more efficient. I am not really sure which approach is better for performance, or if it even matters at all.
(same applies to Double below)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes #2680
Description
As mentioned in #2680, an unintended side-effect of 3e3266c was that serializing to a
JsonElementtree is now converting all integral numbers toLonginstances.This pull request here tries to solve this by using the
JsonWriter#value(Number)overload again, boxing the converted number. If the type is already matching (e.g. serializing aByteasByte) then this also has the advantage that it avoids unwrapping and havingJsonTreeWriterwrap the value again.But on the other hand when using
JsonWriterand notJsonTreeWriterthere might be a bit overhead fromJsonWriter#value(Number)compared toJsonWriter#value(long).An alternative to this PR could be to instead revert 3e3266c partially or completely. After all the underlying issue #2156 had been reported by me (and was not blocking me in any way), so maybe other users would not have cared about the missing numeric conversion.
Checklist
This is automatically checked by
mvn verify, but can also be checked on its own usingmvn spotless:check.Style violations can be fixed using
mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.null@since $next-version$(
$next-version$is a special placeholder which is automatically replaced during release)TestCase)mvn clean verify javadoc:jarpasses without errors