Improved JSON.stringify for wrapped java objects

[rPraml]

Adds better support for JSON.stringify when used with LiveConnect java objects.

Old behaviour:

• Tries to stringify java objects by accessing all internal fields with reflection.
• may fail with stackoverflow

New behaviour:

• Uses "toJSON" if there is any, otherwise toString is used.
• date/time objects (java.util.Date/java.sql.Date/Calendar/...) are converted to an ISO-8601 compatible representation
• fixes also a possible NPE, when NativeJavaList contains null values

[gbrail]

In general this looks good to me and makes sense. However recently we seem to have a few more people who are more intensively using Java interoperability than I am so perhaps they will have some comments. If we don't hear anything for a few days then IMO we should merge this.

[tuchida]

What happens in this case?

var h = new java.util.HashMap();
// key1 !== key2 && key1.toString() === key2.toString()
h.put('0.0', 'a');
h.put(0, 'b');
h; // {0.0=b, 0.0=a}
JSON.stringify(h); // ???

[tuchida]

What happens if java.math.BigInteger or java.math.BigDecimal?
For example, BigInt in ES2020 throw a TypeError exception.
https://www.ecma-international.org/ecma-262/11.0/index.html#sec-serializejsonproperty

10. If Type(value) is BigInt, throw a TypeError exception.

// SpiderMonkey
JSON.stringify(0n); // Uncaught TypeError: BigInt value can't be serialized in JSON

[rPraml]

@tuchida @gbrail Thanks for your feedback.

I agree, we should discuss some edge cases and wait for feedback from other people.

What happens if java.math.BigInteger or java.math.BigDecimal?

// Rhino
JSON.stringify(0n); // org.mozilla.javascript.EvaluatorException: missing ) after argument list

It seems, that bigints are not yet supported by Rhino. The current implementation treats java.math.BigInteger and BigDecimal as normal Java objects, so the current implementation will use "BigInteger.toString()"
So what should we do. My suggesstion is, add BigInt support to rhino first (means: extend the parser, implement NativeBigInteger and throw exception in its toJSON method)

The second example with the hashMap is also a bit tricky.
Currently only Map<String, ?> can be used in rhino. Other maps do not work well.
Especially, if you have a Map<SomeEnum,?> it does not work at all. The same is, if you use different keys with same toString method.

