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

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

Closed
hashtag-smashtag opened this issue Apr 11, 2019 · 34 comments
Assignees
Labels
interop Polyglot Language Interoperability

Comments

@hashtag-smashtag
Copy link

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
Copy link
Member

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
Copy link
Author

@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 Custom Java -> JS Conversion #144), we could at least have 1 bit of code that converts Map to a ProxyObject everywhere.

@chumer
Copy link
Member

chumer commented Apr 11, 2019

@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.

@wirthi wirthi added the interop Polyglot Language Interoperability label May 20, 2019
@ghost
Copy link

ghost commented May 20, 2019

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
Copy link
Author

@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
Copy link

jonnermut commented Jun 20, 2019

@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
Copy link

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
Copy link

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
Copy link

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
Copy link
Author

@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
Copy link
Author

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
Copy link

Patrixe commented Dec 10, 2019

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

@dsmolyarenko
Copy link

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

@wirthi
Copy link
Member

wirthi commented Dec 16, 2019

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
Copy link

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
Copy link

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

@eolivelli
Copy link

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

@zhouxiangdev
Copy link

It is still unavailable.

@Lzw2016
Copy link

Lzw2016 commented Oct 9, 2020

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
Copy link
Author

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
Copy link

Lzw2016 commented Oct 12, 2020

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
Copy link

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
Copy link

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)

knoxg added a commit to randomnoun/jessop that referenced this issue Dec 19, 2020
@firatkucuk
Copy link

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

@BbIKTOP
Copy link

BbIKTOP commented Jan 6, 2021

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
Copy link

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
Copy link

BbIKTOP commented Feb 18, 2021

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
Copy link

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
Copy link

firatkucuk commented Feb 19, 2021

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
Copy link

voidmain commented Feb 26, 2021

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
Copy link

BbIKTOP commented Feb 27, 2021

@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
Copy link
Member

wirthi commented Mar 1, 2021

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 woess self-assigned this Mar 22, 2021
@woess
Copy link
Member

woess commented Mar 22, 2021

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
Copy link
Member

woess commented Mar 22, 2021

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

@woess woess closed this as completed Mar 22, 2021
Phillipus added a commit to archimatetool/archi-scripting-plugin that referenced this issue Sep 6, 2022
Phillipus added a commit to archimatetool/archi-scripting-plugin that referenced this issue Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Polyglot Language Interoperability
Projects
None yet
Development

No branches or pull requests