diff --git a/.gitignore b/.gitignore index 51aa6bf1e..d9e394a71 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ objectify.iml .svn .gwt .gradle +.factorypath diff --git a/src/main/java/com/googlecode/objectify/impl/DeleterImpl.java b/src/main/java/com/googlecode/objectify/impl/DeleterImpl.java index 80b112353..0a1279829 100644 --- a/src/main/java/com/googlecode/objectify/impl/DeleterImpl.java +++ b/src/main/java/com/googlecode/objectify/impl/DeleterImpl.java @@ -4,6 +4,7 @@ import com.googlecode.objectify.Result; import com.googlecode.objectify.cmd.DeleteType; import com.googlecode.objectify.cmd.Deleter; +import com.googlecode.objectify.util.ResultNow; import java.util.ArrayList; import java.util.Arrays; @@ -58,6 +59,9 @@ public Result keys(Iterable> keys) { for (Key key: keys) rawKeys.add(key.getRaw()); + if (rawKeys.isEmpty()) + return new ResultNow(null); + return ofy.createWriteEngine().delete(rawKeys); } @@ -78,6 +82,9 @@ public Result entities(Iterable entities) { for (Object obj: entities) keys.add(ofy.factory().keys().anythingToRawKey(obj)); + if (keys.isEmpty()) + return new ResultNow(null); + return ofy.createWriteEngine().delete(keys); } diff --git a/src/main/java/com/googlecode/objectify/impl/LoadEngine.java b/src/main/java/com/googlecode/objectify/impl/LoadEngine.java index c7d2bc006..8c368a9ce 100644 --- a/src/main/java/com/googlecode/objectify/impl/LoadEngine.java +++ b/src/main/java/com/googlecode/objectify/impl/LoadEngine.java @@ -131,7 +131,7 @@ public Result, Object>> translate(final Result, Object> nowUncached() { + protected Map, Object> nowUncached() { Map, Object> result = new HashMap<>(raw.now().size() * 2); ctx = new LoadContext(LoadEngine.this); diff --git a/src/main/java/com/googlecode/objectify/impl/LoadTypeImpl.java b/src/main/java/com/googlecode/objectify/impl/LoadTypeImpl.java index 39ec86440..7015242ce 100644 --- a/src/main/java/com/googlecode/objectify/impl/LoadTypeImpl.java +++ b/src/main/java/com/googlecode/objectify/impl/LoadTypeImpl.java @@ -1,6 +1,7 @@ package com.googlecode.objectify.impl; import com.google.appengine.api.datastore.Query.Filter; +import com.google.common.collect.Maps; import com.googlecode.objectify.Key; import com.googlecode.objectify.LoadResult; import com.googlecode.objectify.cmd.LoadIds; @@ -85,7 +86,7 @@ public Query order(String condition) { */ @Override public LoadResult id(long id) { - return loader.key(this.makeKey(id)); + return loader.key(this.makeKey(id)); } /* (non-Javadoc) @@ -93,7 +94,7 @@ public LoadResult id(long id) { */ @Override public LoadResult id(String id) { - return loader.key(this.makeKey(id)); + return loader.key(this.makeKey(id)); } /* (non-Javadoc) @@ -120,11 +121,14 @@ public Map ids(Iterable ids) { final Map, S> keymap = new LinkedHashMap<>(); for (S id: ids) - keymap.put(this.makeKey(id), id); + keymap.put(this.makeKey(id), id); + + if (keymap.isEmpty()) + return Maps.newLinkedHashMap(); final Map, T> loaded = loader.keys(keymap.keySet()); - return ResultProxy.create(Map.class, new ResultCache>() { + return ResultProxy.createMap(new ResultCache>() { @Override protected Map nowUncached() { Map proper = new LinkedHashMap<>(loaded.size() * 2); @@ -140,7 +144,7 @@ protected Map nowUncached() { /** * Make a key for the given id */ - private Key makeKey(Object id) { + private Key makeKey(Object id) { return DatastoreUtils.createKey(parent, kind, id); } diff --git a/src/main/java/com/googlecode/objectify/impl/LoaderImpl.java b/src/main/java/com/googlecode/objectify/impl/LoaderImpl.java index d771e0e40..42dbefb7f 100644 --- a/src/main/java/com/googlecode/objectify/impl/LoaderImpl.java +++ b/src/main/java/com/googlecode/objectify/impl/LoaderImpl.java @@ -183,6 +183,9 @@ public Map, E> values(Iterable values) { for (Object keyish: values) keys.add((Key)ofy.factory().keys().anythingToKey(keyish)); + if (keys.isEmpty()) + return Maps.newLinkedHashMap(); + LoadEngine engine = createLoadEngine(); final Map, Result> results = new LinkedHashMap<>(); @@ -194,9 +197,9 @@ public Map, E> values(Iterable values) { // Now asynchronously translate into a normal-looking map. We must be careful to exclude results with // null (missing) values because that is the contract established by DatastoreService.get(). // We use the ResultProxy and create a new map because the performance of filtered views is questionable. - return ResultProxy.create(Map.class, new ResultCache, E>>() { + return ResultProxy.createMap(new ResultCache, E>>() { @Override - public Map, E> nowUncached() { + protected Map, E> nowUncached() { return Maps.newLinkedHashMap( Maps.filterValues( diff --git a/src/main/java/com/googlecode/objectify/impl/QueryImpl.java b/src/main/java/com/googlecode/objectify/impl/QueryImpl.java index 0bd051962..7ea1da140 100644 --- a/src/main/java/com/googlecode/objectify/impl/QueryImpl.java +++ b/src/main/java/com/googlecode/objectify/impl/QueryImpl.java @@ -363,7 +363,7 @@ public QueryResultIterator iterator() { */ @Override public List list() { - return ResultProxy.create(List.class, new MakeListResult<>(this.chunk(Integer.MAX_VALUE).iterable())); + return ResultProxy.createList(new MakeListResult<>(this.chunk(Integer.MAX_VALUE).iterable())); } /** diff --git a/src/main/java/com/googlecode/objectify/impl/QueryKeysImpl.java b/src/main/java/com/googlecode/objectify/impl/QueryKeysImpl.java index be6938745..f765c04e9 100644 --- a/src/main/java/com/googlecode/objectify/impl/QueryKeysImpl.java +++ b/src/main/java/com/googlecode/objectify/impl/QueryKeysImpl.java @@ -44,7 +44,7 @@ public QueryResultIterable> iterable() { @Override public List> list() { - return ResultProxy.create(List.class, new MakeListResult<>(impl.chunk(Integer.MAX_VALUE).keysIterable())); + return ResultProxy.createList(new MakeListResult<>(impl.chunk(Integer.MAX_VALUE).keysIterable())); } @Override diff --git a/src/main/java/com/googlecode/objectify/impl/Round.java b/src/main/java/com/googlecode/objectify/impl/Round.java index 78baf483f..7471cb835 100644 --- a/src/main/java/com/googlecode/objectify/impl/Round.java +++ b/src/main/java/com/googlecode/objectify/impl/Round.java @@ -63,7 +63,7 @@ public Result get(final Key key) { Result result = new ResultCache() { @Override @SuppressWarnings("unchecked") - public T nowUncached() { + protected T nowUncached() { // Because clients could conceivably get() in the middle of our operations (see LoadCollectionRefsTest.specialListWorks()), // we need to check for early execution. This will perform poorly, but at least it will work. //assert Round.this.isExecuted(); @@ -167,9 +167,9 @@ private Result> fetchPending } else { final Result> fetched = loadEngine.fetch(fetch); - return new Result>() { + return new ResultCache>() { @Override - public Map now() { + protected Map nowUncached() { combined.putAll(fetched.now()); return combined; } diff --git a/src/main/java/com/googlecode/objectify/impl/SaverImpl.java b/src/main/java/com/googlecode/objectify/impl/SaverImpl.java index 93965d510..1656b84b5 100644 --- a/src/main/java/com/googlecode/objectify/impl/SaverImpl.java +++ b/src/main/java/com/googlecode/objectify/impl/SaverImpl.java @@ -1,10 +1,13 @@ package com.googlecode.objectify.impl; import com.google.appengine.api.datastore.Entity; +import com.google.common.collect.Iterables; +import com.google.common.collect.Maps; import com.googlecode.objectify.Key; import com.googlecode.objectify.Result; import com.googlecode.objectify.cmd.Saver; import com.googlecode.objectify.impl.translate.SaveContext; +import com.googlecode.objectify.util.ResultNow; import com.googlecode.objectify.util.ResultWrapper; import java.util.Arrays; @@ -57,6 +60,9 @@ public Result, E>> entities(E... entities) { */ @Override public Result, E>> entities(final Iterable entities) { + if (Iterables.isEmpty(entities)) + return new ResultNow, E>>(Maps., E> newLinkedHashMap()); + return ofy.createWriteEngine().save(entities); } diff --git a/src/main/java/com/googlecode/objectify/util/ResultCache.java b/src/main/java/com/googlecode/objectify/util/ResultCache.java index 1260d7d83..80505f7ea 100644 --- a/src/main/java/com/googlecode/objectify/util/ResultCache.java +++ b/src/main/java/com/googlecode/objectify/util/ResultCache.java @@ -19,7 +19,7 @@ abstract public class ResultCache implements Result, Serializable protected abstract T nowUncached(); /** */ - protected boolean isExecuted() { + protected final boolean isExecuted() { return cached; } diff --git a/src/main/java/com/googlecode/objectify/util/ResultProxy.java b/src/main/java/com/googlecode/objectify/util/ResultProxy.java index 51627f302..c2ee216e2 100644 --- a/src/main/java/com/googlecode/objectify/util/ResultProxy.java +++ b/src/main/java/com/googlecode/objectify/util/ResultProxy.java @@ -1,41 +1,57 @@ package com.googlecode.objectify.util; +import com.google.common.collect.ForwardingList; +import com.google.common.collect.ForwardingMap; import com.googlecode.objectify.Result; import java.io.ObjectStreamException; import java.io.Serializable; -import java.lang.reflect.InvocationHandler; -import java.lang.reflect.Method; -import java.lang.reflect.Proxy; +import java.util.List; +import java.util.Map; +import lombok.RequiredArgsConstructor; /** - * A dynamic proxy that wraps a Result value. For example, if you had a Result>, the + * A utility class that wraps a Result value. For example, if you had a Result>, the * proxy would implement List and call through to the inner object. */ -public class ResultProxy implements InvocationHandler, Serializable +public class ResultProxy { - /** - * Create a ResultProxy for the given interface. - */ - @SuppressWarnings("unchecked") - public static S create(Class interf, Result result) { - return (S)Proxy.newProxyInstance(result.getClass().getClassLoader(), new Class[] { interf }, new ResultProxy<>(result)); + public static Map createMap(final Result> result) { + return new ResultProxyMap(result); } - Result result; + public static List createList(final Result> result) { + return new ResultProxyList(result); + } + + @RequiredArgsConstructor + private static class ResultProxyList extends ForwardingList implements Serializable { + + private final Result> result; + + @Override + protected List delegate() { + return result.now(); + } - private ResultProxy(Result result) { - this.result = result; + private Object writeReplace() throws ObjectStreamException { + return delegate(); + } } - @Override - public Object invoke(Object obj, Method meth, Object[] params) throws Throwable { - return meth.invoke(result.now(), params); + @RequiredArgsConstructor + private static class ResultProxyMap extends ForwardingMap implements Serializable { + + private final Result> result; + + @Override + protected Map delegate() { + return result.now(); + } + + private Object writeReplace() throws ObjectStreamException { + return delegate(); + } } - - private Object writeReplace() throws ObjectStreamException { - return new NowProxy<>(result.now()); - } - } diff --git a/src/main/java/com/googlecode/objectify/util/ResultTranslator.java b/src/main/java/com/googlecode/objectify/util/ResultTranslator.java index 4aa2f3c24..b696dc6fc 100644 --- a/src/main/java/com/googlecode/objectify/util/ResultTranslator.java +++ b/src/main/java/com/googlecode/objectify/util/ResultTranslator.java @@ -17,7 +17,7 @@ public ResultTranslator(F from) { protected abstract T translate(F from); @Override - public T nowUncached() { + protected final T nowUncached() { return translate(from); } diff --git a/src/test/java/com/googlecode/objectify/test/IgnoreEmptyOperationTests.java b/src/test/java/com/googlecode/objectify/test/IgnoreEmptyOperationTests.java new file mode 100644 index 000000000..df0677952 --- /dev/null +++ b/src/test/java/com/googlecode/objectify/test/IgnoreEmptyOperationTests.java @@ -0,0 +1,91 @@ +package com.googlecode.objectify.test; + +import static com.googlecode.objectify.ObjectifyService.factory; +import static com.googlecode.objectify.ObjectifyService.ofy; + +import java.util.Arrays; + +import org.mockito.Mockito; +import org.testng.annotations.Test; + +import com.google.appengine.api.datastore.DatastoreServiceConfig; +import com.google.common.collect.Lists; +import com.googlecode.objectify.Key; +import com.googlecode.objectify.test.entity.Trivial; +import com.googlecode.objectify.test.util.TestBase; +import com.googlecode.objectify.test.util.TestObjectifyFactory; + +public class IgnoreEmptyOperationTests extends TestBase { + + @Override + protected void setUpObjectifyFactory(TestObjectifyFactory factory) { + factory.register(Trivial.class); + super.setUpObjectifyFactory(Mockito.spy(factory)); + Mockito.verify(factory(), Mockito.times(1)).begin(); + } + + // engines are responsible for datastore actions, and because all of them requires an AsyncDatastoreService + // it seems like a good idea to verify if it has been created or not + private void verifyNoRemoteCall() { + Mockito.verify(factory(), Mockito.never()) + .createAsyncDatastoreService(Mockito.any(DatastoreServiceConfig.class), Mockito.anyBoolean()); + Mockito.verifyNoMoreInteractions(factory()); + } + + private void verifyThatRemoteCallWasExecuted(int times) { + Mockito.verify(factory(), Mockito.times(times)) + .createAsyncDatastoreService(Mockito.any(DatastoreServiceConfig.class), Mockito.anyBoolean()); + } + + @Test + public void testEmptySave() { + ofy().save().entities(Lists.newArrayList()).now().size(); + + verifyNoRemoteCall(); + } + + @Test + public void testNotEmptySave() { + ofy().save().entities(Arrays.asList(new Trivial())).now().size(); + + verifyThatRemoteCallWasExecuted(1); + } + + @Test + public void testEmptyDelete() { + ofy().delete().entities(Lists.newArrayList()).now(); + ofy().delete().keys(Lists.> newArrayList()).now(); + ofy().delete().type(Trivial.class).ids(Lists.newArrayList()).now(); + + verifyNoRemoteCall(); + } + + @Test + public void testNotEmptyDelete() { + final Trivial trivial = new Trivial(1L, "", 1L); + ofy().delete().entities(Arrays.asList(trivial)).now(); + ofy().delete().keys(Arrays.asList(Key.create(trivial))).now(); + ofy().delete().type(Trivial.class).ids(Arrays.asList(trivial.getId())).now(); + + verifyThatRemoteCallWasExecuted(3); + } + + @Test + public void testEmptyLoad() { + ofy().load().entities(Lists.newArrayList()).size(); + ofy().load().keys(Lists.> newArrayList()).size(); + ofy().load().type(Trivial.class).ids(Lists.newArrayList()).size(); + + verifyNoRemoteCall(); + } + + @Test + public void testNotEmptyLoad() { + final Trivial trivial = new Trivial(1L, "", 1L); + ofy().load().entities(Arrays.asList(trivial)).size(); + ofy().load().keys(Arrays.asList(Key. create(trivial))).size(); + ofy().load().type(Trivial.class).ids(Arrays.asList(trivial.getId())).size(); + + verifyThatRemoteCallWasExecuted(3); + } +} diff --git a/src/test/java/com/googlecode/objectify/test/ProxySerializationTests.java b/src/test/java/com/googlecode/objectify/test/ProxySerializationTests.java index 1eb17ca64..190c27f2d 100644 --- a/src/test/java/com/googlecode/objectify/test/ProxySerializationTests.java +++ b/src/test/java/com/googlecode/objectify/test/ProxySerializationTests.java @@ -8,18 +8,29 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; +import java.io.NotSerializableException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; +import java.io.Serializable; import java.util.List; import java.util.Map; import java.util.logging.Logger; +import lombok.RequiredArgsConstructor; + +import org.testng.Assert; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import com.google.common.collect.ForwardingList; +import com.google.common.collect.ForwardingMap; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import com.googlecode.objectify.Key; +import com.googlecode.objectify.Result; import com.googlecode.objectify.test.entity.Trivial; import com.googlecode.objectify.test.util.TestBase; +import com.googlecode.objectify.util.ResultProxy; /** * Make sure that the proxies we return can be serialized sanely. @@ -53,6 +64,12 @@ private T deserialize() throws Exception { return (T)in.readObject(); } + // we could have broken equals() rules by writeReplace + private void assertEqualsBothWays(Object obj1, Object obj2) { + Assert.assertEquals(obj1, obj2); + Assert.assertEquals(obj2, obj1); + } + /** */ @Test public void queryListCanBeSerialized() throws Exception { @@ -69,6 +86,7 @@ public void queryListCanBeSerialized() throws Exception { assert back.size() == 1; assert back.get(0).getId().equals(triv.getId()); + assertEqualsBothWays(trivs, back); } /** */ @@ -88,5 +106,66 @@ public void loadMapCanBeSerialized() throws Exception { assert back.size() == 1; assert back.get(k).getId().equals(triv.getId()); + assertEqualsBothWays(trivs, back); + } + + // the Result can't be serialized, but the value given by Result.now() could be depending on the parameters + // this guarantees we fail without calling now() during serialization, but it could work fine unwrapped + @RequiredArgsConstructor + static class NotSerializableResult implements Result { + private final T wrapped; + + @Override + public T now() { + return wrapped; + } + } + + private static class NotSerializableList extends ForwardingList { + @Override + protected List delegate() { + return null; // not required + } + } + + private static class NotSerializableMap extends ForwardingMap { + @Override + protected Map delegate() { + return null; // not required + } + } + + private void assertSerialization(Object proxy) throws Exception { + serialize(proxy); + Object back = deserialize(); + assertEqualsBothWays(proxy, back); + } + + private void assertProxiedListSerialization(List list) throws Exception { + assertSerialization(ResultProxy.createList(new NotSerializableResult>(list))); + } + + private void assertProxiedMapSerialization(Map map) throws Exception { + assertSerialization(ResultProxy.createMap(new NotSerializableResult>(map))); + } + + @Test + public void listProxyCanBeSerializedWithSerializableWrappedValue() throws Exception { + assertProxiedListSerialization(Lists. newArrayList()); + } + + @Test(expectedExceptions = NotSerializableException.class) + public void listProxyCantBeSerializedWithNotSerializableWrappedValue() throws Exception { + assertProxiedListSerialization(new NotSerializableList()); + } + + @Test + public void mapProxyCanBeSerializedWithSerializableWrappedValue() throws Exception { + assertProxiedMapSerialization(Maps. newHashMap()); + } + + @Test(expectedExceptions = NotSerializableException.class) + public void mapProxyCantBeSerializedWithNotSerializableWrappedValue() throws Exception { + assertProxiedMapSerialization(new NotSerializableMap()); } } \ No newline at end of file