Combine the two Lock classes into one class (#1126)

* Combine the two Lock classes into one class

This allows us to remove the DAO and to just treat locks the same as we
would treat any other object -- generically grabbing them from the
transaction manager.

We do not need to be concerned about the changeover between Datastore
and SQL because we assume that any such changeover will require
sufficient downtime that any currently-valid acquired locks will expire
during the downtime. Otherwise, we could get into a situation where an
action has acquired a particular lock in Datastore but not SQL.

core/src/test/java/google/registry/model/server/LockTest.java

@@ -26,35 +26,30 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
-import google.registry.model.ofy.Ofy;
+import google.registry.model.EntityTestCase;
 import google.registry.model.server.Lock.LockState;
-import google.registry.testing.AppEngineExtension;
-import google.registry.testing.FakeClock;
-import google.registry.testing.InjectExtension;
+import google.registry.testing.DualDatabaseTest;
+import google.registry.testing.TestOfyAndSql;
 import google.registry.util.RequestStatusChecker;
 import java.util.Optional;
 import org.joda.time.Duration;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link Lock}. */
-public class LockTest {
+@DualDatabaseTest
+public class LockTest extends EntityTestCase {
 
   private static final String RESOURCE_NAME = "foo";
   private static final Duration ONE_DAY = Duration.standardDays(1);
   private static final Duration TWO_MILLIS = Duration.millis(2);
   private static final RequestStatusChecker requestStatusChecker = mock(RequestStatusChecker.class);
-  private static final FakeClock clock = new FakeClock();
 
   private LockMetrics origLockMetrics;
 
-  @RegisterExtension
-  public final AppEngineExtension appEngine =
-      AppEngineExtension.builder().withDatastoreAndCloudSql().withClock(clock).build();
-
-  @RegisterExtension public final InjectExtension inject = new InjectExtension();
+  public LockTest() {
+    super(JpaEntityCoverageCheck.ENABLED);
+  }
 
   private Optional<Lock> acquire(String tld, Duration leaseLength, LockState expectedLockState) {
     Lock.lockMetrics = mock(LockMetrics.class);
@@ -76,7 +71,6 @@ public class LockTest {
 
   @BeforeEach
   void beforeEach() {
-    inject.setStaticField(Ofy.class, "clock", clock);
     origLockMetrics = Lock.lockMetrics;
     Lock.lockMetrics = null;
     when(requestStatusChecker.getLogId()).thenReturn("current-request-id");
@@ -88,32 +82,32 @@ public class LockTest {
     Lock.lockMetrics = origLockMetrics;
   }
 
-  @Test
+  @TestOfyAndSql
   void testReleasedExplicitly() {
     Optional<Lock> lock = acquire("", ONE_DAY, FREE);
     assertThat(lock).isPresent();
     // We can't get it again at the same time.
     assertThat(acquire("", ONE_DAY, IN_USE)).isEmpty();
     // But if we release it, it's available.
-    clock.advanceBy(Duration.millis(123));
+    fakeClock.advanceBy(Duration.millis(123));
     release(lock.get(), "", 123);
     assertThat(acquire("", ONE_DAY, FREE)).isPresent();
   }
 
-  @Test
+  @TestOfyAndSql
   void testReleasedAfterTimeout() {
     assertThat(acquire("", TWO_MILLIS, FREE)).isPresent();
     // We can't get it again at the same time.
     assertThat(acquire("", TWO_MILLIS, IN_USE)).isEmpty();
     // A second later we still can't get the lock.
-    clock.advanceOneMilli();
+    fakeClock.advanceOneMilli();
     assertThat(acquire("", TWO_MILLIS, IN_USE)).isEmpty();
     // But two seconds later we can get it.
-    clock.advanceOneMilli();
+    fakeClock.advanceOneMilli();
     assertThat(acquire("", TWO_MILLIS, TIMED_OUT)).isPresent();
   }
 
-  @Test
+  @TestOfyAndSql
   void testReleasedAfterRequestFinish() {
     assertThat(acquire("", ONE_DAY, FREE)).isPresent();
     // We can't get it again while request is active
@@ -123,7 +117,7 @@ public class LockTest {
     assertThat(acquire("", ONE_DAY, OWNER_DIED)).isPresent();
   }
 
-  @Test
+  @TestOfyAndSql
   void testTldsAreIndependent() {
     Optional<Lock> lockA = acquire("a", ONE_DAY, FREE);
     assertThat(lockA).isPresent();
@@ -133,12 +127,12 @@ public class LockTest {
     // We can't get lockB again at the same time.
     assertThat(acquire("b", ONE_DAY, IN_USE)).isEmpty();
     // Releasing lockA has no effect on lockB (even though we are still using the "b" tld).
-    clock.advanceOneMilli();
+    fakeClock.advanceOneMilli();
     release(lockA.get(), "a", 1);
     assertThat(acquire("b", ONE_DAY, IN_USE)).isEmpty();
   }
 
-  @Test
+  @TestOfyAndSql
   void testFailure_emptyResourceName() {
     IllegalArgumentException thrown =
         assertThrows(

core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java

@@ -31,6 +31,7 @@ import google.registry.model.registry.RegistryTest;
 import google.registry.model.registry.label.ReservedListSqlDaoTest;
 import google.registry.model.reporting.Spec11ThreatMatchTest;
 import google.registry.model.server.KmsSecretRevisionSqlDaoTest;
+import google.registry.model.server.LockTest;
 import google.registry.model.server.ServerSecretTest;
 import google.registry.model.smd.SignedMarkRevocationListDaoTest;
 import google.registry.model.tmch.ClaimsListSqlDaoTest;
@@ -41,7 +42,6 @@ import google.registry.schema.integration.SqlIntegrationTestSuite.AfterSuiteTest
 import google.registry.schema.integration.SqlIntegrationTestSuite.BeforeSuiteTest;
 import google.registry.schema.registrar.RegistrarDaoTest;
 import google.registry.schema.replay.SqlReplayCheckpointTest;
-import google.registry.schema.server.LockDaoTest;
 import google.registry.schema.tld.PremiumListSqlDaoTest;
 import google.registry.testing.AppEngineExtension;
 import org.junit.jupiter.api.AfterAll;
@@ -90,7 +90,7 @@ import org.junit.runner.RunWith;
   DomainHistoryTest.class,
   HostHistoryTest.class,
   KmsSecretRevisionSqlDaoTest.class,
-  LockDaoTest.class,
+  LockTest.class,
   PollMessageTest.class,
   PremiumListSqlDaoTest.class,
   RdeRevisionTest.class,

core/src/test/java/google/registry/schema/server/LockDaoTest.java

@@ -1,188 +0,0 @@
-// Copyright 2020 The Nomulus Authors. All Rights Reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package google.registry.schema.server;
-
-import static com.google.common.truth.Truth.assertThat;
-import static google.registry.testing.LogsSubject.assertAboutLogs;
-
-import com.google.common.testing.TestLogHandler;
-import google.registry.persistence.transaction.JpaTestRules;
-import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension;
-import google.registry.testing.DatastoreEntityExtension;
-import google.registry.testing.FakeClock;
-import java.util.Optional;
-import java.util.logging.Level;
-import java.util.logging.Logger;
-import org.joda.time.Duration;
-import org.junit.jupiter.api.Order;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.RegisterExtension;
-
-/** Unit tests for {@link Lock}. */
-public class LockDaoTest {
-
-  private final FakeClock fakeClock = new FakeClock();
-  private final TestLogHandler logHandler = new TestLogHandler();
-  private final Logger loggerToIntercept = Logger.getLogger(LockDao.class.getCanonicalName());
-
-  @RegisterExtension
-  @Order(value = 1)
-  DatastoreEntityExtension datastoreEntityExtension = new DatastoreEntityExtension();
-
-  @RegisterExtension
-  final JpaIntegrationWithCoverageExtension jpa =
-      new JpaTestRules.Builder().withClock(fakeClock).buildIntegrationWithCoverageExtension();
-
-  @Test
-  void save_worksSuccessfully() {
-    Lock lock =
-        Lock.create("testResource", "tld", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.save(lock);
-    Optional<Lock> returnedLock = LockDao.load("testResource", "tld");
-    assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime);
-    assertThat(returnedLock.get().requestLogId).isEqualTo(lock.requestLogId);
-  }
-
-  @Test
-  void save_succeedsWhenLockAlreadyExists() {
-    Lock lock =
-        Lock.create("testResource", "tld", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.save(lock);
-    Lock lock2 =
-        Lock.create("testResource", "tld", "testLogId2", fakeClock.nowUtc(), Duration.millis(4));
-    LockDao.save(lock2);
-    assertThat(LockDao.load("testResource", "tld").get().requestLogId).isEqualTo("testLogId2");
-  }
-
-  @Test
-  void save_worksSuccesfullyGlobalLock() {
-    Lock lock =
-        Lock.createGlobal("testResource", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.save(lock);
-    Optional<Lock> returnedLock = LockDao.load("testResource");
-    assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime);
-    assertThat(returnedLock.get().requestLogId).isEqualTo(lock.requestLogId);
-  }
-
-  @Test
-  void load_worksSuccessfully() {
-    Lock lock =
-        Lock.create("testResource", "tld", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.save(lock);
-    Optional<Lock> returnedLock = LockDao.load("testResource", "tld");
-    assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime);
-    assertThat(returnedLock.get().requestLogId).isEqualTo(lock.requestLogId);
-  }
-
-  @Test
-  void load_worksSuccessfullyGlobalLock() {
-    Lock lock =
-        Lock.createGlobal("testResource", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.save(lock);
-    Optional<Lock> returnedLock = LockDao.load("testResource");
-    assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime);
-    assertThat(returnedLock.get().requestLogId).isEqualTo(lock.requestLogId);
-  }
-
-  @Test
-  void load_worksSuccesfullyLockDoesNotExist() {
-    Optional<Lock> returnedLock = LockDao.load("testResource", "tld");
-    assertThat(returnedLock.isPresent()).isFalse();
-  }
-
-  @Test
-  void delete_worksSuccesfully() {
-    Lock lock =
-        Lock.create("testResource", "tld", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.save(lock);
-    Optional<Lock> returnedLock = LockDao.load("testResource", "tld");
-    assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime);
-    LockDao.delete("testResource", "tld");
-    returnedLock = LockDao.load("testResource", "tld");
-    assertThat(returnedLock.isPresent()).isFalse();
-  }
-
-  @Test
-  void delete_worksSuccessfullyGlobalLock() {
-    Lock lock =
-        Lock.createGlobal("testResource", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.save(lock);
-    Optional<Lock> returnedLock = LockDao.load("testResource");
-    assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime);
-    LockDao.delete("testResource");
-    returnedLock = LockDao.load("testResource");
-    assertThat(returnedLock.isPresent()).isFalse();
-  }
-
-  @Test
-  void delete_succeedsLockDoesntExist() {
-    LockDao.delete("testResource");
-  }
-
-  @Test
-  void compare_logsWarningWhenCloudSqlLockMissing() {
-    loggerToIntercept.addHandler(logHandler);
-    google.registry.model.server.Lock datastoreLock =
-        google.registry.model.server.Lock.create(
-            "resourceName", "tld", "id", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.compare(Optional.of(datastoreLock), Optional.empty());
-    assertAboutLogs()
-        .that(logHandler)
-        .hasLogAtLevelWithMessage(
-            Level.WARNING,
-            String.format("Datastore lock: %s was not found in Cloud SQL", datastoreLock));
-  }
-
-  @Test
-  void compare_logsWarningWhenCloudSqlLockExistsWhenItShouldNot() {
-    loggerToIntercept.addHandler(logHandler);
-    Lock lock =
-        Lock.createGlobal("testResource", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.compare(Optional.ofNullable(null), Optional.of(lock));
-    assertAboutLogs()
-        .that(logHandler)
-        .hasLogAtLevelWithMessage(
-            Level.WARNING, "Cloud SQL lock for testResource with tld GLOBAL should be null");
-  }
-
-  @Test
-  void compare_logsWarningWhenLocksDontMatch() {
-    loggerToIntercept.addHandler(logHandler);
-    Lock cloudSqlLock =
-        Lock.create("testResource", "tld", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    google.registry.model.server.Lock datastoreLock =
-        google.registry.model.server.Lock.create(
-            "testResource", "tld", "wrong", fakeClock.nowUtc().minusDays(1), Duration.millis(3));
-    LockDao.compare(Optional.of(datastoreLock), Optional.of(cloudSqlLock));
-    assertAboutLogs()
-        .that(logHandler)
-        .hasLogAtLevelWithMessage(
-            Level.WARNING,
-            "Datastore lock requestLogId of wrong does not equal Cloud SQL lock requestLogId"
-                + " of testLogId");
-    assertAboutLogs()
-        .that(logHandler)
-        .hasLogAtLevelWithMessage(
-            Level.WARNING,
-            "Datastore lock acquiredTime of 1969-12-31T00:00:00.000Z does not equal Cloud SQL"
-                + " lock acquiredTime of 1970-01-01T00:00:00.000Z");
-    assertAboutLogs()
-        .that(logHandler)
-        .hasLogAtLevelWithMessage(
-            Level.WARNING,
-            "Datastore lock expirationTime of 1969-12-31T00:00:00.003Z does not equal Cloud"
-                + " SQL lock expirationTime of 1970-01-01T00:00:00.002Z");
-  }
-}