Remove extra space after bashslash (#743)

* Add retrier to retry exceptions we've seen so far

* Check for nested exceptions

* fix formatting

* check for nested transactions

* add JDBCConnectionException to isFailedQueryRetriable

* Add retrier to methods with suppliers

core/src/main/java/google/registry/persistence/JpaRetries.java

@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableSet;
 import java.sql.SQLException;
 import java.util.function.Predicate;
 import javax.persistence.OptimisticLockException;
+import org.hibernate.exception.JDBCConnectionException;
 
 /** Helpers for identifying retriable database operations. */
 public final class JpaRetries {
@@ -40,6 +41,13 @@ public final class JpaRetries {
               e instanceof SQLException
                   && RETRIABLE_TXN_SQL_STATE.contains(((SQLException) e).getSQLState()));
 
+  private static final Predicate<Throwable> RETRIABLE_QUERY_PREDICATE =
+      Predicates.or(
+          JDBCConnectionException.class::isInstance,
+          e ->
+              e instanceof SQLException
+                  && RETRIABLE_TXN_SQL_STATE.contains(((SQLException) e).getSQLState()));
+
   public static boolean isFailedTxnRetriable(Throwable throwable) {
     Throwable t = throwable;
     while (t != null) {
@@ -53,6 +61,13 @@ public final class JpaRetries {
 
   public static boolean isFailedQueryRetriable(Throwable throwable) {
     // TODO(weiminyu): check for more error codes.
-    return isFailedTxnRetriable(throwable);
+    Throwable t = throwable;
+    while (t != null) {
+      if (RETRIABLE_QUERY_PREDICATE.test(t)) {
+        return true;
+      }
+      t = t.getCause();
+    }
+    return false;
   }
 }

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

@@ -15,6 +15,7 @@
 package google.registry.persistence.transaction;
 
 import google.registry.persistence.VKey;
+import java.util.function.Supplier;
 import javax.persistence.EntityManager;
 
 /** Sub-interface of {@link TransactionManager} which defines JPA related methods. */
@@ -23,6 +24,12 @@ public interface JpaTransactionManager extends TransactionManager {
   /** Returns the {@link EntityManager} for the current request. */
   EntityManager getEntityManager();
 
+  /** Executes the work in a transaction with no retries and returns the result. */
+  <T> T transactNoRetry(Supplier<T> work);
+
+  /** Executes the work in a transaction with no retries. */
+  void transactNoRetry(Runnable work);
+
   /** Deletes the entity by its id, throws exception if the entity is not deleted. */
   public abstract <T> void assertDelete(VKey<T> key);
 

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

@@ -27,8 +27,11 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.FluentLogger;
 import google.registry.config.RegistryConfig;
+import google.registry.persistence.JpaRetries;
 import google.registry.persistence.VKey;
 import google.registry.util.Clock;
+import google.registry.util.Retrier;
+import google.registry.util.SystemSleeper;
 import java.lang.reflect.Field;
 import java.util.Map.Entry;
 import java.util.NoSuchElementException;
@@ -49,6 +52,7 @@ import org.joda.time.DateTime;
 public class JpaTransactionManagerImpl implements JpaTransactionManager {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+  private static final Retrier retrier = new Retrier(new SystemSleeper(), 3);
 
   // EntityManagerFactory is thread safe.
   private final EntityManagerFactory emf;
@@ -94,6 +98,40 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
   public <T> T transact(Supplier<T> work) {
     // TODO(shicong): Investigate removing transactNew functionality after migration as it may
     //  be same as this one.
+    return retrier.callWithRetry(
+        () -> {
+          if (inTransaction()) {
+            return work.get();
+          }
+          TransactionInfo txnInfo = transactionInfo.get();
+          txnInfo.entityManager = emf.createEntityManager();
+          EntityTransaction txn = txnInfo.entityManager.getTransaction();
+          try {
+            txn.begin();
+            txnInfo.start(clock);
+            T result = work.get();
+            txnInfo.recordTransaction();
+            txn.commit();
+            return result;
+          } catch (RuntimeException | Error e) {
+            // Error is unchecked!
+            try {
+              txn.rollback();
+              logger.atWarning().log("Error during transaction; transaction rolled back");
+            } catch (Throwable rollbackException) {
+              logger.atSevere().withCause(rollbackException).log(
+                  "Rollback failed; suppressing error");
+            }
+            throw e;
+          } finally {
+            txnInfo.clear();
+          }
+        },
+        JpaRetries::isFailedTxnRetriable);
+  }
+
+  @Override
+  public <T> T transactNoRetry(Supplier<T> work) {
     if (inTransaction()) {
       return work.get();
     }
@@ -130,6 +168,15 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
         });
   }
 
+  @Override
+  public void transactNoRetry(Runnable work) {
+    transactNoRetry(
+        () -> {
+          work.run();
+          return null;
+        });
+  }
+
   @Override
   public <T> T transactNew(Supplier<T> work) {
     return transact(work);
@@ -142,11 +189,14 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
 
   @Override
   public <T> T transactNewReadOnly(Supplier<T> work) {
-    return transact(
-        () -> {
-          getEntityManager().createNativeQuery("SET TRANSACTION READ ONLY").executeUpdate();
-          return work.get();
-        });
+    return retrier.callWithRetry(
+        () ->
+            transact(
+                () -> {
+                  getEntityManager().createNativeQuery("SET TRANSACTION READ ONLY").executeUpdate();
+                  return work.get();
+                }),
+        JpaRetries::isFailedQueryRetriable);
   }
 
   @Override
@@ -160,7 +210,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
 
   @Override
   public <T> T doTransactionless(Supplier<T> work) {
-    return transact(work);
+    return retrier.callWithRetry(() -> transact(work), JpaRetries::isFailedQueryRetriable);
   }
 
   @Override

core/src/main/java/google/registry/tools/CreateOrUpdateDomainCommand.java

@@ -15,20 +15,11 @@
 package google.registry.tools;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.collect.ImmutableList.toImmutableList;
 import static google.registry.util.CollectionUtils.findDuplicates;
 
-import com.beust.jcommander.IStringConverter;
 import com.beust.jcommander.Parameter;
-import com.google.auto.value.AutoValue;
-import com.google.common.base.Ascii;
-import com.google.common.base.CharMatcher;
 import com.google.common.base.Joiner;
-import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.io.BaseEncoding;
-import com.google.template.soy.data.SoyListData;
-import com.google.template.soy.data.SoyMapData;
 import google.registry.tools.params.NameserversParameter;
 import java.util.ArrayList;
 import java.util.HashSet;
@@ -80,77 +71,15 @@ abstract class CreateOrUpdateDomainCommand extends MutatingEppToolCommand {
   String password;
 
   @Parameter(
-    names = "--ds_records",
-    description =
-        "Comma-separated list of DS records. Each DS record is given as "
-        + "<keyTag> <alg> <digestType> <digest>, in order, as it appears in the Zonefile.",
-    converter = DsRecordConverter.class
-  )
+      names = "--ds_records",
+      description =
+          "Comma-separated list of DS records. Each DS record is given as "
+              + "<keyTag> <alg> <digestType> <digest>, in order, as it appears in the Zonefile.",
+      converter = DsRecord.Converter.class)
   List<DsRecord> dsRecords = new ArrayList<>();
 
   Set<String> domains;
 
-  @AutoValue
-  abstract static class DsRecord {
-    private static final Splitter SPLITTER =
-        Splitter.on(CharMatcher.whitespace()).omitEmptyStrings();
-
-    public abstract int keyTag();
-    public abstract int alg();
-    public abstract int digestType();
-    public abstract String digest();
-
-    private static DsRecord create(int keyTag, int alg, int digestType, String digest) {
-      digest = Ascii.toUpperCase(digest);
-      checkArgument(
-          BaseEncoding.base16().canDecode(digest),
-          "digest should be even-lengthed hex, but is %s (length %s)",
-          digest,
-          digest.length());
-      return new AutoValue_CreateOrUpdateDomainCommand_DsRecord(keyTag, alg, digestType, digest);
-    }
-
-    /**
-     * Parses a string representation of the DS record.
-     *
-     * <p>The string format accepted is "[keyTag] [alg] [digestType] [digest]" (i.e., the 4
-     * arguments separated by any number of spaces, as it appears in the Zone file)
-     */
-    public static DsRecord parse(String dsRecord) {
-      List<String> elements = SPLITTER.splitToList(dsRecord);
-      checkArgument(
-          elements.size() == 4,
-          "dsRecord %s should have 4 parts, but has %s",
-          dsRecord,
-          elements.size());
-      return DsRecord.create(
-          Integer.parseUnsignedInt(elements.get(0)),
-          Integer.parseUnsignedInt(elements.get(1)),
-          Integer.parseUnsignedInt(elements.get(2)),
-          elements.get(3));
-    }
-
-    public SoyMapData toSoyData() {
-      return new SoyMapData(
-          "keyTag", keyTag(),
-          "alg", alg(),
-          "digestType", digestType(),
-          "digest", digest());
-    }
-
-    public static SoyListData convertToSoy(List<DsRecord> dsRecords) {
-      return new SoyListData(
-          dsRecords.stream().map(DsRecord::toSoyData).collect(toImmutableList()));
-    }
-  }
-
-  public static class DsRecordConverter implements IStringConverter<DsRecord> {
-    @Override
-    public DsRecord convert(String dsRecord) {
-      return DsRecord.parse(dsRecord);
-    }
-  }
-
   @Override
   protected void initEppToolCommand() throws Exception {
     checkArgument(nameservers.size() <= 13, "There can be at most 13 nameservers.");

core/src/main/java/google/registry/tools/DsRecord.java

@@ -0,0 +1,90 @@
+// Copyright 2017 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.tools;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.beust.jcommander.IStringConverter;
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Ascii;
+import com.google.common.base.CharMatcher;
+import com.google.common.base.Splitter;
+import com.google.common.io.BaseEncoding;
+import com.google.template.soy.data.SoyListData;
+import com.google.template.soy.data.SoyMapData;
+import java.util.List;
+
+@AutoValue
+abstract class DsRecord {
+  private static final Splitter SPLITTER = Splitter.on(CharMatcher.whitespace()).omitEmptyStrings();
+
+  public abstract int keyTag();
+
+  public abstract int alg();
+
+  public abstract int digestType();
+
+  public abstract String digest();
+
+  private static DsRecord create(int keyTag, int alg, int digestType, String digest) {
+    digest = Ascii.toUpperCase(digest);
+    checkArgument(
+        BaseEncoding.base16().canDecode(digest),
+        "digest should be even-lengthed hex, but is %s (length %s)",
+        digest,
+        digest.length());
+    return new AutoValue_DsRecord(keyTag, alg, digestType, digest);
+  }
+
+  /**
+   * Parses a string representation of the DS record.
+   *
+   * <p>The string format accepted is "[keyTag] [alg] [digestType] [digest]" (i.e., the 4 arguments
+   * separated by any number of spaces, as it appears in the Zone file)
+   */
+  public static DsRecord parse(String dsRecord) {
+    List<String> elements = SPLITTER.splitToList(dsRecord);
+    checkArgument(
+        elements.size() == 4,
+        "dsRecord %s should have 4 parts, but has %s",
+        dsRecord,
+        elements.size());
+    return DsRecord.create(
+        Integer.parseUnsignedInt(elements.get(0)),
+        Integer.parseUnsignedInt(elements.get(1)),
+        Integer.parseUnsignedInt(elements.get(2)),
+        elements.get(3));
+  }
+
+  public SoyMapData toSoyData() {
+    return new SoyMapData(
+        "keyTag", keyTag(),
+        "alg", alg(),
+        "digestType", digestType(),
+        "digest", digest());
+  }
+
+  public static SoyListData convertToSoy(List<DsRecord> dsRecords) {
+    return new SoyListData(dsRecords.stream().map(DsRecord::toSoyData).collect(toImmutableList()));
+  }
+
+  public static class Converter implements IStringConverter<DsRecord> {
+    @Override
+    public DsRecord convert(String dsRecord) {
+      return DsRecord.parse(dsRecord);
+    }
+  }
+}

core/src/main/java/google/registry/tools/UniformRapidSuspensionCommand.java

@@ -15,7 +15,7 @@
 package google.registry.tools;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Strings.nullToEmpty;
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.Sets.difference;
 import static google.registry.model.EppResourceUtils.checkResourcesExist;
 import static google.registry.model.EppResourceUtils.loadByForeignKey;
@@ -26,9 +26,11 @@ import static org.joda.time.DateTimeZone.UTC;
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
 import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
+import com.google.template.soy.data.SoyListData;
 import com.google.template.soy.data.SoyMapData;
 import google.registry.model.domain.DomainBase;
 import google.registry.model.domain.secdns.DelegationSignerData;
@@ -37,14 +39,10 @@ import google.registry.model.host.HostResource;
 import google.registry.tools.soy.UniformRapidSuspensionSoyInfo;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import javax.xml.bind.annotation.adapters.HexBinaryAdapter;
 import org.joda.time.DateTime;
-import org.json.simple.JSONArray;
-import org.json.simple.JSONValue;
-import org.json.simple.parser.ParseException;
 
 /** A command to suspend a domain for the Uniform Rapid Suspension process. */
 @Parameters(separators = " =",
@@ -59,9 +57,6 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
   /** Client id that made this change. Only recorded in the history entry. **/
   private static final String CLIENT_ID = "CharlestonRoad";
 
-  private static final ImmutableSet<String> DSDATA_FIELDS =
-      ImmutableSet.of("keyTag", "alg", "digestType", "digest");
-
   @Parameter(
       names = {"-n", "--domain_name"},
       description = "Domain to suspend.",
@@ -76,10 +71,12 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
 
   @Parameter(
       names = {"-s", "--dsdata"},
-      description = "Comma-delimited set of dsdata to replace the current dsdata on the domain, "
-          + "where each dsdata is represented as a JSON object with fields 'keyTag', 'alg', "
-          + "'digestType' and 'digest'.")
-  private String newDsData;
+      description =
+          "Comma-delimited set of dsdata to replace the current dsdata on the domain, "
+              + "Each DS record is given as <keyTag> <alg> <digestType> <digest>, in order, as it "
+              + "appears in the Zonefile.",
+      converter = DsRecord.Converter.class)
+  private List<DsRecord> newDsData;
 
   @Parameter(
       names = {"-p", "--locks_to_preserve"},
@@ -88,6 +85,13 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
               + "locks: serverDeleteProhibited, serverTransferProhibited, serverUpdateProhibited")
   private List<String> locksToPreserve = new ArrayList<>();
 
+  @Parameter(
+      names = {"--restore_client_hold"},
+      description =
+          "Restores a CLIENT_HOLD status that was previously removed for a URS suspension (only "
+              + "valid with --undo).")
+  private boolean restoreClientHold;
+
   @Parameter(
       names = {"--undo"},
       description = "Flag indicating that is is an undo command, which removes locks.")
@@ -100,28 +104,16 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
   ImmutableSortedSet<String> existingNameservers;
 
   /** Set of existing dsdata jsons that need to be restored during undo, sorted for nicer output. */
-  ImmutableSortedSet<String> existingDsData;
+  ImmutableList<ImmutableMap<String, Object>> existingDsData;
+
+  /** Set of status values to remove. */
+  ImmutableSet<String> removeStatuses;
 
   @Override
   protected void initMutatingEppToolCommand() {
     superuser = true;
     DateTime now = DateTime.now(UTC);
     ImmutableSet<String> newHostsSet = ImmutableSet.copyOf(newHosts);
-    ImmutableSet.Builder<Map<String, Object>> newDsDataBuilder = new ImmutableSet.Builder<>();
-    try {
-      // Add brackets around newDsData to convert it to a parsable JSON array.
-      String jsonArrayString = String.format("[%s]", nullToEmpty(newDsData));
-      for (Object dsData : (JSONArray) JSONValue.parseWithException(jsonArrayString)) {
-        @SuppressWarnings("unchecked")
-        Map<String, Object> dsDataJson = (Map<String, Object>) dsData;
-        checkArgument(
-            dsDataJson.keySet().equals(DSDATA_FIELDS),
-            "Incorrect fields on --dsdata JSON: " + JSONValue.toJSONString(dsDataJson));
-        newDsDataBuilder.add(dsDataJson);
-      }
-    } catch (ClassCastException | ParseException e) {
-      throw new IllegalArgumentException("Invalid --dsdata JSON", e);
-    }
     Optional<DomainBase> domain = loadByForeignKey(DomainBase.class, domainName, now);
     checkArgumentPresent(domain, "Domain '%s' does not exist or is deleted", domainName);
     Set<String> missingHosts =
@@ -133,18 +125,39 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
     existingNameservers = getExistingNameservers(domain.get());
     existingLocks = getExistingLocks(domain.get());
     existingDsData = getExistingDsData(domain.get());
+    removeStatuses =
+        (hasClientHold(domain.get()) && !undo)
+            ? ImmutableSet.of(StatusValue.CLIENT_HOLD.getXmlName())
+            : ImmutableSet.of();
+    ImmutableSet<String> statusesToApply;
+    if (undo) {
+      statusesToApply =
+          restoreClientHold
+              ? ImmutableSet.of(StatusValue.CLIENT_HOLD.getXmlName())
+              : ImmutableSet.of();
+    } else {
+      statusesToApply = URS_LOCKS;
+    }
     setSoyTemplate(
         UniformRapidSuspensionSoyInfo.getInstance(),
         UniformRapidSuspensionSoyInfo.UNIFORMRAPIDSUSPENSION);
-    addSoyRecord(CLIENT_ID, new SoyMapData(
-        "domainName", domainName,
-        "hostsToAdd", difference(newHostsSet, existingNameservers),
-        "hostsToRemove", difference(existingNameservers, newHostsSet),
-        "locksToApply", undo ? ImmutableSet.of() : URS_LOCKS,
-        "locksToRemove",
-            undo ? difference(URS_LOCKS, ImmutableSet.copyOf(locksToPreserve)) : ImmutableSet.of(),
-        "newDsData", newDsDataBuilder.build(),
-        "reason", (undo ? "Undo " : "") + "Uniform Rapid Suspension"));
+    addSoyRecord(
+        CLIENT_ID,
+        new SoyMapData(
+            "domainName",
+            domainName,
+            "hostsToAdd",
+            difference(newHostsSet, existingNameservers),
+            "hostsToRemove",
+            difference(existingNameservers, newHostsSet),
+            "statusesToApply",
+            statusesToApply,
+            "statusesToRemove",
+            undo ? difference(URS_LOCKS, ImmutableSet.copyOf(locksToPreserve)) : removeStatuses,
+            "newDsData",
+            newDsData != null ? DsRecord.convertToSoy(newDsData) : new SoyListData(),
+            "reason",
+            (undo ? "Undo " : "") + "Uniform Rapid Suspension"));
   }
 
   private ImmutableSortedSet<String> getExistingNameservers(DomainBase domain) {
@@ -165,15 +178,25 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
     return locks.build();
   }
 
-  private ImmutableSortedSet<String> getExistingDsData(DomainBase domain) {
-    ImmutableSortedSet.Builder<String> dsDataJsons = ImmutableSortedSet.naturalOrder();
+  private boolean hasClientHold(DomainBase domain) {
+    for (StatusValue status : domain.getStatusValues()) {
+      if (status == StatusValue.CLIENT_HOLD) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  private ImmutableList<ImmutableMap<String, Object>> getExistingDsData(DomainBase domain) {
+    ImmutableList.Builder<ImmutableMap<String, Object>> dsDataJsons = new ImmutableList.Builder();
     HexBinaryAdapter hexBinaryAdapter = new HexBinaryAdapter();
     for (DelegationSignerData dsData : domain.getDsData()) {
-      dsDataJsons.add(JSONValue.toJSONString(ImmutableMap.of(
-          "keyTag", dsData.getKeyTag(),
-          "algorithm", dsData.getAlgorithm(),
-          "digestType", dsData.getDigestType(),
-          "digest", hexBinaryAdapter.marshal(dsData.getDigest()))));
+      dsDataJsons.add(
+          ImmutableMap.of(
+              "keyTag", dsData.getKeyTag(),
+              "algorithm", dsData.getAlgorithm(),
+              "digestType", dsData.getDigestType(),
+              "digest", hexBinaryAdapter.marshal(dsData.getDigest())));
     }
     return dsDataJsons.build();
   }
@@ -194,8 +217,23 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
     if (!existingLocks.isEmpty()) {
       undoBuilder.append(" --locks_to_preserve ").append(Joiner.on(',').join(existingLocks));
     }
+    if (removeStatuses.contains(StatusValue.CLIENT_HOLD.getXmlName())) {
+      undoBuilder.append(" --restore_client_hold");
+    }
     if (!existingDsData.isEmpty()) {
-      undoBuilder.append(" --dsdata ").append(Joiner.on(',').join(existingDsData));
+      ImmutableList<String> formattedDsRecords =
+          existingDsData.stream()
+              .map(
+                  rec ->
+                      String.format(
+                          "%s %s %s %s",
+                          rec.get("keyTag"),
+                          rec.get("algorithm"),
+                          rec.get("digestType"),
+                          rec.get("digest")))
+              .sorted()
+              .collect(toImmutableList());
+      undoBuilder.append(" --dsdata ").append(Joiner.on(',').join(formattedDsRecords));
     }
     return undoBuilder.toString();
   }

core/src/main/java/google/registry/tools/UpdateDomainCommand.java

@@ -76,10 +76,10 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
   private List<String> addStatuses = new ArrayList<>();
 
   @Parameter(
-    names = "--add_ds_records",
-    description = "DS records to add. Cannot be set if --ds_records or --clear_ds_records is set.",
-    converter = DsRecordConverter.class
-  )
+      names = "--add_ds_records",
+      description =
+          "DS records to add. Cannot be set if --ds_records or --clear_ds_records is set.",
+      converter = DsRecord.Converter.class)
   private List<DsRecord> addDsRecords = new ArrayList<>();
 
   @Parameter(
@@ -110,11 +110,10 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
   private List<String> removeStatuses = new ArrayList<>();
 
   @Parameter(
-    names = "--remove_ds_records",
-    description =
-        "DS records to remove. Cannot be set if --ds_records or --clear_ds_records is set.",
-    converter = DsRecordConverter.class
-  )
+      names = "--remove_ds_records",
+      description =
+          "DS records to remove. Cannot be set if --ds_records or --clear_ds_records is set.",
+      converter = DsRecord.Converter.class)
   private List<DsRecord> removeDsRecords = new ArrayList<>();
 
   @Parameter(

core/src/main/resources/google/registry/tools/soy/UniformRapidSuspension.soy

@@ -21,8 +21,8 @@
 {@param domainName: string}
 {@param hostsToAdd: list<string>}
 {@param hostsToRemove: list<string>}
-{@param locksToApply: list<string>}
-{@param locksToRemove: list<string>}
+{@param statusesToApply: list<string>}
+{@param statusesToRemove: list<string>}
 {@param newDsData: list<[keyTag:int, alg:int, digestType:int, digest:string]>}
 {@param reason: string}
 <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
@@ -39,7 +39,7 @@
           {/for}
           </domain:ns>
         {/if}
-        {for $la in $locksToApply}
+        {for $la in $statusesToApply}
           <domain:status s="{$la}" />
         {/for}
         </domain:add>
@@ -51,7 +51,7 @@
           {/for}
           </domain:ns>
         {/if}
-        {for $lr in $locksToRemove}
+        {for $lr in $statusesToRemove}
           <domain:status s="{$lr}" />
         {/for}
         </domain:rem>

core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java

@@ -18,6 +18,11 @@ import static com.google.common.truth.Truth.assertThat;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.testing.TestDataHelper.fileClassPath;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 
 import com.google.common.collect.ImmutableList;
 import google.registry.model.ImmutableObject;
@@ -26,12 +31,16 @@ import google.registry.persistence.transaction.JpaTestRules.JpaUnitTestExtension
 import google.registry.testing.FakeClock;
 import java.io.Serializable;
 import java.math.BigInteger;
+import java.sql.SQLException;
 import java.util.NoSuchElementException;
+import java.util.function.Supplier;
 import javax.persistence.Entity;
 import javax.persistence.EntityManager;
 import javax.persistence.Id;
 import javax.persistence.IdClass;
+import javax.persistence.OptimisticLockException;
 import javax.persistence.RollbackException;
+import org.hibernate.exception.JDBCConnectionException;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
@@ -130,6 +139,117 @@ class JpaTransactionManagerImplTest {
     assertThat(jpaTm().transact(() -> jpaTm().load(theEntityKey))).isEqualTo(theEntity);
   }
 
+  @Test
+  public void transact_retriesOptimisticLockExceptions() {
+    JpaTransactionManager spyJpaTm = spy(jpaTm());
+    doThrow(OptimisticLockException.class).when(spyJpaTm).delete(any(VKey.class));
+    spyJpaTm.transact(() -> spyJpaTm.saveNew(theEntity));
+    assertThrows(
+        OptimisticLockException.class,
+        () -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey)));
+    verify(spyJpaTm, times(3)).delete(theEntityKey);
+    Supplier<Runnable> supplier =
+        () -> {
+          Runnable work = () -> spyJpaTm.delete(theEntityKey);
+          work.run();
+          return null;
+        };
+    assertThrows(OptimisticLockException.class, () -> spyJpaTm.transact(supplier));
+    verify(spyJpaTm, times(6)).delete(theEntityKey);
+  }
+
+  @Test
+  public void transactNoRetry_doesNotRetryOptimisticLockException() {
+    JpaTransactionManager spyJpaTm = spy(jpaTm());
+    doThrow(OptimisticLockException.class).when(spyJpaTm).delete(any(VKey.class));
+    spyJpaTm.transactNoRetry(() -> spyJpaTm.saveNew(theEntity));
+    assertThrows(
+        OptimisticLockException.class,
+        () -> spyJpaTm.transactNoRetry(() -> spyJpaTm.delete(theEntityKey)));
+    verify(spyJpaTm, times(1)).delete(theEntityKey);
+    Supplier<Runnable> supplier =
+        () -> {
+          Runnable work = () -> spyJpaTm.delete(theEntityKey);
+          work.run();
+          return null;
+        };
+    assertThrows(OptimisticLockException.class, () -> spyJpaTm.transactNoRetry(supplier));
+    verify(spyJpaTm, times(2)).delete(theEntityKey);
+  }
+
+  @Test
+  public void transact_retriesNestedOptimisticLockExceptions() {
+    JpaTransactionManager spyJpaTm = spy(jpaTm());
+    doThrow(new RuntimeException().initCause(new OptimisticLockException()))
+        .when(spyJpaTm)
+        .delete(any(VKey.class));
+    spyJpaTm.transact(() -> spyJpaTm.saveNew(theEntity));
+    assertThrows(
+        RuntimeException.class, () -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey)));
+    verify(spyJpaTm, times(3)).delete(theEntityKey);
+    Supplier<Runnable> supplier =
+        () -> {
+          Runnable work = () -> spyJpaTm.delete(theEntityKey);
+          work.run();
+          return null;
+        };
+    assertThrows(RuntimeException.class, () -> spyJpaTm.transact(supplier));
+    verify(spyJpaTm, times(6)).delete(theEntityKey);
+  }
+
+  @Test
+  public void transactNewReadOnly_retriesJdbcConnectionExceptions() {
+    JpaTransactionManager spyJpaTm = spy(jpaTm());
+    doThrow(JDBCConnectionException.class).when(spyJpaTm).load(any(VKey.class));
+    spyJpaTm.transact(() -> spyJpaTm.saveNew(theEntity));
+    assertThrows(
+        JDBCConnectionException.class,
+        () -> spyJpaTm.transactNewReadOnly(() -> spyJpaTm.load(theEntityKey)));
+    verify(spyJpaTm, times(3)).load(theEntityKey);
+    Supplier<Runnable> supplier =
+        () -> {
+          Runnable work = () -> spyJpaTm.load(theEntityKey);
+          work.run();
+          return null;
+        };
+    assertThrows(JDBCConnectionException.class, () -> spyJpaTm.transactNewReadOnly(supplier));
+    verify(spyJpaTm, times(6)).load(theEntityKey);
+  }
+
+  @Test
+  public void transactNewReadOnly_retriesNestedJdbcConnectionExceptions() {
+    JpaTransactionManager spyJpaTm = spy(jpaTm());
+    doThrow(
+            new RuntimeException()
+                .initCause(new JDBCConnectionException("connection exception", new SQLException())))
+        .when(spyJpaTm)
+        .load(any(VKey.class));
+    spyJpaTm.transact(() -> spyJpaTm.saveNew(theEntity));
+    assertThrows(
+        RuntimeException.class,
+        () -> spyJpaTm.transactNewReadOnly(() -> spyJpaTm.load(theEntityKey)));
+    verify(spyJpaTm, times(3)).load(theEntityKey);
+    Supplier<Runnable> supplier =
+        () -> {
+          Runnable work = () -> spyJpaTm.load(theEntityKey);
+          work.run();
+          return null;
+        };
+    assertThrows(RuntimeException.class, () -> spyJpaTm.transactNewReadOnly(supplier));
+    verify(spyJpaTm, times(6)).load(theEntityKey);
+  }
+
+  @Test
+  public void doTransactionless_retriesJdbcConnectionExceptions() {
+    JpaTransactionManager spyJpaTm = spy(jpaTm());
+    doThrow(JDBCConnectionException.class).when(spyJpaTm).load(any(VKey.class));
+    spyJpaTm.transact(() -> spyJpaTm.saveNew(theEntity));
+    assertThrows(
+        RuntimeException.class,
+        () -> spyJpaTm.doTransactionless(() -> spyJpaTm.load(theEntityKey)));
+    verify(spyJpaTm, times(3)).load(theEntityKey);
+  }
+
   @Test
   void saveNew_throwsExceptionIfEntityExists() {
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse();

core/src/test/java/google/registry/tools/UniformRapidSuspensionCommandTest.java

@@ -71,7 +71,7 @@ class UniformRapidSuspensionCommandTest
     runCommandForced(
         "--domain_name=evil.tld",
         "--hosts=urs1.example.com,urs2.example.com",
-        "--dsdata={\"keyTag\":1,\"alg\":1,\"digestType\":1,\"digest\":\"abc\"}");
+        "--dsdata=1 1 1 abcd");
     eppVerifier
         .expectClientId("CharlestonRoad")
         .expectSuperuser()
@@ -79,10 +79,9 @@ class UniformRapidSuspensionCommandTest
     assertInStdout("uniform_rapid_suspension --undo");
     assertInStdout("--domain_name evil.tld");
     assertInStdout("--hosts ns1.example.com,ns2.example.com");
-    assertInStdout("--dsdata "
-        + "{\"keyTag\":1,\"algorithm\":2,\"digestType\":3,\"digest\":\"DEAD\"},"
-        + "{\"keyTag\":4,\"algorithm\":5,\"digestType\":6,\"digest\":\"BEEF\"}");
+    assertInStdout("--dsdata 1 2 3 DEAD,4 5 6 BEEF");
     assertNotInStdout("--locks_to_preserve");
+    assertNotInStdout("--restore_client_hold");
   }
 
   @Test
@@ -122,6 +121,29 @@ class UniformRapidSuspensionCommandTest
     assertInStdout("--locks_to_preserve serverDeleteProhibited");
   }
 
+  @Test
+  void testCommand_removeClientHold() throws Exception {
+    persistResource(
+        newDomainBase("evil.tld")
+            .asBuilder()
+            .addStatusValue(StatusValue.CLIENT_HOLD)
+            .addNameserver(ns1.createVKey())
+            .addNameserver(ns2.createVKey())
+            .build());
+    runCommandForced(
+        "--domain_name=evil.tld",
+        "--hosts=urs1.example.com,urs2.example.com",
+        "--dsdata=1 1 1 abcd");
+    eppVerifier
+        .expectClientId("CharlestonRoad")
+        .expectSuperuser()
+        .verifySent("uniform_rapid_suspension_with_client_hold.xml");
+    assertInStdout("uniform_rapid_suspension --undo");
+    assertInStdout("--domain_name evil.tld");
+    assertInStdout("--hosts ns1.example.com,ns2.example.com");
+    assertInStdout("--restore_client_hold");
+  }
+
   @Test
   void testUndo_removesLocksReplacesHostsAndDsData() throws Exception {
     persistDomainWithHosts(urs1, urs2);
@@ -149,6 +171,21 @@ class UniformRapidSuspensionCommandTest
     assertNotInStdout("--undo");  // Undo shouldn't print a new undo command.
   }
 
+  @Test
+  void testUndo_restoresClientHolds() throws Exception {
+    persistDomainWithHosts(urs1, urs2);
+    runCommandForced(
+        "--domain_name=evil.tld",
+        "--undo",
+        "--hosts=ns1.example.com,ns2.example.com",
+        "--restore_client_hold");
+    eppVerifier
+        .expectClientId("CharlestonRoad")
+        .expectSuperuser()
+        .verifySent("uniform_rapid_suspension_undo_client_hold.xml");
+    assertNotInStdout("--undo"); // Undo shouldn't print a new undo command.
+  }
+
   @Test
   void testFailure_locksToPreserveWithoutUndo() {
     persistActiveDomain("evil.tld");
@@ -177,11 +214,10 @@ class UniformRapidSuspensionCommandTest
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
-            () ->
-                runCommandForced(
-                    "--domain_name=evil.tld",
-                    "--dsdata={\"keyTag\":1,\"alg\":1,\"digestType\":1,\"digest\":\"abc\",\"foo\":1}"));
-    assertThat(thrown).hasMessageThat().contains("Incorrect fields on --dsdata JSON");
+            () -> runCommandForced("--domain_name=evil.tld", "--dsdata=1 1 1 abc 1"));
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains("dsRecord 1 1 1 abc 1 should have 4 parts, but has 5");
   }
 
   @Test
@@ -190,11 +226,8 @@ class UniformRapidSuspensionCommandTest
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
-            () ->
-                runCommandForced(
-                    "--domain_name=evil.tld",
-                    "--dsdata={\"keyTag\":1,\"alg\":1,\"digestType\":1}"));
-    assertThat(thrown).hasMessageThat().contains("Incorrect fields on --dsdata JSON");
+            () -> runCommandForced("--domain_name=evil.tld", "--dsdata=1 1 1"));
+    assertThat(thrown).hasMessageThat().contains("dsRecord 1 1 1 should have 4 parts, but has 3");
   }
 
   @Test
