diff --git a/java/google/registry/model/ImmutableObject.java b/java/google/registry/model/ImmutableObject.java index 6afaf644a..29b8f6d60 100644 --- a/java/google/registry/model/ImmutableObject.java +++ b/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 sortedFields = new TreeMap<>(); + for (Entry entry : ModelUtils.getFieldValues(this).entrySet()) { + sortedFields.put(entry.getKey().getName(), entry.getValue()); + } + return toStringHelper(sortedFields); } - /** - * Similar to toString(), with a full expansion of embedded ImmutableObjects, - * collections, and referenced keys. - */ + /** Similar to toString(), with a full expansion of referenced keys, including in collections. */ public String toHydratedString() { - return toStringHelper(new Function() { - @Override - public Object apply(Object input) { - if (input instanceof ReferenceUnion) { - return apply(((ReferenceUnion) input).getLinked()); - } else if (input instanceof Key) { - Object target = ofy().load().key((Key) input).now(); - return target != null && target.getClass().isAnnotationPresent(DoNotHydrate.class) - ? input - : apply(target); - } 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; - }}); + // We can't use ImmutableSortedMap because we need to allow null values. + NavigableMap sortedFields = new TreeMap<>(); + for (Entry entry : ModelUtils.getFieldValues(this).entrySet()) { + Field field = entry.getKey(); + Object value = entry.getValue(); + sortedFields.put( + field.getName(), + field.isAnnotationPresent(DoNotHydrate.class) ? value : HYDRATOR.apply(value)); + } + return toStringHelper(sortedFields); } - public String toStringHelper(Function transformation) { - Map sortedFields = Maps.newTreeMap(); - sortedFields.putAll( - transformValues(ModelUtils.getFieldValues(this), transformation)); + public String toStringHelper(SortedMap fields) { 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 HYDRATOR = + new Function() { + @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 TO_MAP_HELPER = new Function() { @Override @@ -153,8 +166,11 @@ public abstract class ImmutableObject implements Cloneable { if (o == null) { return null; } else if (o instanceof ImmutableObject) { - Map result = - Maps.transformValues(ModelUtils.getFieldValues(o), this); + // LinkedHashMap to preserve field ordering and because ImmutableMap forbids null values. + Map result = new LinkedHashMap<>(); + for (Entry 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); diff --git a/java/google/registry/model/ModelUtils.java b/java/google/registry/model/ModelUtils.java index 1388ba65d..d00cd098e 100644 --- a/java/google/registry/model/ModelUtils.java +++ b/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. * *

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 getFieldValues(Object instance) { + static Map getFieldValues(Object instance) { // Don't make this ImmutableMap because field values can be null. - Map values = new LinkedHashMap<>(); + Map 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; } diff --git a/java/google/registry/model/billing/BillingEvent.java b/java/google/registry/model/billing/BillingEvent.java index ac3f4b711..8c4857b17 100644 --- a/java/google/registry/model/billing/BillingEvent.java +++ b/java/google/registry/model/billing/BillingEvent.java @@ -87,6 +87,7 @@ public abstract class BillingEvent extends ImmutableObject long id; @Parent + @DoNotHydrate Key parent; /** The registrar to bill. */ diff --git a/java/google/registry/model/host/HostResource.java b/java/google/registry/model/host/HostResource.java index 82b3b6342..fd84594fb 100644 --- a/java/google/registry/model/host/HostResource.java +++ b/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 superordinateDomain; /** diff --git a/java/google/registry/model/poll/PollMessage.java b/java/google/registry/model/poll/PollMessage.java index 9ed27be2d..dded85d1b 100644 --- a/java/google/registry/model/poll/PollMessage.java +++ b/java/google/registry/model/poll/PollMessage.java @@ -84,6 +84,7 @@ public abstract class PollMessage extends ImmutableObject long id; @Parent + @DoNotHydrate Key parent; /** The registrar that this poll message will be delivered to. */ diff --git a/java/google/registry/model/reporting/HistoryEntry.java b/java/google/registry/model/reporting/HistoryEntry.java index b8f24f6b1..9d63f3a58 100644 --- a/java/google/registry/model/reporting/HistoryEntry.java +++ b/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. */ diff --git a/javatests/google/registry/model/ImmutableObjectTest.java b/javatests/google/registry/model/ImmutableObjectTest.java index d6842bb46..a09733e5f 100644 --- a/javatests/google/registry/model/ImmutableObjectTest.java +++ b/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(UnhydratableObject.class); + ObjectifyService.register(ValueObject.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 hydratable; + Key hydrateMe; - Key unhydratable; + @DoNotHydrate + Key skipMe; - Map> map; + Map> map; - Set> set; + Set> 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. */ - @Entity - @DoNotHydrate - public static class UnhydratableObject extends ImmutableObject { - @Id - long id = 1; - - String value; + static ValueObject create(long id, String value) { + ValueObject instance = new ValueObject(); + instance.id = id; + instance.value = value; + return instance; + } } @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.unhydratable = Key.create(persistResource(unhydratable)); - assertThat(root.toHydratedString()).contains("expected"); - assertThat(root.toHydratedString()).doesNotContain("unexpected"); + root.hydrateMe = Key.create(persistResource(ValueObject.create(1, "foo"))); + root.skipMe = Key.create(persistResource(ValueObject.create(2, "bar"))); + String hydratedString = root.toHydratedString(); + 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.>of( - "hydratable", Key.create(persistResource(hydratable)), - "unhydratable", Key.create(persistResource(unhydratable))); - assertThat(root.toHydratedString()).contains("expected"); - assertThat(root.toHydratedString()).doesNotContain("unexpected"); + root.map = ImmutableMap.of("foo", Key.create(persistResource(ValueObject.create(1, "bar")))); + String hydratedString = root.toHydratedString(); + assertThat(hydratedString).contains("foo"); + assertThat(hydratedString).contains("bar"); } @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.>of( - Key.create(persistResource(hydratable)), - Key.create(persistResource(unhydratable))); - assertThat(root.toHydratedString()).contains("expected"); - assertThat(root.toHydratedString()).doesNotContain("unexpected"); + root.set = ImmutableSet.of(Key.create(persistResource(ValueObject.create(1, "foo")))); + assertThat(root.toHydratedString()).contains("foo"); } @Test public void testToHydratedString_expandsReferenceUnions() { RootObject root = new RootObject(); - root.referenceUnion = ReferenceUnion.create(Key.create(persistActiveContact("expected"))); - assertThat(root.toHydratedString()).contains("expected"); + root.referenceUnion = ReferenceUnion.create(Key.create(persistActiveContact("foo"))); + assertThat(root.toHydratedString()).contains("foo"); } } diff --git a/javatests/google/registry/model/ModelUtilsTest.java b/javatests/google/registry/model/ModelUtilsTest.java index 4c5469ef2..75c3a8556 100644 --- a/javatests/google/registry/model/ModelUtilsTest.java +++ b/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).entrySet()) - .containsExactlyElementsIn(ImmutableMap.of("id", "foo", "a", "a", "b", "b").entrySet()) + assertThat(ModelUtils.getFieldValues(testInstance)) + .containsExactly( + 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).entrySet()) - .containsExactlyElementsIn(ImmutableMap.of("id", "bar", "a", "1", "b", "2").entrySet()) + assertThat(ModelUtils.getFieldValues(testInstance)) + .containsExactly( + TestClass.class.getDeclaredField("id"), "bar", + TestClass.class.getDeclaredField("a"), "1", + TestClass.class.getDeclaredField("b"), "2") .inOrder(); } diff --git a/javatests/google/registry/model/contact/ContactResourceTest.java b/javatests/google/registry/model/contact/ContactResourceTest.java index 264976bbb..eedc6361b 100644 --- a/javatests/google/registry/model/contact/ContactResourceTest.java +++ b/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(); + } } diff --git a/javatests/google/registry/model/domain/DomainApplicationTest.java b/javatests/google/registry/model/domain/DomainApplicationTest.java index 3dc83b35a..e70cf3756 100644 --- a/javatests/google/registry/model/domain/DomainApplicationTest.java +++ b/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(); + } } diff --git a/javatests/google/registry/model/domain/DomainResourceTest.java b/javatests/google/registry/model/domain/DomainResourceTest.java index 51ea9b724..71d2412b8 100644 --- a/javatests/google/registry/model/domain/DomainResourceTest.java +++ b/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 domainKey = Key.create(null, DomainResource.class, "4-COM"); + Key hostKey = Key.create(persistResource( new HostResource.Builder() .setFullyQualifiedHostName("ns1.example.com") + .setSuperordinateDomain(domainKey) .setRepoId("1-COM") - .build()); - ContactResource contactResource1 = persistResource( + .build())); + Key contact1Key = Key.create(persistResource( new ContactResource.Builder() .setContactId("contact_id1") .setRepoId("2-COM") - .build()); - ContactResource contactResource2 = persistResource( + .build())); + Key contact2Key = Key.create(persistResource( new ContactResource.Builder() .setContactId("contact_id2") .setRepoId("3-COM") - .build()); + .build())); + Key historyEntryKey = + Key.create(persistResource(new HistoryEntry.Builder().setParent(domainKey).build())); + Key oneTimeBillKey = + Key.create(historyEntryKey, BillingEvent.OneTime.class, 1); + Key recurringBillKey = + Key.create(historyEntryKey, BillingEvent.Recurring.class, 2); + Key autorenewPollKey = + Key.create(historyEntryKey, PollMessage.Autorenew.class, 3); + Key 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)) - .setContacts(ImmutableSet.of(DesignatedContact.create( - DesignatedContact.Type.ADMIN, - Key.create(contactResource2)))) - .setNameservers(ImmutableSet.of(Key.create(hostResource))) + .setRegistrant(contact1Key) + .setContacts(ImmutableSet.of(DesignatedContact.create(Type.ADMIN, contact2Key))) + .setNameservers(ImmutableSet.of(hostKey)) .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.>of( - Key.create(BillingEvent.OneTime.class, 1), - Key.create(BillingEvent.Recurring.class, 2), - Key.create(PollMessage.Autorenew.class, 3))) - .setServerApproveBillingEvent( - Key.create(BillingEvent.OneTime.class, 1)) - .setServerApproveAutorenewEvent( - Key.create(BillingEvent.Recurring.class, 2)) - .setServerApproveAutorenewPollMessage( - Key.create(PollMessage.Autorenew.class, 3)) + oneTimeBillKey, + recurringBillKey, + autorenewPollKey)) + .setServerApproveBillingEvent(oneTimeBillKey) + .setServerApproveAutorenewEvent(recurringBillKey) + .setServerApproveAutorenewPollMessage(autorenewPollKey) .setTransferRequestTime(clock.nowUtc().plusDays(1)) .setTransferStatus(TransferStatus.SERVER_APPROVED) .setTransferRequestTrid(Trid.create("client trid")) .build()) - .setDeletePollMessage(Key.create(PollMessage.OneTime.class, 1)) - .setAutorenewBillingEvent(Key.create(BillingEvent.Recurring.class, 1)) - .setAutorenewPollMessage(Key.create(PollMessage.Autorenew.class, 2)) + .setDeletePollMessage(onetimePollKey) + .setAutorenewBillingEvent(recurringBillKey) + .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. + } } diff --git a/javatests/google/registry/model/host/HostResourceTest.java b/javatests/google/registry/model/host/HostResourceTest.java index 93d88c512..c63a7ea1c 100644 --- a/javatests/google/registry/model/host/HostResourceTest.java +++ b/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(); + } }