Remove commitAdditionalLogicChanges(), part 1

Now that the flows are flattened, the commitAdditionalLogicChanges() call, which used to come later in the flow to actually save the Datastore objects, is now happening right after the performAdditionalXXXLogic() call. So we can instead just do the saves in performAdditionalXXXLogic(), and get rid of the separate call. As a first step, this CL simply makes commitAdditionalLogicChanges() a private method that gets called internally by the extra logic manager. Later, we can move the saves into their proper position, affecting only the extra logic class itself.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=137529991

java/google/registry/flows/domain/DomainApplicationCreateFlow.java

@@ -353,7 +353,6 @@ public final class DomainApplicationCreateFlow extends Flow implements Transacti
           years,
           eppInput,
           historyEntry);
-      extraFlowLogic.get().commitAdditionalLogicChanges();
     }
   }
 

java/google/registry/flows/domain/DomainCreateFlow.java

@@ -404,7 +404,6 @@ public class DomainCreateFlow extends Flow implements TransactionalFlow {
           years,
           eppInput,
           historyEntry);
-      extraFlowLogic.get().commitAdditionalLogicChanges();
     }
   }
 

java/google/registry/flows/domain/DomainDeleteFlow.java

@@ -198,7 +198,6 @@ public final class DomainDeleteFlow extends Flow implements TransactionalFlow {
     if (extraFlowLogic.isPresent()) {
       extraFlowLogic.get().performAdditionalDomainDeleteLogic(
           existingResource, clientId, now, eppInput, historyEntry);
-      extraFlowLogic.get().commitAdditionalLogicChanges();
     }
   }
 

java/google/registry/flows/domain/DomainRenewFlow.java

@@ -161,7 +161,6 @@ public final class DomainRenewFlow extends Flow implements TransactionalFlow {
     if (extraFlowLogic.isPresent()) {
       extraFlowLogic.get().performAdditionalDomainRenewLogic(
           existingDomain, clientId, now, years, eppInput, historyEntry);
-      extraFlowLogic.get().commitAdditionalLogicChanges();
     }
     DomainResource newDomain = existingDomain.asBuilder()
         .setRegistrationExpirationTime(newExpirationTime)

java/google/registry/flows/domain/DomainRestoreRequestFlow.java

@@ -155,7 +155,6 @@ public final class DomainRestoreRequestFlow extends Flow implements Transactiona
     if (extraFlowLogic.isPresent()) {
       extraFlowLogic.get().performAdditionalDomainRestoreLogic(
           existingDomain, clientId, now, eppInput, historyEntry);
-      extraFlowLogic.get().commitAdditionalLogicChanges();
     }
     DomainResource newDomain =
         performRestore(existingDomain, newExpirationTime, autorenewEvent, autorenewPollMessage);

java/google/registry/flows/domain/DomainTransferRequestFlow.java

@@ -372,7 +372,6 @@ public final class DomainTransferRequestFlow extends Flow implements Transaction
     if (extraFlowLogic.isPresent()) {
       extraFlowLogic.get().performAdditionalDomainTransferLogic(
           existingDomain, gainingClientId, now, years, eppInput, historyEntry);
-      extraFlowLogic.get().commitAdditionalLogicChanges();
     }
   }
 

java/google/registry/flows/domain/DomainUpdateFlow.java

@@ -333,7 +333,6 @@ public final class DomainUpdateFlow extends Flow implements TransactionalFlow {
     if (extraFlowLogic.isPresent()) {
       extraFlowLogic.get().performAdditionalDomainUpdateLogic(
           existingDomain, clientId, now, eppInput, historyEntry);
-      extraFlowLogic.get().commitAdditionalLogicChanges();
     }
   }
 }

java/google/registry/flows/domain/RegistryExtraFlowLogic.java

@@ -63,12 +63,7 @@ public interface RegistryExtraFlowLogic {
       int years,
       EppInput eppInput) throws EppException;
 
