From 78a36925f4534f25f30cf28bcba949fa7da10c90 Mon Sep 17 00:00:00 2001 From: mountford Date: Fri, 28 Oct 2016 11:07:38 -0700 Subject: [PATCH] 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 --- .../domain/DomainApplicationCreateFlow.java | 1 - .../flows/domain/DomainCreateFlow.java | 1 - .../flows/domain/DomainDeleteFlow.java | 1 - .../flows/domain/DomainRenewFlow.java | 1 - .../domain/DomainRestoreRequestFlow.java | 1 - .../domain/DomainTransferRequestFlow.java | 1 - .../flows/domain/DomainUpdateFlow.java | 1 - .../flows/domain/RegistryExtraFlowLogic.java | 45 ++------------ .../google/registry/flows/FlowTestCase.java | 2 +- .../domain_update_restore_request.xml | 17 +++++ .../model/domain/TestExtraLogicManager.java | 62 +++++-------------- .../google/registry/testing/EppLoader.java | 3 +- 12 files changed, 43 insertions(+), 93 deletions(-) create mode 100644 javatests/google/registry/flows/dotapp/testdata/domain_update_restore_request.xml diff --git a/java/google/registry/flows/domain/DomainApplicationCreateFlow.java b/java/google/registry/flows/domain/DomainApplicationCreateFlow.java index aae3207ac..716e5fa20 100644 --- a/java/google/registry/flows/domain/DomainApplicationCreateFlow.java +++ b/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(); } } diff --git a/java/google/registry/flows/domain/DomainCreateFlow.java b/java/google/registry/flows/domain/DomainCreateFlow.java index e0c13b99e..812deaac3 100644 --- a/java/google/registry/flows/domain/DomainCreateFlow.java +++ b/java/google/registry/flows/domain/DomainCreateFlow.java @@ -404,7 +404,6 @@ public class DomainCreateFlow extends Flow implements TransactionalFlow { years, eppInput, historyEntry); - extraFlowLogic.get().commitAdditionalLogicChanges(); } } diff --git a/java/google/registry/flows/domain/DomainDeleteFlow.java b/java/google/registry/flows/domain/DomainDeleteFlow.java index 9435bf43a..4f60ea191 100644 --- a/java/google/registry/flows/domain/DomainDeleteFlow.java +++ b/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(); } } diff --git a/java/google/registry/flows/domain/DomainRenewFlow.java b/java/google/registry/flows/domain/DomainRenewFlow.java index 10ccd2666..79496ec5b 100644 --- a/java/google/registry/flows/domain/DomainRenewFlow.java +++ b/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) diff --git a/java/google/registry/flows/domain/DomainRestoreRequestFlow.java b/java/google/registry/flows/domain/DomainRestoreRequestFlow.java index 6b02e8d4b..f85838f06 100644 --- a/java/google/registry/flows/domain/DomainRestoreRequestFlow.java +++ b/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); diff --git a/java/google/registry/flows/domain/DomainTransferRequestFlow.java b/java/google/registry/flows/domain/DomainTransferRequestFlow.java index ac8c3b33d..89d7bc804 100644 --- a/java/google/registry/flows/domain/DomainTransferRequestFlow.java +++ b/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(); } } diff --git a/java/google/registry/flows/domain/DomainUpdateFlow.java b/java/google/registry/flows/domain/DomainUpdateFlow.java index a530b3255..30ec444bb 100644 --- a/java/google/registry/flows/domain/DomainUpdateFlow.java +++ b/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(); } } } diff --git a/java/google/registry/flows/domain/RegistryExtraFlowLogic.java b/java/google/registry/flows/domain/RegistryExtraFlowLogic.java index 088e7749e..a39eff57d 100644 --- a/java/google/registry/flows/domain/RegistryExtraFlowLogic.java +++ b/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. - * - *

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. - * - *

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. - * - *

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. - * - *

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. - * - *

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. - * - *

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(); } diff --git a/javatests/google/registry/flows/FlowTestCase.java b/javatests/google/registry/flows/FlowTestCase.java index 13f4345a2..ef6e048a9 100644 --- a/javatests/google/registry/flows/FlowTestCase.java +++ b/javatests/google/registry/flows/FlowTestCase.java @@ -118,7 +118,7 @@ public abstract class FlowTestCase 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(); } diff --git a/javatests/google/registry/flows/dotapp/testdata/domain_update_restore_request.xml b/javatests/google/registry/flows/dotapp/testdata/domain_update_restore_request.xml new file mode 100644 index 000000000..124bc0b89 --- /dev/null +++ b/javatests/google/registry/flows/dotapp/testdata/domain_update_restore_request.xml @@ -0,0 +1,17 @@ + + + + + example.tld + + + + + + + + + ABC-12345 + + diff --git a/javatests/google/registry/model/domain/TestExtraLogicManager.java b/javatests/google/registry/model/domain/TestExtraLogicManager.java index fa10838d6..42360d332 100644 --- a/javatests/google/registry/model/domain/TestExtraLogicManager.java +++ b/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())); } } diff --git a/javatests/google/registry/testing/EppLoader.java b/javatests/google/registry/testing/EppLoader.java index f849b862a..28275a8db 100644 --- a/javatests/google/registry/testing/EppLoader.java +++ b/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)); }