Make the new datastore testing proxy actually store the requests

This will improve error messages and allow for easier debugging
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=123893831

java/google/registry/model/ofy/ObjectifyService.java

@@ -109,7 +109,7 @@ public class ObjectifyService {
         // examine the number of requests sent to datastore.
         AsyncDatastoreService service = super.createRawAsyncDatastoreService(cfg);
         return RegistryEnvironment.get().equals(RegistryEnvironment.UNITTEST)
-            ? new RequestCountingAsyncDatastoreService(service)
+            ? new RequestCapturingAsyncDatastoreService(service)
             : service;
       }});
 

java/google/registry/model/ofy/RequestCapturingAsyncDatastoreService.java

@@ -14,6 +14,8 @@
 
 package google.registry.model.ofy;
 
+import static java.util.Collections.synchronizedList;
+
 import com.google.appengine.api.datastore.AsyncDatastoreService;
 import com.google.appengine.api.datastore.DatastoreAttributes;
 import com.google.appengine.api.datastore.Entity;
@@ -25,39 +27,41 @@ import com.google.appengine.api.datastore.PreparedQuery;
 import com.google.appengine.api.datastore.Query;
 import com.google.appengine.api.datastore.Transaction;
 import com.google.appengine.api.datastore.TransactionOptions;
+import com.google.common.collect.ImmutableList;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Future;
-import java.util.concurrent.atomic.AtomicInteger;
 
 /** A proxy for {@link AsyncDatastoreService} that exposes call counts. */