-  /**
-   * Performs additional tasks required for a create command.
-   *
-   * <p>Any changes should not be persisted to Datastore until commitAdditionalLogicChanges is
-   * called.
-   */
+  /** Performs additional tasks required for a create command. */
   public void performAdditionalDomainCreateLogic(
       DomainResource domain,
       String clientId,
@@ -77,12 +72,7 @@ public interface RegistryExtraFlowLogic {
       EppInput eppInput,
       HistoryEntry historyEntry) throws EppException;
 
-  /**
-   * Performs additional tasks required for a delete command.
-   *
-   * <p>Any changes should not be persisted to Datastore until commitAdditionalLogicChanges is
-   * called.
-   */
+  /** Performs additional tasks required for a delete command. */
   public void performAdditionalDomainDeleteLogic(
       DomainResource domain,
       String clientId,
@@ -102,12 +92,7 @@ public interface RegistryExtraFlowLogic {
       int years,
       EppInput eppInput) throws EppException;
 
-  /**
-   * Performs additional tasks required for a renew command.
-   *
-   * <p>Any changes should not be persisted to Datastore until commitAdditionalLogicChanges is
-   * called.
-   */
+  /** Performs additional tasks required for a renew command. */
   public void performAdditionalDomainRenewLogic(
       DomainResource domain,
       String clientId,
@@ -116,12 +101,7 @@ public interface RegistryExtraFlowLogic {
       EppInput eppInput,
       HistoryEntry historyEntry) throws EppException;
 
-  /**
-   * Performs additional tasks required for a restore command.
-   *
-   * <p>Any changes should not be persisted to Datastore until commitAdditionalLogicChanges is
-   * called.
-   */
+  /** Performs additional tasks required for a restore command. */
   public void performAdditionalDomainRestoreLogic(
       DomainResource domain,
       String clientId,
@@ -129,12 +109,7 @@ public interface RegistryExtraFlowLogic {
       EppInput eppInput,
       HistoryEntry historyEntry) throws EppException;
 
-  /**
-   * Performs additional tasks required for a transfer command.
-   *
-   * <p>Any changes should not be persisted to Datastore until commitAdditionalLogicChanges is
-   * called.
-   */
+  /** Performs additional tasks required for a transfer command. */
   public void performAdditionalDomainTransferLogic(
       DomainResource domain,
       String clientId,
@@ -154,19 +129,11 @@ public interface RegistryExtraFlowLogic {
       DateTime asOfDate,
       EppInput eppInput) throws EppException;
 
-  /**
-   * Performs additional tasks required for an update command.
-   *
-   * <p>Any changes should not be persisted to Datastore until commitAdditionalLogicChanges is
-   * called.
-   */
+  /** Performs additional tasks required for an update command. */
   public void performAdditionalDomainUpdateLogic(
       DomainResource domain,
       String clientId,
       DateTime asOfDate,
       EppInput eppInput,
       HistoryEntry historyEntry) throws EppException;
-
-  /** Commits any changes made as a result of a call to one of the performXXX methods. */
-  public void commitAdditionalLogicChanges();
 }

javatests/google/registry/flows/FlowTestCase.java

@@ -118,7 +118,7 @@ public abstract class FlowTestCase<F extends Flow> extends ShardableTestCase {
   }
 
   /** Returns the EPP data loaded by a previous call to setEppInput. */
-  protected EppInput getEppInput() throws Exception {
+  protected EppInput getEppInput() throws EppException {
     return eppLoader.getEpp();
   }
 

javatests/google/registry/flows/dotapp/testdata/domain_update_restore_request.xml

@@ -0,0 +1,17 @@
+<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>example.tld</domain:name>
+        <domain:chg/>
+      </domain:update>
+    </update>
+    <extension>
+      <rgp:update xmlns:rgp="urn:ietf:params:xml:ns:rgp-1.0">
+        <rgp:restore op="request"/>
+      </rgp:update>
+    </extension>
+    <clTRID>ABC-12345</clTRID>
+  </command>
+</epp>

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

@@ -15,7 +15,6 @@
 package google.registry.model.domain;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.base.Ascii;
 import com.google.common.base.Joiner;
@@ -43,11 +42,9 @@ import org.joda.time.DateTime;
  */
 public class TestExtraLogicManager implements RegistryExtraFlowLogic {
 
-  private String messageToThrow = null;
-
   /**
-   * Dummy exception used to signal success. This is thrown by the commitAdditionalLogicChanges
-   * method to indicate to the test that everything worked properly, because the
+   * Dummy exception used to signal success. This is thrown by the performAdditionalXXXLogic()
+   * methods to indicate to the test that everything worked properly, because the
    * TestExtraLogicManager instance used by the flow will have been created deep in the flow and is
    * not accessible to the test code directly.  We should fix this when we make the extra flow logic
    * injected.
@@ -116,7 +113,7 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
     if (flags == null) {
       return;
     }
-    messageToThrow = Joiner.on(',').join(flags.getFlags());
+    throw new TestExtraLogicManagerSuccessException(Joiner.on(',').join(flags.getFlags()));
   }
 
   /** Computes the expected create cost, for use in fee challenges and the like. */
@@ -130,10 +127,7 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
     return domainNameToFeeOrCredit(domainName);
   }
 
-  /**
-   * Performs additional tasks required for a create command. Any changes should not be persisted to
-   * Datastore until commitAdditionalLogicChanges is called.
-   */
+  /** Performs additional tasks required for a create command. */
   @Override
   public void performAdditionalDomainCreateLogic(
       DomainResource domain,
@@ -147,13 +141,10 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
     if (flags == null) {
       return;
     }
-    messageToThrow = Joiner.on(',').join(flags.getFlags());
+    throw new TestExtraLogicManagerSuccessException(Joiner.on(',').join(flags.getFlags()));
   }
 
-  /**
-   * Performs additional tasks required for a delete command. Any changes should not be persisted to
-   * Datastore until commitAdditionalLogicChanges is called.
-   */
+  /** Performs additional tasks required for a delete command. */
   @Override
   public void performAdditionalDomainDeleteLogic(
       DomainResource domainResource,
@@ -161,7 +152,7 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
       DateTime asOfDate,
       EppInput eppInput,
       HistoryEntry historyEntry) throws EppException {
-    messageToThrow = "deleted";
+    throw new TestExtraLogicManagerSuccessException("deleted");
   }
 
   /** Computes the expected renewal cost, for use in fee challenges and the like. */
@@ -175,10 +166,7 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
     return domainNameToFeeOrCredit(domain.getFullyQualifiedDomainName());
   }
 
-  /**
-   * Performs additional tasks required for a renew command. Any changes should not be persisted
-   * to Datastore until commitAdditionalLogicChanges is called.
-   */
+  /** Performs additional tasks required for a renew command. */
   @Override
   public void performAdditionalDomainRenewLogic(
       DomainResource domainResource,
@@ -187,13 +175,10 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
       int years,
       EppInput eppInput,
       HistoryEntry historyEntry) throws EppException {
-    messageToThrow = "renewed";
+    throw new TestExtraLogicManagerSuccessException("renewed");
   }
 
-  /**
-   * Performs additional tasks required for a restore command. Any changes should not be persisted
-   * to Datastore until commitAdditionalLogicChanges is called.
-   */
+  /** Performs additional tasks required for a restore command. */
   @Override
   public void performAdditionalDomainRestoreLogic(
       DomainResource domainResource,
@@ -201,13 +186,10 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
       DateTime asOfDate,
       EppInput eppInput,
       HistoryEntry historyEntry) throws EppException {
-    messageToThrow = "restored";
+    throw new TestExtraLogicManagerSuccessException("restored");
   }
 
-  /**
-   * Performs additional tasks required for a transfer command. Any changes should not be persisted
-   * to Datastore until commitAdditionalLogicChanges is called.
-   */
+  /** Performs additional tasks required for a transfer command. */
   @Override
   public void performAdditionalDomainTransferLogic(
       DomainResource domainResource,
@@ -221,11 +203,11 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
     if (flags == null) {
       return;
     }
-    messageToThrow =
+    throw new TestExtraLogicManagerSuccessException(
         "add:"
         + Joiner.on(',').join(flags.getAddFlags().getFlags())
         + ";remove:"
-        + Joiner.on(',').join(flags.getRemoveFlags().getFlags());
+        + Joiner.on(',').join(flags.getRemoveFlags().getFlags()));
   }
 
   /** Computes the expected update cost, for use in fee challenges and the like. */
@@ -238,10 +220,7 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
     return domainNameToFeeOrCredit(domain.getFullyQualifiedDomainName());
   }
 
-  /**
-   * Performs additional tasks required for an update command. Any changes should not be persisted
-   * to Datastore until commitAdditionalLogicChanges is called.
-   */
+  /** Performs additional tasks required for an update command. */
   @Override
   public void performAdditionalDomainUpdateLogic(
       DomainResource domainResource,
@@ -254,17 +233,10 @@ public class TestExtraLogicManager implements RegistryExtraFlowLogic {
     if (flags == null) {
       return;
     }
-    messageToThrow =
+    throw new TestExtraLogicManagerSuccessException(
         "add:"
         + Joiner.on(',').join(flags.getAddFlags().getFlags())
         + ";remove:"
-        + Joiner.on(',').join(flags.getRemoveFlags().getFlags());
-  }
-
-  @Override
-  public void commitAdditionalLogicChanges() {
-    checkNotNull(messageToThrow);
-    // Throw a specific exception as a signal to the test code that we made it through to here.
-    throw new TestExtraLogicManagerSuccessException(messageToThrow);
+        + Joiner.on(',').join(flags.getRemoveFlags().getFlags()));
   }
 }

javatests/google/registry/testing/EppLoader.java

@@ -19,6 +19,7 @@ import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.collect.ImmutableMap;
+import google.registry.flows.EppException;
 import google.registry.model.eppinput.EppInput;
 import java.util.Map;
 
@@ -35,7 +36,7 @@ public class EppLoader {
     this.eppXml = loadFileWithSubstitutions(context.getClass(), eppXmlFilename, substitutions);
   }
 
-  public EppInput getEpp() throws Exception {
+  public EppInput getEpp() throws EppException {
     return unmarshal(EppInput.class, eppXml.getBytes(UTF_8));
   }