Skip to content
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

TypeAdapterFactory INTEGER_FACTORY will change INTEGER type to LONG type #2680

Open
L-Ymeng opened this issue May 22, 2024 · 2 comments
Open
Labels

Comments

@L-Ymeng
Copy link

L-Ymeng commented May 22, 2024

Gson version

2.10.1

Java / Android version

21

Description

I try to get a JsonElement by toJsonTree method. but i found that when i use integer object as input, i will get a long object with method JsonElement.getAsNumber()

I think this commit caused this problem:3e3266c

Is this a bug or a feature?

Expected behavior

use integer type as input, get integer type as output

Actual behavior

use integer type as input, get long type as output

Reproduction steps

    @Test
    public void test(){
        Integer testNum = 1;
        JsonElement jsonTree = new Gson().toJsonTree(testNum);
        System.out.println(jsonTree.getAsNumber().getClass());
    }
@L-Ymeng L-Ymeng added the bug label May 22, 2024
@Marcono1234
Copy link
Collaborator

Marcono1234 commented May 22, 2024

Yes can confirm this; it looks like this was indeed introduced by 3e3266c (by accident):
There is only JsonWriter.value(long), so the adapters for all other integral primitive types (byte, short, int) actually call value(long).

Sorry for that. Maybe it would make sense to (partially) revert that commit.

It seems AtomicInteger is affected as well, but it's adapter always behaved this way, that a Long instead of the AtomicInteger is stored in the JsonPrimitive. However, this might be a good thing since it is mutable and might cause confusing behavior if it was stored in JsonPrimitive.

@Marcono1234
Copy link
Collaborator

Ok, so the potential issues with the current implementation are:

  • Conversion to Long might be unexpected (especially when implicitly using JsonTreeWriter as described in your issue here)
    Might depend on the use case whether that is a problem for user code.
  • Conversion to Long instead of keeping for example the Byte or Integer might take up more memory
    This might be especially noticeable when you have a lot of JsonPrimitives containing them. But maybe that also depends on the JVM object layout for these boxed objects. Maybe a Long is not actually that much larger than an Integer, despite long being larger than int (?), but I haven't investigated that yet.

Reverting 3e3266c (or at least parts of it) would also revert narrowing numeric conversion, e.g. 1.5f -> (byte) 1, see #2156. Though that was originally reported by me, and I am not sure if / how many users were actually negatively affected by that behavior.
That could be solved again by performing the narrowing conversion, but then boxing as Number again and then call JsonWriter#value(Number) instead of JsonWriter#value(long), but performance-wise this will likely not be ideal.

Maybe JsonWriter#value(Number) could also be refactored to first check using a new isTrustedIntegralNumberType, to avoid redundant string.equals("-Infinity") checks in these cases. Though it would have to be verified if that actually makes a noticeable performance difference.

So am I currently not sure what the best approach here would be.
@eamonnmcmanus, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants