Ignore no-op load/save/delete actions properly

.gitignore

@@ -9,3 +9,4 @@ objectify.iml
 .svn
 .gwt
 .gradle
+.factorypath

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<Void> keys(Iterable<? extends Key<?>> keys) {
 		for (Key<?> key: keys)
 			rawKeys.add(key.getRaw());
 
+		if (rawKeys.isEmpty())
+			return new ResultNow<Void>(null);
+
 		return ofy.createWriteEngine().delete(rawKeys);
 	}
 
@@ -78,6 +82,9 @@ public Result<Void> entities(Iterable<?> entities) {
 		for (Object obj: entities)
 			keys.add(ofy.factory().keys().anythingToRawKey(obj));
 
+		if (keys.isEmpty())
+			return new ResultNow<Void>(null);
+
 		return ofy.createWriteEngine().delete(keys);
 	}
 

src/main/java/com/googlecode/objectify/impl/LoadEngine.java

@@ -131,7 +131,7 @@ public Result<Map<Key<?>, Object>> translate(final Result<Map<com.google.appengi
 
 			/** */
 			@Override
-			public Map<Key<?>, Object> nowUncached() {
+			protected Map<Key<?>, Object> nowUncached() {
 				Map<Key<?>, Object> result = new HashMap<>(raw.now().size() * 2);
 
 				ctx = new LoadContext(LoadEngine.this);

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,15 +86,15 @@ public Query<T> order(String condition) {
 	 */
 	@Override
 	public LoadResult<T> id(long id) {
-		return loader.key(this.<T>makeKey(id));
+		return loader.key(this.makeKey(id));
 	}
 
 	/* (non-Javadoc)
 	 * @see com.googlecode.objectify.cmd.LoadIds#id(java.lang.String)
 	 */
 	@Override
 	public LoadResult<T> id(String id) {
-		return loader.key(this.<T>makeKey(id));
+		return loader.key(this.makeKey(id));
 	}
 
 	/* (non-Javadoc)
@@ -120,11 +121,14 @@ public <S> Map<S, T> ids(Iterable<S> ids) {
 
 		final Map<Key<T>, S> keymap = new LinkedHashMap<>();
 		for (S id: ids)
-			keymap.put(this.<T>makeKey(id), id);
+			keymap.put(this.makeKey(id), id);
+
+		if (keymap.isEmpty())
+			return Maps.newLinkedHashMap();
 
 		final Map<Key<T>, T> loaded = loader.keys(keymap.keySet());
 
-		return ResultProxy.create(Map.class, new ResultCache<Map<S, T>>() {
+		return ResultProxy.createMap(new ResultCache<Map<S, T>>() {
 			@Override
 			protected Map<S, T> nowUncached() {
 				Map<S, T> proper = new LinkedHashMap<>(loaded.size() * 2);
@@ -140,7 +144,7 @@ protected Map<S, T> nowUncached() {
 	/**
 	 * Make a key for the given id
 	 */
-	private <T> Key<T> makeKey(Object id) {
+	private Key<T> makeKey(Object id) {
 		return DatastoreUtils.createKey(parent, kind, id);
 	}
 

src/main/java/com/googlecode/objectify/impl/LoaderImpl.java

@@ -183,6 +183,9 @@ public <E> Map<Key<E>, E> values(Iterable<?> values) {
 		for (Object keyish: values)
 			keys.add((Key<E>)ofy.factory().keys().anythingToKey(keyish));
 
+		if (keys.isEmpty())
+			return Maps.newLinkedHashMap();
+
 		LoadEngine engine = createLoadEngine();
 
 		final Map<Key<E>, Result<E>> results = new LinkedHashMap<>();
@@ -194,9 +197,9 @@ public <E> Map<Key<E>, 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<Map<Key<E>, E>>() {
+		return ResultProxy.createMap(new ResultCache<Map<Key<E>, E>>() {
 			@Override
-			public Map<Key<E>, E> nowUncached() {
+			protected Map<Key<E>, E> nowUncached() {
 				return
 					Maps.newLinkedHashMap(
 						Maps.filterValues(

src/main/java/com/googlecode/objectify/impl/QueryImpl.java

@@ -363,7 +363,7 @@ public QueryResultIterator<T> iterator() {
 	 */
 	@Override
 	public List<T> 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()));
 	}
 
 	/**

src/main/java/com/googlecode/objectify/impl/QueryKeysImpl.java

@@ -44,7 +44,7 @@ public QueryResultIterable<Key<T>> iterable() {
 
 	@Override
 	public List<Key<T>> 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

src/main/java/com/googlecode/objectify/impl/Round.java

@@ -63,7 +63,7 @@ public <T> Result<T> get(final Key<T> key) {
 			Result<T> result = new ResultCache<T>() {
 				@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<Map<com.google.appengine.api.datastore.Key, Entity>> fetchPending
 		} else {
 			final Result<Map<com.google.appengine.api.datastore.Key, Entity>> fetched = loadEngine.fetch(fetch);
 
-			return new Result<Map<com.google.appengine.api.datastore.Key, Entity>>() {
+			return new ResultCache<Map<com.google.appengine.api.datastore.Key, Entity>>() {
 				@Override
-				public Map<com.google.appengine.api.datastore.Key, Entity> now() {
+				protected Map<com.google.appengine.api.datastore.Key, Entity> nowUncached() {
 					combined.putAll(fetched.now());
 					return combined;
 				}

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 <E> Result<Map<Key<E>, E>> entities(E... entities) {
 	 */
 	@Override
 	public <E> Result<Map<Key<E>, E>> entities(final Iterable<E> entities) {
+		if (Iterables.isEmpty(entities))
+			return new ResultNow<Map<Key<E>, E>>(Maps.<Key<E>, E> newLinkedHashMap());
+
 		return ofy.createWriteEngine().<E>save(entities);
 	}
 

src/main/java/com/googlecode/objectify/util/ResultCache.java

@@ -19,7 +19,7 @@ abstract public class ResultCache<T> implements Result<T>, Serializable
 	protected abstract T nowUncached();
 
 	/** */
-	protected boolean isExecuted() {
+	protected final boolean isExecuted() {
 		return cached;
 	}
 

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<List<String>>, the
+ * A utility class that wraps a Result<?> value. For example, if you had a Result<List<String>>, the
  * proxy would implement List<String> and call through to the inner object.
  */
-public class ResultProxy<T> implements InvocationHandler, Serializable
+public class ResultProxy<T>
 {
-	/**
-	 * Create a ResultProxy for the given interface.
-	 */
-	@SuppressWarnings("unchecked")
-	public static <S> S create(Class<? super S> interf, Result<S> result) {
-		return (S)Proxy.newProxyInstance(result.getClass().getClassLoader(), new Class[] { interf }, new ResultProxy<>(result));
+	public static <K, V> Map<K, V> createMap(final Result<Map<K, V>> result) {
+		return new ResultProxyMap<K, V>(result);
 	}
 
-	Result<T> result;
+	public static <E> List<E> createList(final Result<List<E>> result) {
+		return new ResultProxyList<E>(result);
+	}
+
+	@RequiredArgsConstructor
+	private static class ResultProxyList<E> extends ForwardingList<E> implements Serializable {
+
+		private final Result<List<E>> result;
+
+		@Override
+		protected List<E> delegate() {
+			return result.now();
+		}
 
-	private ResultProxy(Result<T> 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<K, V> extends ForwardingMap<K, V> implements Serializable {
+
+		private final Result<Map<K, V>> result;
+
+		@Override
+		protected Map<K, V> delegate() {
+			return result.now();
+		}
+
+		private Object writeReplace() throws ObjectStreamException {
+			return delegate();
+		}
 	}
-
-	private Object writeReplace() throws ObjectStreamException {
-        return new NowProxy<>(result.now());
-    }
-
 }

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);
 	}
 

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.<Key<Trivial>> 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.<Key<Trivial>> 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.<Trivial> create(trivial))).size();
+		ofy().load().type(Trivial.class).ids(Arrays.asList(trivial.getId())).size();
+
+		verifyThatRemoteCallWasExecuted(3);
+	}
+}