From dc3f1bec543774836883e561838b942ad5f879bb Mon Sep 17 00:00:00 2001 From: Bryan Donlan Date: Sat, 12 Mar 2016 16:59:48 -0800 Subject: [PATCH] Make mutating operations on wrapped maps work Currently, wrapped maps use the passed-in Map as-is. This produces intuitive results when the Map is mutated, but as soon as you try to modify the DynamicObject, you get cryptic errors due to attempts to use this Map as an IPersistentMap or IObj (in the case of metadata operations). This change introduces a wrapper which proxies non-mutating calls through to the underlying map directly, but which creates a copy of the map (as a PersistentHashMap) if mutating operations are performed. I considered simply having wrap() copy the map directly, but I felt that the name 'wrap' would be seen to imply that mutations on the underlying map would be reflected in the wrapped DynamicObject, so I chose this route instead. --- .../rschmitt/dynamicobject/DynamicObject.java | 3 +- .../dynamicobject/internal/Instances.java | 11 +- .../dynamicobject/internal/WrappingMap.java | 140 ++++++++++++++++++ .../rschmitt/dynamicobject/MapTest.java | 47 ++++++ 4 files changed, 199 insertions(+), 2 deletions(-) create mode 100644 src/main/java/com/github/rschmitt/dynamicobject/internal/WrappingMap.java diff --git a/src/main/java/com/github/rschmitt/dynamicobject/DynamicObject.java b/src/main/java/com/github/rschmitt/dynamicobject/DynamicObject.java index a244004..d50e70d 100644 --- a/src/main/java/com/github/rschmitt/dynamicobject/DynamicObject.java +++ b/src/main/java/com/github/rschmitt/dynamicobject/DynamicObject.java @@ -161,7 +161,8 @@ static Stream deserializeFressianStream(InputStream is, Class type) { } /** - * Use the supplied {@code map} to back an instance of {@code type}. + * Use the supplied {@code map} to back an instance of {@code type}. The map will be copied upon any modification + * attempt, but until then will reflect changes made to the underlying map. */ static > D wrap(Map map, Class type) { return Instances.wrap(map, type); diff --git a/src/main/java/com/github/rschmitt/dynamicobject/internal/Instances.java b/src/main/java/com/github/rschmitt/dynamicobject/internal/Instances.java index 4c143f3..975967c 100644 --- a/src/main/java/com/github/rschmitt/dynamicobject/internal/Instances.java +++ b/src/main/java/com/github/rschmitt/dynamicobject/internal/Instances.java @@ -1,5 +1,6 @@ package com.github.rschmitt.dynamicobject.internal; +import clojure.lang.IPersistentMap; import com.github.rschmitt.dynamicobject.DynamicObject; import net.fushizen.invokedynamic.proxy.DynamicProxy; @@ -23,7 +24,15 @@ public static > D wrap(Map map, Class type) { if (map instanceof DynamicObject) return type.cast(map); - return createIndyProxy(map, type); + return createIndyProxy(convertMap(map), type); + } + + private static Map convertMap(Map map) { + if (map instanceof IPersistentMap) { + return map; + } + + return (Map) WrappingMap.create(map); } private static > D createIndyProxy(Map map, Class type) { diff --git a/src/main/java/com/github/rschmitt/dynamicobject/internal/WrappingMap.java b/src/main/java/com/github/rschmitt/dynamicobject/internal/WrappingMap.java new file mode 100644 index 0000000..8c2eb67 --- /dev/null +++ b/src/main/java/com/github/rschmitt/dynamicobject/internal/WrappingMap.java @@ -0,0 +1,140 @@ +package com.github.rschmitt.dynamicobject.internal; + +import clojure.lang.Cons; +import clojure.lang.IMeta; +import clojure.lang.IObj; +import clojure.lang.IPersistentMap; +import clojure.lang.PersistentHashMap; +import com.github.rschmitt.collider.ClojureMap; +import net.fushizen.invokedynamic.proxy.DynamicProxy; + +import java.lang.invoke.CallSite; +import java.lang.invoke.ConstantCallSite; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import java.lang.reflect.Method; +import java.util.Map; + +import static java.lang.invoke.MethodHandles.publicLookup; +import static java.lang.invoke.MethodType.methodType; + +/** + * A map class that wraps some other Map; however, it also implements IPersistentMap and IObj, creating a copy of the + * original map when IPersistentMap or IObj methods such as assoc are invoked. + */ +abstract class WrappingMap implements IMeta { + private static final IPersistentMap EMPTY_MAP = PersistentHashMap.create(); + + protected final Map backingMap; + + protected WrappingMap(Map backingMap) { + this.backingMap = backingMap; + } + + @Override + public IPersistentMap meta() { + // Avoid copying the map if we're doing a read-only metadata access. + return EMPTY_MAP; + } + + static Map create(Map other) { + try { + return (Map)proxy_ctor.invokeExact(other); + } catch (Throwable t) { + throw new Error("unexpected exception", t); + } + } + + private static final MethodHandle get_backingMap; + private static final MethodHandle proxy_ctor; + + static { + try { + get_backingMap = MethodHandles.lookup().findGetter(WrappingMap.class, "backingMap", Map.class); + proxy_ctor = DynamicProxy.builder() + .withConstructor(Map.class) + .withSuperclass(WrappingMap.class) + .withInterfaces(IPersistentMap.class, IObj.class, Map.class) + .withInvocationHandler(WrappingMap::invocationHandler) + .build() + .constructor() + .asType(methodType(Map.class, Map.class)); + } catch (Exception e) { + throw new Error(e); + } + } + + private static CallSite invocationHandler( + MethodHandles.Lookup lookup, + String methodName, + MethodType methodType, + MethodHandle superHandle + ) throws Throwable { + // Forward calls that are overridden on WrappingMap to that implementation + try { + Method m = WrappingMap.class.getDeclaredMethod(methodName, methodType.dropParameterTypes(0, 1).parameterArray()); + + // since the method exists, we can just use superHandle + return new ConstantCallSite(superHandle.asType(methodType)); + } catch (NoSuchMethodException e) { + // continue + } + + CallSite result; + + // Forward calls that are declared on Map, or Object to the backing map. + result = forwardCalls(Map.class, methodName, methodType); + if (result != null) return result; + + result = forwardCalls(Object.class, methodName, methodType); + if (result != null) return result; + + // Any other calls are IPersistentMap calls. We'll want to construct a PersistentHashMap and reinvoke the call + // on it. + MethodHandle makeMap = MethodHandles.lookup().findVirtual(WrappingMap.class, "createPersistentMap", methodType(IPersistentMap.class)); + + MethodType targetMethodType = methodType.dropParameterTypes(0, 1); + + MethodHandle target; + try { + target = publicLookup().findVirtual(IPersistentMap.class, methodName, targetMethodType); + } catch (NoSuchMethodException e) { + // It's not on IPersistentMap, so try IObj instead + target = publicLookup().findVirtual(IObj.class, methodName, targetMethodType); + // If we're successful, we need to do an asType cast since IPersistentMap doesn't extend IObj + makeMap = makeMap.asType(methodType(IObj.class, WrappingMap.class)); + } + + MethodHandle createMapAndForward = MethodHandles.filterArguments(target, 0, makeMap); + + return new ConstantCallSite(createMapAndForward.asType(methodType)); + } + + private static CallSite forwardCalls( + Class klass, + String methodName, + MethodType methodType + ) throws Throwable { + try { + MethodHandle mapHandle = publicLookup().findVirtual(klass, methodName, methodType.changeParameterType(0, Map.class)); + // ok, this is a call to the class in question, we just need to look up the backing map and invoke the + // method on it instead + MethodHandle combinedHandle = MethodHandles.filterArguments(mapHandle, 0, get_backingMap); + + return new ConstantCallSite(combinedHandle.asType(methodType)); + } catch (NoSuchMethodException e) { + return null; + } + } + + @SuppressWarnings("unused") // invoked by reflection + protected IPersistentMap createPersistentMap() { + return PersistentHashMap.create(backingMap); + } + + @SuppressWarnings("unused") // invoked by reflection + private static Object throwUnsupported() { + throw new UnsupportedOperationException(); + } +} diff --git a/src/test/java/com/github/rschmitt/dynamicobject/MapTest.java b/src/test/java/com/github/rschmitt/dynamicobject/MapTest.java index a669f95..d7190a3 100644 --- a/src/test/java/com/github/rschmitt/dynamicobject/MapTest.java +++ b/src/test/java/com/github/rschmitt/dynamicobject/MapTest.java @@ -5,6 +5,7 @@ import static java.lang.String.format; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import org.junit.After; import org.junit.Before; @@ -13,6 +14,8 @@ import clojure.lang.EdnReader; import clojure.lang.PersistentHashMap; +import java.util.HashMap; + public class MapTest { static final String SimpleEdn = "{:str \"expected value\", :i 4, :d 3.14}"; static final String NestedEdn = format("{:version 1, :simple %s}", SimpleEdn); @@ -59,6 +62,42 @@ public void mapDefaultMethodsAreUsable() throws Exception { object.getOrDefault("some key", "some value"); } + @Test + public void wrappedMapGettersWork() throws Exception { + HashMap map = new HashMap<>(); + map.put("foo", "bar"); + + TestObject obj = DynamicObject.wrap(map, TestObject.class); + + assertEquals("bar", obj.foo()); + } + + + @Test + public void wrappedMapSettersWork() throws Exception { + HashMap map = new HashMap<>(); + map.put("foo", "bar"); + + TestObject obj = DynamicObject.wrap(map, TestObject.class); + obj = obj.withFoo("quux"); + + assertEquals("quux", obj.foo()); + } + + @Test + public void wrappedMapMetaWorks() throws Exception { + HashMap map = new HashMap<>(); + map.put("foo", "bar"); + + TestObject obj = DynamicObject.wrap(map, TestObject.class); + + assertNull(obj.getMeta()); + obj = obj.withMeta("x"); + assertEquals("x", obj.getMeta()); + + assertEquals(1, obj.getMap().size()); + } + private void binaryRoundTrip(Object expected) { Object actual = DynamicObject.fromFressianByteArray(DynamicObject.toFressianByteArray(expected)); assertEquals(expected, actual); @@ -66,4 +105,12 @@ private void binaryRoundTrip(Object expected) { public interface EmptyObject extends DynamicObject { } + + public interface TestObject extends DynamicObject { + @Key("foo") String foo(); + @Key("foo") TestObject withFoo(String foo); + + @Meta @Key(":meta") String getMeta(); + @Meta @Key(":meta") TestObject withMeta(String meta); + } }