From dc6d3a47569c8b9608f8387372d72e3f355feaf4 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Fri, 18 Sep 2020 15:55:17 -0400 Subject: [PATCH] Add domain-specific history fields to DomainHistory objects (#794) * Add domain-specific history fields to DomainHistory objects * Add javadoc for Hibernate-only methods * V52 -> V54 * Use only a single DomainTransactionRecord table * Add nullables and fix up a comment * V54 -> V55 * Regenerate db schema * Regen SQL file --- .../registry/model/domain/DomainHistory.java | 51 +++++++++++++++ .../google/registry/model/domain/Period.java | 16 ++++- .../reporting/DomainTransactionRecord.java | 19 ++++++ .../model/reporting/HistoryEntry.java | 22 ++++++- .../main/resources/META-INF/persistence.xml | 27 ++++---- .../model/history/DomainHistoryTest.java | 24 ++++++- .../sql/flyway/V55__domain_history_fields.sql | 34 ++++++++++ .../sql/schema/db-schema.sql.generated | 19 ++++++ .../resources/sql/schema/nomulus.golden.sql | 62 ++++++++++++++++++- 9 files changed, 255 insertions(+), 19 deletions(-) create mode 100644 db/src/main/resources/sql/flyway/V55__domain_history_fields.sql diff --git a/core/src/main/java/google/registry/model/domain/DomainHistory.java b/core/src/main/java/google/registry/model/domain/DomainHistory.java index b6e101c11..fd52ab5eb 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/core/src/main/java/google/registry/model/domain/DomainHistory.java @@ -23,12 +23,17 @@ import google.registry.model.EppResource; import google.registry.model.ImmutableObject; import google.registry.model.domain.DomainHistory.DomainHistoryId; import google.registry.model.host.HostResource; +import google.registry.model.reporting.DomainTransactionRecord; import google.registry.model.reporting.HistoryEntry; import google.registry.persistence.VKey; import java.io.Serializable; import java.util.Set; +import javax.annotation.Nullable; import javax.persistence.Access; import javax.persistence.AccessType; +import javax.persistence.AttributeOverride; +import javax.persistence.AttributeOverrides; +import javax.persistence.CascadeType; import javax.persistence.Column; import javax.persistence.ElementCollection; import javax.persistence.Entity; @@ -37,7 +42,9 @@ import javax.persistence.GenerationType; import javax.persistence.Id; import javax.persistence.IdClass; import javax.persistence.Index; +import javax.persistence.JoinColumn; import javax.persistence.JoinTable; +import javax.persistence.OneToMany; import javax.persistence.PostLoad; import javax.persistence.Table; @@ -75,6 +82,50 @@ public class DomainHistory extends HistoryEntry { @Column(name = "host_repo_id") Set> nsHosts; + @Override + @Nullable + @Access(AccessType.PROPERTY) + @AttributeOverrides({ + @AttributeOverride( + name = "unit", + column = @Column(name = "historyPeriodUnit")), + @AttributeOverride( + name = "value", + column = @Column(name = "historyPeriodValue")) + }) + public Period getPeriod() { + return super.getPeriod(); + } + + /** + * For transfers, the id of the other registrar. + * + *

For requests and cancels, the other registrar is the losing party (because the registrar + * sending the EPP transfer command is the gaining party). For approves and rejects, the other + * registrar is the gaining party. + */ + @Nullable + @Access(AccessType.PROPERTY) + @Column(name = "historyOtherRegistrarId") + public String getOtherRegistrarId() { + return super.getOtherClientId(); + } + + /** + * Logging field for transaction reporting. + * + *

This will be empty for any DomainHistory/HistoryEntry generated before this field was added, + * mid-2017, as well as any action that does not generate billable events (e.g. updates). + */ + @Access(AccessType.PROPERTY) + @OneToMany(cascade = {CascadeType.ALL}) + @JoinColumn(name = "historyRevisionId", referencedColumnName = "historyRevisionId") + @JoinColumn(name = "domainRepoId", referencedColumnName = "domainRepoId") + @Override + public Set getDomainTransactionRecords() { + return super.getDomainTransactionRecords(); + } + @Id @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "TempHistorySequenceGenerator") @Column(name = "historyRevisionId") diff --git a/core/src/main/java/google/registry/model/domain/Period.java b/core/src/main/java/google/registry/model/domain/Period.java index 9960acc38..6d0df8df3 100644 --- a/core/src/main/java/google/registry/model/domain/Period.java +++ b/core/src/main/java/google/registry/model/domain/Period.java @@ -31,9 +31,9 @@ public class Period extends ImmutableObject { @XmlAttribute Unit unit; - @XmlValue - Integer value; + @XmlValue Integer value; + @Enumerated(EnumType.STRING) public Unit getUnit() { return unit; } @@ -42,6 +42,18 @@ public class Period extends ImmutableObject { return value; } + /** This method exists solely to satisfy Hibernate. Use {@link #create(int, Unit)} instead. */ + @SuppressWarnings("UnusedMethod") + private void setUnit(Unit unit) { + this.unit = unit; + } + + /** This method exists solely to satisfy Hibernate. Use {@link #create(int, Unit)} instead. */ + @SuppressWarnings("UnusedMethod") + private void setValue(Integer value) { + this.value = value; + } + /** The unit enum. */ public enum Unit { @XmlEnumValue("y") diff --git a/core/src/main/java/google/registry/model/reporting/DomainTransactionRecord.java b/core/src/main/java/google/registry/model/reporting/DomainTransactionRecord.java index 26c5a3a07..1259ba4a5 100644 --- a/core/src/main/java/google/registry/model/reporting/DomainTransactionRecord.java +++ b/core/src/main/java/google/registry/model/reporting/DomainTransactionRecord.java @@ -19,8 +19,16 @@ import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; import com.google.common.collect.ImmutableSet; import com.googlecode.objectify.annotation.Embed; +import com.googlecode.objectify.annotation.Ignore; import google.registry.model.Buildable; import google.registry.model.ImmutableObject; +import javax.persistence.Column; +import javax.persistence.Entity; +import javax.persistence.EnumType; +import javax.persistence.Enumerated; +import javax.persistence.GeneratedValue; +import javax.persistence.GenerationType; +import javax.persistence.Id; import org.joda.time.DateTime; /** @@ -34,9 +42,16 @@ import org.joda.time.DateTime; * uses HistoryEntry.otherClientId because the losing party in a transfer is always the otherClient. */ @Embed +@Entity public class DomainTransactionRecord extends ImmutableObject implements Buildable { + @Id + @Ignore + @GeneratedValue(strategy = GenerationType.IDENTITY) + Long id; + /** The TLD this record operates on. */ + @Column(nullable = false) String tld; /** @@ -50,9 +65,12 @@ public class DomainTransactionRecord extends ImmutableObject implements Buildabl * href="https://www.icann.org/resources/unthemed-pages/registry-agmt-appc-10-2001-05-11-en"> * Grace period spec */ + @Column(nullable = false) DateTime reportingTime; /** The transaction report field we add reportAmount to for this registrar. */ + @Column(nullable = false) + @Enumerated(value = EnumType.STRING) TransactionReportField reportField; /** @@ -67,6 +85,7 @@ public class DomainTransactionRecord extends ImmutableObject implements Buildabl * original SUCCESSFUL transfer counters. Finally, if we explicitly allow a transfer, the report * amount is 0, as we've already counted the transfer in the original request. */ + @Column(nullable = false) Integer reportAmount; /** diff --git a/core/src/main/java/google/registry/model/reporting/HistoryEntry.java b/core/src/main/java/google/registry/model/reporting/HistoryEntry.java index 326d25d13..0d3c4c25f 100644 --- a/core/src/main/java/google/registry/model/reporting/HistoryEntry.java +++ b/core/src/main/java/google/registry/model/reporting/HistoryEntry.java @@ -202,7 +202,7 @@ public class HistoryEntry extends ImmutableObject implements Buildable, Datastor return id == null ? 0L : id; } - // This method is required by Hibernate. + /** This method exists solely to satisfy Hibernate. Use the {@link Builder} instead. */ @SuppressWarnings("UnusedMethod") private void setId(long id) { this.id = id; @@ -254,10 +254,28 @@ public class HistoryEntry extends ImmutableObject implements Buildable, Datastor return requestedByRegistrar; } - public ImmutableSet getDomainTransactionRecords() { + public Set getDomainTransactionRecords() { return nullToEmptyImmutableCopy(domainTransactionRecords); } + /** This method exists solely to satisfy Hibernate. Use the {@link Builder} instead. */ + @SuppressWarnings("UnusedMethod") + private void setPeriod(Period period) { + this.period = period; + } + + /** This method exists solely to satisfy Hibernate. Use the {@link Builder} instead. */ + @SuppressWarnings("UnusedMethod") + private void setOtherRegistrarId(String otherRegistrarId) { + this.otherClientId = otherRegistrarId; + } + + /** This method exists solely to satisfy Hibernate. Use the {@link Builder} instead. */ + @SuppressWarnings("UnusedMethod") + private void setDomainTransactionRecords(Set domainTransactionRecords) { + this.domainTransactionRecords = ImmutableSet.copyOf(domainTransactionRecords); + } + public static VKey createVKey(Key key) { // TODO(b/159207551): This will likely need some revision. As it stands, this method was // introduced purely to facilitate testing of VKey specialization in VKeyTranslatorFactory. diff --git a/core/src/main/resources/META-INF/persistence.xml b/core/src/main/resources/META-INF/persistence.xml index e3a37665f..ee9a68c45 100644 --- a/core/src/main/resources/META-INF/persistence.xml +++ b/core/src/main/resources/META-INF/persistence.xml @@ -45,26 +45,27 @@ google.registry.model.contact.ContactResource google.registry.model.domain.DomainBase google.registry.model.domain.DomainHistory + google.registry.model.domain.GracePeriod + google.registry.model.domain.secdns.DelegationSignerData google.registry.model.domain.token.AllocationToken google.registry.model.host.HostHistory google.registry.model.host.HostResource - google.registry.model.registrar.Registrar - google.registry.model.registrar.RegistrarContact - google.registry.model.registry.label.PremiumList - google.registry.model.registry.Registry - google.registry.model.reporting.Spec11ThreatMatch - google.registry.persistence.transaction.TransactionEntity - google.registry.model.tmch.ClaimsListShard - google.registry.schema.domain.RegistryLock - google.registry.schema.cursor.Cursor - google.registry.schema.server.Lock - google.registry.schema.tld.PremiumEntry - google.registry.model.domain.secdns.DelegationSignerData - google.registry.model.domain.GracePeriod google.registry.model.poll.PollMessage google.registry.model.poll.PollMessage$OneTime google.registry.model.poll.PollMessage$Autorenew + google.registry.model.registrar.Registrar + google.registry.model.registrar.RegistrarContact + google.registry.model.registry.label.PremiumList google.registry.model.registry.label.ReservedList + google.registry.model.registry.Registry + google.registry.model.reporting.DomainTransactionRecord + google.registry.model.reporting.Spec11ThreatMatch + google.registry.model.tmch.ClaimsListShard + google.registry.persistence.transaction.TransactionEntity + google.registry.schema.cursor.Cursor + google.registry.schema.domain.RegistryLock + google.registry.schema.server.Lock + google.registry.schema.tld.PremiumEntry google.registry.persistence.converter.AllocationTokenStatusTransitionConverter diff --git a/core/src/test/java/google/registry/model/history/DomainHistoryTest.java b/core/src/test/java/google/registry/model/history/DomainHistoryTest.java index 9fb520b10..3082a797d 100644 --- a/core/src/test/java/google/registry/model/history/DomainHistoryTest.java +++ b/core/src/test/java/google/registry/model/history/DomainHistoryTest.java @@ -17,6 +17,7 @@ package google.registry.model.history; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects; +import static google.registry.model.ImmutableObjectSubject.immutableObjectCorrespondence; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatastoreHelper.newContactResourceWithRoid; @@ -25,14 +26,18 @@ import static google.registry.testing.DatastoreHelper.newHostResourceWithRoid; import static google.registry.testing.SqlHelper.saveRegistrar; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.collect.ImmutableSet; import com.googlecode.objectify.Key; import google.registry.model.EntityTestCase; import google.registry.model.contact.ContactResource; import google.registry.model.domain.DomainBase; import google.registry.model.domain.DomainContent; import google.registry.model.domain.DomainHistory; +import google.registry.model.domain.Period; import google.registry.model.eppcommon.Trid; import google.registry.model.host.HostResource; +import google.registry.model.reporting.DomainTransactionRecord; +import google.registry.model.reporting.DomainTransactionRecord.TransactionReportField; import google.registry.model.reporting.HistoryEntry; import google.registry.persistence.VKey; import org.junit.jupiter.api.Test; @@ -125,10 +130,24 @@ public class DomainHistoryTest extends EntityTestCase { static void assertDomainHistoriesEqual(DomainHistory one, DomainHistory two) { assertAboutImmutableObjects() .that(one) - .isEqualExceptFields(two, "domainContent", "domainRepoId", "parent", "nsHosts"); + .isEqualExceptFields( + two, "domainContent", "domainRepoId", "parent", "nsHosts", "domainTransactionRecords"); + // NB: the record's ID gets reset by Hibernate, causing the hash code to differ so we have to + // compare it separately + assertThat(one.getDomainTransactionRecords()) + .comparingElementsUsing(immutableObjectCorrespondence()) + .containsExactlyElementsIn(two.getDomainTransactionRecords()); } private DomainHistory createDomainHistory(DomainContent domain) { + DomainTransactionRecord transactionRecord = + new DomainTransactionRecord.Builder() + .setTld("tld") + .setReportingTime(fakeClock.nowUtc()) + .setReportField(TransactionReportField.NET_ADDS_1_YR) + .setReportAmount(1) + .build(); + return new DomainHistory.Builder() .setType(HistoryEntry.Type.DOMAIN_CREATE) .setXmlBytes("".getBytes(UTF_8)) @@ -140,6 +159,9 @@ public class DomainHistoryTest extends EntityTestCase { .setRequestedByRegistrar(true) .setDomainContent(domain) .setDomainRepoId(domain.getRepoId()) + .setDomainTransactionRecords(ImmutableSet.of(transactionRecord)) + .setOtherClientId("otherClient") + .setPeriod(Period.create(1, Period.Unit.YEARS)) .build(); } } diff --git a/db/src/main/resources/sql/flyway/V55__domain_history_fields.sql b/db/src/main/resources/sql/flyway/V55__domain_history_fields.sql new file mode 100644 index 000000000..b5d8f8a65 --- /dev/null +++ b/db/src/main/resources/sql/flyway/V55__domain_history_fields.sql @@ -0,0 +1,34 @@ +-- Copyright 2020 The Nomulus Authors. All Rights Reserved. +-- +-- Licensed under the Apache License, Version 2.0 (the "License"); +-- you may not use this file except in compliance with the License. +-- You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. + + +ALTER TABLE "DomainHistory" ADD COLUMN history_other_registrar_id text; +ALTER TABLE "DomainHistory" ADD COLUMN history_period_unit text; +ALTER TABLE "DomainHistory" ADD COLUMN history_period_value int4; + +CREATE TABLE "DomainTransactionRecord" ( + id bigserial NOT NULL, + report_amount int4 NOT NULL, + report_field text NOT NULL, + reporting_time timestamptz NOT NULL, + tld text NOT NULL, + domain_repo_id text, + history_revision_id int8, + PRIMARY KEY (id) +); + +ALTER TABLE IF EXISTS "DomainTransactionRecord" + ADD CONSTRAINT FKcjqe54u72kha71vkibvxhjye7 + FOREIGN KEY (domain_repo_id, history_revision_id) + REFERENCES "DomainHistory"; 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 9d6c8e990..46ea00511 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/db/src/main/resources/sql/schema/db-schema.sql.generated @@ -342,6 +342,9 @@ create sequence temp_history_id_sequence start 1 increment 50; last_epp_update_time timestamptz, statuses text[], update_timestamp timestamptz, + history_other_registrar_id text, + history_period_unit text, + history_period_value int4, primary key (domain_repo_id, history_revision_id) ); @@ -356,6 +359,17 @@ create sequence temp_history_id_sequence start 1 increment 50; host_repo_id text ); + create table "DomainTransactionRecord" ( + id bigserial not null, + report_amount int4 not null, + report_field text not null, + reporting_time timestamptz not null, + tld text not null, + domain_repo_id text, + history_revision_id int8, + primary key (id) + ); + create table "GracePeriod" ( id bigserial not null, billing_event_id int8, @@ -696,6 +710,11 @@ create index spec11threatmatch_check_date_idx on "Spec11ThreatMatch" (check_date foreign key (domain_repo_id) references "Domain"; + alter table if exists "DomainTransactionRecord" + add constraint FKcjqe54u72kha71vkibvxhjye7 + foreign key (domain_repo_id, history_revision_id) + references "DomainHistory"; + alter table if exists "GracePeriod" add constraint FK2mys4hojm6ev2g9tmy5aq6m7g foreign key (domain_repo_id) diff --git a/db/src/main/resources/sql/schema/nomulus.golden.sql b/db/src/main/resources/sql/schema/nomulus.golden.sql index fb10bac42..c286db816 100644 --- a/db/src/main/resources/sql/schema/nomulus.golden.sql +++ b/db/src/main/resources/sql/schema/nomulus.golden.sql @@ -474,7 +474,10 @@ CREATE TABLE public."DomainHistory" ( statuses text[], update_timestamp timestamp with time zone, domain_repo_id text NOT NULL, - autorenew_end_time timestamp with time zone + autorenew_end_time timestamp with time zone, + history_other_registrar_id text, + history_period_unit text, + history_period_value integer ); @@ -499,6 +502,40 @@ CREATE TABLE public."DomainHost" ( ); +-- +-- Name: DomainTransactionRecord; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public."DomainTransactionRecord" ( + id bigint NOT NULL, + report_amount integer NOT NULL, + report_field text NOT NULL, + reporting_time timestamp with time zone NOT NULL, + tld text NOT NULL, + domain_repo_id text, + history_revision_id bigint +); + + +-- +-- Name: DomainTransactionRecord_id_seq; Type: SEQUENCE; Schema: public; Owner: - +-- + +CREATE SEQUENCE public."DomainTransactionRecord_id_seq" + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + + +-- +-- Name: DomainTransactionRecord_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - +-- + +ALTER SEQUENCE public."DomainTransactionRecord_id_seq" OWNED BY public."DomainTransactionRecord".id; + + -- -- Name: GracePeriod; Type: TABLE; Schema: public; Owner: - -- @@ -1016,6 +1053,13 @@ ALTER TABLE ONLY public."BillingRecurrence" ALTER COLUMN billing_recurrence_id S ALTER TABLE ONLY public."ClaimsList" ALTER COLUMN revision_id SET DEFAULT nextval('public."ClaimsList_revision_id_seq"'::regclass); +-- +-- Name: DomainTransactionRecord id; Type: DEFAULT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public."DomainTransactionRecord" ALTER COLUMN id SET DEFAULT nextval('public."DomainTransactionRecord_id_seq"'::regclass); + + -- -- Name: GracePeriod id; Type: DEFAULT; Schema: public; Owner: - -- @@ -1145,6 +1189,14 @@ ALTER TABLE ONLY public."DomainHistory" ADD CONSTRAINT "DomainHistory_pkey" PRIMARY KEY (domain_repo_id, history_revision_id); +-- +-- Name: DomainTransactionRecord DomainTransactionRecord_pkey; Type: CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public."DomainTransactionRecord" + ADD CONSTRAINT "DomainTransactionRecord_pkey" PRIMARY KEY (id); + + -- -- Name: Domain Domain_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- @@ -1983,6 +2035,14 @@ ALTER TABLE ONLY public."DomainHistoryHost" ADD CONSTRAINT fka9woh3hu8gx5x0vly6bai327n FOREIGN KEY (domain_history_domain_repo_id, domain_history_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id); +-- +-- Name: DomainTransactionRecord fkcjqe54u72kha71vkibvxhjye7; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public."DomainTransactionRecord" + ADD CONSTRAINT fkcjqe54u72kha71vkibvxhjye7 FOREIGN KEY (domain_repo_id, history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id); + + -- -- Name: DomainHost fkfmi7bdink53swivs390m2btxg; Type: FK CONSTRAINT; Schema: public; Owner: - --