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.
Add and use temp_history_id_sequence to avoid release error (#795 )
2026-07-06 00:04:50 +00:00 · 2020-09-11 13:45:51 -04:00 · 2020-09-11 12:25:08 -04:00 · 2020-09-10 17:36:22 -04:00 · 2020-09-10 12:02:24 -04:00
15 changed files with 354 additions and 13 deletions
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,37 @@
+// 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.
+
+package google.registry.persistence.converter;
+
+import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
+
+import com.googlecode.objectify.Key;
+import google.registry.model.registry.label.PremiumList;
+import javax.persistence.AttributeConverter;
+import javax.persistence.Converter;
+
+/** JPA converter for a {@link Key} containing a {@link PremiumList} */
+@Converter(autoApply = true)
+public class PremiumListKeyConverter implements AttributeConverter<Key<PremiumList>, String> {
+
+  @Override
+  public String convertToDatabaseColumn(Key<PremiumList> attribute) {
+    return (attribute == null) ? null : attribute.getName();
+  }
+
+  @Override
+  public Key<PremiumList> convertToEntityAttribute(String dbData) {
+    return (dbData == null) ? null : Key.create(getCrossTldKey(), PremiumList.class, dbData);
+  }
+}
@@ -0,0 +1,37 @@
+// 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.
+
+package google.registry.persistence.converter;
+
+import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
+
+import com.googlecode.objectify.Key;
+import google.registry.model.registry.label.ReservedList;
+import javax.persistence.AttributeConverter;
+import javax.persistence.Converter;
+
+/** JPA converter for a {@link Key} containing a {@link ReservedList} */
+@Converter(autoApply = true)
+public class ReservedListKeyConverter implements AttributeConverter<Key<ReservedList>, String> {
+
+  @Override
+  public String convertToDatabaseColumn(Key<ReservedList> attribute) {
+    return (attribute == null) ? null : attribute.getName();
+  }
+
+  @Override
+  public Key<ReservedList> convertToEntityAttribute(String dbData) {
+    return (dbData == null) ? null : Key.create(getCrossTldKey(), ReservedList.class, dbData);
+  }
+}
@@ -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;
@@ -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>
@@ -79,7 +79,9 @@
    <class>google.registry.persistence.converter.InetAddressSetConverter</class>
    <class>google.registry.persistence.converter.LocalDateConverter</class>
    <class>google.registry.persistence.converter.PostalInfoChoiceListConverter</class>
+    <class>google.registry.persistence.converter.PremiumListKeyConverter</class>
    <class>google.registry.persistence.converter.RegistrarPocSetConverter</class>
+    <class>google.registry.persistence.converter.ReservedListKeyConverter</class>
    <class>google.registry.persistence.converter.Spec11ThreatMatchThreatTypeSetConverter</class>
    <class>google.registry.persistence.converter.StatusValueSetConverter</class>
    <class>google.registry.persistence.converter.StringListConverter</class>
@@ -0,0 +1,90 @@
+// 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.
+
+package google.registry.persistence.converter;
+
+import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+
+import com.googlecode.objectify.Key;
+import google.registry.model.ImmutableObject;
+import google.registry.model.registry.label.PremiumList;
+import google.registry.testing.AppEngineExtension;
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+/** Unit tests for {@link PremiumListKeyConverter}. */
+class PremiumListKeyConverterTest {
+
+  @RegisterExtension
+  final AppEngineExtension appEngine =
+      AppEngineExtension.builder()
+          .withDatastoreAndCloudSql()
+          .withJpaUnitTestEntities(PremiumListEntity.class)
+          .build();
+
+  private final PremiumListKeyConverter converter = new PremiumListKeyConverter();
+
+  @Test
+  void convertToDatabaseColumn_returnsNullIfInputIsNull() {
+    assertThat(converter.convertToDatabaseColumn(null)).isNull();
+  }
+
+  @Test
+  void convertToDatabaseColumn_convertsCorrectly() {
+    assertThat(
+            converter.convertToDatabaseColumn(
+                Key.create(getCrossTldKey(), PremiumList.class, "testList")))
+        .isEqualTo("testList");
+  }
+
+  @Test
+  void convertToEntityAttribute_returnsNullIfInputIsNull() {
+    assertThat(converter.convertToEntityAttribute(null)).isNull();
+  }
+
+  @Test
+  void convertToEntityAttribute_convertsCorrectly() {
+    assertThat(converter.convertToEntityAttribute("testList"))
+        .isEqualTo(Key.create(getCrossTldKey(), PremiumList.class, "testList"));
+  }
+
+  @Test
+  void testRoundTrip() {
+    Key<PremiumList> key = Key.create(getCrossTldKey(), PremiumList.class, "test");
+    PremiumListEntity testEntity = new PremiumListEntity(key);
+    jpaTm().transact(() -> jpaTm().getEntityManager().persist(testEntity));
+    PremiumListEntity persisted =
+        jpaTm().transact(() -> jpaTm().getEntityManager().find(PremiumListEntity.class, "test"));
+    assertThat(persisted.premiumList).isEqualTo(key);
+  }
+
+  @Entity(name = "PremiumListEntity")
+  private static class PremiumListEntity extends ImmutableObject {
+
+    @Id String name;
+
+    Key<PremiumList> premiumList;
+
+    public PremiumListEntity() {}
+
+    PremiumListEntity(Key<PremiumList> premiumList) {
+      this.name = premiumList.getName();
+      this.premiumList = premiumList;
+    }
+  }
+}
@@ -0,0 +1,90 @@
+// 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.
+
+package google.registry.persistence.converter;
+
+import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+
+import com.googlecode.objectify.Key;
+import google.registry.model.ImmutableObject;
+import google.registry.model.registry.label.ReservedList;
+import google.registry.testing.AppEngineExtension;
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+/** Unit tests for {@link ReservedListKeyConverter}. */
+class ReservedListKeyConverterTest {
+
+  @RegisterExtension
+  final AppEngineExtension appEngine =
+      AppEngineExtension.builder()
+          .withDatastoreAndCloudSql()
+          .withJpaUnitTestEntities(ReservedListEntity.class)
+          .build();
+
+  private final ReservedListKeyConverter converter = new ReservedListKeyConverter();
+
+  @Test
+  void convertToDatabaseColumn_returnsNullIfInputIsNull() {
+    assertThat(converter.convertToDatabaseColumn(null)).isNull();
+  }
+
+  @Test
+  void convertToDatabaseColumn_convertsCorrectly() {
+    assertThat(
+            converter.convertToDatabaseColumn(
+                Key.create(getCrossTldKey(), ReservedList.class, "testList")))
+        .isEqualTo("testList");
+  }
+
+  @Test
+  void convertToEntityAttribute_returnsNullIfInputIsNull() {
+    assertThat(converter.convertToEntityAttribute(null)).isNull();
+  }
+
+  @Test
+  void convertToEntityAttribute_convertsCorrectly() {
+    assertThat(converter.convertToEntityAttribute("testList"))
+        .isEqualTo(Key.create(getCrossTldKey(), ReservedList.class, "testList"));
+  }
+
+  @Test
+  void testRoundTrip() {
+    Key<ReservedList> key = Key.create(getCrossTldKey(), ReservedList.class, "test");
+    ReservedListEntity testEntity = new ReservedListEntity(key);
+    jpaTm().transact(() -> jpaTm().getEntityManager().persist(testEntity));
+    ReservedListEntity persisted =
+        jpaTm().transact(() -> jpaTm().getEntityManager().find(ReservedListEntity.class, "test"));
+    assertThat(persisted.reservedList).isEqualTo(key);
+  }
+
+  @Entity(name = "ReservedListEntity")
+  private static class ReservedListEntity extends ImmutableObject {
+
+    @Id String name;
+
+    Key<ReservedList> reservedList;
+
+    public ReservedListEntity() {}
+
+    ReservedListEntity(Key<ReservedList> reservedList) {
+      this.name = reservedList.getName();
+      this.reservedList = reservedList;
+    }
+  }
+}
@@ -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;
    }