-public class RequestCountingAsyncDatastoreService implements AsyncDatastoreService {
+public class RequestCapturingAsyncDatastoreService implements AsyncDatastoreService {
 
   private final AsyncDatastoreService delegate;
 
-  // We use static counters because we care about overall calls to datastore, not calls via a
+  // Each outer lists represents datastore operations, with inner lists representing the keys or
-  // specific instance of the service.
+  // entities involved in that operation. We use static lists because we care about overall calls to
+  // datastore, not calls via a specific instance of the service.
 
-  private static AtomicInteger reads = new AtomicInteger();
+  private static List<List<Key>> reads = synchronizedList(new ArrayList<List<Key>>());
-  private static AtomicInteger puts = new AtomicInteger();
+  private static List<List<Key>> deletes = synchronizedList(new ArrayList<List<Key>>());
-  private static AtomicInteger deletes = new AtomicInteger();
+  private static List<List<Entity>> puts = synchronizedList(new ArrayList<List<Entity>>());
 
-  RequestCountingAsyncDatastoreService(AsyncDatastoreService delegate) {
+  RequestCapturingAsyncDatastoreService(AsyncDatastoreService delegate) {
     this.delegate = delegate;
   }
 
-  public static int getReadsCount() {
+  public static List<List<Key>> getReads() {
-    return reads.get();
+    return reads;
   }
 
-  public static int getPutsCount() {
+  public static List<List<Key>> getDeletes() {
-    return puts.get();
+    return deletes;
   }
 
-  public static int getDeletesCount() {
+  public static List<List<Entity>> getPuts() {
-    return deletes.get();
+    return puts;
   }
 
   @Override
@@ -107,49 +111,49 @@ public class RequestCountingAsyncDatastoreService implements AsyncDatastoreServi
 
   @Override
   public Future<Void> delete(Key... keys) {
-    deletes.incrementAndGet();
+    deletes.add(ImmutableList.copyOf(keys));
     return delegate.delete(keys);
   }
 
   @Override
   public Future<Void> delete(Iterable<Key> keys) {
-    deletes.incrementAndGet();
+    deletes.add(ImmutableList.copyOf(keys));
     return delegate.delete(keys);
   }
 
   @Override
   public Future<Void> delete(Transaction transaction, Key... keys) {
-    deletes.incrementAndGet();
+    deletes.add(ImmutableList.copyOf(keys));
     return delegate.delete(transaction, keys);
   }
 
   @Override
   public Future<Void> delete(Transaction transaction, Iterable<Key> keys) {
-    deletes.incrementAndGet();
+    deletes.add(ImmutableList.copyOf(keys));
     return delegate.delete(transaction, keys);
   }
 
   @Override
   public Future<Entity> get(Key key) {
-    reads.incrementAndGet();
+    reads.add(ImmutableList.of(key));
     return delegate.get(key);
   }
 
   @Override
   public Future<Map<Key, Entity>> get(Iterable<Key> keys) {
-    reads.incrementAndGet();
+    reads.add(ImmutableList.copyOf(keys));
     return delegate.get(keys);
   }
 
   @Override
   public Future<Entity> get(Transaction transaction, Key key) {
-    reads.incrementAndGet();
+    reads.add(ImmutableList.of(key));
     return delegate.get(transaction, key);
   }
 
   @Override
   public Future<Map<Key, Entity>> get(Transaction transaction, Iterable<Key> keys) {
-    reads.incrementAndGet();
+    reads.add(ImmutableList.copyOf(keys));
     return delegate.get(transaction, keys);
   }
 
@@ -165,25 +169,25 @@ public class RequestCountingAsyncDatastoreService implements AsyncDatastoreServi
 
   @Override
   public Future<Key> put(Entity entity) {
-    puts.incrementAndGet();
+    puts.add(ImmutableList.of(entity));
     return delegate.put(entity);
   }
 
   @Override
   public Future<List<Key>> put(Iterable<Entity> entities) {
-    puts.incrementAndGet();
+    puts.add(ImmutableList.copyOf(entities));
     return delegate.put(entities);
   }
 
   @Override
   public Future<Key> put(Transaction transaction, Entity entity) {
-    puts.incrementAndGet();
+    puts.add(ImmutableList.of(entity));
     return delegate.put(transaction, entity);
   }
 
   @Override
   public Future<List<Key>> put(Transaction transaction, Iterable<Entity> entities) {
-    puts.incrementAndGet();
+    puts.add(ImmutableList.copyOf(entities));
     return delegate.put(transaction, entities);
   }
 }

javatests/google/registry/model/domain/DomainResourceTest.java

@@ -16,6 +16,7 @@ package google.registry.model.domain;
 
 import static com.google.appengine.tools.development.testing.LocalMemcacheServiceTestConfig.getLocalMemcacheService;
 import static com.google.common.collect.Iterables.getOnlyElement;
+import static com.google.common.collect.Iterables.skip;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.EppResourceUtils.loadByUniqueId;
 import static google.registry.testing.DatastoreHelper.cloneAndSetAutoTimestamps;
@@ -54,7 +55,7 @@ import google.registry.model.eppoutput.Response;
 import google.registry.model.eppoutput.Result;
 import google.registry.model.eppoutput.Result.Code;
 import google.registry.model.host.HostResource;
-import google.registry.model.ofy.RequestCountingAsyncDatastoreService;
+import google.registry.model.ofy.RequestCapturingAsyncDatastoreService;
 import google.registry.model.poll.PollMessage;
 import google.registry.model.registry.Registry;
 import google.registry.model.reporting.HistoryEntry;
@@ -432,7 +433,7 @@ public class DomainResourceTest extends EntityTestCase {
     // Clear out memcache so that we count actual datastore calls.
     getLocalMemcacheService().flushAll(
         new LocalRpcService.Status(), MemcacheFlushRequest.newBuilder().build());
-    int previousReads = RequestCountingAsyncDatastoreService.getReadsCount();
+    int numPreviousReads = RequestCapturingAsyncDatastoreService.getReads().size();
     EppXmlTransformer.marshal(
         EppOutput.create(new Response.Builder()
             .setResult(Result.create(Code.Success))
@@ -440,6 +441,7 @@ public class DomainResourceTest extends EntityTestCase {
             .setTrid(Trid.create(null, "abc"))
             .build()),
         ValidationMode.STRICT);
-    assertThat(RequestCountingAsyncDatastoreService.getReadsCount() - previousReads).isEqualTo(1);
+    // Assert that there was only one call to datastore (that may have loaded many keys).
+    assertThat(skip(RequestCapturingAsyncDatastoreService.getReads(), numPreviousReads)).hasSize(1);
   }
 }