Add a bit of documentation about the replica config (#1488)

* Add support for null or empty params

* Add Null or empty check in CollectionUtils

* Remove content type header for empty params in POST request

core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java

@@ -17,7 +17,7 @@ package google.registry.backup;
 import static google.registry.backup.ExportCommitLogDiffAction.LOWER_CHECKPOINT_TIME_PARAM;
 import static google.registry.backup.ExportCommitLogDiffAction.UPPER_CHECKPOINT_TIME_PARAM;
 import static google.registry.model.ofy.ObjectifyService.auditedOfy;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
 import static google.registry.util.DateTimeUtils.isBeforeOrAt;
 
 import com.google.common.collect.ImmutableMultimap;
@@ -67,7 +67,8 @@ public final class CommitLogCheckpointAction implements Runnable {
     final CommitLogCheckpoint checkpoint = strategy.computeCheckpoint();
     logger.atInfo().log(
         "Generated candidate checkpoint for time: %s", checkpoint.getCheckpointTime());
-    tm().transact(
+    ofyTm()
+        .transact(
             () -> {
               DateTime lastWrittenTime = CommitLogCheckpointRoot.loadRoot().getLastWrittenTime();
               if (isBeforeOrAt(checkpoint.getCheckpointTime(), lastWrittenTime)) {

core/src/main/java/google/registry/backup/DeleteOldCommitLogsAction.java

@@ -18,7 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static google.registry.mapreduce.MapreduceRunner.PARAM_DRY_RUN;
 import static google.registry.model.ofy.ObjectifyService.auditedOfy;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
 import static java.lang.Boolean.FALSE;
 import static java.lang.Boolean.TRUE;
 
@@ -288,7 +288,8 @@ public final class DeleteOldCommitLogsAction implements Runnable {
       }
 
       DeletionResult deletionResult =
-          tm().transactNew(
+          ofyTm()
+              .transactNew(
                   () -> {
                     CommitLogManifest manifest = auditedOfy().load().key(manifestKey).now();
                     // It is possible that the same manifestKey was run twice, if a shard had to be

core/src/main/java/google/registry/config/files/default-config.yaml

@@ -242,6 +242,9 @@ cloudSql:
   jdbcUrl: jdbc:postgresql://localhost
   # This name is used by Cloud SQL when connecting to the database.
   instanceConnectionName: project-id:region:instance-id
+  # If non-null, we will use this instance for certain read-only actions or
+  # pipelines, e.g. RDE, in order to offload some work from the primary
+  # instance. Expect any write actions on this instance to fail.
   replicaInstanceConnectionName: null
 
 cloudDns:

core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java

@@ -141,14 +141,15 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     // Postgresql-specific: 'set transaction' command must be called inside a transaction
     assertInTransaction();
 
-    EntityManager entityManager = getEntityManager();
+    ReadOnlyCheckingEntityManager entityManager =
+        (ReadOnlyCheckingEntityManager) getEntityManager();
     // Isolation is hardcoded to REPEATABLE READ, as specified by parent's Javadoc.
     entityManager
         .createNativeQuery("SET TRANSACTION ISOLATION LEVEL REPEATABLE READ")
-        .executeUpdate();
+        .executeUpdateIgnoringReadOnly();
     entityManager
         .createNativeQuery(String.format("SET TRANSACTION SNAPSHOT '%s'", snapshotId))
-        .executeUpdate();
+        .executeUpdateIgnoringReadOnly();
     return this;
   }
 

core/src/main/java/google/registry/persistence/transaction/ReadOnlyCheckingEntityManager.java

@@ -206,7 +206,7 @@ public class ReadOnlyCheckingEntityManager implements EntityManager {
   }
 
   @Override
-  public Query createNativeQuery(String sqlString) {
+  public ReadOnlyCheckingQuery createNativeQuery(String sqlString) {
     return new ReadOnlyCheckingQuery(delegate.createNativeQuery(sqlString));
   }
 

core/src/test/java/google/registry/cron/TldFanoutActionTest.java

@@ -98,11 +98,7 @@ class TldFanoutActionTest {
   }
 
   private void assertTaskWithoutTld() {
-    cloudTasksHelper.assertTasksEnqueued(
-        QUEUE,
-        new TaskMatcher()
-            .url(ENDPOINT)
-            .header("content-type", "application/x-www-form-urlencoded"));
+    cloudTasksHelper.assertTasksEnqueued(QUEUE, new TaskMatcher().url(ENDPOINT));
   }
 
   @Test

core/src/test/java/google/registry/testing/CloudTasksHelper.java

@@ -227,22 +227,20 @@ public class CloudTasksHelper implements Serializable {
               });
       headers = headerBuilder.build();
       ImmutableMultimap.Builder<String, String> paramBuilder = new ImmutableMultimap.Builder<>();
-      String query = null;
+      // Note that UriParameters.parse() does not throw an IAE on a bad query string (e.g. one
+      // where parameters are not properly URL-encoded); it always does a best-effort parse.
       if (method == HttpMethod.GET) {
-        query = uri.getQuery();
-      } else if (method == HttpMethod.POST) {
+        paramBuilder.putAll(UriParameters.parse(uri.getQuery()));
+      } else if (method == HttpMethod.POST && !task.getAppEngineHttpRequest().getBody().isEmpty()) {
         assertThat(
                 headers.containsEntry(
                     Ascii.toLowerCase(HttpHeaders.CONTENT_TYPE), MediaType.FORM_DATA.toString()))
             .isTrue();
-        query = task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8);
-      }
-      if (query != null) {
-        // Note that UriParameters.parse() does not throw an IAE on a bad query string (e.g. one
-        // where parameters are not properly URL-encoded); it always does a best-effort parse.
-        paramBuilder.putAll(UriParameters.parse(query));
-        params = paramBuilder.build();
+        paramBuilder.putAll(
+            UriParameters.parse(
+                task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8)));
       }
+      params = paramBuilder.build();
     }
 
     public Map<String, Object> toMap() {

util/src/main/java/google/registry/util/CloudTasksUtils.java

@@ -106,30 +106,34 @@ public class CloudTasksUtils implements Serializable {
     checkArgument(
         path != null && !path.isEmpty() && path.charAt(0) == '/',
         "The path must start with a '/'.");
+    checkArgument(
+        method.equals(HttpMethod.GET) || method.equals(HttpMethod.POST),
+        "HTTP method %s is used. Only GET and POST are allowed.",
+        method);
     AppEngineHttpRequest.Builder requestBuilder =
         AppEngineHttpRequest.newBuilder()
             .setHttpMethod(method)
             .setAppEngineRouting(AppEngineRouting.newBuilder().setService(service).build());
-    Escaper escaper = UrlEscapers.urlPathSegmentEscaper();
-    String encodedParams =
-        Joiner.on("&")
-            .join(
-                params.entries().stream()
-                    .map(
-                        entry ->
-                            String.format(
-                                "%s=%s",
-                                escaper.escape(entry.getKey()), escaper.escape(entry.getValue())))
-                    .collect(toImmutableList()));
-    if (method == HttpMethod.GET) {
-      path = String.format("%s?%s", path, encodedParams);
-    } else if (method == HttpMethod.POST) {
-      requestBuilder
-          .putHeaders(HttpHeaders.CONTENT_TYPE, MediaType.FORM_DATA.toString())
-          .setBody(ByteString.copyFrom(encodedParams, StandardCharsets.UTF_8));
-    } else {
-      throw new IllegalArgumentException(
-          String.format("HTTP method %s is used. Only GET and POST are allowed.", method));
+
+    if (!CollectionUtils.isNullOrEmpty(params)) {
+      Escaper escaper = UrlEscapers.urlPathSegmentEscaper();
+      String encodedParams =
+          Joiner.on("&")
+              .join(
+                  params.entries().stream()
+                      .map(
+                          entry ->
+                              String.format(
+                                  "%s=%s",
+                                  escaper.escape(entry.getKey()), escaper.escape(entry.getValue())))
+                      .collect(toImmutableList()));
+      if (method == HttpMethod.GET) {
+        path = String.format("%s?%s", path, encodedParams);
+      } else {
+        requestBuilder
+            .putHeaders(HttpHeaders.CONTENT_TYPE, MediaType.FORM_DATA.toString())
+            .setBody(ByteString.copyFrom(encodedParams, StandardCharsets.UTF_8));
+      }
     }
     requestBuilder.setRelativeUri(path);
     return Task.newBuilder().setAppEngineHttpRequest(requestBuilder.build()).build();

util/src/main/java/google/registry/util/CollectionUtils.java

@@ -49,6 +49,11 @@ public class CollectionUtils {
     return potentiallyNull == null || potentiallyNull.isEmpty();
   }
 
+  /** Checks if a Multimap is null or empty. */
+  public static boolean isNullOrEmpty(@Nullable Multimap<?, ?> potentiallyNull) {
+    return potentiallyNull == null || potentiallyNull.isEmpty();
+  }
+
   /** Turns a null set into an empty set. JAXB leaves lots of null sets lying around. */
   public static <T> Set<T> nullToEmpty(@Nullable Set<T> potentiallyNull) {
     return firstNonNull(potentiallyNull, ImmutableSet.of());

util/src/test/java/google/registry/util/CloudTasksUtilsTest.java

@@ -25,6 +25,7 @@ import static org.mockito.Mockito.when;
 import com.google.cloud.tasks.v2.HttpMethod;
 import com.google.cloud.tasks.v2.Task;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.LinkedListMultimap;
 import google.registry.testing.FakeClock;
 import google.registry.testing.FakeSleeper;
@@ -80,6 +81,48 @@ public class CloudTasksUtilsTest {
     assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
   }
 
+  @Test
+  void testSuccess_createGetTasks_withNullParams() {
+    Task task = CloudTasksUtils.createGetTask("/the/path", "myservice", null);
+    assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET);
+    assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path");
+    assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService())
+        .isEqualTo("myservice");
+    assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
+  }
+
+  @Test
+  void testSuccess_createPostTasks_withNullParams() {
+    Task task = CloudTasksUtils.createPostTask("/the/path", "myservice", null);
+    assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST);
+    assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path");
+    assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService())
+        .isEqualTo("myservice");
+    assertThat(task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8)).isEmpty();
+    assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
+  }
+
+  @Test
+  void testSuccess_createGetTasks_withEmptyParams() {
+    Task task = CloudTasksUtils.createGetTask("/the/path", "myservice", ImmutableMultimap.of());
+    assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET);
+    assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path");
+    assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService())
+        .isEqualTo("myservice");
+    assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
+  }
+
+  @Test
+  void testSuccess_createPostTasks_withEmptyParams() {
+    Task task = CloudTasksUtils.createPostTask("/the/path", "myservice", ImmutableMultimap.of());
+    assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST);
+    assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path");
+    assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService())
+        .isEqualTo("myservice");
+    assertThat(task.getAppEngineHttpRequest().getBody().toString(StandardCharsets.UTF_8)).isEmpty();
+    assertThat(task.getScheduleTime().getSeconds()).isEqualTo(0);
+  }
+
   @SuppressWarnings("ProtoTimestampGetSecondsGetNano")
   @Test
   void testSuccess_createGetTasks_withJitterSeconds() {

util/src/test/java/google/registry/util/CollectionUtilsTest.java

@@ -22,6 +22,8 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMultimap;
+import com.google.common.collect.Multimap;
 import java.util.Map;
 import org.junit.jupiter.api.Test;
 
@@ -43,6 +45,12 @@ class CollectionUtilsTest {
     assertThat(convertedMap).isEmpty();
   }
 
+  @Test
+  void testNullOrEmptyMultimap() {
+    assertThat(CollectionUtils.isNullOrEmpty((Multimap<?, ?>) null)).isTrue();
+    assertThat(CollectionUtils.isNullOrEmpty(ImmutableMultimap.of())).isTrue();
+  }
+
   @Test
   void testPartitionMap() {
     Map<String, String> map = ImmutableMap.of("ka", "va", "kb", "vb", "kc", "vc");