@@ -0,0 +1,19 @@
+-- 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 ONLY public."Domain"
+    DROP CONSTRAINT fk_domain_billing_recurrence_id;
+ALTER TABLE ONLY public."Domain"
+    ADD CONSTRAINT fk_domain_billing_recurrence_id FOREIGN KEY (billing_recurrence_id)
+    REFERENCES public."BillingRecurrence"(billing_recurrence_id);
@@ -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;
@@ -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,
@@ -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: -
 --
@@ -1738,7 +1750,7 @@ ALTER TABLE ONLY public."Domain"
 --

 ALTER TABLE ONLY public."Domain"
-    ADD CONSTRAINT fk_domain_billing_recurrence_id FOREIGN KEY (billing_recurrence_id) REFERENCES public."BillingEvent"(billing_event_id);
+    ADD CONSTRAINT fk_domain_billing_recurrence_id FOREIGN KEY (billing_recurrence_id) REFERENCES public."BillingRecurrence"(billing_recurrence_id);


 --
Author	SHA1	Message	Date
Michael Muller	5488e1b323	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.	2020-09-11 13:45:51 -04:00
Shicong Huang	5ab0f97351	Add and use temp_history_id_sequence to avoid release error (#795 )	2020-09-11 12:25:08 -04:00
sarahcaseybot	f7b65327da	Add type converter for Key<ReservedList> and Key<PremiumList> (#796 ) * Add converter for reservedlist and premiumlist keys * Remove public modifier from test classes	2020-09-10 17:36:22 -04:00
Michael Muller	36482ce94f	Fix the billing occurrence foreign key (#797 ) * Fix the billing occurrence foreign key Fix the Domain.billing_occurrence_id foreign key constraint to reference the correct table (BillingRecurrence, not BillingEvent).	2020-09-10 12:02:24 -04:00