Make async flow logic handle missing client transaction IDs

Per EPP RFC 5730, the <clTRID> element is optional. However, we weren't handling
it not being specified in asynchronous contact/host deletions because we were
adding it directly as a parameter value on a task, which does not allow null and
thus threw a NullPointerException.

This fixes handling for nulls (the parameter isn't set at all) and adds a test.

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

java/google/registry/batch/DeleteContactsAndHostsAction.java

@@ -92,6 +92,7 @@ import google.registry.util.SystemClock;
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.List;
+import javax.annotation.Nullable;
 import javax.inject.Inject;
 import javax.inject.Named;
 import org.joda.time.DateTime;
@@ -379,7 +380,7 @@ public class DeleteContactsAndHostsAction implements Runnable {
         EppResource resource,
         boolean deleteAllowed,
         DateTime now) {
-      String clientTransactionId = deletionRequest.clientTransactionId();
+      @Nullable String clientTransactionId = deletionRequest.clientTransactionId();
       String serverTransactionId = deletionRequest.serverTransactionId();
       Trid trid = Trid.create(clientTransactionId, serverTransactionId);
       if (resource instanceof HostResource) {
@@ -445,6 +446,7 @@ public class DeleteContactsAndHostsAction implements Runnable {
 
     abstract Key<? extends EppResource> key();
     abstract DateTime lastUpdateTime();
+
     /**
      * The client id of the registrar that requested this deletion (which might NOT be the same as
      * the actual current owner of the resource).
@@ -452,6 +454,7 @@ public class DeleteContactsAndHostsAction implements Runnable {
     abstract String requestingClientId();
 
     /** First half of TRID for the original request, split for serializability. */
+    @Nullable
     abstract String clientTransactionId();
 
     /** Second half of TRID for the original request, split for serializability. */
@@ -467,7 +470,7 @@ public class DeleteContactsAndHostsAction implements Runnable {
       abstract Builder setKey(Key<? extends EppResource> key);
       abstract Builder setLastUpdateTime(DateTime lastUpdateTime);
       abstract Builder setRequestingClientId(String requestingClientId);
-      abstract Builder setClientTransactionId(String clientTransactionId);
+      abstract Builder setClientTransactionId(@Nullable String clientTransactionId);
       abstract Builder setServerTransactionId(String serverTransactionId);
       abstract Builder setIsSuperuser(boolean isSuperuser);
       abstract Builder setRequestedTime(DateTime requestedTime);
@@ -494,9 +497,8 @@ public class DeleteContactsAndHostsAction implements Runnable {
           .setRequestingClientId(
               checkNotNull(
                   params.get(PARAM_REQUESTING_CLIENT_ID), "Requesting client id not specified"))
-          .setClientTransactionId(
-              checkNotNull(
-                  params.get(PARAM_CLIENT_TRANSACTION_ID), "Client transaction id not specified"))
+          // Note that client transaction ID is optional, in which case this sets it to null.
+          .setClientTransactionId(params.get(PARAM_CLIENT_TRANSACTION_ID))
           .setServerTransactionId(
               checkNotNull(
                   params.get(PARAM_SERVER_TRANSACTION_ID), "Server transaction id not specified"))

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

@@ -83,10 +83,12 @@ public final class AsyncFlowEnqueuer {
             .countdownMillis(asyncDeleteDelay.getMillis())
             .param(PARAM_RESOURCE_KEY, resourceKey.getString())
             .param(PARAM_REQUESTING_CLIENT_ID, requestingClientId)
-            .param(PARAM_CLIENT_TRANSACTION_ID, trid.getClientTransactionId())
             .param(PARAM_SERVER_TRANSACTION_ID, trid.getServerTransactionId())
             .param(PARAM_IS_SUPERUSER, Boolean.toString(isSuperuser))
             .param(PARAM_REQUESTED_TIME, now.toString());
+    if (trid.getClientTransactionId().isPresent()) {
+      task.param(PARAM_CLIENT_TRANSACTION_ID, trid.getClientTransactionId().get());
+    }
     addTaskToQueueWithRetry(asyncDeletePullQueue, task);
   }
 

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

@@ -19,6 +19,7 @@ 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;
 import javax.annotation.Nullable;
 import javax.xml.bind.annotation.XmlElement;
 import javax.xml.bind.annotation.XmlType;
@@ -45,8 +46,8 @@ public class Trid extends ImmutableObject {
     return serverTransactionId;
   }
 
-  public String getClientTransactionId() {
-    return clientTransactionId;
+  public Optional<String> getClientTransactionId() {
+    return Optional.ofNullable(clientTransactionId);
   }
 
   @VisibleForTesting

java/google/registry/tools/AllocateDomainCommand.java

@@ -134,7 +134,7 @@ final class AllocateDomainCommand extends MutatingEppToolCommand {
                             "Could not find any history entries for domain application %s",
                             application.getRepoId());
                     String clientTransactionId =
-                        emptyToNull(history.getTrid().getClientTransactionId());
+                        emptyToNull(history.getTrid().getClientTransactionId().orElse(null));
                     Period period = checkNotNull(extractPeriodFromXml(history.getXmlBytes()));
                     checkArgument(period.getUnit() == Period.Unit.YEARS);
                     ImmutableMap.Builder<String, String> contactsMapBuilder =

java/google/registry/tools/GetHistoryEntriesCommand.java

@@ -77,7 +77,7 @@ final class GetHistoryEntriesCommand implements RemoteApiCommand {
           "Client: %s\nTime: %s\nClient TRID: %s\nServer TRID: %s\n%s\n",
           entry.getClientId(),
           entry.getModificationTime(),
-          (entry.getTrid() == null) ? null : entry.getTrid().getClientTransactionId(),
+          (entry.getTrid() == null) ? null : entry.getTrid().getClientTransactionId().orElse(null),
           (entry.getTrid() == null) ? null : entry.getTrid().getServerTransactionId(),
           entry.getXmlBytes() == null
               ? String.format("[no XML stored for %s]\n", entry.getType())

javatests/google/registry/batch/DeleteContactsAndHostsActionTest.java

@@ -17,6 +17,7 @@ package google.registry.batch;
 import static com.google.appengine.api.taskqueue.QueueFactory.getQueue;
 import static com.google.common.collect.MoreCollectors.onlyElement;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
 import static google.registry.flows.async.AsyncFlowEnqueuer.QUEUE_ASYNC_DELETE;
 import static google.registry.flows.async.AsyncFlowEnqueuer.QUEUE_ASYNC_HOST_RENAME;
 import static google.registry.flows.async.AsyncFlowMetrics.OperationResult.STALE;
@@ -97,6 +98,7 @@ import google.registry.testing.mapreduce.MapreduceTestCase;
 import google.registry.util.Retrier;
 import google.registry.util.Sleeper;
 import google.registry.util.SystemSleeper;
+import java.util.Optional;
 import org.joda.time.DateTime;
 import org.joda.time.Duration;
 import org.junit.Before;
@@ -182,7 +184,8 @@ public class DeleteContactsAndHostsActionTest
         "TheRegistrar",
         "Can't delete contact blah8221 because it is referenced by a domain.",
         false,
-        contact);
+        contact,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
     verify(action.asyncFlowMetrics).recordContactHostDeletionBatchSize(1L);
     verify(action.asyncFlowMetrics)
@@ -192,13 +195,23 @@ public class DeleteContactsAndHostsActionTest
 
   @Test
   public void testSuccess_contact_notReferenced_getsDeleted_andPiiWipedOut() throws Exception {
+    runSuccessfulContactDeletionTest(Optional.of("fakeClientTrid"));
+  }
+
+  @Test
+  public void testSuccess_contact_andNoClientTrid_deletesSuccessfully() throws Exception {
+    runSuccessfulContactDeletionTest(Optional.empty());
+  }
+
+
+  private void runSuccessfulContactDeletionTest(Optional<String> clientTrid) throws Exception {
     ContactResource contact = persistContactWithPii("jim919");
     DateTime timeEnqueued = clock.nowUtc();
     enqueuer.enqueueAsyncDelete(
         contact,
         timeEnqueued,
         "TheRegistrar",
-        Trid.create("fakeClientTrid", "fakeServerTrid"),
+        Trid.create(clientTrid.orElse(null), "fakeServerTrid"),
         false);
     runMapreduce();
     assertThat(loadByForeignKey(ContactResource.class, "jim919", clock.nowUtc())).isNull();
@@ -223,12 +236,14 @@ public class DeleteContactsAndHostsActionTest
         .and()
         .hasNullFaxNumber();
     HistoryEntry historyEntry = getOnlyHistoryEntryOfType(contactAfterDeletion, CONTACT_DELETE);
-    assertPollMessageFor(historyEntry, "TheRegistrar", "Deleted contact jim919.", true, contact);
+    assertPollMessageFor(
+        historyEntry, "TheRegistrar", "Deleted contact jim919.", true, contact, clientTrid);
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
     verify(action.asyncFlowMetrics).recordContactHostDeletionBatchSize(1L);
     verify(action.asyncFlowMetrics)
         .recordAsyncFlowResult(OperationType.CONTACT_DELETE, OperationResult.SUCCESS, timeEnqueued);
     verifyNoMoreInteractions(action.asyncFlowMetrics);
+
   }
 
   @Test
@@ -339,7 +354,12 @@ public class DeleteContactsAndHostsActionTest
         .hasType(CONTACT_DELETE);
     HistoryEntry historyEntry = getOnlyHistoryEntryOfType(contactBeforeDeletion, CONTACT_DELETE);
     assertPollMessageFor(
-        historyEntry, "TheRegistrar", "Deleted contact blah1234.", true, contactUsed);
+        historyEntry,
+        "TheRegistrar",
+        "Deleted contact blah1234.",
+        true,
+        contactUsed,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
   }
 
@@ -366,7 +386,8 @@ public class DeleteContactsAndHostsActionTest
         "OtherRegistrar",
         "Can't delete contact jane0991 because it was transferred prior to deletion.",
         false,
-        contact);
+        contact,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
   }
 
@@ -402,7 +423,13 @@ public class DeleteContactsAndHostsActionTest
         .and()
         .hasNullFaxNumber();
     HistoryEntry historyEntry = getOnlyHistoryEntryOfType(contactAfterDeletion, CONTACT_DELETE);
-    assertPollMessageFor(historyEntry, "OtherRegistrar", "Deleted contact nate007.", true, contact);
+    assertPollMessageFor(
+        historyEntry,
+        "OtherRegistrar",
+        "Deleted contact nate007.",
+        true,
+        contact,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
   }
 
@@ -539,7 +566,8 @@ public class DeleteContactsAndHostsActionTest
         "TheRegistrar",
         "Can't delete host ns1.example.tld because it is referenced by a domain.",
         false,
-        host);
+        host,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
     verify(action.asyncFlowMetrics).recordContactHostDeletionBatchSize(1L);
     verify(action.asyncFlowMetrics)
@@ -549,13 +577,22 @@ public class DeleteContactsAndHostsActionTest
 
   @Test
   public void testSuccess_host_notReferenced_getsDeleted() throws Exception {
+    runSuccessfulHostDeletionTest(Optional.of("fakeClientTrid"));
+  }
+
+  @Test
+  public void testSuccess_host_andNoClientTrid_deletesSuccessfully() throws Exception {
+    runSuccessfulHostDeletionTest(Optional.empty());
+  }
+
+  private void runSuccessfulHostDeletionTest(Optional<String> clientTrid) throws Exception {
     HostResource host = persistHostPendingDelete("ns2.example.tld");
     DateTime timeEnqueued = clock.nowUtc();
     enqueuer.enqueueAsyncDelete(
         host,
         timeEnqueued,
         "TheRegistrar",
-        Trid.create("fakeClientTrid", "fakeServerTrid"),
+        Trid.create(clientTrid.orElse(null), "fakeServerTrid"),
         false);
     runMapreduce();
     assertThat(loadByForeignKey(HostResource.class, "ns2.example.tld", clock.nowUtc())).isNull();
@@ -572,7 +609,13 @@ public class DeleteContactsAndHostsActionTest
         .hasOnlyOneHistoryEntryWhich()
         .hasType(HOST_DELETE);
     HistoryEntry historyEntry = getOnlyHistoryEntryOfType(hostBeforeDeletion, HOST_DELETE);
-    assertPollMessageFor(historyEntry, "TheRegistrar", "Deleted host ns2.example.tld.", true, host);
+    assertPollMessageFor(
+        historyEntry,
+        "TheRegistrar",
+        "Deleted host ns2.example.tld.",
+        true,
+        host,
+        clientTrid);
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
     verify(action.asyncFlowMetrics).recordContactHostDeletionBatchSize(1L);
     verify(action.asyncFlowMetrics)
@@ -610,7 +653,13 @@ public class DeleteContactsAndHostsActionTest
         .hasOnlyOneHistoryEntryWhich()
         .hasType(HOST_DELETE);
     HistoryEntry historyEntry = getOnlyHistoryEntryOfType(hostBeforeDeletion, HOST_DELETE);
-    assertPollMessageFor(historyEntry, "TheRegistrar", "Deleted host ns1.example.tld.", true, host);
+    assertPollMessageFor(
+        historyEntry,
+        "TheRegistrar",
+        "Deleted host ns1.example.tld.",
+        true,
+        host,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
   }
 
@@ -654,7 +703,13 @@ public class DeleteContactsAndHostsActionTest
         .hasOnlyOneHistoryEntryWhich()
         .hasType(HOST_DELETE);
     HistoryEntry historyEntry = getOnlyHistoryEntryOfType(hostBeforeDeletion, HOST_DELETE);
-    assertPollMessageFor(historyEntry, "TheRegistrar", "Deleted host ns2.example.tld.", true, host);
+    assertPollMessageFor(
+        historyEntry,
+        "TheRegistrar",
+        "Deleted host ns2.example.tld.",
+        true,
+        host,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
   }
 
@@ -681,7 +736,8 @@ public class DeleteContactsAndHostsActionTest
         "OtherRegistrar",
         "Can't delete host ns2.example.tld because it was transferred prior to deletion.",
         false,
-        host);
+        host,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
   }
 
@@ -710,7 +766,12 @@ public class DeleteContactsAndHostsActionTest
         .hasType(HOST_DELETE);
     HistoryEntry historyEntry = getOnlyHistoryEntryOfType(hostBeforeDeletion, HOST_DELETE);
     assertPollMessageFor(
-        historyEntry, "OtherRegistrar", "Deleted host ns66.example.tld.", true, host);
+        historyEntry,
+        "OtherRegistrar",
+        "Deleted host ns66.example.tld.",
+        true,
+        host,
+        Optional.of("fakeClientTrid"));
     assertNoTasksEnqueued(QUEUE_ASYNC_DELETE);
   }
 
