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.

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;
-  }
 }

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) {
+   * Persists a {@link SignedMarkRevocationList} instance and returns the persisted entity.
-    tm().transact(() -> tm().insert(signedMarkRevocationList));
+   *
+   * <p>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.",
+        "Inserted %,d signed mark revocations into Cloud SQL.", persisted.revokes.size());
-        signedMarkRevocationList.revokes.size());
+    return persisted;
   }
 }

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) {
+  public static PremiumList save(PremiumList premiumListToPersist) {
-    tm().transact(
+    PremiumList persisted =
-            () -> {
+        tm().transact(
-              tm().insert(premiumList);
+                () -> {
-              tm().getEntityManager().flush(); // This populates the revisionId.
+                  // Make a new copy in each attempt to insert. See javadoc of the insert method for
-              long revisionId = premiumList.getRevisionId();
+                  // 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())) {
+                  if (!isNullOrEmpty(premiumList.getLabelsToPrices())) {
-                ImmutableSet.Builder<PremiumEntry> entries = new ImmutableSet.Builder<>();
+                    ImmutableSet.Builder<PremiumEntry> entries = new ImmutableSet.Builder<>();
-                premiumList
+                    premiumList
-                    .getLabelsToPrices()
+                        .getLabelsToPrices()
-                    .forEach(
+                        .forEach(
-                        (key, value) -> entries.add(PremiumEntry.create(revisionId, value, key)));
+                            (key, value) ->
-                tm().insertAll(entries.build());
+                                entries.add(PremiumEntry.create(revisionId, value, key)));
-              }
+                    tm().insertAll(entries.build());
-            });
+                  }
-    premiumListCache.invalidate(premiumList.getName());
+                  return premiumList;
-    return premiumList;
+                });
+    premiumListCache.invalidate(persisted.getName());
+    return persisted;
   }
 
   public static void delete(PremiumList premiumList) {

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.
+   *
+   * <p>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. */

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) {
+   * Persists a {@link ClaimsList} instance and returns the persisted entity.
-    tm().transact(() -> tm().insert(claimsList));
+   *
-    CACHE.put(ClaimsListDao.class, claimsList);
+   * <p>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. */

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.
+   *
+   * <p>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.
+   *
+   * <p>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");

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());

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<DomainCreateFlow, Domain
 
   @Test
   void testFail_startDateSunriseRegistration_revokedSignedMark() throws Exception {
-    SmdrlCsvParser.parse(TmchTestData.loadFile("smd/smdrl.csv").lines().collect(toImmutableList()))
+    SignedMarkRevocationListDao.save(
-        .save();
+        SmdrlCsvParser.parse(
+            TmchTestData.loadFile("smd/smdrl.csv").lines().collect(toImmutableList())));
     createTld("tld", START_DATE_SUNRISE);
     clock.setTo(SMD_VALID_TIME);
     String revokedSmd =
@@ -2753,9 +2755,9 @@ class DomainCreateFlowTest extends ResourceFlowTestCase<DomainCreateFlow, Domain
     if (labels.isEmpty()) {
       return;
     }
-    SmdrlCsvParser.parse(
+    SignedMarkRevocationListDao.save(
-            TmchTestData.loadFile("idn/idn_smdrl.csv").lines().collect(toImmutableList()))
+        SmdrlCsvParser.parse(
-        .save();
+            TmchTestData.loadFile("idn/idn_smdrl.csv").lines().collect(toImmutableList())));
     createTld("tld", START_DATE_SUNRISE);
     clock.setTo(SMD_VALID_TIME);
     String revokedSmd =

core/src/test/java/google/registry/model/smd/SignedMarkRevocationListDaoTest.java

@@ -16,9 +16,12 @@ package google.registry.model.smd;
 
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
+import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 
 import com.google.common.collect.ImmutableMap;
 import google.registry.model.EntityTestCase;
+import jakarta.persistence.OptimisticLockException;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.junit.jupiter.api.Test;
 
 public class SignedMarkRevocationListDaoTest extends EntityTestCase {
@@ -32,11 +35,29 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(
             fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    SignedMarkRevocationListDao.save(list);
+    list = SignedMarkRevocationListDao.save(list);
     SignedMarkRevocationList fromDb = SignedMarkRevocationListDao.load();
     assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list);
   }
 
+  @Test
+  void testSave_retrySuccess() {
+    SignedMarkRevocationList list =
+        SignedMarkRevocationList.create(
+            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
+    AtomicBoolean isFirstAttempt = new AtomicBoolean(true);
+    tm().transact(
+            () -> {
+              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 =

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;

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<PremiumList> 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);

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();
   }
 

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);
+    assertThat(insertedClaimsList.getTmdbGenerationTime())
-    // Save ClaimsList with existing revisionId should fail because revisionId is the primary key.
+        .isEqualTo(claimsList.getTmdbGenerationTime());
-    assertThrows(PersistenceException.class, () -> ClaimsListDao.save(insertedClaimsList));
+    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);
+    oldClaimsList = ClaimsListDao.save(oldClaimsList);
-    ClaimsListDao.save(newClaimsList);
+    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);
   }