Change @DoNotHydrate to work on fields, not types.

There was a circular reference when hydrating a domain with a
subordinate host, since the host references the domain. To fix
this, I redid @DoNotHydrate to be the way it should have been,
rather than the hack I had originally submitted. I also beefed
up the unit tests of the epp resource types to check for cycles.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=135792416

java/google/registry/model/ImmutableObject.java

@@ -14,11 +14,10 @@
 
 package google.registry.model;
 
-import static com.google.common.base.Functions.identity;
 import static com.google.common.collect.Iterables.transform;
 import static com.google.common.collect.Maps.transformValues;
 import static google.registry.model.ofy.ObjectifyService.ofy;
-import static java.lang.annotation.ElementType.TYPE;
+import static java.lang.annotation.ElementType.FIELD;
 import static java.lang.annotation.RetentionPolicy.RUNTIME;
 
 import com.google.common.base.Function;
@@ -31,10 +30,16 @@ import google.registry.model.domain.ReferenceUnion;
 import java.lang.annotation.Documented;
 import java.lang.annotation.Retention;
 import java.lang.annotation.Target;
+import java.lang.reflect.Field;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Map.Entry;
+import java.util.NavigableMap;
 import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
 import javax.annotation.concurrent.Immutable;
 import javax.xml.bind.annotation.XmlTransient;
 
