ReactJS: Fixes to proxy object wrapping (#851)

This resolves some more bugs reported by a user. This time I sent them a test JAR and they confirmed that everything now works and they could remove their previous workarounds, so I hope this is the last round of fixes required.

Specifically these commits:

- Refactor the code to ensure map values are always mapped
- Adds unit tests specifically for the proxy mapper
- Adds examples of mapping lists

views-react/src/main/java/io/micronaut/views/react/ProxyObjectWithIntrospectableSupport.java

@@ -29,6 +29,7 @@
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.Map;
 
 /**
@@ -38,38 +39,43 @@
  * the regular polyglot mapping.
  */
 @Internal
-class ProxyObjectWithIntrospectableSupport implements ProxyObject {
+final class ProxyObjectWithIntrospectableSupport implements ProxyObject {
     private final Context context;
     private final Object target;
-    private final boolean isStringMap;
 
     @Nullable
     private final BeanIntrospection<?> introspection;
 
-    ProxyObjectWithIntrospectableSupport(Context context, Object targetObject) {
-        this.context = context;
-
-        if (targetObject == null) {
-            throw new NullPointerException("Cannot proxy a null");
-        }
-
-        this.target = targetObject;
-        this.isStringMap = isStringMap(targetObject);
-        this.introspection = isStringMap ? null : BeanIntrospector.SHARED.findIntrospection(targetObject.getClass()).orElseThrow();
-    }
-
-    private ProxyObjectWithIntrospectableSupport(Context context, Object target, boolean isStringMap, BeanIntrospection<?> introspection) {
+    private ProxyObjectWithIntrospectableSupport(Context context, Object target, BeanIntrospection<?> introspection) {
         this.context = context;
         this.target = target;
-        this.isStringMap = isStringMap;
         this.introspection = introspection;
     }
 
-    private static boolean isStringMap(Object obj) {
-        if (obj instanceof Map<?, ?> map) {
-            return map.keySet().stream().allMatch(it -> it instanceof String);
+    /**
+     * Returns an object as a Truffle {@link Value} suitable for guest access, wrapping introspectable types with {@link ProxyObjectWithIntrospectableSupport}.
+     */
+    static Value wrap(Context context, Object object) {
+        if (object == null) {
+            return context.asValue(null);
+        } else  if (object instanceof Map<?, ?> map) {
+            // We need to recursively map the values.
+            var result = new HashMap<String, Object>();
+            map.forEach((key, value) -> result.put(key.toString(), wrap(context, value)));
+            return context.asValue(ProxyObject.fromMap(result));
+        } else if (object instanceof Collection<?> collection) {
+            // We need to recursively map the items. This could be lazy.
+            return context.asValue(collection.stream().map(it -> wrap(context, it)).toList());
+        } else if (object instanceof String) {
+            // We could ignore this case because we'd fall through the BeanIntrospector check, but that logs some debug spam and it's slower to look up objects we know we won't wrap anyway.
+            return context.asValue(object);
         } else {
-            return false;
+            var introspection = BeanIntrospector.SHARED.findIntrospection(object.getClass()).orElse(null);
+            if (introspection != null) {
+                return context.asValue(new ProxyObjectWithIntrospectableSupport(context, object, introspection));
+            } else {
+                return context.asValue(object);
+            }
         }
     }
 
@@ -80,13 +86,7 @@ public Object getMember(String key) {
         // Is it a property?
         Object result = map.get(key);
         if (result != null) {
-            boolean resultIsMap = isStringMap(result);
-            var resultIntrospection = BeanIntrospector.SHARED.findIntrospection(result.getClass());
-            if (resultIsMap || resultIntrospection.isPresent()) {
-                return new ProxyObjectWithIntrospectableSupport(context, result, resultIsMap, resultIntrospection.orElseThrow());
-            } else {
-                return context.asValue(result);
-            }
+            return wrap(context, result);
         }
 
         // Can it be an @Executable method?
@@ -99,6 +99,7 @@ public Object getMember(String key) {
             }
         }
 
+        // Not found.
         return context.asValue(null);
     }
 
@@ -125,9 +126,13 @@ public void putMember(String key, Value value) {
         throw new UnsupportedOperationException();
     }
 
-    @SuppressWarnings("unchecked")
     private Map<String, Object> asMap() {
-        return isStringMap ? (Map<String, Object>) target : BeanMap.of(target);
+        return BeanMap.of(target);
+    }
+
+    @Override
+    public String toString() {
+        return target.toString();
     }
 
     @SuppressWarnings("rawtypes")

views-react/src/main/java/io/micronaut/views/react/ReactViewsRenderer.java

@@ -25,7 +25,6 @@
 import jakarta.inject.Singleton;
 import org.graalvm.polyglot.HostAccess;
 import org.graalvm.polyglot.Value;
-import org.graalvm.polyglot.proxy.ProxyObject;
 
 import java.io.IOException;
 import java.io.Writer;
@@ -97,8 +96,8 @@ private void render(String componentName, PROPS props, Writer writer, JSContext
         // We wrap the props object so we can use Micronaut's compile-time reflection implementation.
         // This should be more native-image friendly (no need to write reflection config files), and
         // might also be faster.
-        ProxyObject propsObj = new ProxyObjectWithIntrospectableSupport(context.polyglotContext, props);
-        context.render.execute(component, propsObj, renderCallback, reactConfiguration.getClientBundleURL(), request);
+        Value guestProps = ProxyObjectWithIntrospectableSupport.wrap(context.polyglotContext, props);
+        context.render.executeVoid(component, guestProps, renderCallback, reactConfiguration.getClientBundleURL(), request);
     }
 
     /**

views-react/src/test/groovy/io/micronaut/views/react/IntrospectableBeansAreProxiedSpec.groovy

@@ -0,0 +1,41 @@
+package io.micronaut.views.react
+
+import io.micronaut.test.extensions.spock.annotation.MicronautTest
+import jakarta.inject.Inject
+import org.graalvm.polyglot.Value
+import org.graalvm.polyglot.proxy.ProxyObject
+import spock.lang.Specification
+
+@MicronautTest(startApplication = false)
+class IntrospectableBeansAreProxiedSpec extends Specification {
+    @Inject
+    JSContextPool contextPool
+
+    void "introspectable bean can be proxied"() {
+        given:
+        def jsContext = contextPool.acquire()
+        def context = jsContext.polyglotContext
+        def bean = new SomeBean("foo value", "bar value", new SomeBean.InnerBean(10, Map.of("key", 123), List.of("one", "two", "three")))
+
+        when:
+        ProxyObject proxy = ProxyObjectWithIntrospectableSupport.wrap(context, bean).asProxyObject()
+        context.getBindings("js").putMember("bean", proxy)
+
+        then:
+        proxy.getMember("foo") == context.asValue("foo value")
+        proxy.getMember("innerBean") instanceof Value
+
+        when:
+        ProxyObject innerBean = ((Value) proxy.getMember("innerBean")).asProxyObject()
+
+        then:
+        innerBean instanceof ProxyObjectWithIntrospectableSupport
+
+        when:
+        ProxyObject innerBeanMap = ((Value) innerBean.getMember("map")).asProxyObject()
+
+        then:
+        ((Value) innerBeanMap.getMember("key")).asInt() == 123
+        context.eval("js", "bean.innerBean.map[\"key\"]").asInt() == 123
+    }
+}

views-react/src/test/groovy/io/micronaut/views/react/PreactViewRenderSpec.groovy

@@ -21,7 +21,7 @@ class PreactViewRenderSpec extends Specification {
 
     void "views can be rendered with basic props and no request"() {
         when:
-        Writable writable = renderer.render("App", ["name": "Mike", "obj": new SomeBean("foo", null)], null)
+        Writable writable = renderer.render("App", TestProps.basic, null)
         String result = new StringWriter().with {
             writable.writeTo(it)
             it.toString()
@@ -39,7 +39,7 @@ class PreactViewRenderSpec extends Specification {
         req.getUri() >> URI.create("https://localhost/demopage")
 
         when:
-        Writable writable = renderer.render("App", ["name": "Mike", "obj": new SomeBean("foo", null)], req)
+        Writable writable = renderer.render("App", TestProps.basic, req)
         String result = new StringWriter().with {
             writable.writeTo(it)
             it.toString()

views-react/src/test/groovy/io/micronaut/views/react/ReactViewRenderSpec.groovy

@@ -16,7 +16,7 @@ class ReactViewRenderSpec extends Specification {
 
     void "views can be rendered with basic props"() {
         when:
-        Writable writable = renderer.render("App", ["name": "Mike", "someNull": null, "obj": new SomeBean("foo", null)], null)
+        Writable writable = renderer.render("App", TestProps.basic, null)
         String result = new StringWriter().with {
             writable.writeTo(it)
             it.toString()
@@ -27,6 +27,7 @@ class ReactViewRenderSpec extends Specification {
         result.contains("Reading a property works: <!-- -->foo")
         result.contains("Reading a null works: </p>")
         result.contains("\"name\":\"Mike\"")
+        result.contains("\"innerBean\":{\"a\":10,\"list\":[\"one\",\"two\"],\"map\":{}}")
         result.contains("Calling a method works: <!-- -->Goodbye Bob!")
     }
 
@@ -36,7 +37,7 @@ class ReactViewRenderSpec extends Specification {
         req.getUri() >> URI.create("https://localhost/demopage")
 
         when:
-        Writable writable = renderer.render("App", ["name": "Mike", "obj": new SomeBean("foo", null)], req)
+        Writable writable = renderer.render("App", TestProps.basic, req)
         String result = new StringWriter().with {
             writable.writeTo(it)
             it.toString()
@@ -50,7 +51,7 @@ class ReactViewRenderSpec extends Specification {
 
     void "host access is OK if sandbox is disabled"() {
         when:
-        renderer.render("App", ["name": "Mike", "triggerSandbox": true, "obj": new SomeBean("foo", null)], null).writeTo(OutputStream.nullOutputStream())
+        renderer.render("App", TestProps.triggerSandbox, null).writeTo(OutputStream.nullOutputStream())
 
         then:
         notThrown(MessageBodyException)

views-react/src/test/groovy/io/micronaut/views/react/SandboxReactRenderSpec.groovy

@@ -19,11 +19,8 @@ class SandboxReactRenderSpec extends Specification {
     // this unit test can be enabled.
     @FailsWith(BeanInstantiationException)
     void "views can be rendered with sandboxing enabled"() {
-        given:
-        def props = ["name": "Mike", "obj": new SomeBean("bar", null)]
-
         when:
-        Writable writable = renderer.render("App", props, null)
+        Writable writable = renderer.render("App", TestProps.basic, null)
         String result = new StringWriter().with {
             writable.writeTo(it)
             it.toString()
@@ -36,7 +33,7 @@ class SandboxReactRenderSpec extends Specification {
 
     void "host types are inaccessible with the sandbox enabled"() {
         when:
-        Writable writable = renderer.render("App", ["name": "Mike", "triggerSandbox": true, "obj": new SomeBean("foo", null)], null)
+        Writable writable = renderer.render("App", TestProps.triggerSandbox, null)
         new StringWriter().with {
             writable.writeTo(it)
             it.toString()

views-react/src/test/groovy/io/micronaut/views/react/TestProps.groovy

@@ -0,0 +1,6 @@
+package io.micronaut.views.react
+
+class TestProps {
+    static def basic = ["name": "Mike", "someNull": null, "obj": new SomeBean("foo", null, new SomeBean.InnerBean(10, Map.of(), List.of("one", "two")))]
+    static def triggerSandbox = basic + ["triggerSandbox": true]
+}

views-react/src/test/java/io/micronaut/views/react/SomeBean.java

@@ -4,17 +4,22 @@
 import io.micronaut.core.annotation.Introspected;
 import io.micronaut.core.annotation.Nullable;
 
+import java.util.List;
+import java.util.Map;
+
 /**
  * Test bean accessed from inside the sandbox.
  */
 @Introspected
 public class SomeBean {
     private final String foo;
     private final @Nullable String bar;
+    private final InnerBean innerBean;
 
-    SomeBean(String foo, String bar) {
+    SomeBean(String foo, String bar, InnerBean innerBean) {
         this.foo = foo;
         this.bar = bar;
+        this.innerBean = innerBean;
     }
 
     public String getFoo() {
@@ -25,8 +30,37 @@ public String getBar() {
         return bar;
     }
 
+    public InnerBean getInnerBean() {
+        return innerBean;
+    }
+
     @Executable
     public String sayGoodbye(String name) {
         return "Goodbye " + name + "!";
     }
+
+    @Introspected
+    public static class InnerBean {
+        private final int a;
+        private final Map<Object, Object> map;
+        private final List<String> list;
+
+        InnerBean(int a, Map<Object, Object> map, List<String> list) {
+            this.a = a;
+            this.map = map;
+            this.list = list;
+        }
+
+        public Map<Object, Object> getMap() {
+            return map;
+        }
+
+        public int getA() {
+            return a;
+        }
+
+        public List<String> getList() {
+            return list;
+        }
+    }
 }