Make PremiumList.labelsToPrices "insignificant" (#1167)

* Make PremiumList.labelsToPrices "insignificant"

Add the ImmutableObject.Insignificant annotation to labelsToPrices and also
mark it as Transient.  In order to do lazy-loads on this field, we need to do
so explicitly: doing otherwise breaks the immutability contract and prevents
detaching the object upon load.

Note that this is an expedient solution to this problem, but not the optimal
one.  Ideally, the disassociation between PremiumList and its PremiumEntry's
would be more explicit.  However, breaking labelsToPrices out would at minimum
require reworking the Create/UpdatePremiumList commands, which currently rely
on passing around a self-contained PremiumList object, both from the parser
interfaces and to the database.

If this approach is acceptable, we can apply it to ReservedList and ClaimsList
as well (though it may be easier to break the association in those cases).

* Fix premium list "delete" to support a test

* Fix a few more tests

* Changes for review (updated javadocs)

* Minor fixes

* Updated getLablesToPrices() comment

* Format fixes, fixed PremiumEntry interfaces

PremiumEntry can now be SQL only.

core/src/main/java/google/registry/model/registry/label/PremiumList.java

@@ -16,9 +16,12 @@ package google.registry.model.registry.label;
 
 import static com.google.common.base.Charsets.US_ASCII;
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
 import static com.google.common.hash.Funnels.stringFunnel;
 import static com.google.common.hash.Funnels.unencodedCharsFunnel;
 import static google.registry.model.ofy.ObjectifyService.allocateId;
+import static google.registry.persistence.transaction.QueryComposer.Comparator.EQ;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Splitter;
@@ -36,6 +39,7 @@ import google.registry.model.annotations.ReportedOn;
 import google.registry.model.registry.Registry;
 import google.registry.schema.replay.DatastoreOnlyEntity;
 import google.registry.schema.replay.NonReplicatedEntity;
+import google.registry.schema.tld.PremiumEntry;
 import google.registry.schema.tld.PremiumListDao;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -45,17 +49,14 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import javax.annotation.Nullable;
-import javax.persistence.CollectionTable;
 import javax.persistence.Column;
-import javax.persistence.ElementCollection;
 import javax.persistence.Index;
-import javax.persistence.JoinColumn;
-import javax.persistence.MapKeyColumn;
 import javax.persistence.PostLoad;
+import javax.persistence.PostPersist;
 import javax.persistence.PrePersist;
+import javax.persistence.PreRemove;
 import javax.persistence.Table;
 import javax.persistence.Transient;
-import org.hibernate.LazyInitializationException;
 import org.joda.money.CurrencyUnit;
 import org.joda.money.Money;
 
@@ -81,14 +82,14 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
   @Column(nullable = false)
   CurrencyUnit currency;
 
-  @Ignore
-  @ElementCollection
-  @CollectionTable(
-      name = "PremiumEntry",
-      joinColumns = @JoinColumn(name = "revisionId", referencedColumnName = "revisionId"))
-  @MapKeyColumn(name = "domainLabel")
-  @Column(name = "price", nullable = false)
-  Map<String, BigDecimal> labelsToPrices;
+  /**
+   * Mapping from unqualified domain names to their prices.
+   *
+   * <p>This field requires special treatment since we want to lazy load it. We have to remove it
+   * from the immutability contract so we can modify it after construction and we have to handle the
+   * database processing on our own so we can detach it after load.
+   */
+  @Ignore @ImmutableObject.Insignificant @Transient ImmutableMap<String, BigDecimal> labelsToPrices;
 
   @Ignore
   @Column(nullable = false)
@@ -170,14 +171,20 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
   /**
    * Returns a {@link Map} of domain labels to prices.
    *
-   * <p>Note that this is lazily loaded and thus will throw a {@link LazyInitializationException} if
-   * used outside the transaction in which the given entity was loaded. You generally should not be
-   * using this anyway as it's inefficient to load all of the PremiumEntry rows if you don't need
-   * them. To check prices, use {@link PremiumListDao#getPremiumPrice} instead.
+   * <p>Note that this is lazily loaded and thus must be called inside a transaction. You generally
+   * should not be using this anyway as it's inefficient to load all of the PremiumEntry rows if you
+   * don't need them. To check prices, use {@link PremiumListDao#getPremiumPrice} instead.
    */
-  @Nullable
-  public ImmutableMap<String, BigDecimal> getLabelsToPrices() {
-    return labelsToPrices == null ? null : ImmutableMap.copyOf(labelsToPrices);
+  public synchronized ImmutableMap<String, BigDecimal> getLabelsToPrices() {
+    if (labelsToPrices == null) {
+      labelsToPrices =
+          jpaTm()
+              .createQueryComposer(PremiumEntry.class)
+              .where("revisionId", EQ, revisionId)
+              .stream()
+              .collect(toImmutableMap(PremiumEntry::getDomainLabel, PremiumEntry::getPrice));
+    }
+    return labelsToPrices;
   }
 
   /**
@@ -320,4 +327,32 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
   void postLoad() {
     creationTime = lastUpdateTime;
   }
+
+  @PreRemove
+  void preRemove() {
+    jpaTm()
+        .query("DELETE FROM PremiumEntry WHERE revision_id = :revisionId")
+        .setParameter("revisionId", revisionId)
+        .executeUpdate();
+  }
+
+  /**
+   * Hibernate hook called on the insert of a new PremiumList. Stores the associated {@link
+   * PremiumEntry}'s.
+   *
+   * <p>We need to persist the list entries, but only on the initial insert (not on update) since
+   * the entries themselves never get changed, so we only annotate it with {@link PostPersist}, not
+   * {@link PostUpdate}.
+   */
+  @PostPersist
+  void postPersist() {
+    // If the price map is loaded, persist it too.
+    if (labelsToPrices != null) {
+      labelsToPrices.entrySet().stream()
+          .forEach(
+              entry ->
+                  jpaTm()
+                      .insert(PremiumEntry.create(revisionId, entry.getValue(), entry.getKey())));
+    }
+  }
 }

core/src/main/java/google/registry/schema/tld/PremiumEntry.java

@@ -52,8 +52,9 @@ public class PremiumEntry extends ImmutableObject implements Serializable, SqlOn
     return domainLabel;
   }
 