I am experimenting with a key conversion to support also other key types than string (you may see in this commit FOCONIS@de34409#diff-fab16163e35411a29842a0afdd4592dbf45391d96ede85a2ac737744d10ef51bR89)
With this code change, I will get the following result:

      Map h = HashMap();
      // key1 !== key2 && key1.toString() === key2.toString()
      h.put("0", "a");
      h.put(0, 'b');
      String js = "JSON.stringify(obj.x)\n";
      testIt(js, h, "{\"0\":\"b\",\"0\":\"b\"}");

I know the result is not the one we excpect, but what should we do in the case, if we have mixed-type keys?
We can modify the NativeJavaMap.getIds() method to return something like this: "[ '0', '__java.lang.Integer__0']" and use some "valueOf" magic in the toKey method to translate the key back to the real key.
Here I would need some more feedback from the people, that use the java interoperability, but I think this should not be the part of this PR.

My current use-case for JSON.stringify is, that I want debug/log the current state of java objects in javaScript.

Cheers
Roland

[tuchida]

It seems, that bigints are not yet supported by Rhino. The current implementation treats java.math.BigInteger and BigDecimal as normal Java objects, so the current implementation will use "BigInteger.toString()"

Is that true?

rhino/src/org/mozilla/javascript/NativeJSON.java

Lines 307 to 315 in f1f2c58

	if (value instanceof Number) {
	double d = ((Number) value).doubleValue();
	if (!Double.isNaN(d) && d != Double.POSITIVE_INFINITY &&
	d != Double.NEGATIVE_INFINITY)
	{
	return ScriptRuntime.toString(value);
	}
	return "null";
	}

What happens to NaN and ±Infinity if Number.toString is called?

So what should we do. My suggesstion is, add BigInt support to rhino first (means: extend the parser, implement NativeBigInteger and throw exception in its toJSON method)

I'm making this now. master...tuchida:es2020-bigint
(I make it as a hobby, so don't expect it.)
I'm in trouble if there is too much difference between BigInt and java.math.BigInteger.

[rPraml]

It seems, that bigints are not yet supported by Rhino.

Is that true?

As BigInteger implements Number, it could be, that there are different code paths as for 'POJOs'. I just grepped the code and find very few places, where "BigInt" or "BigInteger" occured. And also the parse error of 0n told me, that there is some work to do

I'm making this now. master...tuchida:es2020-bigint

Great

[tuchida]

Probably java.lang.Long will also be rounded.

rhino/src/org/mozilla/javascript/NativeJSON.java

Line 308 in f1f2c58

double d = ((Number) value).doubleValue();

[tuchida]

NativeJavaMap is too big a problem and I don't know what to do. #820

[tonygermano]

@rPraml I'm sorry, I did not see this PR before submitting my similar changes.

Here are some things I found when comparing this PR with #860, which has already been merged.

• Because you are implementing a default toJSON method for all java objects, that is being called before the replacer runs, which does not give someone the opportunity to override your defaults.
• Except for NativeJavaMap and ArrayScriptable you are unwrapping the NativeJavaObject in the toJSON method. I am unwrapping all NativeJavaObjects after the replacer, but then constructing new NativeArray and NativeObject. There probably should be a combination of approaches here.
• You have special handlers for Enum, java.sql.Date, java.sql.Time, java.util.Date, and Calendar, and I think they all make sense, because javascript Dates also get converted to strings in JSON (but they can still be handled by a replacer prior to string conversion.) Edit for clarification: Date.prototype.toJSON is defined by default. If you delete it, then the replacer can act on the Date, and if there is no replacer, then stringify returns an empty object.
• I don't think that all unhandled objects should return a toString() value. I explained in Allow JSON.stringify to operate on Java objects #860 (comment) why I chose to throw a TypeError for classes that were not explicitly handled. I do see where returning toString would be useful. I had also suggested returning undefined or an empty object as other options. Maybe this can be a Context flag?
• You have a way to catch circular references, and I do not, due to the way that I am creating new objects before the jo() or ja() methods are called.
• I did not think of looking at NativeJavaMap separately from the unwrapped java.util.Map, and I don't entirely understand how it works. I iterate over the keys myself, and only include keys of type CharSequence. When I create a test map entry where the key is new java.lang.Object() it is excluded in both of our stringify implementations. I think this may be because NativeJavaMap.getIds() calls ScriptRuntime.toString(key) before returning the id, so the lookup fails, illustrated in your branch below. This behavior also causes stringify to fail if the Map contains a javascript Symbol as a key. The overridden ScriptableObject.getIds() method would exclude Symbol keys. My implementation treats Object keys the same way as the spec treats Symbol keys, and drops them.

js> hm = new java.util.HashMap()
{}
js> o = new java.lang.Object()
java.lang.Object@133e16fd
js> os = o.toString()
java.lang.Object@133e16fd
js> hm.put(o, 'object')
null
js> hm.put(os, 'string')
null
js> hm
{java.lang.Object@133e16fd=object, java.lang.Object@133e16fd=string}
js> JSON.stringify(hm)
{"java.lang.Object@133e16fd":"string","java.lang.Object@133e16fd":"string"}
js> hm.get(o)
object

• I handle java.util.Collection as a javascript array instead of only java.util.List. One of my tests with a LinkedHashSet worked on my branch, but not in yours, and it took me a moment to figure out why.
• I'm still undecided on NativeJavaMap. @gbrail made a suggestion here Backward compatibility java.util.{List,Map} interop #820 (comment) to only enabled it by Context feature flag, and here RFC: Add generic type support and improve java.util.{List,Map} handling #827 (comment) to instead call a Rhino function to wrap the java.util.Map in a NativeObject-like proxy object rather than changing the default behavior of NativeJavaObject. I think that would be a preferable solution for me.

[rPraml]

Hello @tonygermano , I've updated my PR, maybe you can take a look at it and maybe take some ideas.
Unfortunately, my build pipeline is broken, but I'll give you some feedback:

Because you are implementing a default toJSON method for all java objects, that is being called before the replacer runs, which does not give someone the opportunity to override your defaults.

I removed that code.

Except for NativeJavaMap and ArrayScriptable you are unwrapping the NativeJavaObject in the toJSON method. I am unwrapping all NativeJavaObjects after the replacer, but then constructing new NativeArray and NativeObject. There probably should be a combination of approaches here.

I do not unwrap NativeMap/Lists, as they can passed directly to ja and jo. I adapted your code for wrapping java.util.Map and Collection.

You have special handlers for Enum...

I've added JSON.javaConverter that handles the special java types.

I don't think that all unhandled objects should return a toString() value.

I partially agree. I understand your arguments, but there may be a lot of java objects in the wild that needs to be converted.
If you take a look at Jackson, it can convert a lot of objects.
Currently, the javaConverter could be modified, to handle all the use cases. But I think it would be better to have the converter somewhere in the context/wrapFactory...

Currently, all my tests will pass. Maybe you have time to take a look at this.

cheers
Roland

[rPraml]

CHECKME: There are a lot of candidates, that may be converted with "toString". Locale, Duration, ...

[tonygermano]

I like how you did this. I was thinking almost the same thing. I still think the default should be to throw TypeErrors, but it should be easy to swap in this implementation. What do you think about another property on the JSON object that is a behavior selector for java objects between TypeError, javaConverter, empty object, or undefined, with this default implementation of javaConverter being provided?

[rPraml]

The problem is, to distinguish, when use "toString" and when throw a TypeError (or return undefined):
Some brainstorming:
Idea 1: IF obj.getClass().startsWith("java.") THEN return obj.toString ELSE throw TypeError
This will cover all java objects from the JRE. But there may be some custom add ons (e.g. joda-time) where toString makes also sense

Idea 2: IF isLiveConnect(obj) THEN return obj.toString ELSE throw TypeError
The idea is, to detect, if the object comes from outside (not from the org.mozilla.javascript package). In this case, toString may be the best choice.

Idea 3: The user has to provide an implementation to convert LiveConnect objects to JSON.
This is the idea, I mostly like.
We use a lot of Java-Beans and use Jackson on the java side. It would be great, if we produce the same result like jackson.
And the javaConverter already goes in that direction, but I'm not yet sure if this is the best solution for now.

We use a shared scope for several threads. So if one thread would modify the javaConverter (which should not be possible, as NativeJSON should be sealed), the other thread would see the modification.

What would you think, when we delegate all unconvertable objects to the WrapFactory and it can decide, what to do.

Object json = cx.getWrapFactory().javaToJson(value, indent, gap)
if (json != null) {
    return json;
}
if (!Undefined.isUndefined(value)) {
    throw ScriptRuntime.typeErrorById("msg.json.cant.serialize", value.getClass().getName());
}

[rPraml]

closing this PR as it is obsolete now due #875