oracle / graaljs

A high-performance, ECMAScript compliant, and embeddable JavaScript runtime for Java
https://www.graalvm.org/javascript/
Universal Permissive License v1.0
1.8k stars 190 forks source link

Java Map to JS Object via experimental-foreign-object-prototype Has No Members #143

Closed hashtag-smashtag closed 3 years ago

hashtag-smashtag commented 5 years ago

I'm using RC15. Here's a failing test (in Groovy) that shows the issue:

@Test
void "Java Map converts to JS Object, but members are not visible"() {
    def context = Context.newBuilder("js")
            .allowExperimentalOptions(true)
            .option("js.experimental-foreign-object-prototype", "true")
            .build()

    def map = new HashMap<String, Integer>()
    map.put("one", 1)
    map.put("two", 2)

    context.getBindings("js").putMember("map", map)

    def value = context.eval("js", "Object.getPrototypeOf(map) === Object.prototype")
    assert value.asBoolean() // passes, hooray!

    value = context.eval("js", "Object.keys(map).length")
    assert value.asInt() == 2 // fails, finds 0

    value = context.eval("js", "map.one")
    assert value.asInt() == 1 // fails, finds undefined

    context.eval("js", "map['two']")
    assert value.asInt() == 2 // fails, finds undefined
}

I noticed for Java List -> JS Array to work required using not only experimental-foreign-object-prototype but also hostAccessBuilder.allowListAccess(true). Are we missing some equivalent .allowMapAccess(boolean) API?

iamstolis commented 5 years ago

AFAIK, there is no special support for Map now, i.e., map.one will not work even with allowHostAccess(HostAccess.ALL) because there is no field/method called one in Map. On the other hand, you can make your test-case work by using ProxyObject.fromMap(map) instead of map (context.getBindings("js").putMember("map", ProxyObject.fromMap(map))).

hashtag-smashtag commented 5 years ago

@iamstolis Thanks for the quick response. Yes, we're using ProxyObject, but hoping that: 1) The future of experimental-foreign-object-prototype might enable Map -> JSObject so we can greatly reduce the number of proxies we need to use. The recent capability of allowListAccess should enable us to eliminate all/many of our proxies for List, for example. 2) If there is some way to consistently convert Java types to JS types (see https://github.com/graalvm/graaljs/issues/144), we could at least have 1 bit of code that converts Map to a ProxyObject everywhere.

chumer commented 5 years ago

@hashtag-smashtag Thanks a lot for the suggestions.

Yes allowMapAccess is planned to be added. There are some changes to the interop protocol required for this, that will take a bit longer. Expect this to land in the next 2-3 releases/months.

ghost commented 5 years ago

I'm using

  objectConstructor = context.getBindings("js").getMember("Object");

  public Value createJSObject(final Map<String, Object> map) {
    Value object = objectConstructor.execute();
    for (Entry<String, Object> entry : map.entrySet()) {
      object.putMember(entry.getKey(), entry.getValue());
    }
    return object;
  }

What are the pros/cons compared to eg ProxyObject.fromMap() ?

hashtag-smashtag commented 5 years ago

@hanzr This issue is so one doesn't need to create a proxy nor assemble an object manually like your snippet. Btw some cons of that approach instead of a Proxy: additional boilerplate, additional object(s)/memory, JS object isn't linked to Java map such that additions/deletions to the JS object are not mirrored to the Java map

jonnermut commented 5 years ago

@chumer I hate to be the one that asks, but how's the estimate of 2-3 months looking? I just did a POC of replacing our Nashorn based scripting system with graaljs, and this issue is the only blocker.

It's a blocker because we have a bunch of user scripts out there that we can't break, using x.y and x['y'] syntax on java maps that are nested in deep object graphs, so we can't proxy them.

Unless there is some other workaround for applying proxies, or modifying the Object prototype somehow?

nhoughto commented 5 years ago

Basically you need to proxy all the way down, wrap getter() return values in a Proxy if they return Map, or the equivalent for your setup.. can be pretty tough if you don't own the objects in question, or can't easily change them though. Would be much nicer if it was OTB.

jonnermut commented 5 years ago

I don't think its going to be practical to recursively proxy the whole graph, unfortunately. So just have to suck it up and wait...

everestbt commented 5 years ago