-  public static PremiumEntry create(BigDecimal price, String domainLabel) {
+  public static PremiumEntry create(long revisionId, BigDecimal price, String domainLabel) {
     PremiumEntry result = new PremiumEntry();
+    result.revisionId = revisionId;
     result.price = price;
     result.domainLabel = domainLabel;
     return result;

core/src/main/java/google/registry/tools/server/ListPremiumListsAction.java

@@ -19,6 +19,7 @@ import static google.registry.persistence.transaction.TransactionManagerFactory.
 import static google.registry.request.Action.Method.GET;
 import static google.registry.request.Action.Method.POST;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import google.registry.model.registry.label.PremiumList;
 import google.registry.request.Action;
@@ -27,7 +28,6 @@ import google.registry.schema.tld.PremiumListDao;
 import java.util.Comparator;
 import java.util.Optional;
 import javax.inject.Inject;
-import org.hibernate.Hibernate;
 
 /**
  * An action that lists premium lists, for use by the {@code nomulus list_premium_lists} command.
@@ -58,7 +58,16 @@ public final class ListPremiumListsAction extends ListObjectsAction<PremiumList>
                     .map(PremiumListDao::getLatestRevision)
                     .filter(Optional::isPresent)
                     .map(Optional::get)
-                    .peek(list -> Hibernate.initialize(list.getLabelsToPrices()))
+                    .peek(list -> list.getLabelsToPrices())
                     .collect(toImmutableSortedSet(Comparator.comparing(PremiumList::getName))));
   }
+
+  /**
+   * Provide a field override for labelsToPrices, since it is an {@code Insignificant} field and
+   * doesn't get returned from {@link google.registry.model.ImmutableObject#toDiffableFieldMap}.
+   */
+  @Override
+  public ImmutableMap<String, String> getFieldOverrides(PremiumList list) {
+    return ImmutableMap.of("labelsToPrices", list.getLabelsToPrices().toString());
+  }
 }

core/src/test/java/google/registry/backup/ReplayCommitLogsToSqlActionTest.java

@@ -61,6 +61,7 @@ import google.registry.persistence.VKey;
 import google.registry.persistence.transaction.JpaTransactionManager;
 import google.registry.persistence.transaction.TransactionManagerFactory;
 import google.registry.schema.replay.SqlReplayCheckpoint;
+import google.registry.schema.tld.PremiumEntry;
 import google.registry.testing.AppEngineExtension;
 import google.registry.testing.FakeClock;
 import google.registry.testing.FakeResponse;
@@ -98,6 +99,7 @@ public class ReplayCommitLogsToSqlActionTest {
               DomainBase.class,
               GracePeriod.class,
               PremiumList.class,
+              PremiumEntry.class,
               RegistrarContact.class,
               SqlReplayCheckpoint.class,
               TestObject.class)

db/src/main/resources/sql/schema/db-schema.sql.generated

@@ -862,11 +862,6 @@ create index spec11threatmatch_check_date_idx on "Spec11ThreatMatch" (check_date
        foreign key (domain_repo_id, domain_history_revision_id) 
        references "DomainHistory";
 
-    alter table if exists "PremiumEntry" 
-       add constraint FKo0gw90lpo1tuee56l0nb6y6g5 
-       foreign key (revision_id) 
-       references "PremiumList";
-
     alter table if exists "RegistryLock" 
        add constraint FK2lhcwpxlnqijr96irylrh1707 
        foreign key (relock_revision_id)