@@ -43,10 +48,10 @@ import javax.xml.bind.annotation.XmlTransient;
 @XmlTransient
 public abstract class ImmutableObject implements Cloneable {
 
-  /** Marker to indicate that {@link #toHydratedString} should not hydrate a field of this type. */
+  /** Marker to indicate that {@link #toHydratedString} should not hydrate a field. */
   @Documented
   @Retention(RUNTIME)
-  @Target(TYPE)
+  @Target(FIELD)
   public static @interface DoNotHydrate {}
 
   @Ignore
@@ -105,47 +110,55 @@ public abstract class ImmutableObject implements Cloneable {
    */
   @Override
   public String toString() {
-    return toStringHelper(identity());
+    NavigableMap<String, Object> sortedFields = new TreeMap<>();
+    for (Entry<Field, Object> entry : ModelUtils.getFieldValues(this).entrySet()) {
+      sortedFields.put(entry.getKey().getName(), entry.getValue());
+    }
+    return toStringHelper(sortedFields);
   }
 
-  /**
+  /** Similar to toString(), with a full expansion of referenced keys, including in collections. */
-   * Similar to toString(), with a full expansion of embedded ImmutableObjects,
-   * collections, and referenced keys.
-   */
   public String toHydratedString() {
-    return toStringHelper(new Function<Object, Object>() {
+    // We can't use ImmutableSortedMap because we need to allow null values.
-        @Override
+    NavigableMap<String, Object> sortedFields = new TreeMap<>();
-        public Object apply(Object input) {
+    for (Entry<Field, Object> entry : ModelUtils.getFieldValues(this).entrySet()) {
-          if (input instanceof ReferenceUnion) {
+      Field field = entry.getKey();
-            return apply(((ReferenceUnion<?>) input).getLinked());
+      Object value = entry.getValue();
-          } else if (input instanceof Key) {
+      sortedFields.put(
-            Object target = ofy().load().key((Key<?>) input).now();
+          field.getName(),
-            return target != null && target.getClass().isAnnotationPresent(DoNotHydrate.class)
+          field.isAnnotationPresent(DoNotHydrate.class) ? value : HYDRATOR.apply(value));
-                ? input
+    }
-                : apply(target);
+    return toStringHelper(sortedFields);
-          } else if (input instanceof Map) {
-            return transformValues((Map<?, ?>) input, this);
-          } else if (input instanceof Collection) {
-            return transform((Collection<?>) input, this);
-          } else if (input instanceof ImmutableObject) {
-            return ((ImmutableObject) input).toHydratedString();
-          }
-          return input;
-        }});
   }
 
-  public String toStringHelper(Function<Object, Object> transformation) {
+  public String toStringHelper(SortedMap<String, Object> fields) {
-    Map<String, Object> sortedFields = Maps.newTreeMap();
-    sortedFields.putAll(
-        transformValues(ModelUtils.getFieldValues(this), transformation));
     return String.format(
         "%s (@%s): {\n%s",
         getClass().getSimpleName(),
         System.identityHashCode(this),
-        Joiner.on('\n').join(sortedFields.entrySet()))
+        Joiner.on('\n').join(fields.entrySet()))
             .replaceAll("\n", "\n    ") + "\n}";
   }
 
+  /** Helper function to recursively hydrate an ImmutableObject. */
+  private static final Function<Object, Object> HYDRATOR =
+      new Function<Object, Object>() {
+        @Override
+        public Object apply(Object value) {
+          if (value instanceof ReferenceUnion) {
+            return apply(((ReferenceUnion<?>) value).getLinked());
+          } else if (value instanceof Key) {
+            return apply(ofy().load().key((Key<?>) value).now());
+          } else if (value instanceof Map) {
+            return transformValues((Map<?, ?>) value, this);
+          } else if (value instanceof Collection) {
+            return transform((Collection<?>) value, this);
+          } else if (value instanceof ImmutableObject) {
+            return ((ImmutableObject) value).toHydratedString();
+          }
+          return value;
+        }};
+
   /** Helper function to recursively convert a ImmutableObject to a Map of generic objects. */
   private static final Function<Object, Object> TO_MAP_HELPER = new Function<Object, Object>() {
     @Override
@@ -153,8 +166,11 @@ public abstract class ImmutableObject implements Cloneable {
       if (o == null) {
         return null;
       } else if (o instanceof ImmutableObject) {
-        Map<String, Object> result =
+        // LinkedHashMap to preserve field ordering and because ImmutableMap forbids null values.
-            Maps.transformValues(ModelUtils.getFieldValues(o), this);
+        Map<String, Object> result = new LinkedHashMap<>();
+        for (Entry<Field, Object> entry : ModelUtils.getFieldValues(o).entrySet()) {
+          result.put(entry.getKey().getName(), apply(entry.getValue()));
+        }
         return result;
       } else if (o instanceof Map) {
         return Maps.transformValues((Map<?, ?>) o, this);

java/google/registry/model/ModelUtils.java

@@ -209,15 +209,15 @@ public class ModelUtils {
   }
 
   /**
-   * Returns a map from field names (including non-public and inherited fields) to values.
+   * Returns a map from Field objects (including non-public and inherited fields) to values.
    *
    * <p>This turns arrays into {@link List} objects so that ImmutableObject can more easily use the
    * returned map in its implementation of {@link ImmutableObject#toString} and {@link
    * ImmutableObject#equals}, which work by comparing and printing these maps.
    */
-  static Map<String, Object> getFieldValues(Object instance) {
+  static Map<Field, Object> getFieldValues(Object instance) {
     // Don't make this ImmutableMap because field values can be null.
-    Map<String, Object> values = new LinkedHashMap<>();
+    Map<Field, Object> values = new LinkedHashMap<>();
     for (Field field : getAllFields(instance.getClass()).values()) {
       Object value = getFieldValue(instance, field);
       if (value != null && value.getClass().isArray()) {
@@ -234,7 +234,7 @@ public class ModelUtils {
               return Array.getLength(arrayValue);
             }};
       }
-      values.put(field.getName(), value);
+      values.put(field, value);
     }
     return values;
   }

java/google/registry/model/billing/BillingEvent.java

@@ -87,6 +87,7 @@ public abstract class BillingEvent extends ImmutableObject
   long id;
 
   @Parent
+  @DoNotHydrate
   Key<HistoryEntry> parent;
 
   /** The registrar to bill. */

java/google/registry/model/host/HostResource.java

@@ -89,6 +89,7 @@ public class HostResource extends EppResource implements ForeignKeyedEppResource
   @Index
   @IgnoreSave(IfNull.class)
   @XmlTransient
+  @DoNotHydrate
   Key<DomainResource> superordinateDomain;
 
   /**

java/google/registry/model/poll/PollMessage.java

@@ -84,6 +84,7 @@ public abstract class PollMessage extends ImmutableObject
   long id;
 
   @Parent
+  @DoNotHydrate
   Key<HistoryEntry> parent;
 
   /** The registrar that this poll message will be delivered to. */

java/google/registry/model/reporting/HistoryEntry.java

@@ -24,14 +24,12 @@ import com.googlecode.objectify.condition.IfNull;
 import google.registry.model.Buildable;
 import google.registry.model.EppResource;
 import google.registry.model.ImmutableObject;
-import google.registry.model.ImmutableObject.DoNotHydrate;
 import google.registry.model.domain.Period;
 import google.registry.model.eppcommon.Trid;
 import org.joda.time.DateTime;
 
 /** A record of an EPP command that mutated a resource. */
 @Entity
-@DoNotHydrate
 public class HistoryEntry extends ImmutableObject implements Buildable {
 
   /** Represents the type of history entry. */

javatests/google/registry/model/ImmutableObjectTest.java

@@ -31,7 +31,6 @@ import com.googlecode.objectify.Key;
 import com.googlecode.objectify.ObjectifyService;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
-import google.registry.model.ImmutableObject.DoNotHydrate;
 import google.registry.model.domain.ReferenceUnion;
 import google.registry.testing.AppEngineRule;
 import google.registry.util.CidrAddressBlock;
@@ -59,8 +58,7 @@ public class ImmutableObjectTest {
 
   @Before
   public void register() {
-    ObjectifyService.register(HydratableObject.class);
+    ObjectifyService.register(ValueObject.class);
-    ObjectifyService.register(UnhydratableObject.class);
   }
 
   /** Simple subclass of ImmutableObject. */
@@ -257,81 +255,64 @@ public class ImmutableObjectTest {
 
   /** Subclass of ImmutableObject with keys to other objects. */
   public static class RootObject extends ImmutableObject {
-    Key<HydratableObject> hydratable;
+    Key<ValueObject> hydrateMe;
 
-    Key<UnhydratableObject> unhydratable;
+    @DoNotHydrate
+    Key<ValueObject> skipMe;
 
-    Map<String, Key<?>> map;
+    Map<String, Key<ValueObject>> map;
 
-    Set<Key<?>> set;
+    Set<Key<ValueObject>> set;
 
     ReferenceUnion<?> referenceUnion;
   }
 
-  /** Hydratable subclass of ImmutableObject. */
+  /** Simple subclass of ImmutableObject. */
   @Entity
-  public static class HydratableObject extends ImmutableObject {
+  public static class ValueObject extends ImmutableObject {
     @Id
-    long id = 1;
+    long id;
 
     String value;
-  }
 
-  /** Unhydratable subclass of SimpleObject. */
+    static ValueObject create(long id, String value) {
-  @Entity
+      ValueObject instance = new ValueObject();
-  @DoNotHydrate
+      instance.id = id;
-  public static class UnhydratableObject extends ImmutableObject {
+      instance.value = value;
-    @Id
+      return instance;
-    long id = 1;
+    }
-
-    String value;
   }
 
   @Test
   public void testToHydratedString_skipsDoNotHydrate() {
-    HydratableObject hydratable = new HydratableObject();
-    hydratable.value = "expected";
-    UnhydratableObject unhydratable = new UnhydratableObject();
-    unhydratable.value = "unexpected";
     RootObject root = new RootObject();
-    root.hydratable = Key.create(persistResource(hydratable));
+    root.hydrateMe = Key.create(persistResource(ValueObject.create(1, "foo")));
-    root.unhydratable = Key.create(persistResource(unhydratable));
+    root.skipMe = Key.create(persistResource(ValueObject.create(2, "bar")));
-    assertThat(root.toHydratedString()).contains("expected");
+    String hydratedString = root.toHydratedString();
-    assertThat(root.toHydratedString()).doesNotContain("unexpected");
+    assertThat(hydratedString).contains("foo");
+    assertThat(hydratedString).doesNotContain("bar");
   }
 
   @Test
   public void testToHydratedString_expandsMaps() {
-    HydratableObject hydratable = new HydratableObject();
-    hydratable.value = "expected";
-    UnhydratableObject unhydratable = new UnhydratableObject();
-    unhydratable.value = "unexpected";
     RootObject root = new RootObject();
-    root.map = ImmutableMap.<String, Key<?>>of(
+    root.map = ImmutableMap.of("foo", Key.create(persistResource(ValueObject.create(1, "bar"))));
-        "hydratable", Key.create(persistResource(hydratable)),
+    String hydratedString = root.toHydratedString();
-        "unhydratable", Key.create(persistResource(unhydratable)));
+    assertThat(hydratedString).contains("foo");
-    assertThat(root.toHydratedString()).contains("expected");
+    assertThat(hydratedString).contains("bar");
-    assertThat(root.toHydratedString()).doesNotContain("unexpected");
   }
 
   @Test
   public void testToHydratedString_expandsCollections() {
-    HydratableObject hydratable = new HydratableObject();
-    hydratable.value = "expected";
-    UnhydratableObject unhydratable = new UnhydratableObject();
-    unhydratable.value = "unexpected";
     RootObject root = new RootObject();
-    root.set = ImmutableSet.<Key<?>>of(
+    root.set = ImmutableSet.of(Key.create(persistResource(ValueObject.create(1, "foo"))));
-        Key.create(persistResource(hydratable)),
+    assertThat(root.toHydratedString()).contains("foo");
-        Key.create(persistResource(unhydratable)));
-    assertThat(root.toHydratedString()).contains("expected");
-    assertThat(root.toHydratedString()).doesNotContain("unexpected");
   }
 
   @Test
   public void testToHydratedString_expandsReferenceUnions() {
     RootObject root = new RootObject();
-    root.referenceUnion = ReferenceUnion.create(Key.create(persistActiveContact("expected")));
+    root.referenceUnion = ReferenceUnion.create(Key.create(persistActiveContact("foo")));
-    assertThat(root.toHydratedString()).contains("expected");
+    assertThat(root.toHydratedString()).contains("foo");
   }
 }

javatests/google/registry/model/ModelUtilsTest.java

@@ -110,17 +110,21 @@ public class ModelUtilsTest {
     testInstance.id = "foo";
     testInstance.a = "a";
     testInstance.b = "b";
-    // More complicated version of isEqualTo() so that we check for ordering.
+    assertThat(ModelUtils.getFieldValues(testInstance))
-    assertThat(ModelUtils.getFieldValues(testInstance).entrySet())
+        .containsExactly(
-        .containsExactlyElementsIn(ImmutableMap.of("id", "foo", "a", "a", "b", "b").entrySet())
+            TestClass.class.getDeclaredField("id"), "foo",
+            TestClass.class.getDeclaredField("a"), "a",
+            TestClass.class.getDeclaredField("b"), "b")
         .inOrder();
     // Test again, to make sure we aren't caching values.
     testInstance.id = "bar";
     testInstance.a = "1";
     testInstance.b = "2";
-    // More complicated version of isEqualTo() so that we check for ordering.
+    assertThat(ModelUtils.getFieldValues(testInstance))
-    assertThat(ModelUtils.getFieldValues(testInstance).entrySet())
+        .containsExactly(
-        .containsExactlyElementsIn(ImmutableMap.of("id", "bar", "a", "1", "b", "2").entrySet())
+            TestClass.class.getDeclaredField("id"), "bar",
+            TestClass.class.getDeclaredField("a"), "1",
+            TestClass.class.getDeclaredField("b"), "2")
         .inOrder();
   }
 

javatests/google/registry/model/contact/ContactResourceTest.java

@@ -227,4 +227,10 @@ public class ContactResourceTest extends EntityTestCase {
     thrown.expect(IllegalStateException.class, "creationTime can only be set once");
     contactResource.asBuilder().setCreationTime(END_OF_TIME);
   }
+
+  @Test
+  public void testToHydratedString_notCircular() {
+    // If there are circular references, this will overflow the stack.
+    contactResource.toHydratedString();
+  }
 }

javatests/google/registry/model/domain/DomainApplicationTest.java

@@ -179,4 +179,10 @@ public class DomainApplicationTest extends EntityTestCase {
     assertThat(withNull).isEqualTo(withEmpty);
     assertThat(withEmpty.hasTransferData()).isFalse();
   }
+
+  @Test
+  public void testToHydratedString_notCircular() {
+    // If there are circular references, this will overflow the stack.
+    domainApplication.toHydratedString();
+  }
 }

javatests/google/registry/model/domain/DomainResourceTest.java

@@ -41,6 +41,7 @@ import google.registry.model.EntityTestCase;
 import google.registry.model.billing.BillingEvent;
 import google.registry.model.billing.BillingEvent.Reason;
 import google.registry.model.contact.ContactResource;
+import google.registry.model.domain.DesignatedContact.Type;
 import google.registry.model.domain.launch.LaunchNotice;
 import google.registry.model.domain.rgp.GracePeriodStatus;
 import google.registry.model.domain.secdns.DelegationSignerData;
@@ -79,26 +80,38 @@ public class DomainResourceTest extends EntityTestCase {
   @Before
   public void setUp() throws Exception {
     createTld("com");
-    HostResource hostResource = persistResource(
+    Key<DomainResource> domainKey = Key.create(null, DomainResource.class, "4-COM");
+    Key<HostResource> hostKey = Key.create(persistResource(
         new HostResource.Builder()
             .setFullyQualifiedHostName("ns1.example.com")
+            .setSuperordinateDomain(domainKey)
             .setRepoId("1-COM")
-            .build());
+            .build()));
-    ContactResource contactResource1 = persistResource(
+    Key<ContactResource> contact1Key = Key.create(persistResource(
         new ContactResource.Builder()
             .setContactId("contact_id1")
             .setRepoId("2-COM")
-            .build());
+            .build()));
-    ContactResource contactResource2 = persistResource(
+    Key<ContactResource> contact2Key = Key.create(persistResource(
         new ContactResource.Builder()
             .setContactId("contact_id2")
             .setRepoId("3-COM")
-            .build());
+            .build()));
+    Key<HistoryEntry> historyEntryKey =
+        Key.create(persistResource(new HistoryEntry.Builder().setParent(domainKey).build()));
+    Key<BillingEvent.OneTime> oneTimeBillKey =
+        Key.create(historyEntryKey, BillingEvent.OneTime.class, 1);
+    Key<BillingEvent.Recurring> recurringBillKey =
+        Key.create(historyEntryKey, BillingEvent.Recurring.class, 2);
+    Key<PollMessage.Autorenew> autorenewPollKey =
+        Key.create(historyEntryKey, PollMessage.Autorenew.class, 3);
+    Key<PollMessage.OneTime> onetimePollKey =
+        Key.create(historyEntryKey, PollMessage.OneTime.class, 1);
     // Set up a new persisted domain entity.
     domain = cloneAndSetAutoTimestamps(
         new DomainResource.Builder()
             .setFullyQualifiedDomainName("example.com")
-        .setRepoId("4-COM")
+            .setRepoId("4-COM")
             .setCreationClientId("a registrar")
             .setLastEppUpdateTime(clock.nowUtc())
             .setLastEppUpdateClientId("AnotherRegistrar")
@@ -110,11 +123,9 @@ public class DomainResourceTest extends EntityTestCase {
                 StatusValue.SERVER_UPDATE_PROHIBITED,
                 StatusValue.SERVER_RENEW_PROHIBITED,
                 StatusValue.SERVER_HOLD))
-            .setRegistrant(Key.create(contactResource1))
+            .setRegistrant(contact1Key)
-            .setContacts(ImmutableSet.of(DesignatedContact.create(
+            .setContacts(ImmutableSet.of(DesignatedContact.create(Type.ADMIN, contact2Key)))
-                DesignatedContact.Type.ADMIN,
+            .setNameservers(ImmutableSet.of(hostKey))
-                Key.create(contactResource2))))
-            .setNameservers(ImmutableSet.of(Key.create(hostResource)))
             .setSubordinateHosts(ImmutableSet.of("ns1.example.com"))
             .setCurrentSponsorClientId("ThirdRegistrar")
             .setRegistrationExpirationTime(clock.nowUtc().plusYears(1))
@@ -130,22 +141,19 @@ public class DomainResourceTest extends EntityTestCase {
                     .setPendingTransferExpirationTime(clock.nowUtc())
                     .setServerApproveEntities(
                         ImmutableSet.<Key<? extends TransferServerApproveEntity>>of(
-                            Key.create(BillingEvent.OneTime.class, 1),
+                            oneTimeBillKey,
-                            Key.create(BillingEvent.Recurring.class, 2),
+                            recurringBillKey,
-                            Key.create(PollMessage.Autorenew.class, 3)))
+                            autorenewPollKey))
-                    .setServerApproveBillingEvent(
+                    .setServerApproveBillingEvent(oneTimeBillKey)
-                        Key.create(BillingEvent.OneTime.class, 1))
+                    .setServerApproveAutorenewEvent(recurringBillKey)
-                    .setServerApproveAutorenewEvent(
+                    .setServerApproveAutorenewPollMessage(autorenewPollKey)
-                        Key.create(BillingEvent.Recurring.class, 2))
-                    .setServerApproveAutorenewPollMessage(
-                        Key.create(PollMessage.Autorenew.class, 3))
                     .setTransferRequestTime(clock.nowUtc().plusDays(1))
                     .setTransferStatus(TransferStatus.SERVER_APPROVED)
                     .setTransferRequestTrid(Trid.create("client trid"))
                     .build())
-            .setDeletePollMessage(Key.create(PollMessage.OneTime.class, 1))
+            .setDeletePollMessage(onetimePollKey)
-            .setAutorenewBillingEvent(Key.create(BillingEvent.Recurring.class, 1))
+            .setAutorenewBillingEvent(recurringBillKey)
-            .setAutorenewPollMessage(Key.create(PollMessage.Autorenew.class, 2))
+            .setAutorenewPollMessage(autorenewPollKey)
             .setSmdId("smdid")
             .setApplicationTime(START_OF_TIME)
             .setApplication(Key.create(DomainApplication.class, 1))
@@ -439,4 +447,9 @@ public class DomainResourceTest extends EntityTestCase {
     // Assert that there was only one call to datastore (that may have loaded many keys).
     assertThat(skip(RequestCapturingAsyncDatastoreService.getReads(), numPreviousReads)).hasSize(1);
   }
+
+  @Test
+  public void testToHydratedString_notCircular() {
+    domain.toHydratedString();  // If there are circular references, this will overflow the stack.
+  }
 }

javatests/google/registry/model/host/HostResourceTest.java

@@ -284,4 +284,10 @@ public class HostResourceTest extends EntityTestCase {
     assertThat(afterTransfer.getCurrentSponsorClientId()).isEqualTo("winner");
     assertThat(afterTransfer.getLastTransferTime()).isEqualTo(clock.nowUtc().plusDays(1));
   }
+
+  @Test
+  public void testToHydratedString_notCircular() {
+    // If there are circular references, this will overflow the stack.
+    hostResource.toHydratedString();
+  }
 }