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
core: have JsonUtil support parsing String value as number #8711
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -96,57 +96,92 @@ public static List<String> getListOfStrings(Map<String, ?> obj, String key) { | |||||||||
|
||||||||||
/** | ||||||||||
* Gets a number from an object for the given key. If the key is not present, this returns null. | ||||||||||
* If the value is not a Double, throws an exception. | ||||||||||
* If the value does not represent a double, throws an exception. | ||||||||||
*/ | ||||||||||
@Nullable | ||||||||||
public static Double getNumber(Map<String, ?> obj, String key) { | ||||||||||
public static Double getNumberAsDouble(Map<String, ?> obj, String key) { | ||||||||||
assert key != null; | ||||||||||
if (!obj.containsKey(key)) { | ||||||||||
return null; | ||||||||||
} | ||||||||||
Object value = obj.get(key); | ||||||||||
if (!(value instanceof Double)) { | ||||||||||
throw new ClassCastException( | ||||||||||
String.format("value '%s' for key '%s' in '%s' is not Double", value, key, obj)); | ||||||||||
if (value instanceof Double) { | ||||||||||
return (Double) value; | ||||||||||
} | ||||||||||
if (value instanceof String) { | ||||||||||
try { | ||||||||||
return Double.parseDouble((String) value); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're returning wrapped
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll keep these as-is as they are nits/minor. |
||||||||||
} catch (NumberFormatException e) { | ||||||||||
throw new IllegalArgumentException( | ||||||||||
String.format("value '%s' for key '%s' is not a double", value, key)); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking how to express that the string representation failed. Maybe
Suggested change
or
Suggested change
|
||||||||||
} | ||||||||||
} | ||||||||||
return (Double) value; | ||||||||||
throw new IllegalArgumentException( | ||||||||||
String.format("value '%s' for key '%s' in '%s' is not a number", value, key, obj)); | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Gets a number from an object for the given key, casted to an integer. If the key is not | ||||||||||
* present, this returns null. If the value is not a Double or loses precision when cast to an | ||||||||||
* integer, throws an exception. | ||||||||||
* present, this returns null. If the value does not represent an integer, throws an exception. | ||||||||||
*/ | ||||||||||
@Nullable | ||||||||||
public static Integer getNumberAsInteger(Map<String, ?> obj, String key) { | ||||||||||
Double d = getNumber(obj, key); | ||||||||||
if (d == null) { | ||||||||||
assert key != null; | ||||||||||
if (!obj.containsKey(key)) { | ||||||||||
return null; | ||||||||||
} | ||||||||||
int i = d.intValue(); | ||||||||||
if (i != d) { | ||||||||||
throw new ClassCastException("Number expected to be integer: " + d); | ||||||||||
Object value = obj.get(key); | ||||||||||
if (value instanceof Double) { | ||||||||||
Double d = (Double) value; | ||||||||||
int i = d.intValue(); | ||||||||||
if (i != d) { | ||||||||||
throw new ClassCastException("Number expected to be integer: " + d); | ||||||||||
} | ||||||||||
return i; | ||||||||||
} | ||||||||||
if (value instanceof String) { | ||||||||||
try { | ||||||||||
return Integer.parseInt((String) value); | ||||||||||
} catch (NumberFormatException e) { | ||||||||||
throw new IllegalArgumentException( | ||||||||||
String.format("value '%s' for key '%s' is not an integer", value, key)); | ||||||||||
} | ||||||||||
} | ||||||||||
return i; | ||||||||||
throw new IllegalArgumentException( | ||||||||||
String.format("value '%s' for key '%s' is not an integer", value, key)); | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Gets a number from an object for the given key, casted to an long. If the key is not | ||||||||||
* present, this returns null. If the value is not a Double or loses precision when cast to an | ||||||||||
* long, throws an exception. | ||||||||||
* present, this returns null. If the value does not represent a long integer, throws an | ||||||||||
* exception. | ||||||||||
*/ | ||||||||||
public static Long getNumberAsLong(Map<String, ?> obj, String key) { | ||||||||||
Double d = getNumber(obj, key); | ||||||||||
if (d == null) { | ||||||||||
assert key != null; | ||||||||||
if (!obj.containsKey(key)) { | ||||||||||
return null; | ||||||||||
} | ||||||||||
long l = d.longValue(); | ||||||||||
if (l != d) { | ||||||||||
throw new ClassCastException("Number expected to be long: " + d); | ||||||||||
Object value = obj.get(key); | ||||||||||
if (value instanceof Double) { | ||||||||||
Double d = (Double) value; | ||||||||||
long l = d.longValue(); | ||||||||||
if (l != d) { | ||||||||||
throw new ClassCastException("Number expected to be long: " + d); | ||||||||||
} | ||||||||||
return l; | ||||||||||
} | ||||||||||
if (value instanceof String) { | ||||||||||
try { | ||||||||||
return Long.parseLong((String) value); | ||||||||||
} catch (NumberFormatException e) { | ||||||||||
throw new IllegalArgumentException( | ||||||||||
String.format("value '%s' for key '%s' is not a long integer", value, key)); | ||||||||||
} | ||||||||||
} | ||||||||||
return l; | ||||||||||
throw new IllegalArgumentException( | ||||||||||
String.format("value '%s' for key '%s' is not a long integer", value, key)); | ||||||||||
} | ||||||||||
|
||||||||||
|
||||||||||
/** | ||||||||||
* Gets a string from an object for the given key. If the key is not present, this returns null. | ||||||||||
* If the value is not a String, throws an exception. | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
/* | ||
* Copyright 2021, gRPC Authors All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package io.grpc.internal; | ||
|
||
import static com.google.common.truth.Truth.assertThat; | ||
import static org.junit.Assert.fail; | ||
|
||
import java.util.HashMap; | ||
import java.util.Map; | ||
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
import org.junit.runners.JUnit4; | ||
|
||
/** Tests for {@link JsonUtil}. */ | ||
@RunWith(JUnit4.class) | ||
public class JsonUtilTest { | ||
sergiitk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@Test | ||
public void getNumber() { | ||
Map<String, Object> map = new HashMap<>(); | ||
map.put("key_number_1", 1D); | ||
map.put("key_string_2.0", "2.0"); | ||
map.put("key_string_3", "3"); | ||
map.put("key_string_nan", "NaN"); | ||
map.put("key_number_5.5", 5.5D); | ||
map.put("key_string_six", "six"); | ||
map.put("key_string_infinity", "Infinity"); | ||
map.put("key_string_minus_infinity", "-Infinity"); | ||
map.put("key_string_exponent", "2.998e8"); | ||
map.put("key_string_minus_zero", "-0"); | ||
|
||
assertThat(JsonUtil.getNumberAsDouble(map, "key_number_1")).isEqualTo(1D); | ||
assertThat(JsonUtil.getNumberAsInteger(map, "key_number_1")).isEqualTo(1); | ||
assertThat(JsonUtil.getNumberAsLong(map, "key_number_1")).isEqualTo(1L); | ||
|
||
assertThat(JsonUtil.getNumberAsDouble(map, "key_string_2.0")).isEqualTo(2D); | ||
try { | ||
JsonUtil.getNumberAsInteger(map, "key_string_2.0"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo( | ||
"value '2.0' for key 'key_string_2.0' is not an integer"); | ||
} | ||
try { | ||
JsonUtil.getNumberAsLong(map, "key_string_2.0"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo( | ||
"value '2.0' for key 'key_string_2.0' is not a long integer"); | ||
} | ||
|
||
assertThat(JsonUtil.getNumberAsDouble(map, "key_string_3")).isEqualTo(3D); | ||
assertThat(JsonUtil.getNumberAsInteger(map, "key_string_3")).isEqualTo(3); | ||
assertThat(JsonUtil.getNumberAsLong(map, "key_string_3")).isEqualTo(3L); | ||
|
||
assertThat(JsonUtil.getNumberAsDouble(map, "key_string_nan")).isNaN(); | ||
try { | ||
JsonUtil.getNumberAsInteger(map, "key_string_nan"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo( | ||
"value 'NaN' for key 'key_string_nan' is not an integer"); | ||
} | ||
try { | ||
JsonUtil.getNumberAsLong(map, "key_string_nan"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo( | ||
"value 'NaN' for key 'key_string_nan' is not a long integer"); | ||
} | ||
|
||
assertThat(JsonUtil.getNumberAsDouble(map, "key_number_5.5")).isEqualTo(5.5D); | ||
try { | ||
JsonUtil.getNumberAsInteger(map, "key_number_5.5"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo("Number expected to be integer: 5.5"); | ||
} | ||
try { | ||
JsonUtil.getNumberAsLong(map, "key_number_5.5"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo("Number expected to be long: 5.5"); | ||
} | ||
|
||
try { | ||
JsonUtil.getNumberAsDouble(map, "key_string_six"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo( | ||
"value 'six' for key 'key_string_six' is not a double"); | ||
} | ||
try { | ||
JsonUtil.getNumberAsInteger(map, "key_string_six"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo( | ||
"value 'six' for key 'key_string_six' is not an integer"); | ||
} | ||
try { | ||
JsonUtil.getNumberAsLong(map, "key_string_six"); | ||
fail("expecting to throw but did not"); | ||
} catch (RuntimeException e) { | ||
assertThat(e).hasMessageThat().isEqualTo( | ||
"value 'six' for key 'key_string_six' is not a long integer"); | ||
} | ||
|
||
assertThat(JsonUtil.getNumberAsDouble(map, "key_string_infinity")).isPositiveInfinity(); | ||
assertThat(JsonUtil.getNumberAsDouble(map, "key_string_minus_infinity")).isNegativeInfinity(); | ||
assertThat(JsonUtil.getNumberAsDouble(map, "key_string_exponent")).isEqualTo(2.998e8D); | ||
|
||
assertThat(JsonUtil.getNumberAsDouble(map, "key_string_minus_zero")).isZero(); | ||
assertThat(JsonUtil.getNumberAsInteger(map, "key_string_minus_zero")).isEqualTo(0); | ||
assertThat(JsonUtil.getNumberAsLong(map, "key_string_minus_zero")).isEqualTo(0L); | ||
|
||
assertThat(JsonUtil.getNumberAsDouble(map, "key_nonexistent")).isNull(); | ||
assertThat(JsonUtil.getNumberAsInteger(map, "key_nonexistent")).isNull(); | ||
assertThat(JsonUtil.getNumberAsLong(map, "key_nonexistent")).isNull(); | ||
} | ||
} |
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.
This might be out of scope, but what I find really strange is that we mix getting an object from a map, and converting it to something. Why not
and then call it as, for example,
JsonUtil.getNumberAsInteger(retryPolicy.get("maxAttempts"));
Seems like cleaner approach, easier to read, and easier to test.
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.
getListOfObects
,getListOfStrings
). It is rare (currently non-existent) to need to mix-and-match types within a list.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.
Maybe something like this?
Pros:
getFromObj
(name tentative) can be reused in allgetNumberAs*FromObj
getNumberAs*
can be reused elsewheregetNumberAs*
are not nullableThere 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.
I don't understand what
getFromObj()
is providing, asobj.get(key)
returnnull
for non-present keys. This isn't Python. Note, if we had such a method it should throw an exception instead of returning null. The current methods consider null to be just another type and so fall-through to catch-all exceptions.Nobody should use
getNumberAsDouble(Object)
; "you're doing it wrong" and the caller will probably have poor error handling. I'm going to argue pretty strongly that we should not have the method.The method can exist, but it can be private. So this is really not something that needs to be debated now. As I said, the main strong possibility for reuse is with arrays of all of the same time. Maybe some day we'll need per-index type management, but I think that would just be a third method when we need it; I question if we'll ever need it.
There are no nullability checks in our builds, and nullability is known to be wrong many places and overall normally missing. Nullability annotations are a non-required annotation that just acts as a part of the javadoc, essentially. Nullability is good and fine, but it doesn't help anyone catch bugs any more than documentation.
It's also not clear to me why we need so much exception handling in
getNumberAsDoubleFromObj()
.Renaming stuff to include
*FromObj
feels more like bikeshedding to me, or fixing a concern that the current callers don't suffer from. Some of those changes may be fine, but not as part of this change. That would be a separate refactor to the callers that should not change existing behaviors.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.
I think you missed the assert there.
getFromObj
does exactly what happens right now, it's a carbon copy of the first 5 lines of nearly everyget*
method in this class. So at least it's DRY.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.
To clarify: you mean this as an argument for the private methods that we discussed above?
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.
No, those private methods were for something different. I'm talking about how
getNumberAsInteger()
could be implemented as: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.
Note that
getNumberAsLong
produces slightly different error messages, f.e: "value '%s' for key '%s' is not a long integer"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.
Yeah, it was more of a sketch. And you have to figure out if the cost of merging is worth the benefit. I'm fine with it both ways (I think there are some ways that may not be too ugly to do it). Honestly, I'd be tempted to do it mostly because s/long/int/ is so error prone when comparing that logic.
@sergiitk and I talked. We're actually more on the same page than it seemed. The root of this is chain was really spawned by the tests, and I agree the tests exhibit some code smells that are a bit hard to track down the source for and the suggestions would improve things (for the tests).
The end result is probably not to change the API now. But we were also marveling at how many different issues something this simple brought up. Things could be improved with just changes to the tests, but we're probably not going to be overly critical because some of this should have been noticed with tests (that don't exist) introduced when JsonUtil was introduced. But we're also able to see "how we ended up here."
We should continue thinking about improvements to this area, but won't see changes right now.
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.
I agree there is space for improvement, but that's not something we should do in this PR.