@@ -784,7 +845,8 @@ public class DeleteContactsAndHostsActionTest
       String clientId,
       String msg,
       boolean expectedActionResult,
-      EppResource resource) {
+      EppResource resource,
+      Optional<String> clientTrid) {
     PollMessage.OneTime pollMessage = (OneTime) getOnlyPollMessageForHistoryEntry(historyEntry);
     assertThat(pollMessage.getMsg()).isEqualTo(msg);
     assertThat(pollMessage.getClientId()).isEqualTo(clientId);
@@ -806,7 +868,7 @@ public class DeleteContactsAndHostsActionTest
     assertThat(pendingResponse.getActionResult()).isEqualTo(expectedActionResult);
     assertThat(pendingResponse.getNameAsString()).isEqualTo(expectedResourceName);
     Trid trid = pendingResponse.getTrid();
-    assertThat(trid.getClientTransactionId()).isEqualTo("fakeClientTrid");
+    assertThat(trid.getClientTransactionId()).isEqualTo(clientTrid);
     assertThat(trid.getServerTransactionId()).isEqualTo("fakeServerTrid");
   }
 

javatests/google/registry/flows/ResourceFlowTestCase.java

@@ -163,16 +163,17 @@ public abstract class ResourceFlowTestCase<F extends Flow, R extends EppResource
   /** Asserts the presence of a single enqueued async contact or host deletion */
   protected <T extends EppResource> void assertAsyncDeletionTaskEnqueued(
       T resource, String requestingClientId, Trid trid, boolean isSuperuser) throws Exception {
-    assertTasksEnqueued(
-        "async-delete-pull",
-        new TaskMatcher()
+    TaskMatcher expected = new TaskMatcher()
         .etaDelta(Duration.standardSeconds(75), Duration.standardSeconds(105)) // expected: 90
         .param("resourceKey", Key.create(resource).getString())
         .param("requestingClientId", requestingClientId)
-            .param("clientTransactionId", trid.getClientTransactionId())
         .param("serverTransactionId", trid.getServerTransactionId())
         .param("isSuperuser", Boolean.toString(isSuperuser))
-            .param("requestedTime", clock.nowUtc().toString()));
+        .param("requestedTime", clock.nowUtc().toString());
+    if (trid.getClientTransactionId().isPresent()) {
+      expected.param("clientTransactionId", trid.getClientTransactionId().get());
+    }
+    assertTasksEnqueued("async-delete-pull", expected);
   }
 
 

