Fix accessing superclass fields in checkExists() (#799)

* Fix accessing superclass fields in checkExists()

JpaTransactionManagerImpl doesn't respect @Id fields in mapped superclasses.
Replace calls to getDeclaredId() and getDeclaredField() with superclass
friendly counterparts.

core/src/main/java/google/registry/model/contact/ContactHistory.java

@@ -53,7 +53,7 @@ public class ContactHistory extends HistoryEntry {
   VKey<ContactResource> contactRepoId;
 
   @Id
-  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "HistorySequenceGenerator")
+  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "TempHistorySequenceGenerator")
   @Column(name = "historyRevisionId")
   @Access(AccessType.PROPERTY)
   @Override

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

@@ -76,7 +76,7 @@ public class DomainHistory extends HistoryEntry {
   Set<VKey<HostResource>> nsHosts;
 
   @Id
-  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "HistorySequenceGenerator")
+  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "TempHistorySequenceGenerator")
   @Column(name = "historyRevisionId")
   @Access(AccessType.PROPERTY)
   @Override

core/src/main/java/google/registry/model/host/HostHistory.java

@@ -54,7 +54,7 @@ public class HostHistory extends HistoryEntry {
   VKey<HostResource> hostRepoId;
 
   @Id
-  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "HistorySequenceGenerator")
+  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "TempHistorySequenceGenerator")
   @Column(name = "historyRevisionId")
   @Access(AccessType.PROPERTY)
   @Override

core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java

@@ -391,7 +391,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
   private static ImmutableSet<EntityId> getEntityIdsFromEntity(
       EntityType<?> entityType, Object entity) {
     if (entityType.hasSingleIdAttribute()) {
-      String idName = entityType.getDeclaredId(entityType.getIdType().getJavaType()).getName();
+      String idName = entityType.getId(entityType.getIdType().getJavaType()).getName();
       Object idValue = getFieldValue(entity, idName);
       return ImmutableSet.of(new EntityId(idName, idValue));
     } else {
@@ -402,7 +402,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
   private static ImmutableSet<EntityId> getEntityIdsFromSqlKey(
       EntityType<?> entityType, Object sqlKey) {
     if (entityType.hasSingleIdAttribute()) {
-      String idName = entityType.getDeclaredId(entityType.getIdType().getJavaType()).getName();
+      String idName = entityType.getId(entityType.getIdType().getJavaType()).getName();
       return ImmutableSet.of(new EntityId(idName, sqlKey));
     } else {
       return getEntityIdsFromIdContainer(entityType, sqlKey);
@@ -429,7 +429,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
 
   private static Object getFieldValue(Object object, String fieldName) {
     try {
-      Field field = object.getClass().getDeclaredField(fieldName);
+      Field field = getField(object.getClass(), fieldName);
       field.setAccessible(true);
       return field.get(object);
     } catch (NoSuchFieldException | IllegalAccessException e) {
@@ -437,6 +437,21 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     }
   }
 
+  /** Gets the field definition from clazz or any superclass. */
+  private static Field getField(Class clazz, String fieldName) throws NoSuchFieldException {
+    try {
+      // Note that we have to use getDeclaredField() for this, getField() just finds public fields.
+      return clazz.getDeclaredField(fieldName);
+    } catch (NoSuchFieldException e) {
+      Class base = clazz.getSuperclass();
+      if (base != null) {
+        return getField(base, fieldName);
+      } else {
+        throw e;
+      }
+    }
+  }
+
   private static class TransactionInfo {
     EntityManager entityManager;
     boolean inTransaction = false;

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

@@ -11,7 +11,10 @@
     </attributes>
   </embeddable>
 
-  <sequence-generator name="HistorySequenceGenerator" sequence-name="history_id_sequence" />
+  <sequence-generator name="HistorySequenceGenerator" sequence-name="history_id_sequence"/>
+
+  <!-- TODO(shicong): Drop this sequence and change all history tables to use the above one. -->
+  <sequence-generator name="TempHistorySequenceGenerator" sequence-name="temp_history_id_sequence"/>
 
   <persistence-unit-metadata>
     <persistence-unit-defaults>

core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java

@@ -37,6 +37,8 @@ import java.util.List;
 import java.util.NoSuchElementException;
 import java.util.Set;
 import java.util.stream.Stream;
+import javax.persistence.Embeddable;
+import javax.persistence.MappedSuperclass;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.TestTemplate;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -65,7 +67,7 @@ public class TransactionManagerTest {
           .withClock(fakeClock)
           .withDatastoreAndCloudSql()
           .withOfyTestEntities(TestEntity.class)
-          .withJpaUnitTestEntities(TestEntity.class)
+          .withJpaUnitTestEntities(TestEntity.class, TestEntityBase.class)
           .build();
 
   TransactionManagerTest() {}
@@ -324,17 +326,31 @@ public class TransactionManagerTest {
     entities.forEach(TransactionManagerTest::assertEntityNotExist);
   }
 
+  /**
+   * We put the id field into a base class to test that id fields can be discovered in a base class.
+   */
+  @MappedSuperclass
+  @Embeddable
+  private static class TestEntityBase extends ImmutableObject {
+    @Id @javax.persistence.Id protected String name;
+
+    TestEntityBase(String name) {
+      this.name = name;
+    }
+
+    TestEntityBase() {}
+  }
+
   @Entity(name = "TxnMgrTestEntity")
   @javax.persistence.Entity(name = "TestEntity")
-  private static class TestEntity extends ImmutableObject {
-    @Id @javax.persistence.Id private String name;
+  private static class TestEntity extends TestEntityBase {
 
     private String data;
 
     private TestEntity() {}
 
     private TestEntity(String name, String data) {
-      this.name = name;
+      super(name);
       this.data = data;
     }
 

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

@@ -0,0 +1,20 @@
+-- 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.
+
+create sequence "temp_history_id_sequence"
+    start with 1
+    increment by 50
+    no minvalue
+    no maxvalue
+    cache 1;

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

@@ -11,7 +11,7 @@
 -- 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.
-create sequence history_id_sequence start 1 increment 50;
+create sequence temp_history_id_sequence start 1 increment 50;
 
     create table "AllocationToken" (
        token text not null,

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

@@ -930,6 +930,18 @@ CREATE SEQUENCE public.history_id_sequence
     CACHE 1;
 
 
+--
+-- Name: temp_history_id_sequence; Type: SEQUENCE; Schema: public; Owner: -
+--
+
+CREATE SEQUENCE public.temp_history_id_sequence
+    START WITH 1
+    INCREMENT BY 50
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+
 --
 -- Name: BillingCancellation billing_cancellation_id; Type: DEFAULT; Schema: public; Owner: -
 --