Add more absent clTrid unit tests

In RFC 5730, clTrid is specified as optional. We ran into an error earlier this
year in which a registrar was not passing a client transaction id and we didn't
handle it correctly. So, this CL adds some tests of common EPP operations verify
that they work correctly when the clTrid is not specified.

This also slightly improves some flow logic to make it more obvious at first
glance that clTrid is indeed optional.

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

java/google/registry/flows/FlowModule.java

@@ -156,7 +156,8 @@ public class FlowModule {
   @FlowScope
   Trid provideTrid(EppInput eppInput, ServerTridProvider serverTridProvider) {
     return Trid.create(
-        eppInput.getCommandWrapper().getClTrid(), serverTridProvider.createServerTrid());
+        eppInput.getCommandWrapper().getClTrid().orElse(null),
+        serverTridProvider.createServerTrid());
   }
 
   @Provides

java/google/registry/flows/async/AsyncFlowEnqueuer.java

@@ -124,9 +124,8 @@ public final class AsyncFlowEnqueuer {
             .param(PARAM_SERVER_TRANSACTION_ID, trid.getServerTransactionId())
             .param(PARAM_IS_SUPERUSER, Boolean.toString(isSuperuser))
             .param(PARAM_REQUESTED_TIME, now.toString());
-    if (trid.getClientTransactionId().isPresent()) {
+    trid.getClientTransactionId()
-      task.param(PARAM_CLIENT_TRANSACTION_ID, trid.getClientTransactionId().get());
+        .ifPresent(clTrid -> task.param(PARAM_CLIENT_TRANSACTION_ID, clTrid));
-    }
     addTaskToQueueWithRetry(asyncDeletePullQueue, task);
   }
 

java/google/registry/model/eppcommon/Trid.java

@@ -16,7 +16,6 @@ package google.registry.model.eppcommon;
 
 import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.googlecode.objectify.annotation.Embed;
 import google.registry.model.ImmutableObject;
 import java.util.Optional;
