From a20eb8d1e993df24d53deacf14dff124f2ceddcc Mon Sep 17 00:00:00 2001 From: Weimin Yu Date: Fri, 1 Aug 2025 18:53:50 +0000 Subject: [PATCH] Fix failures in retries when inserting new objects (#2788) Given an entity with auto-filled id fields (annotated with @GeneratedValue, with null as initial value), when inserting it using Hibernate, the id fields will be filled with non-nulls even if the transaction fails. If the same entity instance is used again in a retry, Hibernate mistakes it as a detached entity and raises an error. The work around is to make a new copy of the entity in each transaction. This PR applies this pattern to affected entity types. We considered applying this pattern to JpaTransactionManagerImpl's insert method so that individual call sites do not have to change. However, we decided against it because: - It is unnecessary for entity types that do not have auto-filled id - The JpaTransactionManager cannot tell if copying is cheap or expensive. It is better exposing this to the user. - The JpaTransactionManager needs to know how to clone entities. A new interface may need to be introduced just for a handful of use cases. --- .../model/smd/SignedMarkRevocationList.java | 6 --- .../smd/SignedMarkRevocationListDao.java | 24 ++++++++--- .../model/tld/label/PremiumListDao.java | 40 +++++++++++-------- .../model/tld/label/ReservedListDao.java | 18 +++++++-- .../registry/model/tmch/ClaimsListDao.java | 21 ++++++++-- .../JpaTransactionManagerImpl.java | 12 ++++++ .../google/registry/tmch/TmchSmdrlAction.java | 3 +- .../flows/domain/DomainCreateFlowTest.java | 12 +++--- .../smd/SignedMarkRevocationListDaoTest.java | 23 ++++++++++- .../smd/SignedMarkRevocationListTest.java | 3 +- .../model/tld/label/PremiumListDaoTest.java | 23 +++++++++++ .../model/tld/label/ReservedListDaoTest.java | 29 +++++++++++++- .../model/tmch/ClaimsListDaoTest.java | 35 ++++++++++------ 13 files changed, 191 insertions(+), 58 deletions(-) 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); }