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

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<VKey<HostResource>> 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.
+   *
+   * <p>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.
+   *
+   * <p>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<DomainTransactionRecord> getDomainTransactionRecords() {
+    return super.getDomainTransactionRecords();
+  }
+
   @Id
   @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "TempHistorySequenceGenerator")
   @Column(name = "historyRevisionId")

core/src/main/java/google/registry/model/domain/Period.java

@@ -31,9 +31,9 @@ public class Period extends ImmutableObject {
   @XmlAttribute
   Unit unit;
 
-  @XmlValue
+  @XmlValue Integer value;
-  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")

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</a>
    */
+  @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;
 
   /**

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<DomainTransactionRecord> getDomainTransactionRecords() {
+  public Set<DomainTransactionRecord> 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<DomainTransactionRecord> domainTransactionRecords) {
+    this.domainTransactionRecords = ImmutableSet.copyOf(domainTransactionRecords);
+  }
+
   public static VKey<HistoryEntry> createVKey(Key<HistoryEntry> 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.

core/src/main/resources/META-INF/persistence.xml

@@ -45,26 +45,27 @@
     <class>google.registry.model.contact.ContactResource</class>
     <class>google.registry.model.domain.DomainBase</class>
     <class>google.registry.model.domain.DomainHistory</class>
+    <class>google.registry.model.domain.GracePeriod</class>
+    <class>google.registry.model.domain.secdns.DelegationSignerData</class>
     <class>google.registry.model.domain.token.AllocationToken</class>
     <class>google.registry.model.host.HostHistory</class>
     <class>google.registry.model.host.HostResource</class>
-    <class>google.registry.model.registrar.Registrar</class>
-    <class>google.registry.model.registrar.RegistrarContact</class>
-    <class>google.registry.model.registry.label.PremiumList</class>
-    <class>google.registry.model.registry.Registry</class>
-    <class>google.registry.model.reporting.Spec11ThreatMatch</class>
-    <class>google.registry.persistence.transaction.TransactionEntity</class>
-    <class>google.registry.model.tmch.ClaimsListShard</class>
-    <class>google.registry.schema.domain.RegistryLock</class>
-    <class>google.registry.schema.cursor.Cursor</class>
-    <class>google.registry.schema.server.Lock</class>
-    <class>google.registry.schema.tld.PremiumEntry</class>
-    <class>google.registry.model.domain.secdns.DelegationSignerData</class>
-    <class>google.registry.model.domain.GracePeriod</class>
     <class>google.registry.model.poll.PollMessage</class>
     <class>google.registry.model.poll.PollMessage$OneTime</class>
     <class>google.registry.model.poll.PollMessage$Autorenew</class>
+    <class>google.registry.model.registrar.Registrar</class>
+    <class>google.registry.model.registrar.RegistrarContact</class>
+    <class>google.registry.model.registry.label.PremiumList</class>
     <class>google.registry.model.registry.label.ReservedList</class>
+    <class>google.registry.model.registry.Registry</class>
+    <class>google.registry.model.reporting.DomainTransactionRecord</class>
+    <class>google.registry.model.reporting.Spec11ThreatMatch</class>
+    <class>google.registry.model.tmch.ClaimsListShard</class>
+    <class>google.registry.persistence.transaction.TransactionEntity</class>
+    <class>google.registry.schema.cursor.Cursor</class>
+    <class>google.registry.schema.domain.RegistryLock</class>
+    <class>google.registry.schema.server.Lock</class>
+    <class>google.registry.schema.tld.PremiumEntry</class>
 
     <!-- Customized type converters -->
     <class>google.registry.persistence.converter.AllocationTokenStatusTransitionConverter</class>

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("<xml></xml>".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();
   }
 }

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

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) 

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: -
 --