RFC: Add generic type support and improve java.util.{List,Map} handling

[rPraml]

We use rhino to access beans, so you can have a bean like this:

public static class Bean {
        public List<Integer> integers = new ArrayList<>();
        private List<Double> doubles = new ArrayList<>();
        public List<Number> numbers = new ArrayList<>();
} 

you can add the proper numbers.

Old behavoiur:
"bean.doubles[0] = 3" may store an integer object in List<Double>

New behavoiur:
"bean.doubles[0] = 3" will be stored as 3.0D

As this change

• makes an API change (WrapFactory.wrap(... Class staticType) -> WrapFactory.wrap(... Type staticType)
• adds an improved map and string handling (which strongly depends on the type change)
• the key translation im map may also targets Backward compatibility java.util.{List,Map} interop #820

I would appreciate what other think of this change and if/how this change can be merged
I can split up this PR in more smaller PRs if the direction is clear

[gbrail]

I'm glad that we're focusing on improving things in this area. We've had a few PRs lately that touch on List and Map handling for Java objects, and we made a change in the last release that resulted in it working differently than it did before.

Would you, @tuchida, or someone else out there have the time to think about how we really want List and Map objects to show up in Rhino, based in part on what Nashorn did? I'm also fine with us having a few different choices based on a configuration parameter if we had to.

I'm afraid to make any more changes in this area until we're all clear on where this should go.

[tuchida]

This looks good to me.

This is just my preference, but I'd rather use for-of than for each-in. If you implement Symbol.iterator, you can use Object.entries and spread syntax (...) etc. in the future.

Also, this isn't main topic, do you need a setter of length in NativeJavaList?

var list = new java.util.ArrayList();
list.length; // 0
list.length = 3;
list; // [null, null, null]
list.length; // 3

[rPraml]

Thanks for your feedback.

First, a short introduction:
We use rhino to access beans (data object) or a list/map of them (as you do it in a lot of ORM applications)

A very early approach was, (before map/list support was added) that we converted our object graph to "NativeArray/NativeObjects".
This was good, as long we did not modify anything. But changes on the NativeArray/Object were not reflected back to the original object in the object graph. (In addition there was much conversion overhead)

So the NativeJavaMap/List wrappers are really great.
For us, a java.util.Map/List should behave as much as possible as a javscript Object/Array. This isn't always possible. E.g. the JS forEach(value, key, map) is defined in java as forEach(key, value)

Some Must-Haves (from my perspective)

• Modifications on JS-objects must be reflected on the wrapped objects (I think this point is indiscutable and also implemented)
• Types should be preserved: A bean that defines a property as Map<String, Integer> should not store a double when you do a bean.myProp = 3.0 in javaScript. This is what this PR addresses mainly
• for (elem in obj) and for each(elem in obj) should work the same way as in javascript on java.util.Map/List (also addressed by this PR)

Some Good-To-Haves

• Support maps, where the key != String. (e.g. EnumMap) - it would be ok for us, not to introcuce this change, as we can work around this, if we really need.
• Support for each(elem in obj) for Iterables.
• list.length=3 should grow the list (Hint from @tuchida )

Unclear:

• What should happen if I do this on a map javaMap.size=42. This overwrites the size() method and I cannot get the size any more with javaMap.size(). For this I made a fix, that existing functions cannot be overwritten: FOCONIS@13483f1 (Of course, I can use Object.keys(map).length)
• This is not mainly part of this PR, but as said above, we do a lot of with beans:

public class Customer {
   private String firstName;
   private String lastName;
   @ManyToMany
   private List<Item> purchasedItems;
   // ... getter and setters
}

what should happen if you do a customer.purchasedItems[] = {} and what shoud happen if you do a Object.keys(customer)

• and last but not least JSON.stringify(customer) should work

[gbrail]

For your purposes, does it make more sense for instead have a Java API that lets a Java Map appear as a JavaScript "map"? That would eliminate all ambiguity but require you to call some Rhino function to wrap a JS Map around your Java Map.

…

On Fri, Jan 22, 2021 at 4:52 AM Roland Praml ***@***.***> wrote: Thanks for your feedback. First, a short introduction: We use rhino to access beans (data object) or a list/map of them (as you do it in a lot of ORM applications) A very early approach was, (before map/list support was added) that we converted our object graph to "NativeArray/NativeObjects". This was good, as long we did not modify anything. But changes on the NativeArray/Object were not reflected back to the original object in the object graph. (In addition there was much conversion overhead) So the NativeJavaMap/List wrappers are really great. For us, a java.util.Map/List should behave as much as possible as a javscript Object/Array. This isn't always possible. E.g. the JS forEach(value, key, map) is defined in java as forEach(key, value) *Some Must-Haves (from my perspective)* - Modifications on JS-objects must be reflected on the wrapped objects (I think this point is indiscutable and also implemented) - Types should be preserved: A bean that defines a property as Map<String, Integer> should not store a double when you do a bean.myProp = 3.0 in javaScript. This is what this PR addresses mainly - for (elem in obj) and for each(elem in obj) should work the same way as in javascript on java.util.Map/List (also addressed by this PR) *Some Good-To-Haves* - Support maps, where the key != String. (e.g. EnumMap) - it would be ok for us, not to introcuce this change, as we can work around this, if we really need. - Support for each(elem in obj) for Iterables. - list.length=3 should grow the list (Hint from @tuchida <https://github.com/tuchida> ) *Unclear:* - What should happen if I do this on a map javaMap.size=42. This overwrites the size() method and I cannot get the size any more with javaMap.size(). For this I made a fix, that existing functions cannot be overwritten: ***@***.*** <FOCONIS@13483f1> (Of course, I can use Object.keys(map).length) - This is not mainly part of this PR, but as said above, we do a lot of with beans: public class Customer { private String firstName; private String lastName; @manytomany private List<Item> purchasedItems; // ... getter and setters } what should happen if you do a customer.purchasedItems[] = {} and what shoud happen if you do a Object.keys(customer) - and last but not least JSON.stringify(customer) should work — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#827 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I26KI3CJAD4HTVSDJZTS3FYKNANCNFSM4WHA5NWQ> .

[tuchida]

Is this the test for NativeJavaList#ensureCapacity?

rhino/testsrc/org/mozilla/javascript/tests/JavaListAccessTest.java

Lines 24 to 28 in 6b4b1e2

	String js = "bean.integers[0] = 3;\n"
	+ "bean.doubles[0] = 3;"
	+ "bean.doubles[0].getClass().getSimpleName() + ' ' "
	+ "+ bean.integers[0].getClass().getSimpleName()\n";
	testIt(js, "Double Integer");

• generic type support
• list[list.length]=3 should grow the list
• Implement __iterator__

Since these are separate features, it would have been easier to understand if it split commits or pull requests.

[NITS] Do you need a delete in NativeJavaList and NativeJavaMap? (delete list[0], delete map.a)

[tuchida]

• list[list.length]=3 should grow the list

[NITS] In the case of NativeArray, when you put to a index of large number, the behavior switches to map-like.
Refer to NativeArray#dense to determine if NativeJavaList#ensureCapacity behavior is appropriate.

[tuchida]

[NITS]
This is the case for NativeArray.

var a = [];
a[10] = 123;
a[0]; // undefined
0 in a; // false

This is probably the case for NativeJavaList.

var list = new java.util.ArrayList();
list[10] = 123;
list[0]; // null
0 in list; // true

Is this difference acceptable?

[rPraml]

For your purposes, does it make more sense for instead have a Java API that lets a Java Map appear as a JavaScript "map"? That would eliminate all ambiguity but require you to call some Rhino function to wrap a JS Map around your Java Map.

An interesting idea, but I do not have control, where the objects are created. So I have a third-party class that returns a List<Map<String, Integer>>.

I took a lot of inputs and made a separate PR that extracts most of the list handling code.
If this is merged, I'll try to make more smaller PRs from this PR.

[tuchida]

ref. #839

[tonygermano]

For your purposes, does it make more sense for instead have a Java API that lets a Java Map appear as a JavaScript "map"? That would eliminate all ambiguity but require you to call some Rhino function to wrap a JS Map around your Java Map.

@gbrail I was thinking this would be preferable for me, if I could use a Rhino method to return a wrapped java.util.Map that always treated property access as getting or putting Map keys, and also still be able to have a reference to the original object where I could unambiguously access any Java method or field supported by the actual class implementing Map.

I've noticed even Nashorn has some undesirable behavior dealing with this

jjs> function hello(name) {print('hello ' + name)}
jjs> var map = new java.util.HashMap()
jjs> map.a = hello
function hello(name) {print('hello ' + name)}
jjs> map.get = hello
function hello(name) {print('hello ' + name)}
jjs> map.a('alice')
hello alice
jjs> map.get('bob')
null
jjs> map.get('get')('bob')
hello bob

[tuchida]

#820 (comment)
Probably the instance changes between direct-call and other.

[p-bakker]

What is the status of this PR? Is it still something that is planned to be merged at some point?

If so, based on #972 (comment) I assume it still has breaking API changes and as such it should be added to the discussion list for v2.0.0?

[rPraml]

@p-bakker Yes, this would be a candidate for 2.0.0 - I plan to make this mergeable, but I need some time

[p-bakker]

v2.0.0 is a long way of, so no rush. Just wanted to be sure whether I should include it in the v2.0.0 project

[rPraml]

This PR is now ready for review. It adds generic type support, so that the generic type definitions in beans etc. are respected in

• property access
• method invocation
• list value types
• map value and key types (for java maps, only Integer and String keys are fully supported when FEATURE_ENABLE_JAVA_MAP_ACCESS is set)

[rPraml]

This PR is quite old, but still relevant for our fork, so I fixed the merge conflicts.
Is there a chance to get this into rhino 2.0 ;)