Weird null behaviour

[koen-serry]

When parsing {"abc":null}

jsonObject.getLong("abc",0);

throws a NumberFormatException because the value is somehow converted to "null"

Unsure if we're supposed to use jsonObject.get().isNull() or if we could enhance the method below to do the null checking instead.

public JsonValue get(String name) {
        if(name == null) {
            throw new NullPointerException("name is null");
        } else {
            int index = this.indexOf(name);
           // if NullLiteral.equals(this.values.get(index)) ??
            return index != -1?(JsonValue)this.values.get(index):null;
        }
    }

[sbandara]

Since the API specification says

If this member's value does not represent a JSON number or if it cannot be interpreted as Java long, an exception is thrown.

So I think the behavior you observe is correct. If you know that members can be the JSON literal null, checking with isNull() seems appropriate. I don't think there is any reasonable value asLong() could or should possibly return in this situation.

[koen-serry]

It's a pity though to provide such a handy (clean) default function and then have to wrap it with an isNull? To which spec are you referring to? Json spec doesn't seem to mention java. I've added a pull request, just in case.

[sbandara]

I meant to refer to the API specification, but the issue you point out is even more subtle than that. The more obvious problem with replacing the JSON literal null as defined in JsonLiteral.NULL for null is that it would result in JsonObject.get return null for both the JSON literal null and for any non-existent member. Many people use minimal-json and expect it to return null if the member does not exist and to return the JsonLiteral.NULL if the member's value is the JSON literal null. The more subtle issue is that it would blur an important semantic divide between Java's null and the JSON literal which the current minimal-json is consistent about -- in contrast to Crockford's library, which screwed up in that regard. As a user of the library I would get very upset if the specification changed in such core behavior. The current behavior of throwing a NumberFormatException seems reasonable if you consider that "null" does not represent a number. In contrast, a NullPointerException would imply a Java null, although JsonLiteral.NULL is not a null pointer.

[koen-serry]

Would you agree this sound like implementing Undefined in java?

[sbandara]

Maybe you can convince Ralf to change the API specification slightly such that getLong and its siblings return the default value not only when get returns null but also for JsonLiteral.NULL. That may be a bit less invasive...

[ralfstx]

As it has been pointed out already, there is a difference between {"foo": null} and {}. Calling jsonObject.get("foo") would return JsonValue.NULL in the first case and null in the latter case. I think it's obvious that this distinction is needed.

As for @sbandara's suggestion to let the getter functions with a default return that default also in case of null, this sounds reasonable to me at a first glance. However, what should happen in case of {"foo": true}? What if the value is a number that exceeds Long.MAX_VALUE? Should the default method always return the default value, if the value is either missing or of a different type?

[koen-serry]

Yes that sounds reasonable (default to null when eg. getString('foo','n/a') is being called). As for the invalid entries I would deal with them the same way. The whole idea of having a default is to have a fallback in case something is wrong (type or value)

[ralfstx]

Wouldn't it be unexpected when getInt("size", -1) returns 23 for {"size": 23} but -1 for {"size": 23.5}?

[koen-serry]

Not really as it's a default. If one would want a fallback you can put a wrapping try catch block around it without the default.

[ralfstx]

Did you really get a NumberFormatException or an UnsupportedOperationException? I get the following trace:

java.lang.UnsupportedOperationException: Not a number: null
at com.eclipsesource.json.JsonValue.asLong(JsonValue.java:329)
at com.eclipsesource.json.JsonObject.getLong(JsonObject.java:577)

That's expected with the current contract, but a NFE would be a bug.

[bernardosulzbach]

I want to express my opinion about this. I know I don't need to update my dependencies, but this proposal is just wrong.

The whole idea of having a default is to have a fallback in case something is wrong (type or value)

No. It is a default, not a fallback. If it is not present, give me the default, if I find an elephant when expecting a maple tree, please let me know.

A default value is not supposed to be the if-anything-wrong-happens-give-me-this, if a system wrote a string to a value that should be an integer I want an exception. The current API has a simple and straightforward contract (never surprised me at least) and I don't want to change it without reason.

Wouldn't it be unexpected when getInt("size", -1) returns 23 for {"size": 23} but -1 for {"size": 23.5}?

Not really as it's a default. If one would want a fallback you can put a wrapping try catch block around it without the default.

WHAT?!

This snippet could be used in your scenario, which would be much better than changing a dependency that multiple projects rely on.

private static long safeGetLong(JsonObject jsonObject, String name, long fallback) {
  JsonValue value = jsonObject.get(name);
  if (value != null) {
    if (value.isNumber()) {
      try {
        return value.asLong();
      } catch (NumberFormatException exception) {
        // You will most certainly want to (at least) log this.
        // Maybe throw an AssertionError.
        // Or, even, get the string and pass it to the BigInteger constructor.
      }
    } else {
      // Not a number, should also do something about it.
    }
  }
  return fallback;
}

[koen-serry]