@@ -40,6 +39,7 @@ public class Trid extends ImmutableObject {
 
   /** The client transaction id, if provided by the client, otherwise null. */
   @XmlElement(name = "clTRID", namespace = "urn:ietf:params:xml:ns:epp-1.0")
+  @Nullable
   String clientTransactionId;
 
   public String getServerTransactionId() {
@@ -50,7 +50,6 @@ public class Trid extends ImmutableObject {
     return Optional.ofNullable(clientTransactionId);
   }
 
-  @VisibleForTesting
   public static Trid create(@Nullable String clientTransactionId, String serverTransactionId) {
     checkArgumentNotNull(serverTransactionId, "serverTransactionId cannot be null");
     Trid instance = new Trid();

java/google/registry/model/eppinput/EppInput.java

@@ -366,10 +366,15 @@ public class EppInput extends ImmutableObject {
     @XmlElementWrapper
     List<CommandExtension> extension;
 
-    String clTRID;
+    @Nullable String clTRID;
 
-    public String getClTrid() {
+    /**
-      return clTRID;
+     * Returns the client transaction ID.
+     *
+     * <p>This is optional (i.e. it may not be specified) per RFC 5730.
+     */
+    public Optional<String> getClTrid() {
+      return Optional.ofNullable(clTRID);
     }
 
     public InnerCommand getCommand() {

javatests/google/registry/flows/FlowTestCase.java

@@ -63,6 +63,7 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Function;
+import javax.annotation.Nullable;
 import org.joda.time.DateTime;
 import org.junit.Before;
 import org.junit.Rule;
@@ -141,8 +142,9 @@ public abstract class FlowTestCase<F extends Flow> extends ShardableTestCase {
     return TestDataHelper.loadFile(getClass(), filename, substitutions);
   }
 
+  @Nullable
   protected String getClientTrid() throws Exception {
-    return eppLoader.getEpp().getCommandWrapper().getClTrid();
+    return eppLoader.getEpp().getCommandWrapper().getClTrid().orElse(null);
   }
 
   /** Gets the client ID that the flow will run as. */

javatests/google/registry/flows/ResourceFlowTestCase.java

@@ -170,9 +170,8 @@ public abstract class ResourceFlowTestCase<F extends Flow, R extends EppResource
         .param("serverTransactionId", trid.getServerTransactionId())
         .param("isSuperuser", Boolean.toString(isSuperuser))
         .param("requestedTime", clock.nowUtc().toString());
-    if (trid.getClientTransactionId().isPresent()) {
+    trid.getClientTransactionId()
-      expected.param("clientTransactionId", trid.getClientTransactionId().get());
+        .ifPresent(clTrid -> expected.param("clientTransactionId", clTrid));
-    }
     assertTasksEnqueued("async-delete-pull", expected);
   }
 

javatests/google/registry/flows/domain/DomainCheckFlowTest.java

@@ -114,6 +114,16 @@ public class DomainCheckFlowTest
         create(true, "example3.tld", null));
   }
 
+  @Test
+  public void testSuccess_clTridNotSpecified() throws Exception {
+    setEppInput("domain_check_no_cltrid.xml");
+    persistActiveDomain("example1.tld");
+    doCheckTest(
+        create(false, "example1.tld", "In use"),
+        create(true, "example2.tld", null),
+        create(true, "example3.tld", null));
+  }
+
   @Test
   public void testSuccess_oneExists_allocationTokenIsInvalid() throws Exception {
     setEppInput("domain_check_allocationtoken.xml");

javatests/google/registry/flows/domain/DomainCreateFlowTest.java

@@ -393,6 +393,13 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase<DomainCreateFlow,
     doSuccessfulTest();
   }
 
+  @Test
+  public void testSuccess_clTridNotSpecified() throws Exception {
+    setEppInput("domain_create_no_cltrid.xml");
+    persistContactsAndHosts();
+    doSuccessfulTest("tld", "domain_create_response_no_cltrid.xml");
+  }
+
   @Test
   public void testFailure_invalidAllocationToken() {
     setEppInput("domain_create_allocationtoken.xml");

javatests/google/registry/flows/domain/DomainInfoFlowTest.java

@@ -155,6 +155,12 @@ public class DomainInfoFlowTest extends ResourceFlowTestCase<DomainInfoFlow, Dom
     doSuccessfulTest("domain_info_response.xml");
   }
 
+  @Test
+  public void testSuccess_clTridNotSpecified() throws Exception {
+    setEppInput("domain_info_no_cltrid.xml");
+    doSuccessfulTest("domain_info_response_no_cltrid.xml");
+  }
+
   @Test
   public void testSuccess_allHosts_noDelegatedHosts() throws Exception {
     // There aren't any delegated hosts.

javatests/google/registry/flows/domain/DomainUpdateFlowTest.java

@@ -107,14 +107,11 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
   ContactResource unusedContact;
   HistoryEntry historyEntryDomainCreate;
 
-  public DomainUpdateFlowTest() {
-    // Note that "domain_update.xml" tests adding and removing the same contact type.
-    setEppInput("domain_update.xml");
-  }
-
   @Before
   public void initDomainTest() {
     createTld("tld");
+    // Note that "domain_update.xml" tests adding and removing the same contact type.
+    setEppInput("domain_update.xml");
   }
 
   private void persistReferencedEntities() {
@@ -149,8 +146,12 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
   }
 
   private void doSuccessfulTest() throws Exception {
+    doSuccessfulTest("domain_update_response.xml");
+  }
+
+  private void doSuccessfulTest(String expectedXmlFilename) throws Exception {
     assertTransactionalFlow(true);
-    runFlowAssertResponse(loadFile("domain_update_response.xml"));
+    runFlowAssertResponse(loadFile(expectedXmlFilename));
     // Check that the domain was updated. These values came from the xml.
     assertAboutDomains()
         .that(reloadResourceByForeignKey())
@@ -178,6 +179,14 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
     doSuccessfulTest();
   }
 
+  @Test
+  public void testSuccess_clTridNotSpecified() throws Exception {
+    setEppInput("domain_update_no_cltrid.xml");
+    persistReferencedEntities();
+    persistDomain();
+    doSuccessfulTest("generic_success_response_no_cltrid.xml");
+  }
+
   @Test
   public void testSuccess_cachingDisabled() throws Exception {
     RegistryConfig.overrideIsEppResourceCachingEnabledForTesting(false);

javatests/google/registry/flows/domain/testdata/domain_check_no_cltrid.xml

@@ -0,0 +1,11 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <check>
+      <domain:check xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>example1.tld</domain:name>
+        <domain:name>example2.tld</domain:name>
+        <domain:name>example3.tld</domain:name>
+      </domain:check>
+    </check>
+  </command>
+</epp>

javatests/google/registry/flows/domain/testdata/domain_create_no_cltrid.xml

@@ -0,0 +1,21 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <create>
+      <domain:create
+       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>example.tld</domain:name>
+        <domain:period unit="y">2</domain:period>
+        <domain:ns>
+          <domain:hostObj>ns1.example.net</domain:hostObj>
+          <domain:hostObj>ns2.example.net</domain:hostObj>
+        </domain:ns>
+        <domain:registrant>jd1234</domain:registrant>
+        <domain:contact type="admin">sh8013</domain:contact>
+        <domain:contact type="tech">sh8013</domain:contact>
+        <domain:authInfo>
+          <domain:pw>2fooBAR</domain:pw>
+        </domain:authInfo>
+      </domain:create>
+    </create>
+  </command>
+</epp>

javatests/google/registry/flows/domain/testdata/domain_create_response_no_cltrid.xml

@@ -0,0 +1,18 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <response>
+    <result code="1000">
+      <msg>Command completed successfully</msg>
+    </result>
+    <resData>
+      <domain:creData
+       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>example.tld</domain:name>
+        <domain:crDate>1999-04-03T22:00:00.0Z</domain:crDate>
+        <domain:exDate>2001-04-03T22:00:00.0Z</domain:exDate>
+      </domain:creData>
+    </resData>
+    <trID>
+      <svTRID>server-trid</svTRID>
+    </trID>
+  </response>
+</epp>

javatests/google/registry/flows/domain/testdata/domain_info_no_cltrid.xml

@@ -0,0 +1,10 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <info>
+      <domain:info
+       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name hosts="all">example.tld</domain:name>
+      </domain:info>
+    </info>
+  </command>
+</epp>

javatests/google/registry/flows/domain/testdata/domain_info_response_no_cltrid.xml

@@ -0,0 +1,37 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <response>
+    <result code="1000">
+      <msg>Command completed successfully</msg>
+    </result>
+    <resData>
+      <domain:infData
+       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>example.tld</domain:name>
+        <domain:roid>%ROID%</domain:roid>
+        <domain:status s="ok"/>
+        <domain:registrant>jd1234</domain:registrant>
+        <domain:contact type="admin">sh8013</domain:contact>
+        <domain:contact type="tech">sh8013</domain:contact>
+        <domain:ns>
+          <domain:hostObj>ns1.example.tld</domain:hostObj>
+          <domain:hostObj>ns1.example.net</domain:hostObj>
+        </domain:ns>
+        <domain:host>ns1.example.tld</domain:host>
+        <domain:host>ns2.example.tld</domain:host>
+        <domain:clID>NewRegistrar</domain:clID>
+        <domain:crID>TheRegistrar</domain:crID>
+        <domain:crDate>1999-04-03T22:00:00.0Z</domain:crDate>
+        <domain:upID>NewRegistrar</domain:upID>
+        <domain:upDate>1999-12-03T09:00:00.0Z</domain:upDate>
+        <domain:exDate>2005-04-03T22:00:00.0Z</domain:exDate>
+        <domain:trDate>2000-04-08T09:00:00.0Z</domain:trDate>
+        <domain:authInfo>
+          <domain:pw>2fooBAR</domain:pw>
+        </domain:authInfo>
+      </domain:infData>
+    </resData>
+    <trID>
+      <svTRID>server-trid</svTRID>
+    </trID>
+  </response>
+</epp>

javatests/google/registry/flows/domain/testdata/domain_update_no_cltrid.xml

@@ -0,0 +1,31 @@
+<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:add>
+          <domain:ns>
+            <domain:hostObj>ns2.example.foo</domain:hostObj>
+          </domain:ns>
+          <domain:contact type="tech">mak21</domain:contact>
+          <domain:status s="clientHold"
+           lang="en">Payment overdue.</domain:status>
+        </domain:add>
+        <domain:rem>
+          <domain:ns>
+            <domain:hostObj>ns1.example.foo</domain:hostObj>
+          </domain:ns>
+          <domain:contact type="tech">sh8013</domain:contact>
+          <domain:status s="clientUpdateProhibited"/>
+        </domain:rem>
+        <domain:chg>
+          <domain:registrant>sh8013</domain:registrant>
+          <domain:authInfo>
+            <domain:pw>2BARfoo</domain:pw>
+          </domain:authInfo>
+        </domain:chg>
+      </domain:update>
+    </update>
+  </command>
+</epp>

javatests/google/registry/flows/domain/testdata/generic_success_response_no_cltrid.xml

@@ -0,0 +1,10 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <response>
+    <result code="1000">
+      <msg>Command completed successfully</msg>
+    </result>
+    <trID>
+      <svTRID>server-trid</svTRID>
+    </trID>
+  </response>
+</epp>

javatests/google/registry/flows/poll/PollRequestFlowTest.java

@@ -59,28 +59,42 @@ public class PollRequestFlowTest extends FlowTestCase<PollRequestFlow> {
     host = persistActiveHost("ns1.test.example");
   }
 
-  @Test
+  private void persistPendingTransferPollMessage() {
-  public void testSuccess_domainTransferApproved() throws Exception {
     persistResource(
         new PollMessage.OneTime.Builder()
             .setClientId(getClientIdForFlow())
             .setEventTime(clock.nowUtc().minusDays(1))
             .setMsg("Transfer approved.")
-            .setResponseData(ImmutableList.of(new DomainTransferResponse.Builder()
+            .setResponseData(
-                .setFullyQualifiedDomainName("test.example")
+                ImmutableList.of(
-                .setTransferStatus(TransferStatus.SERVER_APPROVED)
+                    new DomainTransferResponse.Builder()
-                .setGainingClientId(getClientIdForFlow())
+                        .setFullyQualifiedDomainName("test.example")
-                .setTransferRequestTime(clock.nowUtc().minusDays(5))
+                        .setTransferStatus(TransferStatus.SERVER_APPROVED)
-                .setLosingClientId("TheRegistrar")
+                        .setGainingClientId(getClientIdForFlow())
-                .setPendingTransferExpirationTime(clock.nowUtc().minusDays(1))
+                        .setTransferRequestTime(clock.nowUtc().minusDays(5))
-                .setExtendedRegistrationExpirationTime(clock.nowUtc().plusYears(1))
+                        .setLosingClientId("TheRegistrar")
-                .build()))
+                        .setPendingTransferExpirationTime(clock.nowUtc().minusDays(1))
+                        .setExtendedRegistrationExpirationTime(clock.nowUtc().plusYears(1))
+                        .build()))
             .setParent(createHistoryEntryForEppResource(domain))
             .build());
+  }
+
+  @Test
+  public void testSuccess_domainTransferApproved() throws Exception {
+    persistPendingTransferPollMessage();
     assertTransactionalFlow(false);
     runFlowAssertResponse(loadFile("poll_response_domain_transfer.xml"));
   }
 
+  @Test
+  public void testSuccess_clTridNotSpecified() throws Exception {
+    setEppInput("poll_no_cltrid.xml");
+    persistPendingTransferPollMessage();
+    assertTransactionalFlow(false);
+    runFlowAssertResponse(loadFile("poll_response_domain_transfer_no_cltrid.xml"));
+  }
+
   @Test
   public void testSuccess_contactTransferPending() throws Exception {
     clock.setTo(DateTime.parse("2000-06-13T22:00:00.0Z"));

javatests/google/registry/flows/poll/testdata/poll_no_cltrid.xml

@@ -0,0 +1,5 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <poll op="req"/>
+  </command>
+</epp>

javatests/google/registry/flows/poll/testdata/poll_response_domain_transfer_no_cltrid.xml

@@ -0,0 +1,25 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <response>
+    <result code="1301">
+      <msg>Command completed successfully; ack to dequeue</msg>
+    </result>
+    <msgQ count="1" id="1-3-EXAMPLE-5-6-2011">
+      <qDate>2011-01-01T01:01:01Z</qDate>
+      <msg>Transfer approved.</msg>
+    </msgQ>
+    <resData>
+      <domain:trnData xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>test.example</domain:name>
+        <domain:trStatus>serverApproved</domain:trStatus>
+        <domain:reID>NewRegistrar</domain:reID>
+        <domain:reDate>2010-12-28T01:01:01Z</domain:reDate>
+        <domain:acID>TheRegistrar</domain:acID>
+        <domain:acDate>2011-01-01T01:01:01Z</domain:acDate>
+        <domain:exDate>2012-01-02T01:01:01Z</domain:exDate>
+      </domain:trnData>
+    </resData>
+    <trID>
+      <svTRID>server-trid</svTRID>
+    </trID>
+  </response>
+</epp>

javatests/google/registry/model/eppinput/EppInputTest.java

@@ -35,7 +35,7 @@ public class EppInputTest {
   public void testUnmarshalling_contactInfo() throws Exception {
     EppInput input =
         unmarshal(EppInput.class, loadBytes(ContactResourceTest.class, "contact_info.xml").read());
-    assertThat(input.getCommandWrapper().getClTrid()).isEqualTo("ABC-12345");
+    assertThat(input.getCommandWrapper().getClTrid()).hasValue("ABC-12345");
     assertThat(input.getCommandType()).isEqualTo("info");
     assertThat(input.getResourceType()).hasValue("contact");
     assertThat(input.getSingleTargetId()).hasValue("sh8013");
@@ -46,7 +46,7 @@ public class EppInputTest {
   public void testUnmarshalling_domainCheck() throws Exception {
     EppInput input =
         unmarshal(EppInput.class, loadBytes(DomainResourceTest.class, "domain_check.xml").read());
-    assertThat(input.getCommandWrapper().getClTrid()).isEqualTo("ABC-12345");
+    assertThat(input.getCommandWrapper().getClTrid()).hasValue("ABC-12345");
     assertThat(input.getCommandType()).isEqualTo("check");
     assertThat(input.getResourceType()).hasValue("domain");
     assertThat(input.getSingleTargetId()).isEmpty();
@@ -56,7 +56,7 @@ public class EppInputTest {
   @Test
   public void testUnmarshalling_login() throws Exception {
     EppInput input = unmarshal(EppInput.class, loadBytes(getClass(), "login_valid.xml").read());
-    assertThat(input.getCommandWrapper().getClTrid()).isEqualTo("ABC-12345");
+    assertThat(input.getCommandWrapper().getClTrid()).hasValue("ABC-12345");
     assertThat(input.getCommandType()).isEqualTo("login");
     assertThat(input.getResourceType()).isEmpty();
     assertThat(input.getSingleTargetId()).isEmpty();