javatests/google/registry/flows/contact/ContactDeleteFlowTest.java

@@ -65,7 +65,26 @@ public class ContactDeleteFlowTest
     assertAboutContacts().that(deletedContact).hasStatusValue(StatusValue.PENDING_DELETE);
     assertAsyncDeletionTaskEnqueued(
         deletedContact, "TheRegistrar", Trid.create("ABC-12345", "server-trid"), false);
-    assertAboutContacts().that(deletedContact)
+    assertAboutContacts()
+        .that(deletedContact)
+        .hasOnlyOneHistoryEntryWhich()
+        .hasType(HistoryEntry.Type.CONTACT_PENDING_DELETE);
+    assertNoBillingEvents();
+  }
+
+  @Test
+  public void testSuccess_clTridNotSpecified() throws Exception {
+    setEppInput("contact_delete_no_cltrid.xml");
+    persistActiveContact(getUniqueIdFromCommand());
+    clock.advanceOneMilli();
+    assertTransactionalFlow(true);
+    runFlowAssertResponse(loadFile("contact_delete_response_no_cltrid.xml"));
+    ContactResource deletedContact = reloadResourceByForeignKey();
+    assertAboutContacts().that(deletedContact).hasStatusValue(StatusValue.PENDING_DELETE);
+    assertAsyncDeletionTaskEnqueued(
+        deletedContact, "TheRegistrar", Trid.create(null, "server-trid"), false);
+    assertAboutContacts()
+        .that(deletedContact)
         .hasOnlyOneHistoryEntryWhich()
         .hasType(HistoryEntry.Type.CONTACT_PENDING_DELETE);
     assertNoBillingEvents();

javatests/google/registry/flows/contact/testdata/contact_delete_no_cltrid.xml

@@ -0,0 +1,10 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <delete>
+      <contact:delete
+       xmlns:contact="urn:ietf:params:xml:ns:contact-1.0">
+        <contact:id>sh8013</contact:id>
+      </contact:delete>
+    </delete>
+  </command>
+</epp>

javatests/google/registry/flows/contact/testdata/contact_delete_response_no_cltrid.xml

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

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

@@ -17,6 +17,7 @@ package google.registry.flows.domain;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.MoreCollectors.onlyElement;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.reporting.DomainTransactionRecord.TransactionReportField.TRANSFER_SUCCESSFUL;
 import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_CREATE;
@@ -344,7 +345,7 @@ public class DomainTransferRequestFlowTest
             .filter(PendingActionNotificationResponse.class::isInstance)
             .map(PendingActionNotificationResponse.class::cast)
             .collect(onlyElement());
-    assertThat(panData.getTrid().getClientTransactionId()).isEqualTo("ABC-12345");
+    assertThat(panData.getTrid().getClientTransactionId()).hasValue("ABC-12345");
     assertThat(panData.getActionResult()).isTrue();
 
     // Two poll messages on the losing registrar's side at the implicit transfer time: a

javatests/google/registry/flows/host/HostDeleteFlowTest.java

@@ -84,6 +84,25 @@ public class HostDeleteFlowTest extends ResourceFlowTestCase<HostDeleteFlow, Hos
     assertNoDnsTasksEnqueued();
   }
 
