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.

Sign up for GitHub

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

Issue 1994 #2031

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Issue 1994 #2031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
08ff7cd
fix issue-1994
NatalieSty Nov 24, 2021
b8b3b51
Added validation for parsing integer types
dctrue2 Dec 4, 2021
6a87ad4
Add checks for lower bounds; Added assertion to verify tests complete
dctrue2 Dec 4, 2021
bedcb10
Condense code in test
dctrue2 Dec 4, 2021
7c58d0a
added comment
NatalieSty Dec 5, 2021
f5aaecb
revert changes for pom files
NatalieSty Dec 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions gson/src/main/java/com/google/gson/JsonPrimitive.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,17 @@ public float getAsFloat() {
*/
@Override
public long getAsLong() {

if (isNumber()) {
final BigInteger val = getAsBigInteger();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is suddenly very expensive if value is already a Long. getAsBigInteger() will convert it to a string which it will then parse back into a new BigInteger object. If this method is called more than once, it will do the same thing every time. I think it would be worth adding instanceof checks to bypass this for when value is Long or any of the smaller java.lang types, and likewise in the other cases. Also, you're checking isNumber() here, so it looks as if you could reasonably move the true-case from the isNumber() ? code below.

Also we can borrow an idea from the implementation of BigInteger.longValueExact() for a simpler overflow check. (Since that method was added in Java 8 we unfortunately can't use it directly.)

I'm imagining code something like this:

if (isNumber()) {
  if (value instanceof Long || value instanceof Integer || value instanceof Short || value instanceof Byte) {
    return ((Number) value).longValue();
  }
  BigInteger val = getAsBigInteger();
  if (val.bitLength() >= 64) {...throw...}
  return val.longValue();
}
return Long.parseLong(getAsString());


if (val.compareTo(BigInteger.valueOf(Long.MAX_VALUE)) > 0) {
throw new NumberFormatException("Expected a Long but was " + getAsNumber());
} else if (val.compareTo(BigInteger.valueOf(Long.MIN_VALUE)) < 0) {
throw new NumberFormatException("Expected a Long but was " + getAsNumber());
}
}

return isNumber() ? getAsNumber().longValue() : Long.parseLong(getAsString());
}

Expand All @@ -214,6 +225,17 @@ public long getAsLong() {
*/
@Override
public short getAsShort() {

if (isNumber()) {
final BigInteger val = getAsBigInteger();

if (val.compareTo(BigInteger.valueOf(Short.MAX_VALUE)) > 0) {
throw new NumberFormatException("Expected a Short but was " + getAsNumber());
} else if (val.compareTo(BigInteger.valueOf(Short.MIN_VALUE)) < 0) {
throw new NumberFormatException("Expected a Short but was " + getAsNumber());
}
}

return isNumber() ? getAsNumber().shortValue() : Short.parseShort(getAsString());
}

Expand All @@ -225,11 +247,39 @@ public short getAsShort() {
*/
@Override
public int getAsInt() {

if (isNumber()) {
final BigInteger val = getAsBigInteger();

if (val.compareTo(BigInteger.valueOf(Integer.MAX_VALUE)) > 0) {
throw new NumberFormatException("Expected an Integer but was " + getAsNumber());
} else if (val.compareTo(BigInteger.valueOf(Integer.MIN_VALUE)) < 0) {
throw new NumberFormatException("Expected an Integer but was " + getAsNumber());
}
}

return isNumber() ? getAsNumber().intValue() : Integer.parseInt(getAsString());
}

/**
* convenience method to get this element as a primitive byte.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods can all throw NumberFormatException, even before this change. I think it is the javadoc of the overridden methods in JsonElement that should be updated to reflect that.

*
* @return get this element as a primitive byte.
* @throws NumberFormatException if the value contained is not a valid integer.
*/
@Override
public byte getAsByte() {

if (isNumber()) {
final BigInteger val = getAsBigInteger();

if (val.compareTo(BigInteger.valueOf(Byte.MAX_VALUE)) > 0) {
throw new NumberFormatException("Expected a Byte but was " + getAsNumber());
} else if (val.compareTo(BigInteger.valueOf(Byte.MIN_VALUE)) < 0) {
throw new NumberFormatException("Expected a Byte but was " + getAsNumber());
}
}

return isNumber() ? getAsNumber().byteValue() : Byte.parseByte(getAsString());
}

Expand Down
76 changes: 76 additions & 0 deletions gson/src/test/java/com/google/gson/JsonParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.io.CharArrayReader;
import java.io.CharArrayWriter;
import java.io.StringReader;
import java.math.BigInteger;

import junit.framework.TestCase;

Expand Down Expand Up @@ -98,6 +99,81 @@ public void testParseReader() {
assertEquals("c", e.getAsJsonObject().get("b").getAsString());
}

public void testParseLongToInt() {
JsonObject jsonObject = new JsonObject();
jsonObject.addProperty("foo", 10000000000L);

try {
jsonObject.get("foo").getAsInt();
fail();
} catch (NumberFormatException expected) {
}
}

public void testParseIntegerTypes() {

JsonObject jsonObject = new JsonObject();

jsonObject.addProperty("bigIntegerU", BigInteger.valueOf(Long.MAX_VALUE).add(BigInteger.valueOf(200L)) );
jsonObject.addProperty("bigIntegerL", BigInteger.valueOf(Long.MIN_VALUE).subtract(BigInteger.valueOf(200L)) );
jsonObject.addProperty("longU", Integer.MAX_VALUE+143L);
jsonObject.addProperty("longL", Integer.MIN_VALUE-143L);
jsonObject.addProperty("integerU", Short.MAX_VALUE+32);
jsonObject.addProperty("integerL", Short.MIN_VALUE-32);
jsonObject.addProperty("shortU", Byte.MAX_VALUE+17);
jsonObject.addProperty("shortL", Byte.MIN_VALUE-17);

try {
jsonObject.get("bigIntegerU").getAsLong();
fail("Big Integer values greater than Long.MAX_VALUE should not parse as a Long");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to write BigInteger here with no space.

} catch (NumberFormatException expected) {
}

try {
jsonObject.get("bigIntegerL").getAsLong();
fail("Big Integer values smaller than Long.MIN_VALUE should not parse as a Long");
} catch (NumberFormatException expected) {
}

try {
jsonObject.get("longU").getAsInt();
fail("Long values larger than Integer.MAX_VALUE should not parse as an Integer");
} catch (NumberFormatException expected) {
}

try {
jsonObject.get("longL").getAsInt();
fail("Long values smaller than Integer.MIN_VALUE should not parse as an Integer");
} catch (NumberFormatException expected) {
}

try {
jsonObject.get("integerU").getAsShort();
fail("Integer values larger than Short.MAX_VALUE should not parse as a Short");
} catch (NumberFormatException expected) {
}

try {
jsonObject.get("integerL").getAsShort();
fail("Integer values smaller than Short.MIN_VALUE should not parse as a Short");
} catch (NumberFormatException expected) {
}

try {
jsonObject.get("shortU").getAsByte();
fail("Short values larger than Byte.MAX_VALUE should not parse as a Byte");
} catch (NumberFormatException expected) {
}

try {
jsonObject.get("shortL").getAsByte();
fail("Short values smaller than Byte.MIN_VALUE should not parse as a Byte");
} catch (NumberFormatException expected) {
}

assertTrue(jsonObject.isJsonObject());
}

public void testReadWriteTwoObjects() throws Exception {
Gson gson = new Gson();
CharArrayWriter writer = new CharArrayWriter();
Expand Down