@@ -203,7 +236,7 @@ class UniformRapidSuspensionCommandTest
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
-            () -> runCommandForced("--domain_name=evil.tld", "--dsdata=[1,2,3]"));
-    assertThat(thrown).hasMessageThat().contains("Invalid --dsdata JSON");
+            () -> runCommandForced("--domain_name=evil.tld", "--dsdata=1,2,3"));
+    assertThat(thrown).hasMessageThat().contains("dsRecord 1 should have 4 parts, but has 1");
   }
 }

core/src/test/resources/google/registry/tools/server/uniform_rapid_suspension.xml

@@ -31,7 +31,7 @@
             <secDNS:keyTag>1</secDNS:keyTag>
             <secDNS:alg>1</secDNS:alg>
             <secDNS:digestType>1</secDNS:digestType>
-            <secDNS:digest>abc</secDNS:digest>
+            <secDNS:digest>ABCD</secDNS:digest>
           </secDNS:dsData>
         </secDNS:add>
       </secDNS:update>

core/src/test/resources/google/registry/tools/server/uniform_rapid_suspension_undo_client_hold.xml

@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <update>
+      <domain:update xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>evil.tld</domain:name>
+        <domain:add>
+          <domain:ns>
+            <domain:hostObj>ns1.example.com</domain:hostObj>
+            <domain:hostObj>ns2.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:status s="clientHold" />
+        </domain:add>
+        <domain:rem>
+          <domain:ns>
+            <domain:hostObj>urs1.example.com</domain:hostObj>
+            <domain:hostObj>urs2.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:status s="serverDeleteProhibited" />
+          <domain:status s="serverTransferProhibited" />
+          <domain:status s="serverUpdateProhibited" />
+        </domain:rem>
+      </domain:update>
+    </update>
+    <extension>
+      <secDNS:update xmlns:secDNS="urn:ietf:params:xml:ns:secDNS-1.1">
+        <secDNS:rem>
+          <secDNS:all>true</secDNS:all>
+        </secDNS:rem>
+      </secDNS:update>
+      <metadata:metadata xmlns:metadata="urn:google:params:xml:ns:metadata-1.0">
+        <metadata:reason>Undo Uniform Rapid Suspension</metadata:reason>
+        <metadata:requestedByRegistrar>false</metadata:requestedByRegistrar>
+      </metadata:metadata>
+    </extension>
+    <clTRID>RegistryTool</clTRID>
+  </command>
+</epp>