+  @Test
+  public void testSuccess_clTridNotSpecified() throws Exception {
+    setEppInput("host_delete_no_cltrid.xml", ImmutableMap.of("HOSTNAME", "ns1.example.tld"));
+    persistActiveHost("ns1.example.tld");
+    clock.advanceOneMilli();
+    assertTransactionalFlow(true);
+    runFlowAssertResponse(loadFile("host_delete_response_no_cltrid.xml"));
+    HostResource deletedHost = reloadResourceByForeignKey();
+    assertAboutHosts().that(deletedHost).hasStatusValue(StatusValue.PENDING_DELETE);
+    assertAsyncDeletionTaskEnqueued(
+        deletedHost, "TheRegistrar", Trid.create(null, "server-trid"), false);
+    assertAboutHosts()
+        .that(deletedHost)
+        .hasOnlyOneHistoryEntryWhich()
+        .hasType(HistoryEntry.Type.HOST_PENDING_DELETE);
+    assertNoBillingEvents();
+    assertNoDnsTasksEnqueued();
+  }
+
   @Test
   public void testFailure_neverExisted() throws Exception {
     ResourceDoesNotExistException thrown =

javatests/google/registry/flows/host/testdata/host_delete_no_cltrid.xml

@@ -0,0 +1,10 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <delete>
+      <host:delete
+       xmlns:host="urn:ietf:params:xml:ns:host-1.0">
+        <host:name>%HOSTNAME%</host:name>
+      </host:delete>
+    </delete>
+  </command>
+</epp>

javatests/google/registry/flows/host/testdata/host_delete_response_no_cltrid.xml

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

javatests/google/registry/rde/imports/RdeImportTestUtils.java

@@ -15,6 +15,7 @@
 package google.registry.rde.imports;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
 
 import google.registry.model.eppcommon.Trid;
 
@@ -34,9 +35,10 @@ public class RdeImportTestUtils {
    */
   public static void checkTrid(Trid trid) {
     assertThat(trid).isNotNull();
-    assertThat(trid.getClientTransactionId()).isNotNull();
-    assertThat(trid.getClientTransactionId().length()).isAtLeast(3);
-    assertThat(trid.getClientTransactionId().length()).isAtMost(64);
-    assertThat(trid.getClientTransactionId()).startsWith("Import_");
+    assertThat(trid.getClientTransactionId()).isPresent();
+    String clTrid = trid.getClientTransactionId().get();
+    assertThat(clTrid.length()).isAtLeast(3);
+    assertThat(clTrid.length()).isAtMost(64);
+    assertThat(clTrid).startsWith("Import_");
   }
 }

javatests/google/registry/testing/TaskQueueHelper.java

@@ -15,6 +15,7 @@
 package google.registry.testing;
 
 import static com.google.appengine.tools.development.testing.LocalTaskQueueTestConfig.getLocalTaskQueue;
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.base.Predicates.in;
 import static com.google.common.base.Predicates.not;
@@ -104,8 +105,7 @@ public class TaskQueueHelper {
     }
 
     public TaskMatcher payload(String payload) {
-      checkState(
-          expected.params.isEmpty(), "Cannot add a payload to a TaskMatcher with params.");
+      checkState(expected.params.isEmpty(), "Cannot add a payload to a TaskMatcher with params");
       expected.payload = payload;
       return this;
     }
@@ -122,16 +122,16 @@ public class TaskQueueHelper {
     }
 
     public TaskMatcher param(String key, String value) {
-      checkState(
-          expected.payload == null, "Cannot add params to a TaskMatcher with a payload.");
+      checkState(expected.payload == null, "Cannot add params to a TaskMatcher with a payload");
+      checkNotNull(value, "Test error: A task can never have a null value, so don't assert it");
       expected.params.put(key, value);
       return this;
     }
 
     public TaskMatcher etaDelta(Duration lowerBound, Duration upperBound) {
-      checkState(!lowerBound.isShorterThan(Duration.ZERO), "lowerBound must be non-negative.");
+      checkState(!lowerBound.isShorterThan(Duration.ZERO), "lowerBound must be non-negative");
       checkState(
-          upperBound.isLongerThan(lowerBound), "upperBound must be greater than lowerBound.");
+          upperBound.isLongerThan(lowerBound), "upperBound must be greater than lowerBound");
       expected.etaDeltaLowerBound = lowerBound.getStandardSeconds();
       expected.etaDeltaUpperBound = upperBound.getStandardSeconds();
       return this;