Fix VKey reconstruction issue in BillingEvent (#805)

* Fix VKey reconstruction issue in BillingEvent

* Rebase on head

core/src/main/java/google/registry/model/billing/BillingEvent.java

@@ -33,12 +33,14 @@ import com.googlecode.objectify.annotation.Id;
 import com.googlecode.objectify.annotation.Ignore;
 import com.googlecode.objectify.annotation.IgnoreSave;
 import com.googlecode.objectify.annotation.Index;
+import com.googlecode.objectify.annotation.OnLoad;
 import com.googlecode.objectify.annotation.Parent;
 import com.googlecode.objectify.condition.IfNull;
 import google.registry.model.Buildable;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.ReportedOn;
 import google.registry.model.common.TimeOfYear;
+import google.registry.model.domain.DomainBase;
 import google.registry.model.domain.GracePeriod;
 import google.registry.model.domain.rgp.GracePeriodStatus;
 import google.registry.model.domain.token.AllocationToken;
@@ -57,9 +59,8 @@ import javax.persistence.Column;
 import javax.persistence.Embedded;
 import javax.persistence.EnumType;
 import javax.persistence.Enumerated;
-import javax.persistence.GeneratedValue;
-import javax.persistence.GenerationType;
 import javax.persistence.MappedSuperclass;
+import javax.persistence.PostLoad;
 import javax.persistence.Transient;
 import org.joda.money.Money;
 import org.joda.time.DateTime;
@@ -108,10 +109,7 @@ public abstract class BillingEvent extends ImmutableObject
   }
 
   /** Entity id. */
-  @Id
+  @Id @javax.persistence.Id Long id;
-  @javax.persistence.Id
-  @GeneratedValue(strategy = GenerationType.IDENTITY)
-  Long id;
 
   @Parent @DoNotHydrate @Transient Key<HistoryEntry> parent;
 
@@ -149,6 +147,21 @@ public abstract class BillingEvent extends ImmutableObject
   @Nullable
   Set<Flag> flags;
 
+  @PostLoad
+  void postLoad() {
+    parent =
+        Key.create(
+            Key.create(DomainBase.class, domainRepoId),
+            HistoryEntry.class,
+            domainHistoryRevisionId);
+  }
+
+  @OnLoad
+  void onLoad() {
+    domainHistoryRevisionId = parent.getId();
+    domainRepoId = parent.getParent().getName();
+  }
+
   public String getClientId() {
     return clientId;
   }
@@ -245,7 +258,6 @@ public abstract class BillingEvent extends ImmutableObject
     }
 
     public B setParent(Key<HistoryEntry> parentKey) {
-      // TODO(shicong): Figure out how to set domainHistoryRevisionId and domainRepoId
       getInstance().parent = parentKey;
       return thisCastToDerived();
     }
@@ -258,6 +270,11 @@ public abstract class BillingEvent extends ImmutableObject
       checkNotNull(instance.eventTime, "Event time must be set");
       checkNotNull(instance.targetId, "Target ID must be set");
       checkNotNull(instance.parent, "Parent must be set");
+      checkNotNull(instance.parent.getParent(), "parent.getParent() must be set");
+      checkNotNull(
+          instance.parent.getParent().getName(), "parent.getParent().getName() must be set");
+      instance.domainHistoryRevisionId = instance.parent.getId();
+      instance.domainRepoId = instance.parent.getParent().getName();
       return super.build();
     }
   }

core/src/test/java/google/registry/model/billing/BillingEventTest.java

@@ -39,7 +39,6 @@ import google.registry.model.domain.rgp.GracePeriodStatus;
 import google.registry.model.domain.token.AllocationToken;
 import google.registry.model.domain.token.AllocationToken.TokenStatus;
 import google.registry.model.reporting.HistoryEntry;
-import google.registry.persistence.VKey;
 import google.registry.util.DateTimeUtils;
 import org.joda.money.Money;
 import org.joda.time.DateTime;
@@ -57,7 +56,6 @@ public class BillingEventTest extends EntityTestCase {
   private HistoryEntry historyEntry;
   private HistoryEntry historyEntry2;
   private DomainBase domain;
-  private BillingEvent.OneTime sqlOneTime;
   private BillingEvent.OneTime oneTime;
   private BillingEvent.OneTime oneTimeSynthetic;
   private BillingEvent.Recurring recurring;
@@ -107,16 +105,6 @@ public class BillingEventTest extends EntityTestCase {
                     .setBillingTime(now.plusDays(5))
                     .setAllocationToken(allocationToken.createVKey())));
 
