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.

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

Allow JSON.stringify to operate on Java objects #860

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

tonygermano
Copy link
Contributor

  • Boxed Primitives and Strings are unwrapped and converted to
    javascript primitives.
  • Collections and Arrays will be converted to javascript arrays.
  • Maps will be converted to javascript objects, but any non-string-
    like keys will be ignored.
  • Other java classes will throw a TypeError.

Conversions occur after the replacer function is called if one is
provided, so custom handling is still possible.

Currently a StackOverflowError occurs when JSON.stringify encounters
a java object without a toJSON method.

fixes #578

- Boxed Primitives and Strings are unwrapped and converted to
  javascript primitives.
- Collections and Arrays will be converted to javascript arrays.
- Maps will be converted to javascript objects, but any non-string-
  like keys will be ignored.
- Other java classes will throw a TypeError.

Conversions occur after the replacer function is called if one is
provided, so custom handling is still possible.

Currently a StackOverflowError occurs when JSON.stringify encounters
a java object without a toJSON method.
Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

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

I'm worried that the new exception will break a lot of existing code. Can we do this without throwing an exception in a new place?

}
return jo((Scriptable) value, state);
} else if (!Undefined.isUndefined(value)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm worried that throwing this exception will break backward compatibility. Previously, someone with a complex object who is trying to stringify it might end up with a weird result, and now they will get a less weird result. But if there's a chance that people who were previously getting weird results will now get an exception, then we could easily break a lot of code. Unless the ECMA spec says that we should throw an exception in this case, then do we really need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code sees it as a NativeJavaObject, which is Scriptable, and so it sends it down the path to try to stringify it as a javascript object, because it is not Callable. This is throwing the StackOverflowError mentioned in the linked issue. I believe the ECMA spec says that all Objects that are not Functions should throw a TypeError if they are cyclic or return a JSON string representing an object.

Since I am unwrapping the NativeJavaObject, it is no longer Scriptable. I think at this point it either has to be Undefined or some other Java Object that can't be serialized, so the options are to either throw a more descriptive exception than it had been throwing, return Undefined, or return "{}".

I believe prior to 4c76ef2 it never made it this far and threw the other exception described in the linked issue:

Java class "[Ljava.lang.reflect.Constructor;" has no public instance field or method named "toJSON"

That commit let the object make it through to the replacer, but created a worse exception if a replacer did not exist to do something with the object.

I think my TypeError is in line with the historical behavior before the StackOverflowError, but I am open to suggestions. This also mimics the behavior of BigInt, which throws a TypeError when you try to stringify it without a replacer or adding a custom BigInt.prototype.toJSON method.

@gbrail
Copy link
Collaborator

gbrail commented Apr 14, 2021

OK -- the fact that it used to fail in worse ways convinces me. This looks good. Thanks!

@gbrail gbrail merged commit 0ae3129 into mozilla:master Apr 14, 2021
@tuchida
Copy link
Contributor

tuchida commented Apr 14, 2021

This is a minor issue, though.

java.lang.Long and java.math.BigDecimal inherit from java.lang.Number, so they seem to output doubleValue values.

js> java.lang.Long.MAX_VALUE
9223372036854776000
js> java.lang.Long.valueOf(java.lang.Long.MAX_VALUE)
9223372036854775807

js> JSON.stringify(java.lang.Long.MAX_VALUE)
9223372036854776000
js> JSON.stringify(java.lang.Long.valueOf(java.lang.Long.MAX_VALUE))
9223372036854776000

The JSON specification seems to recommend that you do not exceed the range of double.
https://tools.ietf.org/html/rfc8259#section-6

@tonygermano
Copy link
Contributor Author

I'm not sure if it's an issue. Anywhere else those types are used as numbers they are converted to their double value as well, e.g.,

js> java.lang.Long.MAX_VALUE === java.lang.Long.MAX_VALUE + 1
true

I believe the recommendation in the spec is to not (as a JSON user) attempt to stringify numbers representing integer values greater than Number.MAX_SAFE_INTEGER for maximum interoperability. A java.lang.Long doesn't exceed the range of Double, but it does exceed the integer range of Double in which there is no loss of precision.

As a JSON user, if you expect to have numbers that are integers which exceed Number.MAX_SAFE_INTEGER and they must be parsed back to their exact value, they should be represented in JSON as strings instead.

This was in the Chrome console. JSON itself can definitely handle integers that large, but javascript can't.

> JSON.parse("9223372036854775807")
9223372036854776000

In the case of storing a java.lang.Long without any loss of precision, I think that would mean doing something like:

js> l1 = java.lang.Long.valueOf(java.lang.Long.MAX_VALUE)
9223372036854775807
js> json = JSON.stringify(l1.toString())
"9223372036854775807"
js> l2 = java.lang.Long.valueOf(JSON.parse(json))
9223372036854775807
js> l1.equals(l2)
true

I would put the responsibility on the person calling JSON.stringify to make sure that any numbers are in a reasonable range.

@tonygermano tonygermano deleted the json-stringify-java branch April 15, 2021 01:40
@tonygermano
Copy link
Contributor Author

For what it's worth, my most common use case is expecting this to work, and it previously did not:

js> var hm = new java.util.HashMap()
js> hm.put('a', 1)
null
js> hm.put('b', 'two')
null
js> hm.put('c', false)
null
js> JSON.stringify({a: hm.get('a'), b: hm.get('b'), c: hm.get('c')})
{"a":1,"b":"two","c":false}

@tuchida
Copy link
Contributor

tuchida commented Apr 15, 2021

Thank you. I think it's a good idea too.
As you are probably seeing, if #837 is merged, java.math.BigInteger will error out. ref. a44b33b

@tonygermano tonygermano restored the json-stringify-java branch April 16, 2021 01:21
@tonygermano tonygermano deleted the json-stringify-java branch April 16, 2021 01:23
@tonygermano
Copy link
Contributor Author

@tuchida Yes, that is a good point. I think that will be consistent with other BigInt related changes and java.math.BigInteger.

@p-bakker
Copy link
Collaborator

@tonygermano any chance you could determine if this PR makes #12 obsolete?

@p-bakker
Copy link
Collaborator

@tonygermano any chance you could see if PR #12 is now obsolete due to merging this PR and thus could be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Issues considered a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stackoverflow with NativeJSON.stringify after update from 1.7.7 to 1.7.11
4 participants