From f7dca7fa96fc2be11f15b8b93356c117c5b79613 Mon Sep 17 00:00:00 2001 From: Michael Muller Date: Thu, 20 May 2021 11:21:37 -0400 Subject: [PATCH] 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. --- .../model/registry/label/PremiumList.java | 75 ++++++++++++++----- .../registry/schema/tld/PremiumEntry.java | 3 +- .../tools/server/ListPremiumListsAction.java | 13 +++- .../ReplayCommitLogsToSqlActionTest.java | 2 + .../sql/schema/db-schema.sql.generated | 5 -- 5 files changed, 70 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/google/registry/model/registry/label/PremiumList.java b/core/src/main/java/google/registry/model/registry/label/PremiumList.java index a10aa6aee..c8597124f 100644 --- a/core/src/main/java/google/registry/model/registry/label/PremiumList.java +++ b/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 labelsToPrices; + /** + * Mapping from unqualified domain names to their prices. + * + *

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 labelsToPrices; @Ignore @Column(nullable = false) @@ -170,14 +171,20 @@ public final class PremiumList extends BaseDomainLabelListNote 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. + *

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 getLabelsToPrices() { - return labelsToPrices == null ? null : ImmutableMap.copyOf(labelsToPrices); + public synchronized ImmutableMap 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 BaseDomainLabelListWe 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()))); + } + } } diff --git a/core/src/main/java/google/registry/schema/tld/PremiumEntry.java b/core/src/main/java/google/registry/schema/tld/PremiumEntry.java index de41b13a0..41a307c5e 100644 --- a/core/src/main/java/google/registry/schema/tld/PremiumEntry.java +++ b/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; diff --git a/core/src/main/java/google/registry/tools/server/ListPremiumListsAction.java b/core/src/main/java/google/registry/tools/server/ListPremiumListsAction.java index 37298feb4..f35a18f46 100644 --- a/core/src/main/java/google/registry/tools/server/ListPremiumListsAction.java +++ b/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 .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 getFieldOverrides(PremiumList list) { + return ImmutableMap.of("labelsToPrices", list.getLabelsToPrices().toString()); + } } diff --git a/core/src/test/java/google/registry/backup/ReplayCommitLogsToSqlActionTest.java b/core/src/test/java/google/registry/backup/ReplayCommitLogsToSqlActionTest.java index c0712b7e7..0ee7b9ceb 100644 --- a/core/src/test/java/google/registry/backup/ReplayCommitLogsToSqlActionTest.java +++ b/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) diff --git a/db/src/main/resources/sql/schema/db-schema.sql.generated b/db/src/main/resources/sql/schema/db-schema.sql.generated index 54a0d71ae..319729525 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/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)