-    sqlOneTime =
-        oneTime
-            .asBuilder()
-            .setDomainRepoId(domain.getRepoId())
-            .setDomainHistoryRevisionId(1L)
-            .setAllocationToken(
-                VKey.create(
-                    AllocationToken.class, allocationToken.getToken(), Key.create(allocationToken)))
-            .build();
-
     recurring =
         persistResource(
             commonInit(
@@ -179,80 +167,43 @@ public class BillingEventTest extends EntityTestCase {
   }
 
   private void saveNewBillingEvent(BillingEvent billingEvent) {
-    billingEvent.id = null;
     jpaTm().transact(() -> jpaTm().insert(billingEvent));
   }
 
   @Test
   void testCloudSqlPersistence_OneTime() {
     saveRegistrar("a registrar");
-    saveNewBillingEvent(sqlOneTime);
+    saveNewBillingEvent(oneTime);
 
-    BillingEvent.OneTime persisted =
+    BillingEvent.OneTime persisted = jpaTm().transact(() -> jpaTm().load(oneTime.createVKey()));
-        jpaTm()
+    // TODO(b/168325240): Remove this fix after VKeyConverter generates symmetric key for
-            .transact(
+    // AllocationToken.
-                () -> jpaTm().load(VKey.createSql(BillingEvent.OneTime.class, sqlOneTime.id)));
-    // TODO(shicong): Remove these fixes after the entities are fully compatible
     BillingEvent.OneTime fixed =
-        persisted
+        persisted.asBuilder().setAllocationToken(oneTime.getAllocationToken().get()).build();
-            .asBuilder()
+    assertThat(fixed).isEqualTo(oneTime);
-            .setParent(sqlOneTime.getParentKey())
-            .setAllocationToken(sqlOneTime.getAllocationToken().get())
-            .build();
-    assertThat(fixed).isEqualTo(sqlOneTime);
   }
 
   @Test
   void testCloudSqlPersistence_Cancellation() {
     saveRegistrar("a registrar");
-    saveNewBillingEvent(sqlOneTime);
+    saveNewBillingEvent(oneTime);
-    VKey<BillingEvent.OneTime> sqlVKey = VKey.createSql(BillingEvent.OneTime.class, sqlOneTime.id);
+    saveNewBillingEvent(cancellationOneTime);
-    BillingEvent sqlCancellationOneTime =
-        cancellationOneTime
-            .asBuilder()
-            .setOneTimeEventKey(sqlVKey)
-            .setDomainRepoId(domain.getRepoId())
-            .setDomainHistoryRevisionId(1L)
-            .build();
-    saveNewBillingEvent(sqlCancellationOneTime);
 
     BillingEvent.Cancellation persisted =
-        jpaTm()
+        jpaTm().transact(() -> jpaTm().load(cancellationOneTime.createVKey()));
-            .transact(
+    // TODO(b/168537779): Remove this fix after VKey<OneTime> can be reconstructed correctly.
-                () ->
-                    jpaTm()
-                        .load(
-                            VKey.createSql(
-                                BillingEvent.Cancellation.class, sqlCancellationOneTime.id)));
-    // TODO(shicong): Remove these fixes after the entities are fully compatible
     BillingEvent.Cancellation fixed =
-        persisted
+        persisted.asBuilder().setOneTimeEventKey(oneTime.createVKey()).build();
-            .asBuilder()
+    assertThat(fixed).isEqualTo(cancellationOneTime);
-            .setParent(sqlCancellationOneTime.getParentKey())
-            .setOneTimeEventKey(sqlVKey)
-            .build();
-    assertThat(fixed).isEqualTo(sqlCancellationOneTime);
   }
 
   @Test
   void testCloudSqlPersistence_Recurring() {
     saveRegistrar("a registrar");
-    BillingEvent.Recurring sqlRecurring =
+    saveNewBillingEvent(recurring);
-        recurring
-            .asBuilder()
-            .setDomainRepoId(domain.getRepoId())
-            .setDomainHistoryRevisionId(1L)
-            .build();
-    saveNewBillingEvent(sqlRecurring);
 
-    BillingEvent.Recurring persisted =
+    BillingEvent.Recurring persisted = jpaTm().transact(() -> jpaTm().load(recurring.createVKey()));
-        jpaTm()
+    assertThat(persisted).isEqualTo(recurring);
-            .transact(
-                () -> jpaTm().load(VKey.createSql(BillingEvent.Recurring.class, sqlRecurring.id)));
-    // TODO(shicong): Remove these fixes after the entities are fully compatible
-    BillingEvent.Recurring fixed =
-        persisted.asBuilder().setParent(sqlRecurring.getParentKey()).build();
-    assertThat(fixed).isEqualTo(sqlRecurring);
   }
 
   @Test

db/src/main/resources/sql/flyway/V58__drop_default_value_and_sequences_for_billing_event.sql

@@ -0,0 +1,21 @@
+-- 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 "BillingCancellation" alter column billing_cancellation_id drop default;
+alter table "BillingEvent" alter column billing_event_id drop default;
+alter table "BillingRecurrence" alter column billing_recurrence_id drop default;
+
+drop sequence "BillingCancellation_billing_cancellation_id_seq";
+drop sequence "BillingEvent_billing_event_id_seq";
+drop sequence "BillingRecurrence_billing_recurrence_id_seq";

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

@@ -30,7 +30,7 @@ create sequence temp_history_id_sequence start 1 increment 50;
     );
 
     create table "BillingCancellation" (
-       billing_cancellation_id  bigserial not null,
+       billing_cancellation_id int8 not null,
         registrar_id text not null,
         domain_history_revision_id int8 not null,
         domain_repo_id text not null,
@@ -45,7 +45,7 @@ create sequence temp_history_id_sequence start 1 increment 50;
     );
 
     create table "BillingEvent" (
-       billing_event_id  bigserial not null,
+       billing_event_id int8 not null,
         registrar_id text not null,
         domain_history_revision_id int8 not null,
         domain_repo_id text not null,
@@ -64,7 +64,7 @@ create sequence temp_history_id_sequence start 1 increment 50;
     );
 
     create table "BillingRecurrence" (
-       billing_recurrence_id  bigserial not null,
+       billing_recurrence_id int8 not null,
         registrar_id text not null,
         domain_history_revision_id int8 not null,
         domain_repo_id text not null,

db/src/main/resources/sql/schema/nomulus.golden.sql

@@ -73,25 +73,6 @@ CREATE TABLE public."BillingCancellation" (
 );
 
 
---
--- Name: BillingCancellation_billing_cancellation_id_seq; Type: SEQUENCE; Schema: public; Owner: -
---
-
-CREATE SEQUENCE public."BillingCancellation_billing_cancellation_id_seq"
-    START WITH 1
-    INCREMENT BY 1
-    NO MINVALUE
-    NO MAXVALUE
-    CACHE 1;
-
-
---
--- Name: BillingCancellation_billing_cancellation_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
---
-
-ALTER SEQUENCE public."BillingCancellation_billing_cancellation_id_seq" OWNED BY public."BillingCancellation".billing_cancellation_id;
-
-
 --
 -- Name: BillingEvent; Type: TABLE; Schema: public; Owner: -
 --
@@ -115,25 +96,6 @@ CREATE TABLE public."BillingEvent" (
 );
 
 
---
--- Name: BillingEvent_billing_event_id_seq; Type: SEQUENCE; Schema: public; Owner: -
---
-
-CREATE SEQUENCE public."BillingEvent_billing_event_id_seq"
-    START WITH 1
-    INCREMENT BY 1
-    NO MINVALUE
-    NO MAXVALUE
-    CACHE 1;
-
-
---
--- Name: BillingEvent_billing_event_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
---
-
-ALTER SEQUENCE public."BillingEvent_billing_event_id_seq" OWNED BY public."BillingEvent".billing_event_id;
-
-
 --
 -- Name: BillingRecurrence; Type: TABLE; Schema: public; Owner: -
 --
@@ -152,25 +114,6 @@ CREATE TABLE public."BillingRecurrence" (
 );
 
 
---
--- Name: BillingRecurrence_billing_recurrence_id_seq; Type: SEQUENCE; Schema: public; Owner: -
---
-
-CREATE SEQUENCE public."BillingRecurrence_billing_recurrence_id_seq"
-    START WITH 1
-    INCREMENT BY 1
-    NO MINVALUE
-    NO MAXVALUE
-    CACHE 1;
-
-
---
--- Name: BillingRecurrence_billing_recurrence_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
---
-
-ALTER SEQUENCE public."BillingRecurrence_billing_recurrence_id_seq" OWNED BY public."BillingRecurrence".billing_recurrence_id;
-
-
 --
 -- Name: ClaimsEntry; Type: TABLE; Schema: public; Owner: -
 --
@@ -1025,27 +968,6 @@ CREATE SEQUENCE public.temp_history_id_sequence
     CACHE 1;
 
 
---
--- Name: BillingCancellation billing_cancellation_id; Type: DEFAULT; Schema: public; Owner: -
---
-
-ALTER TABLE ONLY public."BillingCancellation" ALTER COLUMN billing_cancellation_id SET DEFAULT nextval('public."BillingCancellation_billing_cancellation_id_seq"'::regclass);
-
-
---
--- Name: BillingEvent billing_event_id; Type: DEFAULT; Schema: public; Owner: -
---
-
-ALTER TABLE ONLY public."BillingEvent" ALTER COLUMN billing_event_id SET DEFAULT nextval('public."BillingEvent_billing_event_id_seq"'::regclass);
-
-
---
--- Name: BillingRecurrence billing_recurrence_id; Type: DEFAULT; Schema: public; Owner: -
---
-
-ALTER TABLE ONLY public."BillingRecurrence" ALTER COLUMN billing_recurrence_id SET DEFAULT nextval('public."BillingRecurrence_billing_recurrence_id_seq"'::regclass);
-
-
 --
 -- Name: ClaimsList revision_id; Type: DEFAULT; Schema: public; Owner: -
 --