While I agree that having a default is not necessarily the same as a fallback. It all comes down to what you're going to do with it. The code sample you gave above is a very simple example of what you could do if you want to handle every tiny little variation of the value given. In practice you'd want to avoid it since it'll make your code pretty much unreadable let alone maintainable (if you go away from the basic hello world). As a simple example suppose I'd given "123" as a value. It's not a number perse, but still I'd be able to parse it as a string and convert it into a number.

Raising an error (aka flow by exception) can't be a good way of designing something as basic as a json lib. Maybe a simple fix as in adding asString in the "JsonNumber" object not blowing up with a UnsupportedOperationException would already be an improvement.

K.

[ralfstx]

asString in the "JsonNumber" object not blowing up with a UnsupportedOperationException would already be an improvement

You can call toString on a JsonNumber, which will return the JSON string of that number. If you're asking for implicit type conversions, this would be another issue. But I'm almost certain that is a box we don't want to open.

[bernardosulzbach]

In practice you'd want to avoid it since it'll make your code pretty much unreadable let alone maintainable.

Prove your claim. That function could be placed in a top-level class and shared project-wide, maybe even system-wide.

Raising an error (aka flow by exception) can't be a good way of designing something as basic as a json lib.

You are implying that a JSON library is something simple and here we are discussing API contracts.
Also, it is not an Error. It is a RuntimeException.

There is this little thing called design by contract which is how most serious APIs are written. You are suggesting defensive programming, which is not OK for a library that is not doing something trivial in most scenarios.

[ralfstx]

Thanks @mafagafogigante for your insights, the distinction between a default and a fallback really nails it. I've been pondering with this issue, and I see the point of providing convenient APIs. However, the suggested behavior of returning the default value also in case of an unexpected type doesn't seem right to me.

It would require to swallow exceptions, which is dangerous, as it has the potential of shadowing issues in your code. For example, when you're not aware that certain values in the parsed JSON have a different type (e.g. an array of numbers instead of a number), the suggested API would silently fall back, and you wouldn't ever notice.

[ralfstx]

Still there is a case that concerns me:

String string = null;
jsonObject.set("foo", string);

will accept null and set "foo" to Json.NULL. This was done for the sake of convenience, and it seemed like the obvious implementation as there is a JSON representation for null.

OTOH, object.getString("foo", "unknown") would currently fail because the literal null is not a string. This is at least asymmetric and it may be unexpected.

[bernardosulzbach]

I beg to differ.

Application code should check if foo is there by using object.get("foo").isNull() if the system uses null to mean unknown (otherwise just compare the get result with null or - as aforementioned - recur to the getString method if there is a default value for these cases). It is a problem for whoever started setting "not there" and "unknown" to null.

From the Javadoc

If this member's value does not represent a JSON string, an exception is thrown.

JSON null does not represent a JSON string, unless I am somewhat mistaken about the standard.

Just for the record, Google suggests here:

Empty/Null Property Values

Consider removing empty or null values.

If a property is optional or has an empty or null value, consider dropping the property from the JSON, unless there's a strong semantic reason for its existence.

[Gyannea]

It might be late to comment on this rather long conversation but I was having a problem working with JSON generated by other libraries because it would do something like the following:
"secondName":null
So I solved the issue by taking this

return value != null ? value.asString() : defaultValue;

and making it this

return value != null ? (value.isNull() ? null : value.asString()) : defaultValue;

Basically two null checks. One for a null that should not happen and another one for one that could and does happen.

[manuelp]

I'm pretty late to the party, anyway FYI here is the above @Gyannea solution using Java 8 Optional class:

return Optional.of(value) // Optional<JsonValue>
               .map(JsonValue::asString) // Optional<String>
               .orElse(defaultValue); // JsonString

Or using Functional Java (available also for Java 7-, although you'd have to use anonymous classes since method references are not available):

return Option.fromNull(value).map(JsonValue::asString).orSome(defaultValue);

I also agree that:

• There is no value (the JSON property is absent).
• The value is of the wrong type (you expect a JSON string but there is a JSON boolean).
• The value is a JSON null.

Are very different cases. If in some contexts some of those cases are conflated, it may be totally fine but IMHO it's part of a specific protocol and should not be encouraged by being implemented in a generic JSON library.

As @ralfstx said, {foo: null} and {} are two very different cases, since the fact that null = no value is just one interpretation of those two different JSON structures that may have different semantics in the specific context and a generic JSON library IMO should reflect this fact.

TL;DR: the current contract of minimal-json IMHO is perfectly fine here.

[bernardosulzbach]

This is an existing and used library. Deployed code relies on it and on its interface. There might be legitimate reasons for changing parts of it, but "it is a minor inconvenience to me" is not one. Your argument for the change is

I want default value if it's null. Otherwise, if I call #getString() from an integer, just throw me an exception.

which makes no sense. You want a default value if it is null, so write your own wrapper around the library. This library already exists and works this way, it should not be changed because someone has some expectations of how it should work.

Not having "x" and having it set to null are two different things and need not to be handled the same way.