diff --git a/core/src/main/java/google/registry/model/EppResource.java b/core/src/main/java/google/registry/model/EppResource.java index 067536ccd..753d319d1 100644 --- a/core/src/main/java/google/registry/model/EppResource.java +++ b/core/src/main/java/google/registry/model/EppResource.java @@ -137,7 +137,7 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable { /** Status values associated with this resource. */ @Column(name = "statuses") - // TODO(mmuller): rename to "statuses" once we're off datastore. + // TODO(b/177567432): rename to "statuses" once we're off datastore. Set status; /** diff --git a/core/src/main/java/google/registry/model/domain/DomainContent.java b/core/src/main/java/google/registry/model/domain/DomainContent.java index 305e38aa2..3b19fed04 100644 --- a/core/src/main/java/google/registry/model/domain/DomainContent.java +++ b/core/src/main/java/google/registry/model/domain/DomainContent.java @@ -127,7 +127,7 @@ public class DomainContent extends EppResource * * @invariant fullyQualifiedDomainName == fullyQualifiedDomainName.toLowerCase(Locale.ENGLISH) */ - // TODO(b/158858642): Rename this to domainName when we are off Datastore + // TODO(b/177567432): Rename this to domainName when we are off Datastore @Column(name = "domainName") @Index String fullyQualifiedDomainName; diff --git a/core/src/main/java/google/registry/model/host/HostBase.java b/core/src/main/java/google/registry/model/host/HostBase.java index bfb020126..df4a41b5c 100644 --- a/core/src/main/java/google/registry/model/host/HostBase.java +++ b/core/src/main/java/google/registry/model/host/HostBase.java @@ -64,7 +64,7 @@ public class HostBase extends EppResource { * from (creationTime, deletionTime) there can only be one host in Datastore with this name. * However, there can be many hosts with the same name and non-overlapping lifetimes. */ - // TODO(b/158858642): Rename this to hostName when we are off Datastore + // TODO(b/177567432): Rename this to hostName when we are off Datastore @Index @Column(name = "hostName") String fullyQualifiedHostName; diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index 733890bc5..00d019809 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -236,7 +236,7 @@ public class Registrar extends ImmutableObject * Unique registrar client id. Must conform to "clIDType" as defined in RFC5730. * * @see Shared Structure Schema - *

TODO(b/177568946): Rename this field to registrarId. + *

TODO(b/177567432): Rename this field to registrarId. */ @Id @javax.persistence.Id @@ -301,7 +301,7 @@ public class Registrar extends ImmutableObject String failoverClientCertificateHash; /** An allow list of netmasks (in CIDR notation) which the client is allowed to connect from. */ - // TODO: Rename to ipAddressAllowList once Cloud SQL migration is complete. + // TODO(b/177567432): Rename to ipAddressAllowList once Cloud SQL migration is complete. @Column(name = "ip_address_allow_list") List ipAddressWhitelist; diff --git a/core/src/main/java/google/registry/model/registrar/RegistrarContact.java b/core/src/main/java/google/registry/model/registrar/RegistrarContact.java index a0a244213..fd27a94ae 100644 --- a/core/src/main/java/google/registry/model/registrar/RegistrarContact.java +++ b/core/src/main/java/google/registry/model/registrar/RegistrarContact.java @@ -69,7 +69,7 @@ import javax.persistence.Transient; * *MUST* also modify the persisted Registrar entity with {@link Registrar#contactsRequireSyncing} * set to true. * - *

TODO(b/163366543): Rename the class name to RegistrarPoc after database migration + *

TODO(b/177567432): Rename the class name to RegistrarPoc after database migration */ @ReportedOn @Entity diff --git a/core/src/main/java/google/registry/model/registry/label/ReservedListSqlDao.java b/core/src/main/java/google/registry/model/registry/label/ReservedListSqlDao.java index a41403f86..5185ffda9 100644 --- a/core/src/main/java/google/registry/model/registry/label/ReservedListSqlDao.java +++ b/core/src/main/java/google/registry/model/registry/label/ReservedListSqlDao.java @@ -23,7 +23,7 @@ import java.util.Optional; /** * A {@link ReservedList} DAO for Cloud SQL. * - *

TODO(b/160993806): Rename this class to ReservedListDao after migrating to Cloud SQL. + *

TODO(b/177567432): Rename this class to ReservedListDao after migrating to Cloud SQL. */ public class ReservedListSqlDao { diff --git a/core/src/main/java/google/registry/model/server/KmsSecretRevision.java b/core/src/main/java/google/registry/model/server/KmsSecretRevision.java index b231a09ad..a39002080 100644 --- a/core/src/main/java/google/registry/model/server/KmsSecretRevision.java +++ b/core/src/main/java/google/registry/model/server/KmsSecretRevision.java @@ -49,7 +49,8 @@ import javax.persistence.Transient; * succeeds, we will end up with having two exact same revisions that differ only by revisionKey. * This is fine though, because we only use the revision with the highest revisionKey. * - *

TODO: remove Datastore-specific fields post-Registry-3.0-migration and rename to KmsSecret. + *

TODO(b/177567432): remove Datastore-specific fields post-Registry-3.0-migration and rename to + * KmsSecret. * * @see Google Cloud Key Management Service * Documentation @@ -71,7 +72,7 @@ public class KmsSecretRevision extends ImmutableObject implements NonReplicatedE /** * The revision of this secret. * - *

TODO: change name of the variable to revisionId once we're off Datastore + *

TODO(b/177567432): change name of the variable to revisionId once we're off Datastore */ @Id @javax.persistence.Id diff --git a/core/src/main/java/google/registry/model/server/KmsSecretRevisionSqlDao.java b/core/src/main/java/google/registry/model/server/KmsSecretRevisionSqlDao.java index 219c43be8..b473c56c8 100644 --- a/core/src/main/java/google/registry/model/server/KmsSecretRevisionSqlDao.java +++ b/core/src/main/java/google/registry/model/server/KmsSecretRevisionSqlDao.java @@ -24,7 +24,7 @@ import java.util.Optional; /** * A {@link KmsSecretRevision} DAO for Cloud SQL. * - *

TODO: Rename this class to KmsSecretDao after migrating to Cloud SQL. + *

TODO(b/177567432): Rename this class to KmsSecretDao after migrating to Cloud SQL. */ public class KmsSecretRevisionSqlDao { diff --git a/core/src/main/java/google/registry/model/server/Lock.java b/core/src/main/java/google/registry/model/server/Lock.java index e78655fd6..b448d4360 100644 --- a/core/src/main/java/google/registry/model/server/Lock.java +++ b/core/src/main/java/google/registry/model/server/Lock.java @@ -15,8 +15,9 @@ package google.registry.model.server; import static com.google.common.base.Preconditions.checkArgument; -import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.DateTimeUtils.isAtOrAfter; +import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; @@ -29,40 +30,59 @@ import google.registry.model.ImmutableObject; import google.registry.model.annotations.NotBackedUp; import google.registry.model.annotations.NotBackedUp.Reason; import google.registry.persistence.VKey; -import google.registry.schema.replay.DatastoreOnlyEntity; +import google.registry.schema.replay.DatastoreAndSqlEntity; import google.registry.util.RequestStatusChecker; import google.registry.util.RequestStatusCheckerImpl; import java.io.Serializable; import java.util.Optional; import javax.annotation.Nullable; +import javax.persistence.Column; +import javax.persistence.IdClass; +import javax.persistence.PostLoad; +import javax.persistence.Table; +import javax.persistence.Transient; import org.joda.time.DateTime; import org.joda.time.Duration; /** * A lock on some shared resource. * - *

Locks are either specific to a tld or global to the entire system, in which case a tld of null - * is used. + *

Locks are either specific to a tld or global to the entire system, in which case a scope of + * GLOBAL is used. * *

This is the "barebone" lock implementation, that requires manual locking and unlocking. For * safe calls that automatically lock and unlock, see LockHandler. */ @Entity @NotBackedUp(reason = Reason.TRANSIENT) -public class Lock extends ImmutableObject implements DatastoreOnlyEntity, Serializable { +@javax.persistence.Entity +@Table +@IdClass(Lock.LockId.class) +public class Lock extends ImmutableObject implements DatastoreAndSqlEntity, Serializable { private static final long serialVersionUID = 756397280691684645L; private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - /** Disposition of locking, for monitoring. */ - enum LockState { IN_USE, FREE, TIMED_OUT, OWNER_DIED } + /** + * The scope of a lock that is not specific to a single tld. + * + *

Note: we'd use a null "tld" here for global locks, except Hibernate/Postgres don't allow for + * null values in primary-key columns. + */ + static final String GLOBAL = "GLOBAL"; - @VisibleForTesting - static LockMetrics lockMetrics = new LockMetrics(); + /** Disposition of locking, for monitoring. */ + enum LockState { + IN_USE, + FREE, + TIMED_OUT, + OWNER_DIED + } + + @VisibleForTesting static LockMetrics lockMetrics = new LockMetrics(); /** The name of the locked resource. */ - @Id - String lockId; + @Transient @Id String lockId; /** * Unique log ID of the request that owns this lock. @@ -73,58 +93,62 @@ public class Lock extends ImmutableObject implements DatastoreOnlyEntity, Serial *

See {@link RequestStatusCheckerImpl#getLogId} for details about how it's created in * practice. */ + @Column(nullable = false) String requestLogId; /** When the lock can be considered implicitly released. */ + @Column(nullable = false) DateTime expirationTime; - public String getRequestLogId() { - return requestLogId; - } + /** When was the lock acquired. Used for logging. */ + @Column(nullable = false) + DateTime acquiredTime; + + /** The resource name used to create the lock. */ + @Column(nullable = false) + @javax.persistence.Id + String resourceName; + + /** The tld used to create the lock, or GLOBAL if it's cross-TLD. */ + // TODO(b/177567432): rename to "scope" post-Datastore + @Column(nullable = false, name = "scope") + @javax.persistence.Id + String tld; public DateTime getExpirationTime() { return expirationTime; } - public DateTime getAcquiredTime() { - return acquiredTime; + @PostLoad + void postLoad() { + lockId = makeLockId(resourceName, tld); } - /** When was the lock acquired. Used for logging. */ - DateTime acquiredTime; - - /** The resource name used to create lockId. */ - String resourceName; - - /** The tld used to create lockId. */ - @Nullable - String tld; - /** - * Create a new {@link Lock} for the given resource name in the specified tld (which can be null - * for cross-tld locks). + * Create a new {@link Lock} for the given resource name in the specified tld (or in the GLOBAL + * namespace). */ - public static Lock create( + private static Lock create( String resourceName, - @Nullable String tld, + String scope, String requestLogId, DateTime acquiredTime, Duration leaseLength) { checkArgument(!Strings.isNullOrEmpty(resourceName), "resourceName cannot be null or empty"); Lock instance = new Lock(); - // Add the tld to the Lock's id so that it is unique for locks acquiring the same resource + // Add the scope to the Lock's id so that it is unique for locks acquiring the same resource // across different TLDs. - instance.lockId = makeLockId(resourceName, tld); + instance.lockId = makeLockId(resourceName, scope); instance.requestLogId = requestLogId; instance.expirationTime = acquiredTime.plus(leaseLength); instance.acquiredTime = acquiredTime; instance.resourceName = resourceName; - instance.tld = tld; + instance.tld = scope; return instance; } - private static String makeLockId(String resourceName, @Nullable String tld) { - return String.format("%s-%s", tld, resourceName); + private static String makeLockId(String resourceName, String scope) { + return String.format("%s-%s", scope, resourceName); } @AutoValue @@ -186,21 +210,23 @@ public class Lock extends ImmutableObject implements DatastoreOnlyEntity, Serial Duration leaseLength, RequestStatusChecker requestStatusChecker, boolean checkThreadRunning) { - String lockId = makeLockId(resourceName, tld); + String scope = (tld != null) ? tld : GLOBAL; + String lockId = makeLockId(resourceName, scope); // It's important to use transactNew rather than transact, because a Lock can be used to control // access to resources like GCS that can't be transactionally rolled back. Therefore, the lock // must be definitively acquired before it is used, even when called inside another transaction. AcquireResult acquireResult = - ofyTm() - .transactNew( + tm().transactNew( () -> { - DateTime now = ofyTm().getTransactionTime(); + DateTime now = tm().getTransactionTime(); // Checking if an unexpired lock still exists - if so, the lock can't be acquired. Lock lock = - ofyTm() - .loadByKeyIfPresent( - VKey.createOfy(Lock.class, Key.create(Lock.class, lockId))) + tm().loadByKeyIfPresent( + VKey.create( + Lock.class, + new LockId(resourceName, scope), + Key.create(Lock.class, lockId))) .orElse(null); if (lock != null) { logger.atInfo().log( @@ -220,43 +246,44 @@ public class Lock extends ImmutableObject implements DatastoreOnlyEntity, Serial } Lock newLock = - create(resourceName, tld, requestStatusChecker.getLogId(), now, leaseLength); + create( + resourceName, scope, requestStatusChecker.getLogId(), now, leaseLength); // Locks are not parented under an EntityGroupRoot (so as to avoid write - // contention) and - // don't need to be backed up. - ofyTm().putWithoutBackup(newLock); + // contention) and don't need to be backed up. + tm().putWithoutBackup(newLock); return AcquireResult.create(now, lock, newLock, lockState); }); logAcquireResult(acquireResult); - lockMetrics.recordAcquire(resourceName, tld, acquireResult.lockState()); + lockMetrics.recordAcquire(resourceName, scope, acquireResult.lockState()); return Optional.ofNullable(acquireResult.newLock()); } /** Release the lock. */ public void release() { // Just use the default clock because we aren't actually doing anything that will use the clock. - ofyTm() - .transact( + tm().transact( () -> { // To release a lock, check that no one else has already obtained it and if not // delete it. If the lock in Datastore was different then this lock is gone already; // this can happen if release() is called around the expiration time and the lock // expires underneath us. Lock loadedLock = - ofyTm() - .loadByKeyIfPresent( - VKey.createOfy(Lock.class, Key.create(Lock.class, lockId))) + tm().loadByKeyIfPresent( + VKey.create( + Lock.class, + new LockId(resourceName, tld), + Key.create(Lock.class, lockId))) .orElse(null); if (Lock.this.equals(loadedLock)) { - // Use noBackupOfy() so that we don't create a commit log entry for deleting the - // lock. + // Use deleteWithoutBackup() so that we don't create a commit log entry for deleting + // the lock. logger.atInfo().log("Deleting lock: %s", lockId); - ofyTm().deleteWithoutBackup(Lock.this); + tm().deleteWithoutBackup(Lock.this); lockMetrics.recordRelease( - resourceName, tld, new Duration(acquiredTime, ofyTm().getTransactionTime())); + resourceName, tld, new Duration(acquiredTime, tm().getTransactionTime())); } else { logger.atSevere().log( "The lock we acquired was transferred to someone else before we" @@ -268,4 +295,21 @@ public class Lock extends ImmutableObject implements DatastoreOnlyEntity, Serial } }); } + + static class LockId extends ImmutableObject implements Serializable { + + String resourceName; + + // TODO(b/177567432): rename to "scope" post-Datastore + @Column(name = "scope") + String tld; + + // Required for Hibernate + private LockId() {} + + LockId(String resourceName, String tld) { + this.resourceName = checkArgumentNotNull(resourceName, "The resource name cannot be null"); + this.tld = checkArgumentNotNull(tld, "Scope of a lock cannot be null"); + } + } } diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsListShard.java b/core/src/main/java/google/registry/model/tmch/ClaimsListShard.java index cb0e9e3d3..b1f4e1f66 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsListShard.java +++ b/core/src/main/java/google/registry/model/tmch/ClaimsListShard.java @@ -117,7 +117,7 @@ public class ClaimsListShard extends ImmutableObject implements NonReplicatedEnt * the DNL List creation datetime from the rfc. Since this field has been used by Datastore, we * cannot change its name until we finish the migration. * - *

TODO(b/166784536): Rename this field to tmdbGenerationTime. + *

TODO(b/177567432): Rename this field to tmdbGenerationTime. */ @Column(name = "tmdb_generation_time", nullable = false) DateTime creationTime; diff --git a/core/src/main/java/google/registry/schema/server/Lock.java b/core/src/main/java/google/registry/schema/server/Lock.java deleted file mode 100644 index b6695ead6..000000000 --- a/core/src/main/java/google/registry/schema/server/Lock.java +++ /dev/null @@ -1,140 +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 google.registry.util.PreconditionsUtils.checkArgumentNotNull; - -import google.registry.model.ImmutableObject; -import google.registry.schema.replay.DatastoreEntity; -import google.registry.schema.replay.SqlEntity; -import google.registry.schema.server.Lock.LockId; -import google.registry.util.DateTimeUtils; -import java.io.Serializable; -import java.time.ZonedDateTime; -import java.util.Optional; -import javax.persistence.Column; -import javax.persistence.Entity; -import javax.persistence.Id; -import javax.persistence.IdClass; -import javax.persistence.Table; -import org.joda.time.DateTime; -import org.joda.time.Duration; - -/** - * A lock on some shared resource. - * - *

Locks are either specific to a tld or global to the entire system, in which case a tld of - * {@link GLOBAL} is used. - * - *

This uses a compound primary key as defined in {@link LockId}. - */ -@Entity -@Table -@IdClass(LockId.class) -public class Lock implements SqlEntity { - - /** The resource name used to create the lock. */ - @Column(nullable = false) - @Id - String resourceName; - - /** The tld used to create the lock. */ - @Column(nullable = false) - @Id - String tld; - - /** - * Unique log ID of the request that owns this lock. - * - *

When that request is no longer running (is finished), the lock can be considered implicitly - * released. - * - *

See {@link RequestStatusCheckerImpl#getLogId} for details about how it's created in - * practice. - */ - @Column(nullable = false) - String requestLogId; - - /** When the lock was acquired. Used for logging. */ - @Column(nullable = false) - ZonedDateTime acquiredTime; - - /** When the lock can be considered implicitly released. */ - @Column(nullable = false) - ZonedDateTime expirationTime; - - /** The scope of a lock that is not specific to a single tld. */ - static final String GLOBAL = "GLOBAL"; - - /** - * Validate input and create a new {@link Lock} for the given resource name in the specified tld. - */ - private Lock( - String resourceName, - String tld, - String requestLogId, - DateTime acquiredTime, - Duration leaseLength) { - this.resourceName = checkArgumentNotNull(resourceName, "The resource name cannot be null"); - this.tld = checkArgumentNotNull(tld, "The tld cannot be null. For a global lock, use GLOBAL"); - this.requestLogId = - checkArgumentNotNull(requestLogId, "The requestLogId of the lock cannot be null"); - this.acquiredTime = - DateTimeUtils.toZonedDateTime( - checkArgumentNotNull(acquiredTime, "The acquired time of the lock cannot be null")); - checkArgumentNotNull(leaseLength, "The lease length of the lock cannot be null"); - this.expirationTime = DateTimeUtils.toZonedDateTime(acquiredTime.plus(leaseLength)); - } - - // Hibernate requires a default constructor. - private Lock() {} - - /** Constructs a {@link Lock} object. */ - public static Lock create( - String resourceName, - String tld, - String requestLogId, - DateTime acquiredTime, - Duration leaseLength) { - checkArgumentNotNull( - tld, "The tld cannot be null. To create a global lock, use the createGlobal method"); - return new Lock(resourceName, tld, requestLogId, acquiredTime, leaseLength); - } - - /** Constructs a {@link Lock} object with a {@link GLOBAL} scope. */ - public static Lock createGlobal( - String resourceName, String requestLogId, DateTime acquiredTime, Duration leaseLength) { - return new Lock(resourceName, GLOBAL, requestLogId, acquiredTime, leaseLength); - } - - @Override - public Optional toDatastoreEntity() { - return Optional.empty(); // Locks are not converted since they are ephemeral - } - - static class LockId extends ImmutableObject implements Serializable { - - String resourceName; - - String tld; - - private LockId() {} - - LockId(String resourceName, String tld) { - this.resourceName = checkArgumentNotNull(resourceName, "The resource name cannot be null"); - this.tld = tld; - } - } -} diff --git a/core/src/main/java/google/registry/schema/server/LockDao.java b/core/src/main/java/google/registry/schema/server/LockDao.java deleted file mode 100644 index 874444b73..000000000 --- a/core/src/main/java/google/registry/schema/server/LockDao.java +++ /dev/null @@ -1,136 +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 google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; -import static google.registry.schema.server.Lock.GLOBAL; -import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; - -import com.google.common.flogger.FluentLogger; -import google.registry.schema.server.Lock.LockId; -import google.registry.util.DateTimeUtils; -import java.util.Optional; - -/** Data access object class for {@link Lock}. */ -public class LockDao { - - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - - /** Saves the {@link Lock} object to Cloud SQL. */ - public static void save(Lock lock) { - jpaTm() - .transact( - () -> { - jpaTm().put(lock); - }); - } - - /** - * Loads and returns a {@link Lock} object with the given resourceName and tld from Cloud SQL if - * it exists, else empty. - */ - public static Optional load(String resourceName, String tld) { - checkArgumentNotNull(resourceName, "The resource name of the lock to load cannot be null"); - checkArgumentNotNull(tld, "The tld of the lock to load cannot be null"); - return Optional.ofNullable( - jpaTm() - .transact( - () -> jpaTm().getEntityManager().find(Lock.class, new LockId(resourceName, tld)))); - } - - /** - * Loads a global {@link Lock} object with the given resourceName from Cloud SQL if it exists, - * else empty. - */ - public static Optional load(String resourceName) { - return load(resourceName, GLOBAL); - } - - /** - * Deletes the {@link Lock} object with the given resourceName and tld from Cloud SQL. This method - * is idempotent and will simply return if the lock has already been deleted. - */ - public static void delete(String resourceName, String tld) { - jpaTm() - .transact( - () -> { - Optional loadedLock = load(resourceName, tld); - if (loadedLock.isPresent()) { - jpaTm().delete(loadedLock.get()); - } - }); - } - - /** - * Deletes the global {@link Lock} object with the given resourceName from Cloud SQL. This method - * is idempotent and will simply return if the lock has already been deleted. - */ - public static void delete(String resourceName) { - delete(resourceName, GLOBAL); - } - - /** - * Compares a {@link google.registry.model.server.Lock} object with a {@link Lock} object, logging - * a warning if there are any differences. - */ - public static void compare( - Optional datastoreLockOptional, - Optional cloudSqlLockOptional) { - if (!datastoreLockOptional.isPresent()) { - cloudSqlLockOptional.ifPresent( - value -> - logger.atWarning().log( - String.format( - "Cloud SQL lock for %s with tld %s should be null", - value.resourceName, value.tld))); - return; - } - google.registry.schema.server.Lock cloudSqlLock; - google.registry.model.server.Lock datastoreLock = datastoreLockOptional.get(); - if (cloudSqlLockOptional.isPresent()) { - cloudSqlLock = cloudSqlLockOptional.get(); - if (!datastoreLock.getRequestLogId().equals(cloudSqlLock.requestLogId)) { - logger.atWarning().log( - String.format( - "Datastore lock requestLogId of %s does not equal Cloud SQL lock requestLogId of" - + " %s", - datastoreLock.getRequestLogId(), cloudSqlLock.requestLogId)); - } - if (!datastoreLock - .getAcquiredTime() - .equals(DateTimeUtils.toJodaDateTime(cloudSqlLock.acquiredTime))) { - logger.atWarning().log( - String.format( - "Datastore lock acquiredTime of %s does not equal Cloud SQL lock acquiredTime of" - + " %s", - datastoreLock.getAcquiredTime(), - DateTimeUtils.toJodaDateTime(cloudSqlLock.acquiredTime))); - } - if (!datastoreLock - .getExpirationTime() - .equals(DateTimeUtils.toJodaDateTime(cloudSqlLock.expirationTime))) { - logger.atWarning().log( - String.format( - "Datastore lock expirationTime of %s does not equal Cloud SQL lock expirationTime" - + " of %s", - datastoreLock.getExpirationTime(), - DateTimeUtils.toJodaDateTime(cloudSqlLock.expirationTime))); - } - } else { - logger.atWarning().log( - String.format("Datastore lock: %s was not found in Cloud SQL", datastoreLock)); - } - } -} diff --git a/core/src/main/resources/META-INF/persistence.xml b/core/src/main/resources/META-INF/persistence.xml index fec4743bb..7f82a42c5 100644 --- a/core/src/main/resources/META-INF/persistence.xml +++ b/core/src/main/resources/META-INF/persistence.xml @@ -65,6 +65,7 @@ google.registry.model.reporting.DomainTransactionRecord google.registry.model.reporting.Spec11ThreatMatch google.registry.model.server.KmsSecretRevision + google.registry.model.server.Lock google.registry.model.server.ServerSecret google.registry.model.smd.SignedMarkRevocationList google.registry.model.tmch.ClaimsListShard @@ -72,7 +73,6 @@ google.registry.persistence.transaction.TransactionEntity google.registry.schema.domain.RegistryLock google.registry.schema.replay.SqlReplayCheckpoint - google.registry.schema.server.Lock google.registry.schema.tld.PremiumEntry diff --git a/core/src/test/java/google/registry/model/server/LockTest.java b/core/src/test/java/google/registry/model/server/LockTest.java index d72431310..12526970c 100644 --- a/core/src/test/java/google/registry/model/server/LockTest.java +++ b/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 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 = 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 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( diff --git a/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java b/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java index af9d77ced..a1f8847bd 100644 --- a/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java +++ b/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, diff --git a/core/src/test/java/google/registry/schema/server/LockDaoTest.java b/core/src/test/java/google/registry/schema/server/LockDaoTest.java deleted file mode 100644 index c833a00b0..000000000 --- a/core/src/test/java/google/registry/schema/server/LockDaoTest.java +++ /dev/null @@ -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 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 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 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 returnedLock = LockDao.load("testResource"); - assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime); - assertThat(returnedLock.get().requestLogId).isEqualTo(lock.requestLogId); - } - - @Test - void load_worksSuccesfullyLockDoesNotExist() { - Optional 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 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 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"); - } -} diff --git a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html index dd81c2737..6c2b8c420 100644 --- a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html @@ -261,11 +261,11 @@ td.section { generated on - 2021-04-21 21:56:39.575987 + 2021-05-07 18:02:12.304005 last flyway file - V93__defer_all_fkeys.sql + V94__rename_lock_scope.sql @@ -284,7 +284,7 @@ td.section { generated on - 2021-04-21 21:56:39.575987 + 2021-05-07 18:02:12.304005 @@ -2535,7 +2535,7 @@ td.section { text not null - tld + "scope" @@ -5415,7 +5415,7 @@ td.section { - tld + "scope" text not null @@ -5438,7 +5438,7 @@ td.section { - tld + "scope" diff --git a/db/src/main/resources/sql/er_diagram/full_er_diagram.html b/db/src/main/resources/sql/er_diagram/full_er_diagram.html index 7c0f0fb83..ff0b16ec4 100644 --- a/db/src/main/resources/sql/er_diagram/full_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/full_er_diagram.html @@ -261,11 +261,11 @@ td.section { generated on - 2021-04-21 21:56:37.513728 + 2021-05-07 18:02:10.32026 last flyway file - V93__defer_all_fkeys.sql + V94__rename_lock_scope.sql @@ -274,19 +274,19 @@ td.section { SchemaCrawler_Diagram - + generated by - + SchemaCrawler 16.10.1 - + generated on - - 2021-04-21 21:56:37.513728 + + 2021-05-07 18:02:10.32026 - + allocationtoken_a08ccbef @@ -5655,7 +5655,7 @@ td.section { text not null - tld + "scope" @@ -11297,7 +11297,7 @@ td.section {
-
tld + "scope" text not null @@ -11335,7 +11335,7 @@ td.section { - tld + "scope" @@ -11358,7 +11358,7 @@ td.section { - tld + "scope" ascending diff --git a/db/src/main/resources/sql/flyway.txt b/db/src/main/resources/sql/flyway.txt index 5bc8f8c11..fd099c980 100644 --- a/db/src/main/resources/sql/flyway.txt +++ b/db/src/main/resources/sql/flyway.txt @@ -91,3 +91,4 @@ V90__update_timestamp.sql V91__defer_fkeys.sql V92__singletons.sql V93__defer_all_fkeys.sql +V94__rename_lock_scope.sql diff --git a/db/src/main/resources/sql/flyway/V94__rename_lock_scope.sql b/db/src/main/resources/sql/flyway/V94__rename_lock_scope.sql new file mode 100644 index 000000000..ecb8f17a7 --- /dev/null +++ b/db/src/main/resources/sql/flyway/V94__rename_lock_scope.sql @@ -0,0 +1,15 @@ +-- Copyright 2021 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. + +ALTER TABLE IF EXISTS "Lock" RENAME tld TO scope; diff --git a/db/src/main/resources/sql/schema/db-schema.sql.generated b/db/src/main/resources/sql/schema/db-schema.sql.generated index c6c06137d..da07d9b19 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/db/src/main/resources/sql/schema/db-schema.sql.generated @@ -501,11 +501,11 @@ create table "Lock" ( resource_name text not null, - tld text not null, + scope text not null, acquired_time timestamptz not null, expiration_time timestamptz not null, request_log_id text not null, - primary key (resource_name, tld) + primary key (resource_name, scope) ); create table "PollMessage" ( diff --git a/db/src/main/resources/sql/schema/nomulus.golden.sql b/db/src/main/resources/sql/schema/nomulus.golden.sql index a554c0640..2d8e5a353 100644 --- a/db/src/main/resources/sql/schema/nomulus.golden.sql +++ b/db/src/main/resources/sql/schema/nomulus.golden.sql @@ -653,7 +653,7 @@ CREATE TABLE public."KmsSecret" ( CREATE TABLE public."Lock" ( resource_name text NOT NULL, - tld text NOT NULL, + scope text NOT NULL, acquired_time timestamp with time zone NOT NULL, expiration_time timestamp with time zone NOT NULL, request_log_id text NOT NULL @@ -1303,7 +1303,7 @@ ALTER TABLE ONLY public."KmsSecret" -- ALTER TABLE ONLY public."Lock" - ADD CONSTRAINT "Lock_pkey" PRIMARY KEY (resource_name, tld); + ADD CONSTRAINT "Lock_pkey" PRIMARY KEY (resource_name, scope); --