-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Support arbitrary Number implementation for Object and Number deserialization #1290
Support arbitrary Number implementation for Object and Number deserialization #1290
Conversation
Great! |
Are there plans to merge and release it? |
Still waiting for this to be released :) |
Thank god! Finally you guys fix this! :) |
lgtm |
See gson glitch: google/gson#1290 google/gson#968
* WIP * Revert "WIP" This reverts commit 98b34c4. * WIP * Revert "WIP" This reverts commit 3b9fc96. * Add de/serializer for MinDiskFree * Move MinDiskFree out of Folder * Move MinDiskFree out of Folder (2) * Revert "Move MinDiskFree out of Folder (2)" This reverts commit 65f87db. * Revert "Move MinDiskFree out of Folder" This reverts commit b71350b. * Revert "Add de/serializer for MinDiskFree" This reverts commit 5827426. * RestApi: Add MinDiskFreeSerializer, MinDiskFreeDeserializer * Revert "RestApi: Add MinDiskFreeSerializer, MinDiskFreeDeserializer" This reverts commit 3922f24. * Test * Revert "Test" This reverts commit 3550095. * FolderActivity/DeviceActivity: Fix restApi unavailable in onCreate() * Model/Folder#MinDiskFree: Initialize members (fixes #277) * ConfigXml#getFolders: Add MinDiskFree (fixes #277) * ConfigXml: Write back minDiskFree (fixes #277) * Ignore notices about updating gradle dependencies * ConfigXml: Make number parsing more safe * FolderActivity#initFolder: Add new Folder.MinDiskFree * Handle minDiskFree.value as String instead of float * Revert "Handle minDiskFree.value as String instead of float" This reverts commit 0552cfc. * WIP * Revert "WIP" This reverts commit 0a3df91. * RestApi: Avoid creating duplicate Gson() instances * Model/Folder: Use Integer instead of Float See gson glitch: google/gson#1290 google/gson#968 * Try MinDiskFree.value as Long instead of Integer * Revert "Try MinDiskFree.value as Long instead of Integer" This reverts commit d358862. * Revert "Model/Folder: Use Integer instead of Float" This reverts commit ca3931b. * Update model/Options: MinHomeDiskFree (fixes #277)
please, apply as soon as possible |
Is this PR ever going to be applied? If not with this enhancement, what is the approach recommended by the Gson team for deserializing numbers? If I'm deserializing as a Map<String, Object> and I have some Integer values how to avoid for them to be interpreted as a Double? |
is this PR acceptable now ? |
Could you please not force push next time? It requires looking at all the files again to find out what changed and it does not show any of the previous review comments in the code anymore. For a clean commit history, all the commits can then later be squashed when the pull request is merged. |
:)
https://github.com/lyubomyr-shaydariv/gson/commits/to-number-strategy%2Bhistory
It depends on how the repository maintainers do merges. From what I see in the git log graph, squashes are not a must. and I'd like to keep it as clean as possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more minor things, but it looks pretty good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, still some more minor things which could possibly be improved (if you like).
No problem, please provide either an inline diff (or a patch as a comment) or provide a patch on top of the "+history" branch as a PR into my repository (so that I could merge/cherry-pick your suggestions on top of the branch and then I could squash all these changes into this branch) addressing the unresolved comments (3 remaining as I could resolve the first two only). Thank you. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
8 similar comments
@googlebot I consent. |
@googlebot I consent. |
@googlebot I consent. |
@googlebot I consent. |
@googlebot I consent. |
@googlebot I consent. |
@googlebot I consent. |
@googlebot I consent. |
I don't understand this "feature" of Gson. Assuming objects of this shape: data class MyThing(
val answer: Any? = null
) And assuming incoming JSON like so: [
{
"answer": "1"
},
{
"answer": 1
}
{
"answer": 1.0
},
{
"answer": null
}
] The very obvious solution to the above is for Gson to see the target type of QUITE unfortunately, Gson still doesn't do this and is happy to corrupt incoming JSON values, requiring that AFTER conversion, we manually inspect the resulting object and manually correct all of Gson's errors. To compound this issue, I cannot craft my own custom converter either, because that too, would require selecting a single type only. |
When you tell Gson to deserialize a Java The val gson = GsonBuilder()
.setObjectToNumberStrategy { reader ->
// Read the JSON number as string to inspect its format
val numberAsString = reader.nextString()
if (numberAsString.contains('.')) numberAsString.toDouble()
else numberAsString.toInt()
}
.create() You could also have a look at the implementation of Gson's |
@Marcono1234 Thanks for the very thoughtful response. |
I had to use Gson... And here's the juicy bit that makes it prefer integers: google/gson#1290
RFC 8259, 6 Numbers allows numbers of arbitrary range and precision specifying that a particular implementation may set some limits for numeric values. As of Gson 2.8.4 uses the following deserialization strategies:
Object
, Gson always returnsDouble
s for all encountered JSON numbers. In this case, the minimum and maximum hold number value fits the limits ofDouble
which cannot hold exactLong
values that exceed theDouble
exact precision limits (2^53+1
).Number
and there are no customNumber
adapters, Gson always returnsLazilyParsedNumber
s that hold original string values parsing them to a requested type lazily allowing to use the whole range ofLong
. However, it does not allow to use numbers of arbitrary range and precision, and does not expose its hold string as aBigDecimal
.In order to fix these limitations and preserve backwards compatibility, some sort of "to-number" strategies might be accepted in
GsonBuilder
to override the default behavior of Gson. This pull-request introduces such a strategy interface to be used in the built-inObject
andNumber
type adapters. There are also four strategies implemented in this PR (two standard to keep the backwards compatibility and two enhanced to overcome the limitations) using an enumeration:ToNumberPolicy.DOUBLE
that implements the default behavior of theObject
type adapter returningDouble
s only.ToNumberPolicy.LAZILY_PARSED_NUMBER
that implements the default behavior of theNumber
type adapter.ToNumberPolicy.LONG_OR_DOUBLE
that tries to parse a number as aLong
, otherwise then tries to parse it as aDouble
, if the number cannot be parsed as aLong
.ToNumberPolicy.BIG_DECIMAL
that can parse numbers of arbitrary range and precision.Call-site is expected to extract proper values using the methods declared in
java.lang.Number
.Examples of use:
Default behavior, backwards-compatible with the previous versions of Gson
Object
-declared numbers are alwaysLazilyParsedNumber
sNumber
-declared numbers are alwaysDouble
sObject
-declared numbers are eitherLong
orDouble
Object
-declared numbers areBigDecimal
sCustom bytes-only:
Custom deserialization does not affect
Byte
-declared deserialization: