core: have JsonUtil support parsing String value as number

[dapengzhang0]

As documented in https://developers.google.com/protocol-buffers/docs/proto3#json,
the canonical proto-to-json converter converts int64 (Java long) values to string values in Json rather than Json numbers (Java Double). Conversely, either Json string value or number value are accepted to be converted to int64 proto value.

To better support service configs defined by protobuf messages, support parsing String values as numbers in JsonUtil.

[dapengzhang0]

cc @ejona86

[sergiitk]

Since we're returning wrapped Double, would it make sense to use Double.valueOf instead?

Suggested change

	return Double.parseDouble((String) value);
	return Double.valueOf((String) value);

[dapengzhang0]

I'll keep these as-is as they are nits/minor.

[sergiitk]

I'm thinking how to express that the string representation failed. Maybe

Suggested change

	String.format("value '%s' for key '%s' is not a double", value, key));
	String.format("string value '%s' for key '%s' is not a double", value, key));

or

Suggested change

	String.format("value '%s' for key '%s' is not a double", value, key));
	String.format("value '%s' for key '%s' is not a string representation of a double", value, key));

[sergiitk]

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

public static Double getNumberAsDouble(Object value) { ... }
public static Double getNumberAsDouble(String value) { ... }

and then call it as, for example, JsonUtil.getNumberAsInteger(retryPolicy.get("maxAttempts"));

Seems like cleaner approach, easier to read, and easier to test.

[ejona86]

1. Convenience. We need clear error messages when a field is not present. Getting a field from an object is the strongly-common case, so it would be worth the duplicate method
2. The only other way to get a number is from a list, and we tend to have specialized helpers for that (getListOfObects, getListOfStrings). It is rare (currently non-existent) to need to mix-and-match types within a list.

[sergiitk]

Maybe something like this?

  @Nullable
  public static Double getNumberAsDoubleFromObj(Map<String, ?> obj, String key) {
    try {
      return getNumberAsDouble(getFromObj(obj, key));
    } catch (NumberFormatException e) {
      throw new IllegalArgumentException(String.format("key '%s': %s", key, e.getMessage()));
    } catch (IllegalArgumentException e) {
      throw new IllegalArgumentException(
          String.format("key '%s' in '%s':  %s", key, obj, e.getMessage()));
    }
  }

  public static Double getNumberAsDouble(Object value) {
    if (value instanceof Double) {
      return (Double) value;
    }
    if (value instanceof String) {
      try {
        return Double.parseDouble((String) value);
      } catch (NumberFormatException e) {
        throw new NumberFormatException(
            String.format("value '%s' is not a string representation of a double", value));
      }
    }
    throw new IllegalArgumentException(String.format("value '%s' is not a number", value));
  }

 static Object getFromObj(Map<String, ?> obj, String key) {
    assert key != null;
    if (!obj.containsKey(key)) {
      return null;
    }
    return obj.get(key);
  }

Pros:

1. we get separation of concerns
2. getFromObj (name tentative) can be reused in all getNumberAs*FromObj
3. getNumberAs* can be reused elsewhere
4. getNumberAs* are not nullable
5. straightforward tests

[ejona86]

I don't understand what getFromObj() is providing, as obj.get(key) return null 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.

[sergiitk]

I don't understand what getFromObj() is providing, as obj.get(key) return null 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.

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 every get* method in this class. So at least it's DRY.

[sergiitk]

Honestly, I'm surprised this has become a discussion point instead of something like implementing getNumberAsInteger() via getNumberAsLong() (or shared helper).

To clarify: you mean this as an argument for the private methods that we discussed above?

[ejona86]

No, those private methods were for something different. I'm talking about how getNumberAsInteger() could be implemented as:

Long l = getNumberAsLong(obj, key);
if (l == null)
  return null;
int i = l.intValue();
if ((long) i != l.longValue())
  throw ...;
return i;

[sergiitk]

Note that getNumberAsLong produces slightly different error messages, f.e: "value '%s' for key '%s' is not a long integer"

[ejona86]

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.

[dapengzhang0]

I agree there is space for improvement, but that's not something we should do in this PR.

[sergiitk]

nit: I'd recommend smaller unit tests for each individual method.

[sergiitk]

Per-IRL discussion, the only suggestion I see as important is the one with adding test for the special cases: #8711 (comment)

Everything else is nits/minor suggestion. Happy to approve as is.