diff --git a/core/src/main/java/google/registry/model/smd/SignedMarkRevocationList.java b/core/src/main/java/google/registry/model/smd/SignedMarkRevocationList.java index 72435884c..083882f34 100644 --- a/core/src/main/java/google/registry/model/smd/SignedMarkRevocationList.java +++ b/core/src/main/java/google/registry/model/smd/SignedMarkRevocationList.java @@ -95,10 +95,4 @@ public class SignedMarkRevocationList extends ImmutableObject { public int size() { return revokes.size(); } - - /** Save this list to Cloud SQL. Returns {@code this}. */ - public SignedMarkRevocationList save() { - SignedMarkRevocationListDao.save(this); - return this; - } } diff --git a/core/src/main/java/google/registry/model/smd/SignedMarkRevocationListDao.java b/core/src/main/java/google/registry/model/smd/SignedMarkRevocationListDao.java index ed633c2e7..cf1f8df53 100644 --- a/core/src/main/java/google/registry/model/smd/SignedMarkRevocationListDao.java +++ b/core/src/main/java/google/registry/model/smd/SignedMarkRevocationListDao.java @@ -44,11 +44,25 @@ public class SignedMarkRevocationListDao { return smdrl.orElseGet(() -> SignedMarkRevocationList.create(START_OF_TIME, ImmutableMap.of())); } - /** Save the given {@link SignedMarkRevocationList} */ - static void save(SignedMarkRevocationList signedMarkRevocationList) { - tm().transact(() -> tm().insert(signedMarkRevocationList)); + /** + * Persists a {@link SignedMarkRevocationList} instance and returns the persisted entity. + * + *

Note that the input parameter is untouched. Use the returned object if metadata fields like + * {@code revisionId} are needed. + */ + public static SignedMarkRevocationList save(SignedMarkRevocationList signedMarkRevocationList) { + var persisted = + tm().transact( + () -> { + var entity = + SignedMarkRevocationList.create( + signedMarkRevocationList.getCreationTime(), + ImmutableMap.copyOf(signedMarkRevocationList.revokes)); + tm().insert(entity); + return entity; + }); logger.atInfo().log( - "Inserted %,d signed mark revocations into Cloud SQL.", - signedMarkRevocationList.revokes.size()); + "Inserted %,d signed mark revocations into Cloud SQL.", persisted.revokes.size()); + return persisted; } } diff --git a/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java b/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java index fe4b14c13..531eb8336 100644 --- a/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java +++ b/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java @@ -133,24 +133,30 @@ public final class PremiumListDao { } /** Saves the given premium list (and its premium list entries) to Cloud SQL. */ - public static PremiumList save(PremiumList premiumList) { - tm().transact( - () -> { - tm().insert(premiumList); - tm().getEntityManager().flush(); // This populates the revisionId. - long revisionId = premiumList.getRevisionId(); + public static PremiumList save(PremiumList premiumListToPersist) { + PremiumList persisted = + tm().transact( + () -> { + // Make a new copy in each attempt to insert. See javadoc of the insert method for + // more information. + PremiumList premiumList = premiumListToPersist.asBuilder().build(); + tm().insert(premiumList); + tm().getEntityManager().flush(); // This populates the revisionId. + long revisionId = premiumList.getRevisionId(); - if (!isNullOrEmpty(premiumList.getLabelsToPrices())) { - ImmutableSet.Builder entries = new ImmutableSet.Builder<>(); - premiumList - .getLabelsToPrices() - .forEach( - (key, value) -> entries.add(PremiumEntry.create(revisionId, value, key))); - tm().insertAll(entries.build()); - } - }); - premiumListCache.invalidate(premiumList.getName()); - return premiumList; + if (!isNullOrEmpty(premiumList.getLabelsToPrices())) { + ImmutableSet.Builder entries = new ImmutableSet.Builder<>(); + premiumList + .getLabelsToPrices() + .forEach( + (key, value) -> + entries.add(PremiumEntry.create(revisionId, value, key))); + tm().insertAll(entries.build()); + } + return premiumList; + }); + premiumListCache.invalidate(persisted.getName()); + return persisted; } public static void delete(PremiumList premiumList) { diff --git a/core/src/main/java/google/registry/model/tld/label/ReservedListDao.java b/core/src/main/java/google/registry/model/tld/label/ReservedListDao.java index 6a69dfb0e..19beaed6a 100644 --- a/core/src/main/java/google/registry/model/tld/label/ReservedListDao.java +++ b/core/src/main/java/google/registry/model/tld/label/ReservedListDao.java @@ -27,14 +27,26 @@ public class ReservedListDao { private ReservedListDao() {} - /** Persist a new reserved list to Cloud SQL. */ - public static void save(ReservedList reservedList) { + /** + * Persists a new reserved list to Cloud SQL and returns the persisted entity. + * + *

Note that the input parameter is untouched. Use the returned object if metadata fields like + * {@code revisionId} are needed. + */ + public static ReservedList save(ReservedList reservedList) { checkArgumentNotNull(reservedList, "Must specify reservedList"); logger.atInfo().log("Saving reserved list %s to Cloud SQL.", reservedList.getName()); - tm().transact(() -> tm().insert(reservedList)); + var persisted = + tm().transact( + () -> { + var entity = reservedList.asBuilder().build(); + tm().insert(entity); + return entity; + }); logger.atInfo().log( "Saved reserved list %s with %d entries to Cloud SQL.", reservedList.getName(), reservedList.getReservedListEntries().size()); + return persisted; } /** Deletes a reserved list from Cloud SQL. */ diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java b/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java index 11e22ecf8..ef668d891 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java +++ b/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java @@ -49,10 +49,23 @@ public class ClaimsListDao { return CacheUtils.newCacheBuilder(expiry).build(ignored -> ClaimsListDao.getUncached()); } - /** Saves the given {@link ClaimsList} to Cloud SQL. */ - public static void save(ClaimsList claimsList) { - tm().transact(() -> tm().insert(claimsList)); - CACHE.put(ClaimsListDao.class, claimsList); + /** + * Persists a {@link ClaimsList} instance and returns the persisted entity. + * + *

Note that the input parameter is untouched. Use the returned object if metadata fields like + * {@code revisionId} are needed. + */ + public static ClaimsList save(ClaimsList claimsList) { + var persisted = + tm().transact( + () -> { + var entity = + ClaimsList.create(claimsList.tmdbGenerationTime, claimsList.labelsToKeys); + tm().insert(entity); + return entity; + }); + CACHE.put(ClaimsListDao.class, persisted); + return persisted; } /** Returns the most recent revision of the {@link ClaimsList} from the cache. */ diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java index 5c0ebab80..65f48c834 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -344,6 +344,18 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { return txnInfo.transactionTime; } + /** + * Inserts an object into the database. + * + *

If {@code entity} has an auto-generated identity field (i.e., a field annotated with {@link + * jakarta.persistence.GeneratedValue}), the caller must not assign a value to this field, + * otherwise Hibernate would mistake the entity as detached and raise an error. + * + *

The practical implication of the above is that when inserting such an entity using a + * retriable transaction , the entity should be instantiated inside the transaction body. A failed + * attempt may still assign and ID to the entity, therefore reusing the same entity would cause + * retries to fail. + */ @Override public void insert(Object entity) { checkArgumentNotNull(entity, "entity must be specified"); diff --git a/core/src/main/java/google/registry/tmch/TmchSmdrlAction.java b/core/src/main/java/google/registry/tmch/TmchSmdrlAction.java index 2376bd041..b29801597 100644 --- a/core/src/main/java/google/registry/tmch/TmchSmdrlAction.java +++ b/core/src/main/java/google/registry/tmch/TmchSmdrlAction.java @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.flogger.FluentLogger; import google.registry.keyring.api.KeyModule.Key; import google.registry.model.smd.SignedMarkRevocationList; +import google.registry.model.smd.SignedMarkRevocationListDao; import google.registry.request.Action; import google.registry.request.Action.GaeService; import google.registry.request.auth.Auth; @@ -57,7 +58,7 @@ public final class TmchSmdrlAction implements Runnable { } catch (GeneralSecurityException | IOException | PGPException e) { throw new RuntimeException(e); } - smdrl.save(); + smdrl = SignedMarkRevocationListDao.save(smdrl); logger.atInfo().log( "Inserted %,d smd revocations into the database, created at %s.", smdrl.size(), smdrl.getCreationTime()); diff --git a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java index eda83aa1e..df4e01c38 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java @@ -179,6 +179,7 @@ import google.registry.model.reporting.DomainTransactionRecord; import google.registry.model.reporting.DomainTransactionRecord.TransactionReportField; import google.registry.model.reporting.HistoryEntry; import google.registry.model.reporting.HistoryEntry.HistoryEntryId; +import google.registry.model.smd.SignedMarkRevocationListDao; import google.registry.model.tld.Tld; import google.registry.model.tld.Tld.TldState; import google.registry.model.tld.Tld.TldType; @@ -2727,8 +2728,9 @@ class DomainCreateFlowTest extends ResourceFlowTestCase { + SignedMarkRevocationListDao.save(list); + if (isFirstAttempt.get()) { + isFirstAttempt.set(false); + throw new OptimisticLockException(); + } + }); + SignedMarkRevocationList fromDb = SignedMarkRevocationListDao.load(); + assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list, "revisionId"); + } + @Test void testSaveAndLoad_emptyList() { SignedMarkRevocationList list = diff --git a/core/src/test/java/google/registry/model/smd/SignedMarkRevocationListTest.java b/core/src/test/java/google/registry/model/smd/SignedMarkRevocationListTest.java index bf83ace3c..cc4881600 100644 --- a/core/src/test/java/google/registry/model/smd/SignedMarkRevocationListTest.java +++ b/core/src/test/java/google/registry/model/smd/SignedMarkRevocationListTest.java @@ -48,7 +48,8 @@ public class SignedMarkRevocationListTest { for (int i = 0; i < rows; i++) { revokes.put(Integer.toString(i), clock.nowUtc()); } - SignedMarkRevocationList.create(clock.nowUtc(), revokes.build()).save(); + SignedMarkRevocationListDao.save( + SignedMarkRevocationList.create(clock.nowUtc(), revokes.build())); SignedMarkRevocationList res = SignedMarkRevocationList.get(); assertThat(res.size()).isEqualTo(rows); return res; diff --git a/core/src/test/java/google/registry/model/tld/label/PremiumListDaoTest.java b/core/src/test/java/google/registry/model/tld/label/PremiumListDaoTest.java index 636d4a619..0591f5e98 100644 --- a/core/src/test/java/google/registry/model/tld/label/PremiumListDaoTest.java +++ b/core/src/test/java/google/registry/model/tld/label/PremiumListDaoTest.java @@ -31,10 +31,12 @@ import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationWithCoverageExtension; import google.registry.testing.FakeClock; import google.registry.testing.TestCacheExtension; +import jakarta.persistence.OptimisticLockException; import java.math.BigDecimal; import java.time.Duration; import java.util.Optional; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.IntStream; import org.joda.money.CurrencyUnit; import org.joda.money.Money; @@ -93,6 +95,27 @@ public class PremiumListDaoTest { }); } + @Test + void saveNew_retry_success() { + AtomicBoolean isFirstAttempt = new AtomicBoolean(true); + tm().transact( + () -> { + PremiumListDao.save(testList); + if (isFirstAttempt.get()) { + isFirstAttempt.set(false); + throw new OptimisticLockException(); + } + }); + tm().transact( + () -> { + Optional persistedListOpt = PremiumListDao.getLatestRevision("testname"); + assertThat(persistedListOpt).isPresent(); + PremiumList persistedList = persistedListOpt.get(); + assertThat(persistedList.getLabelsToPrices()).containsExactlyEntriesIn(TEST_PRICES); + assertThat(persistedList.getCreationTimestamp()).isEqualTo(fakeClock.nowUtc()); + }); + } + @Test void update_worksSuccessfully() { PremiumListDao.save(testList); diff --git a/core/src/test/java/google/registry/model/tld/label/ReservedListDaoTest.java b/core/src/test/java/google/registry/model/tld/label/ReservedListDaoTest.java index b609a3de7..625fa6880 100644 --- a/core/src/test/java/google/registry/model/tld/label/ReservedListDaoTest.java +++ b/core/src/test/java/google/registry/model/tld/label/ReservedListDaoTest.java @@ -22,6 +22,8 @@ import google.registry.model.tld.label.ReservedList.ReservedListEntry; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationWithCoverageExtension; import google.registry.testing.FakeClock; +import jakarta.persistence.OptimisticLockException; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -71,11 +73,34 @@ public class ReservedListDaoTest { }); } + @Test + void save_withRetry_worksSuccessfully() { + AtomicBoolean isFirstAttempt = new AtomicBoolean(true); + tm().transact( + () -> { + ReservedListDao.save(testReservedList); + if (isFirstAttempt.get()) { + isFirstAttempt.set(false); + throw new OptimisticLockException(); + } + }); + tm().transact( + () -> { + ReservedList persistedList = + tm().query("FROM ReservedList WHERE name = :name", ReservedList.class) + .setParameter("name", "testlist") + .getSingleResult(); + assertThat(persistedList.getReservedListEntries()) + .containsExactlyEntriesIn(testReservations); + assertThat(persistedList.getCreationTimestamp()).isEqualTo(fakeClock.nowUtc()); + }); + } + @Test void delete_worksSuccessfully() { - ReservedListDao.save(testReservedList); + var persisted = ReservedListDao.save(testReservedList); assertThat(ReservedListDao.checkExists("testlist")).isTrue(); - ReservedListDao.delete(testReservedList); + ReservedListDao.delete(persisted); assertThat(ReservedListDao.checkExists("testlist")).isFalse(); } diff --git a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java index 60f1bc3cf..1f835318e 100644 --- a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java +++ b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java @@ -16,15 +16,15 @@ package google.registry.model.tmch; import static com.google.common.truth.Truth.assertThat; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; -import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableMap; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationWithCoverageExtension; import google.registry.testing.FakeClock; import google.registry.testing.TestCacheExtension; -import jakarta.persistence.PersistenceException; +import jakarta.persistence.OptimisticLockException; import java.time.Duration; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -49,27 +49,36 @@ public class ClaimsListDaoTest { void save_insertsClaimsListSuccessfully() { ClaimsList claimsList = ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2")); - ClaimsListDao.save(claimsList); + claimsList = ClaimsListDao.save(claimsList); ClaimsList insertedClaimsList = ClaimsListDao.get(); assertClaimsListEquals(claimsList, insertedClaimsList); assertThat(insertedClaimsList.getCreationTimestamp()).isEqualTo(fakeClock.nowUtc()); } @Test - void save_fail_duplicateId() { + void save_insertsClaimsListSuccessfully_withRetries() { ClaimsList claimsList = ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2")); - ClaimsListDao.save(claimsList); + AtomicBoolean isFirstAttempt = new AtomicBoolean(true); + tm().transact( + () -> { + ClaimsListDao.save(claimsList); + if (isFirstAttempt.get()) { + isFirstAttempt.set(false); + throw new OptimisticLockException(); + } + }); ClaimsList insertedClaimsList = ClaimsListDao.get(); - assertClaimsListEquals(claimsList, insertedClaimsList); - // Save ClaimsList with existing revisionId should fail because revisionId is the primary key. - assertThrows(PersistenceException.class, () -> ClaimsListDao.save(insertedClaimsList)); + assertThat(insertedClaimsList.getTmdbGenerationTime()) + .isEqualTo(claimsList.getTmdbGenerationTime()); + assertThat(insertedClaimsList.getLabelsToKeys()).isEqualTo(claimsList.getLabelsToKeys()); + assertThat(insertedClaimsList.getCreationTimestamp()).isEqualTo(fakeClock.nowUtc()); } @Test void save_claimsListWithNoEntries() { ClaimsList claimsList = ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of()); - ClaimsListDao.save(claimsList); + claimsList = ClaimsListDao.save(claimsList); ClaimsList insertedClaimsList = ClaimsListDao.get(); assertClaimsListEquals(claimsList, insertedClaimsList); assertThat(insertedClaimsList.getLabelsToKeys()).isEmpty(); @@ -86,8 +95,8 @@ public class ClaimsListDaoTest { ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2")); ClaimsList newClaimsList = ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label3", "key3", "label4", "key4")); - ClaimsListDao.save(oldClaimsList); - ClaimsListDao.save(newClaimsList); + oldClaimsList = ClaimsListDao.save(oldClaimsList); + newClaimsList = ClaimsListDao.save(newClaimsList); assertClaimsListEquals(newClaimsList, ClaimsListDao.get()); } @@ -96,11 +105,11 @@ public class ClaimsListDaoTest { assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isNull(); ClaimsList oldList = ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2")); - ClaimsListDao.save(oldList); + oldList = ClaimsListDao.save(oldList); assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isEqualTo(oldList); ClaimsList newList = ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label3", "key3", "label4", "key4")); - ClaimsListDao.save(newList); + newList = ClaimsListDao.save(newList); assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isEqualTo(newList); }