From a9fb26f66fb49075a3f0b668d64264b226ae2875 Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Fri, 9 Aug 2024 14:54:41 +0200 Subject: [PATCH] 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 --- .../ProxyObjectWithIntrospectableSupport.java | 63 ++++++++++--------- .../views/react/ReactViewsRenderer.java | 5 +- .../IntrospectableBeansAreProxiedSpec.groovy | 41 ++++++++++++ .../views/react/PreactViewRenderSpec.groovy | 4 +- .../views/react/ReactViewRenderSpec.groovy | 7 ++- .../views/react/SandboxReactRenderSpec.groovy | 7 +-- .../io/micronaut/views/react/TestProps.groovy | 6 ++ .../io/micronaut/views/react/SomeBean.java | 36 ++++++++++- 8 files changed, 126 insertions(+), 43 deletions(-) create mode 100644 views-react/src/test/groovy/io/micronaut/views/react/IntrospectableBeansAreProxiedSpec.groovy create mode 100644 views-react/src/test/groovy/io/micronaut/views/react/TestProps.groovy diff --git a/views-react/src/main/java/io/micronaut/views/react/ProxyObjectWithIntrospectableSupport.java b/views-react/src/main/java/io/micronaut/views/react/ProxyObjectWithIntrospectableSupport.java index 73a480d2e..1d9d3f6c7 100644 --- a/views-react/src/main/java/io/micronaut/views/react/ProxyObjectWithIntrospectableSupport.java +++ b/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(); + 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 asMap() { - return isStringMap ? (Map) target : BeanMap.of(target); + return BeanMap.of(target); + } + + @Override + public String toString() { + return target.toString(); } @SuppressWarnings("rawtypes") diff --git a/views-react/src/main/java/io/micronaut/views/react/ReactViewsRenderer.java b/views-react/src/main/java/io/micronaut/views/react/ReactViewsRenderer.java index 6b2660fc0..612b36d89 100644 --- a/views-react/src/main/java/io/micronaut/views/react/ReactViewsRenderer.java +++ b/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); } /** diff --git a/views-react/src/test/groovy/io/micronaut/views/react/IntrospectableBeansAreProxiedSpec.groovy b/views-react/src/test/groovy/io/micronaut/views/react/IntrospectableBeansAreProxiedSpec.groovy new file mode 100644 index 000000000..d84cf8277 --- /dev/null +++ b/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 + } +} diff --git a/views-react/src/test/groovy/io/micronaut/views/react/PreactViewRenderSpec.groovy b/views-react/src/test/groovy/io/micronaut/views/react/PreactViewRenderSpec.groovy index 5748f898b..6b4e40432 100644 --- a/views-react/src/test/groovy/io/micronaut/views/react/PreactViewRenderSpec.groovy +++ b/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() diff --git a/views-react/src/test/groovy/io/micronaut/views/react/ReactViewRenderSpec.groovy b/views-react/src/test/groovy/io/micronaut/views/react/ReactViewRenderSpec.groovy index a37b82b45..a0ad4abe1 100644 --- a/views-react/src/test/groovy/io/micronaut/views/react/ReactViewRenderSpec.groovy +++ b/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:

") 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) diff --git a/views-react/src/test/groovy/io/micronaut/views/react/SandboxReactRenderSpec.groovy b/views-react/src/test/groovy/io/micronaut/views/react/SandboxReactRenderSpec.groovy index c647ef948..73a65cbff 100644 --- a/views-react/src/test/groovy/io/micronaut/views/react/SandboxReactRenderSpec.groovy +++ b/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() diff --git a/views-react/src/test/groovy/io/micronaut/views/react/TestProps.groovy b/views-react/src/test/groovy/io/micronaut/views/react/TestProps.groovy new file mode 100644 index 000000000..114d56212 --- /dev/null +++ b/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] +} diff --git a/views-react/src/test/java/io/micronaut/views/react/SomeBean.java b/views-react/src/test/java/io/micronaut/views/react/SomeBean.java index 0cfdf1a8d..779b4443d 100644 --- a/views-react/src/test/java/io/micronaut/views/react/SomeBean.java +++ b/views-react/src/test/java/io/micronaut/views/react/SomeBean.java @@ -4,6 +4,9 @@ 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. */ @@ -11,10 +14,12 @@ 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 map; + private final List list; + + InnerBean(int a, Map map, List list) { + this.a = a; + this.map = map; + this.list = list; + } + + public Map getMap() { + return map; + } + + public int getA() { + return a; + } + + public List getList() { + return list; + } + } }