core/src/test/resources/google/registry/tools/server/uniform_rapid_suspension_with_client_hold.xml

@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <update>
+      <domain:update xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>evil.tld</domain:name>
+        <domain:add>
+          <domain:ns>
+            <domain:hostObj>urs1.example.com</domain:hostObj>
+            <domain:hostObj>urs2.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:status s="serverDeleteProhibited"/>
+          <domain:status s="serverTransferProhibited"/>
+          <domain:status s="serverUpdateProhibited"/>
+        </domain:add>
+        <domain:rem>
+          <domain:ns>
+            <domain:hostObj>ns2.example.com</domain:hostObj>
+            <domain:hostObj>ns1.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:status s="clientHold"/>
+        </domain:rem>
+      </domain:update>
+    </update>
+    <extension>
+      <secDNS:update xmlns:secDNS="urn:ietf:params:xml:ns:secDNS-1.1">
+        <secDNS:rem>
+          <secDNS:all>true</secDNS:all>
+        </secDNS:rem>
+        <secDNS:add>
+          <secDNS:dsData>
+            <secDNS:keyTag>1</secDNS:keyTag>
+            <secDNS:alg>1</secDNS:alg>
+            <secDNS:digestType>1</secDNS:digestType>
+            <secDNS:digest>ABCD</secDNS:digest>
+          </secDNS:dsData>
+        </secDNS:add>
+      </secDNS:update>
+      <metadata:metadata xmlns:metadata="urn:google:params:xml:ns:metadata-1.0">
+        <metadata:reason>Uniform Rapid Suspension</metadata:reason>
+        <metadata:requestedByRegistrar>false</metadata:requestedByRegistrar>
+      </metadata:metadata>
+    </extension>
+    <clTRID>RegistryTool</clTRID>
+  </command>
+</epp>

java-format/google-java-format-git-diff.sh

@@ -69,7 +69,7 @@ function callGoogleJavaFormatDiff() {
     "format")
       showNoncompliantFiles "$forkPoint" "\033[1mReformatting: "
       callResult=$(git diff -U0 ${forkPoint} | \
-          ${SCRIPT_DIR}/google-java-format-diff.py \ 
+          ${SCRIPT_DIR}/google-java-format-diff.py \
           --google-java-format-jar "${SCRIPT_DIR}/${JAR_NAME}" \
           -p1 -i)
       ;;