I have a question on this issue. If you use the method ProxyObject.fromMap() you get the member access back, however you lose access to the map methods e.g. get().

If this issue is resolved would you have member access as well as access to map methods?

Also would those map methods follow the JS Map methods or Java Map methods, would it use set or put for example?

hashtag-smashtag commented 5 years ago

@everestbt With the requested allowMapAccess interop, you wouldn't need to create or deal with a proxy. In Java you'd simply continue working with your Java Map, and in JS the graal-js interop would present that instance as a JS object.

hashtag-smashtag commented 5 years ago

Yes allowMapAccess is planned to be added. There are some changes to the interop protocol required for this, that will take a bit longer. Expect this to land in the next 2-3 releases/months.

Hey @chumer is this still in the works, or on an updated roadmap? Many thanks.

Patrixe commented 4 years ago

That's a blocker for us as well, any update on this one?

dsmolyarenko commented 4 years ago

Guys, FYI it's the 15th of December, 2-3 releases/months already gone.

wirthi commented 4 years ago

Hi,

sorry for the delay. The feature is currently scheduled for our GraalVM 20 release early next year. I will check with @chumer to make sure someone is actually working on it.

andrea11 commented 4 years ago

Hello, I was looking at this feature as well. The new release 20 has been delivered, but I do not think it includes this .allowMapAccess() method. Do you know if anything is planned ?

Thanks

yuvalp-k2view commented 4 years ago

Also, looking... very interested as to when it can work. I think that without this Graal cannot be considered a Nashorn replacement.

eolivelli commented 4 years ago

This is a real show stopper for the migration of https://magnews.com to JDK15 and switch from Nashorn to GrallVM JS

zhouxiangdev commented 4 years ago

It is still unavailable.

Lzw2016 commented 4 years ago

Is there a repair plan for this problem? Nearly two years! We've dealt with it temporarily in the following ways, but it has brought about other problems

ProxyObject.fromMap(dataMap)
hashtag-smashtag commented 4 years ago

As a workaround we had to build our own public class MapProxy<V> extends AbstractMap<String, V> implements ProxyObject

And note if the members can be nested maps/lists, getMember will potentially need to wrap it in your own workaround proxy before returning it to the guest js caller.

Lzw2016 commented 4 years ago

As a workaround we had to build our own public class MapProxy<V> extends AbstractMap<String, V> implements ProxyObject

And note if the members can be nested maps/lists, getMember will potentially need to wrap it in your own workaround proxy before returning it to the guest js caller.

It does solve the problem, thanks

MirkoTeran commented 4 years ago

Is there any new timeline for allowMapAccess?

In theory we could replace all maps with MapProxy as @hashtag-smashtag suggests, but I would prefer if there was another easier way.

ankostyuk commented 4 years ago

Hello team! Can you prompt me how to implement allowMapAccess (setters and getters for map[key], map.key) yourself? I will make you a pull request)

firatkucuk commented 3 years ago

I cannot understand; how they removed the Nashorn engine from JDK 15 before fixing this?

BbIKTOP commented 3 years ago

I cannot understand; how they removed the Nashorn engine from JDK 15 before fixing this?

Noone understand why they removed working code at all

firatkucuk commented 3 years ago

I have created a PR for this. There might be some missing points 'cause I am not fully familiar with the complete architecture. If you guide me for the missing points. I can try to add but basic functionality seems working.

BbIKTOP commented 3 years ago

I have created a PR for this.

Thank you!There’s List as well, that will be great to have “natively” in the js

ispringer commented 3 years ago

Lists work already. From the initial comment on this issue:

I noticed for Java List -> JS Array to work required using not only experimental-foreign-object-prototype but also hostAccessBuilder.allowListAccess(true). Are we missing some equivalent .allowMapAccess(boolean) API?

firatkucuk commented 3 years ago

With PR I have tested those:

with allowListAccess(true) we are able to access java.util.List In indexed way.

bindings.putMember("list", List.of("1", "2", "3"));
assert "1".equals(context.eval(JavaScriptLanguage.ID, "list[0]").asString());

dot notation and indexed access using ProxyObject for java.util.Map is possible:

bindings.putMember("map", ProxyObject.fromMap(Map.of("one", 1)));
assert Integer.valueOf(1).equals(context.eval(JavaScriptLanguage.ID, "map['one']").asInt());
assert Integer.valueOf(1).equals(context.eval(JavaScriptLanguage.ID, "map.one").asInt());

