Update RegistrarSettingsAction and RegistrarContact to SQL calls (#1042)

* Update RegistrarSettingsAction and RegistrarContact to SQL calls

Relevant potentially-unclear changes:
- Making sure the last update time is always correct and up to date in
the auto timestamp object
- Reloading the domain upon return when updating in a new transaction to
make sure that we use the properly-updated last update time (SQL returns
the correct result if retrieved within the same txn but DS does not)

core/src/test/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java

@@ -40,9 +40,11 @@ import google.registry.request.auth.UserAuthInfo;
 import google.registry.security.XsrfTokenManager;
 import google.registry.testing.AppEngineExtension;
 import google.registry.testing.DeterministicStringGenerator;
+import google.registry.testing.DualDatabaseTest;
 import google.registry.testing.FakeClock;
 import google.registry.testing.FakeResponse;
 import google.registry.testing.SystemPropertyExtension;
+import google.registry.testing.TestOfyAndSql;
 import google.registry.ui.server.SendEmailUtils;
 import google.registry.util.EmailMessage;
 import google.registry.util.SendEmailService;
@@ -52,7 +54,6 @@ import javax.servlet.http.HttpServletRequest;
 import org.joda.money.CurrencyUnit;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Order;
-import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.mockito.ArgumentCaptor;
@@ -60,6 +61,7 @@ import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 
 @ExtendWith(MockitoExtension.class)
+@DualDatabaseTest
 final class ConsoleRegistrarCreatorActionTest {
 
   @RegisterExtension
@@ -124,7 +126,7 @@ final class ConsoleRegistrarCreatorActionTest {
     action.analyticsConfig = ImmutableMap.of("googleAnalyticsId", "sampleId");
   }
 
-  @Test
+  @TestOfyAndSql
   void testNoUser_redirect() {
     when(request.getRequestURI()).thenReturn("/test");
     action.authResult = AuthResult.NOT_AUTHENTICATED;
@@ -133,14 +135,14 @@ final class ConsoleRegistrarCreatorActionTest {
     assertThat(response.getHeaders().get(LOCATION)).isEqualTo("/_ah/login?continue=%2Ftest");
   }
 
-  @Test
+  @TestOfyAndSql
   void testGet_authorized() {
     action.run();
     assertThat(response.getPayload()).contains("<h1>Create Registrar</h1>");
     assertThat(response.getPayload()).contains("gtag('config', 'sampleId')");
   }
 
-  @Test
+  @TestOfyAndSql
   void testGet_authorized_onProduction() {
     RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);
     action.run();
@@ -148,7 +150,7 @@ final class ConsoleRegistrarCreatorActionTest {
     assertThat(response.getPayload()).contains("gtag('config', 'sampleId')");
   }
 
-  @Test
+  @TestOfyAndSql
   void testGet_unauthorized() {
     action.registrarAccessor =
         AuthenticatedRegistrarAccessor.createForTesting(ImmutableSetMultimap.of());
@@ -157,7 +159,7 @@ final class ConsoleRegistrarCreatorActionTest {
     assertThat(response.getPayload()).contains("gtag('config', 'sampleId')");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_authorized_minimalAddress() {
     action.clientId = Optional.of("myclientid");
     action.name = Optional.of("registrar name");
@@ -225,7 +227,7 @@ final class ConsoleRegistrarCreatorActionTest {
                 .build());
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_authorized_allAddress() {
     action.clientId = Optional.of("myclientid");
     action.name = Optional.of("registrar name");
@@ -262,7 +264,7 @@ final class ConsoleRegistrarCreatorActionTest {
         .build());
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_authorized_multipleBillingLines() {
     action.clientId = Optional.of("myclientid");
     action.name = Optional.of("registrar name");
@@ -299,7 +301,7 @@ final class ConsoleRegistrarCreatorActionTest {
             CurrencyUnit.EUR, "billing-account-eur");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_authorized_repeatingCurrency_fails() {
     action.clientId = Optional.of("myclientid");
     action.name = Optional.of("registrar name");
@@ -327,7 +329,7 @@ final class ConsoleRegistrarCreatorActionTest {
                 + " JPY=billing-account-2 and JPY=billing-account-1");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_authorized_badCurrency_fails() {
     action.clientId = Optional.of("myclientid");
     action.name = Optional.of("registrar name");
@@ -354,7 +356,7 @@ final class ConsoleRegistrarCreatorActionTest {
         .contains("Failed: Error parsing billing accounts - Unknown currency &#39;XYZ&#39;");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_authorized_badBillingLine_fails() {
     action.clientId = Optional.of("myclientid");
     action.name = Optional.of("registrar name");
@@ -383,7 +385,7 @@ final class ConsoleRegistrarCreatorActionTest {
                 + " The format should be [currency]=[account ID]");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_authorized_setPassword() {
     action.clientId = Optional.of("myclientid");
     action.name = Optional.of("registrar name");
@@ -412,7 +414,7 @@ final class ConsoleRegistrarCreatorActionTest {
     assertThat(registrar.getPhonePasscode()).isEqualTo("10203");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_badEmailFails() {
     action.clientId = Optional.of("myclientid");
     action.name = Optional.of("registrar name");
@@ -433,7 +435,7 @@ final class ConsoleRegistrarCreatorActionTest {
         .contains("Failed: Provided email lolcat is not a valid email address");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_unauthorized() {
     action.registrarAccessor =
         AuthenticatedRegistrarAccessor.createForTesting(ImmutableSetMultimap.of());

core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java

@@ -15,6 +15,7 @@
 package google.registry.ui.server.registrar;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
 import static google.registry.testing.DatabaseHelper.loadRegistrar;
 import static google.registry.testing.DatabaseHelper.persistResource;
 import static google.registry.testing.TaskQueueHelper.assertNoTasksEnqueued;
@@ -36,8 +37,10 @@ import google.registry.model.registrar.Registrar;
 import google.registry.request.auth.AuthenticatedRegistrarAccessor;
 import google.registry.request.auth.AuthenticatedRegistrarAccessor.Role;
 import google.registry.testing.CertificateSamples;
+import google.registry.testing.DualDatabaseTest;
 import google.registry.testing.SystemPropertyExtension;
 import google.registry.testing.TaskQueueHelper.TaskMatcher;
+import google.registry.testing.TestOfyAndSql;
 import google.registry.util.CidrAddressBlock;
 import google.registry.util.EmailMessage;
 import java.util.Map;
@@ -46,17 +49,17 @@ import java.util.function.Function;
 import org.joda.time.DateTime;
 import org.json.simple.JSONValue;
 import org.json.simple.parser.ParseException;
-import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.mockito.ArgumentCaptor;
 
 /** Tests for {@link RegistrarSettingsAction}. */
+@DualDatabaseTest
 class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
 
   @RegisterExtension
   final SystemPropertyExtension systemPropertyExtension = new SystemPropertyExtension();
 
-  @Test
+  @TestOfyAndSql
   void testSuccess_updateRegistrarInfo_andSendsNotificationEmail() throws Exception {
     String expectedEmailBody = loadFile(getClass(), "update_registrar_email.txt");
     // This update changes some values on the admin contact and makes it a tech contact as well,
@@ -74,7 +77,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
-  @Test
+  @TestOfyAndSql
   void testFailure_updateRegistrarInfo_duplicateContacts() {
     Map<String, Object> response = action.handleJsonRequest(
         readJsonFromFile("update_registrar_duplicate_contacts.json", getLastUpdateTime()));
@@ -91,7 +94,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
    * Make sure that if someone spoofs a different registrar (they don't have access to), we fail.
    * Also relevant if the person's privilege were revoked after the page load.
    */
-  @Test
+  @TestOfyAndSql
   void testFailure_readRegistrarInfo_notAuthorized() {
     setUserWithoutAccess();
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of("id", CLIENT_ID));
@@ -105,7 +108,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
   }
 
   /** This is the default read test for the registrar settings actions. */
-  @Test
+  @TestOfyAndSql
   void testSuccess_readRegistrarInfo_authorizedReadWrite() {
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of("id", CLIENT_ID));
     assertThat(response)
@@ -116,7 +119,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "read", "[OWNER]", "SUCCESS");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_emptyJsonObject_errorLastUpdateTimeFieldRequired() {
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
     args.remove("lastUpdateTime");
@@ -135,7 +138,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormFieldException");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_noEmail_errorEmailFieldRequired() {
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
     args.remove("emailAddress");
@@ -154,7 +157,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormFieldException");
   }
 
-  @Test
+  @TestOfyAndSql
   void testFailure_updateRegistrarInfo_notAuthorized() {
     setUserWithoutAccess();
 
@@ -172,7 +175,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "update", "[]", "ERROR: ForbiddenException");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_badEmail_errorEmailField() {
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
     args.put("emailAddress", "lolcat");
@@ -191,7 +194,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormFieldException");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_nonParsableTime_getsAngry() {
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
     args.put("lastUpdateTime", "cookies");
@@ -210,7 +213,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormFieldException");
   }
 
-  @Test
+  @TestOfyAndSql
   void testPost_nonAsciiCharacters_getsAngry() {
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
     args.put("emailAddress", "ヘ(◕。◕ヘ)@example.com");
@@ -276,7 +279,11 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     // (We check it separately from the next assert to get better error message on failure)
     assertThat(getter.apply(updatedRegistrar)).isEqualTo(newValue);
     // ONLY that value changed:
-    assertThat(updatedRegistrar).isEqualTo(setter.apply(registrar.asBuilder(), newValue).build());
+    // (note: the setter won't properly update last update time, so ignore it)
+    assertAboutImmutableObjects()
+        .that(updatedRegistrar)
+        .isEqualExceptFields(
+            setter.apply(registrar.asBuilder(), newValue).build(), "lastUpdateTime");
     // We increased the correct metric
     assertMetric(CLIENT_ID, "update", String.format("[%s]", role), "SUCCESS");
   }
@@ -331,7 +338,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         CLIENT_ID, "update", allExceptCorrectRoles.toString(), "ERROR: ForbiddenException");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_whoisServer() {
     doTestUpdate(
         Role.OWNER,
@@ -340,24 +347,24 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         Registrar.Builder::setWhoisServer);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_phoneNumber() {
     doTestUpdate(
         Role.OWNER, Registrar::getPhoneNumber, "+1.2345678900", Registrar.Builder::setPhoneNumber);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_faxNumber() {
     doTestUpdate(
         Role.OWNER, Registrar::getFaxNumber, "+1.2345678900", Registrar.Builder::setFaxNumber);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_url() {
     doTestUpdate(Role.OWNER, Registrar::getUrl, "new-url.example", Registrar.Builder::setUrl);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_ipAddressAllowList() {
     doTestUpdate(
         Role.OWNER,
@@ -366,7 +373,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         Registrar.Builder::setIpAddressAllowList);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_clientCertificate() {
     clock.setTo(DateTime.parse("2020-11-02T00:00:00Z"));
     doTestUpdate(
@@ -376,7 +383,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         (builder, s) -> builder.setClientCertificate(s, clock.nowUtc()));
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_clientCertificateWithViolationsFails() {
     clock.setTo(DateTime.parse("2020-11-02T00:00:00Z"));
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
@@ -401,7 +408,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_clientCertificateWithMultipleViolationsFails() {
     clock.setTo(DateTime.parse("2055-11-01T00:00:00Z"));
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
@@ -426,7 +433,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_failoverClientCertificate() {
     clock.setTo(DateTime.parse("2020-11-02T00:00:00Z"));
     doTestUpdate(
@@ -436,7 +443,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         (builder, s) -> builder.setFailoverClientCertificate(s, clock.nowUtc()));
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_failoverClientCertificateWithViolationsAlreadyExistedSucceeds() {
     // TODO(sarahbot): remove this test after November 1, 2020.
 
@@ -469,7 +476,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_failoverClientCertificateWithViolationsFails() {
     clock.setTo(DateTime.parse("2020-11-02T00:00:00Z"));
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
@@ -494,7 +501,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_failoverClientCertificateWithMultipleViolationsFails() {
     clock.setTo(DateTime.parse("2055-11-01T00:00:00Z"));
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
@@ -519,7 +526,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_allowedTlds() {
     doTestUpdate(
         Role.ADMIN,
@@ -528,7 +535,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         (builder, s) -> builder.setAllowedTlds(s));
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_allowedTlds_failedWhenNoWhoisAbuseContactExists() {
     setUserAdmin();
     RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);
@@ -551,7 +558,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_allowedTlds_failedWhenTldNotExist() {
     setUserAdmin();
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
@@ -573,7 +580,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_allowedTlds_failedWhenRemovingTld() {
     setUserAdmin();
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
@@ -595,7 +602,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_allowedTlds_noChange_successWhenUserIsNotAdmin() {
     Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
     args.put("allowedTlds", ImmutableList.of("currenttld"));
@@ -615,7 +622,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_localizedAddress_city() {
     doTestUpdate(
         Role.OWNER,
@@ -624,7 +631,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         Registrar.Builder::setLocalizedAddress);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_localizedAddress_countryCode() {
     doTestUpdate(
         Role.OWNER,
@@ -633,7 +640,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         Registrar.Builder::setLocalizedAddress);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_localizedAddress_state() {
     doTestUpdate(
         Role.OWNER,
@@ -642,7 +649,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         Registrar.Builder::setLocalizedAddress);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_localizedAddress_street() {
     doTestUpdate(
         Role.OWNER,
@@ -655,7 +662,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         Registrar.Builder::setLocalizedAddress);
   }
 
-  @Test
+  @TestOfyAndSql
   void testUpdate_localizedAddress_zip() {
     doTestUpdate(
         Role.OWNER,

core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java

@@ -75,9 +75,15 @@ public abstract class RegistrarSettingsActionTestCase {
 
   static final String CLIENT_ID = "TheRegistrar";
 
+  final FakeClock clock = new FakeClock(DateTime.parse("2014-01-01T00:00:00Z"));
+
   @RegisterExtension
   public final AppEngineExtension appEngine =
-      AppEngineExtension.builder().withDatastoreAndCloudSql().withTaskQueue().build();
+      AppEngineExtension.builder()
+          .withDatastoreAndCloudSql()
+          .withClock(clock)
+          .withTaskQueue()
+          .build();
 
   @RegisterExtension public final InjectExtension inject = new InjectExtension();
 
@@ -88,7 +94,6 @@ public abstract class RegistrarSettingsActionTestCase {
 
   final RegistrarSettingsAction action = new RegistrarSettingsAction();
   private final StringWriter writer = new StringWriter();
-  final FakeClock clock = new FakeClock(DateTime.parse("2014-01-01T00:00:00Z"));
 
   RegistrarContact techContact;