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.

Author: weiminyu

core/src/main/java/google/registry/model/smd/SignedMarkRevocationList.java

@@ -95,10 +95,4 @@ public DateTime getCreationTime() {
   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 @@ static SignedMarkRevocationList load() {
     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.
+   *
+   * <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.",
-        signedMarkRevocationList.revokes.size());
+        "Inserted %,d signed mark revocations into Cloud SQL.", persisted.revokes.size());
+    return persisted;
   }
 }

core/src/main/java/google/registry/model/tld/label/PremiumListDao.java

@@ -133,24 +133,30 @@ public static PremiumList save(String name, CurrencyUnit currencyUnit, List<Stri
   }
 
   /** 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();
-
-              if (!isNullOrEmpty(premiumList.getLabelsToPrices())) {
-                ImmutableSet.Builder<PremiumEntry> 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;
+  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<PremiumEntry> 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) {

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 @@ private static LoadingCache<Class<ClaimsListDao>, ClaimsList> createCache(Durati
     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.
+   *
+   * <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 DateTime getTransactionTime() {
     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.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 void run() {
     } 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.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 @@ void testFail_startDateSunriseRegistration_wrongEncodedSignedMark() {
 
   @Test
   void testFail_startDateSunriseRegistration_revokedSignedMark() throws Exception {
-    SmdrlCsvParser.parse(TmchTestData.loadFile("smd/smdrl.csv").lines().collect(toImmutableList()))
-        .save();
+    SignedMarkRevocationListDao.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 @@ void testFail_startDateSunriseRegistration_IdnRevokedSignedMark(
     if (labels.isEmpty()) {
       return;
     }
-    SmdrlCsvParser.parse(
-            TmchTestData.loadFile("idn/idn_smdrl.csv").lines().collect(toImmutableList()))
-        .save();
+    SignedMarkRevocationListDao.save(
+        SmdrlCsvParser.parse(
+            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 @@
 
 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 @@ void testSave_success() {
     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 @@ private SignedMarkRevocationList createSaveGetHelper(int rows) {
     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;