with allowMapAccess(true):

bindings.putMember("map", Map.of("one", 1));
assert Integer.valueOf(1).equals(context.eval(JavaScriptLanguage.ID, "map['one']").asInt());
assert Integer.valueOf(1).equals(context.eval(JavaScriptLanguage.ID, "map.one").asInt());

TBH: I have no idea why there are those configuration options like allowListAccess, Java can be the first-class citizen in Graal, It's running on JVM and we need an extra effort for supporting Java's most used collection types.

voidmain commented 3 years ago

I'm on 21.0.0.2 (not SemVer compliant - you should consider using SemVer for your versions going forward) and I don't see allowListAccess or allowMapAccess on the builder.

The issue here is nested objects. If I have a POJO like this:

public class User {
  public String name;
  public Map<String, Object> otherStuff = new HashMap<>();
}

I can't pass this to Graal JS at all. Instead, I have to convert this to a Map, proxy it, and then convert it back to a POJO. Not ideal and I'm not sure why Graal doesn't handle this with core Java types and collections.

EDIT: In addition to this POJO issue, it appears that even the ProxyObject does not work as expected. It does not support nested JavaScript objects. For example, if I send in a Map to this JavaScript:

function editUser(user) {
  user.otherStuff.testing = "Testing";
}

And send that in via Java code like this:

Value func = context.getBindings("js").getMember("editUser);
Map<String, Object> userAsMap = convertUserObjectToMap(user);
ProxyObject proxy = ProxyObject.fromMap(userMap);
func.execute(proxy);

At the end of this call, the Map does not contain the value testing in the nested Map named otherStuff. This means I need to do a recursive traversal of the Map and proxy everything, which might not even work (haven't tested it yet).

I'm doing a lot of converting back and forth to JSON and Collections using Jackson with a lot of recursion to fix this all up because Graal JS is not properly handling Maps and Lists.

EDIT # 2: Doing a recursive proxy does work. The code is nasty, but I'm happy to post it somewhere for others to use if anyone needs it. My comment here is getting pretty long, but I could post it here as well.

BbIKTOP commented 3 years ago

@voidmain Not really. It's even worse, if you'd assign an object value in the js, it will be PolyglotMap or PolyglotList, which is

class PolyglotMap<K, V> extends AbstractMap<K, V> implements HostWrapper {

For your example, suppose that you set user.otherStuff.testing={ f: 'v' }, then 'testing' will be stored as PolyglotMap inside your ProxyObject, so, you need to convert it as well. Graal is inconsistent even in itself, but uses AbstractMap and AbstractList inside. It's sad that devs are focused on runing nodejs but not on real needs. Recursion is good, but in case java objects are large and there are many of them, it will be a real hassle to convert and overall performance will be just terrible. The good news is, that nashorn is still alive and available as a standalone module here: https://github.com/openjdk/nashorn

wirthi commented 3 years ago

Hi @voidmain

thanks for your post.

I can't pass this to Graal JS at all.

Can you please clarify what you mean with "at all"? You can't pass it, you can't access it in Java semantics, you can't access it in JavaScript semantics?

I'm not sure why Graal doesn't handle this with core Java types and collections.

That's exactly what we are doing and what others above are complaining about. We treat java.util..Map as Java type on the JS side, you can access it by Java means (e.g., calling Map.get()). I understand this use-case should be fully supported; if not, please let us know with a concrete example.

What is not working to the full extend currently is accessing the HashMap in the syntax ans semantics of JavaScript. Treating it as a JavaScript object and expecting some magic in the back to map that to (Hash)Map. We are doing this for Arrays and Lists, but the support for Map is currently being worked on (CC @chumer). Hacking together support for this between Java and JavaScript would be relatively easy, but we also need to make sure it works between other languages as well, i.e. Ruby<=>Java and Ruby<=>JavaScript.

Best, Christian

woess commented 3 years ago

Good news everyone: Support for this will finally land in 21.1.0. Using hostAccessBuilder.allowMapAccess(true), hashMap[key] and hashMap.key will work the same way as in Nashorn. Thanks for your patience!

woess commented 3 years ago

PS: Object.keys(hashMap) will not return map keys. Note that this does not work in Nashorn either.