diff --git a/core/src/main/java/google/registry/model/contact/ContactAddress.java b/core/src/main/java/google/registry/model/contact/ContactAddress.java index 7aab6e6d7..38bf62a44 100644 --- a/core/src/main/java/google/registry/model/contact/ContactAddress.java +++ b/core/src/main/java/google/registry/model/contact/ContactAddress.java @@ -14,7 +14,6 @@ package google.registry.model.contact; -import com.googlecode.objectify.annotation.Embed; import google.registry.model.eppcommon.Address; import javax.persistence.Embeddable; @@ -30,7 +29,6 @@ import javax.persistence.Embeddable; * * @see PostalInfo */ -@Embed @Embeddable public class ContactAddress extends Address { diff --git a/core/src/main/java/google/registry/model/contact/ContactBase.java b/core/src/main/java/google/registry/model/contact/ContactBase.java index c0304fc0f..9bbd996d3 100644 --- a/core/src/main/java/google/registry/model/contact/ContactBase.java +++ b/core/src/main/java/google/registry/model/contact/ContactBase.java @@ -19,6 +19,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.model.EppResourceUtils.projectResourceOntoBuilderAtTime; import com.google.common.collect.ImmutableList; +import com.googlecode.objectify.annotation.Ignore; import com.googlecode.objectify.annotation.IgnoreSave; import com.googlecode.objectify.annotation.Index; import com.googlecode.objectify.annotation.OnLoad; @@ -68,7 +69,7 @@ public class ContactBase extends EppResource implements ResourceWithTransferData * Localized postal info for the contact. All contained values must be representable in the 7-bit * US-ASCII character set. Personal info; cleared by {@link ContactResource.Builder#wipeOut}. */ - @IgnoreSave(IfNull.class) + @Ignore @Embedded @AttributeOverrides({ @AttributeOverride(name = "name", column = @Column(name = "addr_local_name")), @@ -96,7 +97,7 @@ public class ContactBase extends EppResource implements ResourceWithTransferData * Internationalized postal info for the contact. Personal info; cleared by {@link * ContactResource.Builder#wipeOut}. */ - @IgnoreSave(IfNull.class) + @Ignore @Embedded @AttributeOverrides({ @AttributeOverride(name = "name", column = @Column(name = "addr_i18n_name")), diff --git a/core/src/main/java/google/registry/model/contact/PostalInfo.java b/core/src/main/java/google/registry/model/contact/PostalInfo.java index 0fb422f21..4a29033ab 100644 --- a/core/src/main/java/google/registry/model/contact/PostalInfo.java +++ b/core/src/main/java/google/registry/model/contact/PostalInfo.java @@ -16,7 +16,6 @@ package google.registry.model.contact; import static com.google.common.base.Preconditions.checkState; -import com.googlecode.objectify.annotation.Embed; import google.registry.model.Buildable; import google.registry.model.Buildable.Overlayable; import google.registry.model.ImmutableObject; @@ -36,7 +35,6 @@ import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter; * Implementation of both "postalInfoType" and "chgPostalInfoType" from RFC5733. */ -@Embed @Embeddable @XmlType(propOrder = {"name", "org", "address", "type"}) public class PostalInfo extends ImmutableObject diff --git a/core/src/main/java/google/registry/model/eppcommon/Address.java b/core/src/main/java/google/registry/model/eppcommon/Address.java index 1a4de6b41..3e656be76 100644 --- a/core/src/main/java/google/registry/model/eppcommon/Address.java +++ b/core/src/main/java/google/registry/model/eppcommon/Address.java @@ -15,14 +15,11 @@ package google.registry.model.eppcommon; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.base.Strings.nullToEmpty; import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; -import com.googlecode.objectify.annotation.AlsoLoad; -import com.googlecode.objectify.annotation.Ignore; import google.registry.model.Buildable; import google.registry.model.ImmutableObject; import google.registry.model.JsonMapBuilder; @@ -59,40 +56,62 @@ import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter; @MappedSuperclass public class Address extends ImmutableObject implements Jsonifiable, UnsafeSerializable { - /** The schema validation will enforce that this has 3 lines at most. */ - // TODO(b/177569726): Remove this field after migration. We need to figure out how to generate - // same XML from streetLine[1,2,3]. + /** + * At most three lines of addresses parsed from XML elements. + * + *

This field is used to marshal to/unmarshal from XML elements. When persisting to/from SQL, + * the next three separate fields are used. Those lines are only used for persistence + * purpose and should not be directly used in Java. + * + *

We need to keep the list and the three fields in sync in all the following scenarios when an + * entity containing a {@link Address} is created: + * + *

+ * + * The syncing is especially important because when merging a detached entity into a session, JPA + * provides no guarantee that transient fields will be preserved. In fact, Hibernate chooses to + * discard them when returning a newly merged entity. This means that it is not enough to provide + * callbacks to populate the fields based on the list before persistence, as merging does not + * invoke the callbacks, and we would lose the address fields if only the list is set. Instead, + * the fields must be populated when the list is. + * + *

Schema validation will enforce the 3-line limit. + */ @XmlJavaTypeAdapter(NormalizedStringAdapter.class) @Transient - List street; + protected List street; - @Ignore @XmlTransient @IgnoredInDiffableMap String streetLine1; + @XmlTransient @IgnoredInDiffableMap protected String streetLine1; - @Ignore @XmlTransient @IgnoredInDiffableMap String streetLine2; + @XmlTransient @IgnoredInDiffableMap protected String streetLine2; - @Ignore @XmlTransient @IgnoredInDiffableMap String streetLine3; + @XmlTransient @IgnoredInDiffableMap protected String streetLine3; @XmlJavaTypeAdapter(NormalizedStringAdapter.class) - String city; + protected String city; @XmlElement(name = "sp") @XmlJavaTypeAdapter(NormalizedStringAdapter.class) - String state; + protected String state; @XmlElement(name = "pc") @XmlJavaTypeAdapter(CollapsedStringAdapter.class) - String zip; + protected String zip; @XmlElement(name = "cc") @XmlJavaTypeAdapter(CollapsedStringAdapter.class) - String countryCode; + protected String countryCode; public ImmutableList getStreet() { - if (street == null && streetLine1 != null) { - return ImmutableList.of(streetLine1, nullToEmpty(streetLine2), nullToEmpty(streetLine3)); - } else { - return nullToEmptyImmutableCopy(street); - } + return nullToEmptyImmutableCopy(street); } public String getCity() { @@ -139,7 +158,13 @@ public class Address extends ImmutableObject implements Jsonifiable, UnsafeSeria public Builder setStreet(ImmutableList street) { checkArgument( street == null || (!street.isEmpty() && street.size() <= 3), - "Street address must have [1-3] lines: %s", street); + "Street address must have [1-3] lines: %s", + street); + //noinspection ConstantConditions + checkArgument( + street.stream().noneMatch(String::isEmpty), + "Street address cannot contain empty string: %s", + street); getInstance().street = street; getInstance().streetLine1 = street.get(0); getInstance().streetLine2 = street.size() >= 2 ? street.get(1) : null; @@ -171,24 +196,13 @@ public class Address extends ImmutableObject implements Jsonifiable, UnsafeSeria } } - /** - * Sets {@link #streetLine1}, {@link #streetLine2} and {@link #streetLine3} after loading the - * entity from Datastore. - * - *

This callback method is used by Objectify to set streetLine[1,2,3] fields as they are not - * persisted in the Datastore. - */ - void onLoad(@AlsoLoad("street") List street) { - mapStreetListToIndividualFields(street); - } - /** * Sets {@link #street} after loading the entity from Cloud SQL. * - *

This callback method is used by Hibernate to set {@link #street} field as it is not - * persisted in Cloud SQL. We are doing this because the street list field is exposed by Address - * class and is used everywhere in our code base. Also, setting/reading a list of strings is more - * convenient. + *

This callback method is used by Hibernate to set the {@link #street} field as it is not + * persisted in Cloud SQL. We are doing this because this field is exposed and used everywhere in + * our code base, whereas the individual {@code streetLine} fields are only used by Hibernate for + * persistence. Also, setting/reading a list of strings is more convenient. */ @PostLoad void postLoad() { @@ -206,12 +220,9 @@ public class Address extends ImmutableObject implements Jsonifiable, UnsafeSeria * *

This is a callback function that JAXB invokes after unmarshalling the XML message. */ + @SuppressWarnings("unused") void afterUnmarshal(Unmarshaller unmarshaller, Object parent) { - mapStreetListToIndividualFields(street); - } - - private void mapStreetListToIndividualFields(List street) { - if (street == null || street.size() == 0) { + if (street == null || street.isEmpty()) { return; } streetLine1 = street.get(0); diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index 185a65e14..c01f9efd6 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -59,10 +59,8 @@ import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.Entity; 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.Parent; -import com.googlecode.objectify.condition.IfNull; import google.registry.model.Buildable; import google.registry.model.CreateAutoTimestamp; import google.registry.model.ImmutableObject; @@ -315,7 +313,7 @@ public class Registrar extends ImmutableObject * Localized {@link RegistrarAddress} for this registrar. Contents can be represented in * unrestricted UTF-8. */ - @IgnoreSave(IfNull.class) + @Ignore @Embedded @AttributeOverrides({ @AttributeOverride( @@ -340,7 +338,7 @@ public class Registrar extends ImmutableObject * Internationalized {@link RegistrarAddress} for this registrar. All contained values must be * representable in the 7-bit US-ASCII character set. */ - @IgnoreSave(IfNull.class) + @Ignore @Embedded @AttributeOverrides({ @AttributeOverride(name = "streetLine1", column = @Column(name = "i18n_address_street_line1")), diff --git a/core/src/main/java/google/registry/model/registrar/RegistrarAddress.java b/core/src/main/java/google/registry/model/registrar/RegistrarAddress.java index 45f634522..7bc94878d 100644 --- a/core/src/main/java/google/registry/model/registrar/RegistrarAddress.java +++ b/core/src/main/java/google/registry/model/registrar/RegistrarAddress.java @@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static google.registry.util.CollectionUtils.forceEmptyToNull; import com.google.common.annotations.VisibleForTesting; -import com.googlecode.objectify.annotation.Embed; import google.registry.model.eppcommon.Address; import javax.persistence.Embeddable; @@ -29,7 +28,6 @@ import javax.persistence.Embeddable; * all defined in parent class {@link Address} so that it can share it with other similar address * classes. */ -@Embed @Embeddable public class RegistrarAddress extends Address { diff --git a/core/src/test/java/google/registry/model/eppcommon/AddressTest.java b/core/src/test/java/google/registry/model/eppcommon/AddressTest.java index ac9406285..27368961e 100644 --- a/core/src/test/java/google/registry/model/eppcommon/AddressTest.java +++ b/core/src/test/java/google/registry/model/eppcommon/AddressTest.java @@ -15,63 +15,156 @@ package google.registry.model.eppcommon; import static com.google.common.truth.Truth.assertThat; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; +import static google.registry.testing.DatabaseHelper.insertInDb; +import static google.registry.testing.DatabaseHelper.loadByEntity; +import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableList; +import google.registry.model.ImmutableObject; +import google.registry.model.eppcommon.AddressTest.TestEntity.TestAddress; +import google.registry.persistence.transaction.JpaTestExtensions; +import google.registry.persistence.transaction.JpaTestExtensions.JpaUnitTestExtension; +import java.io.StringReader; +import java.io.StringWriter; +import javax.persistence.Embeddable; +import javax.persistence.Entity; +import javax.persistence.Id; +import javax.xml.bind.JAXBContext; +import javax.xml.bind.Marshaller; +import javax.xml.bind.Unmarshaller; +import javax.xml.bind.annotation.XmlAccessType; +import javax.xml.bind.annotation.XmlAccessorType; +import javax.xml.bind.annotation.XmlElement; +import javax.xml.bind.annotation.XmlRootElement; +import javax.xml.bind.annotation.XmlTransient; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; /** Tests for {@link Address}. */ class AddressTest { - @Test - void onLoad_setsIndividualStreetLinesSuccessfully() { - Address address = new Address(); - address.onLoad(ImmutableList.of("line1", "line2", "line3")); - assertThat(address.streetLine1).isEqualTo("line1"); - assertThat(address.streetLine2).isEqualTo("line2"); - assertThat(address.streetLine3).isEqualTo("line3"); + @RegisterExtension + public final JpaUnitTestExtension jpa = + new JpaTestExtensions.Builder().withEntityClass(TestEntity.class).buildUnitTestExtension(); + + private static final String ENTITY_XML = + "\n" + + "\n" + + "

\n" + + " 123 W 14th St\n" + + " 8th Fl\n" + + " Rm 8\n" + + " New York\n" + + " NY\n" + + " 10011\n" + + " US\n" + + "
\n" + + "\n"; + + private TestAddress address = createAddress("123 W 14th St", "8th Fl", "Rm 8"); + private TestEntity entity = new TestEntity(1L, address); + + private static TestEntity saveAndLoad(TestEntity entity) { + insertInDb(entity); + return loadByEntity(entity); } @Test - void onLoad_setsOnlyNonNullStreetLines() { - Address address = new Address(); - address.onLoad(ImmutableList.of("line1", "line2")); - assertThat(address.streetLine1).isEqualTo("line1"); - assertThat(address.streetLine2).isEqualTo("line2"); - assertThat(address.streetLine3).isNull(); + void testSuccess_setStreet() { + assertAddress(address, "123 W 14th St", "8th Fl", "Rm 8"); + } + /** Test the persist behavior. */ + @Test + void testSuccess_saveAndLoadStreetLines() { + assertAddress(saveAndLoad(entity).address, "123 W 14th St", "8th Fl", "Rm 8"); + } + + /** Test the merge behavior. */ + @Test + void testSuccess_putAndLoadStreetLines() { + jpaTm().transact(() -> jpaTm().put(entity)); + assertAddress(loadByEntity(entity).address, "123 W 14th St", "8th Fl", "Rm 8"); } @Test - void onLoad_doNothingIfInputIsNull() { - Address address = new Address(); - address.onLoad(null); - assertThat(address.streetLine1).isNull(); - assertThat(address.streetLine2).isNull(); - assertThat(address.streetLine3).isNull(); + void testSuccess_setsNullStreetLine() { + entity = new TestEntity(1L, createAddress("line1", "line2")); + TestEntity savedEntity = saveAndLoad(entity); + assertAddress(savedEntity.address, "line1", "line2"); + assertThat(savedEntity.address.streetLine3).isNull(); } @Test - void postLoad_setsStreetListSuccessfully() { - Address address = new Address(); - address.streetLine1 = "line1"; - address.streetLine2 = "line2"; - address.streetLine3 = "line3"; - address.postLoad(); - assertThat(address.street).containsExactly("line1", "line2", "line3"); + void testFailure_tooManyStreetLines() { + assertThrows( + IllegalArgumentException.class, () -> createAddress("line1", "line2", "line3", "line4")); } @Test - void postLoad_setsOnlyNonNullStreetLines() { - Address address = new Address(); - address.streetLine1 = "line1"; - address.streetLine2 = "line2"; - address.postLoad(); - assertThat(address.street).containsExactly("line1", "line2"); + void testFailure_emptyStreetLine() { + assertThrows(IllegalArgumentException.class, () -> createAddress("line1", "", "line3")); } @Test - void postLoad_doNothingIfInputIsNull() { - Address address = new Address(); - address.postLoad(); - assertThat(address.street).isNull(); + void testSuccess_pojoToAndFromXml() throws Exception { + JAXBContext jaxbContext = JAXBContext.newInstance(TestEntity.class); + // POJO to XML + Marshaller marshaller = jaxbContext.createMarshaller(); + marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true); + StringWriter sw = new StringWriter(); + marshaller.marshal(entity, sw); + String xml = sw.toString(); + assertThat(xml).isEqualTo(ENTITY_XML); + // XML to POJO + Unmarshaller unmarshaller = jaxbContext.createUnmarshaller(); + TestEntity unmarshalledEntity = (TestEntity) unmarshaller.unmarshal(new StringReader(xml)); + assertAddress(unmarshalledEntity.address, "123 W 14th St", "8th Fl", "Rm 8"); + } + + private static TestAddress createAddress(String... streetList) { + return new TestAddress.Builder() + .setStreet(ImmutableList.copyOf(streetList)) + .setCity("New York") + .setState("NY") + .setZip("10011") + .setCountryCode("US") + .build(); + } + + private static void assertAddress(TestAddress address, String... streetList) { + assertThat(address.street).containsExactly((Object[]) streetList); + if (streetList.length > 0) { + assertThat(address.streetLine1).isEqualTo(streetList[0]); + } + if (streetList.length > 1) { + assertThat(address.streetLine2).isEqualTo(streetList[1]); + } + if (streetList.length > 2) { + assertThat(address.streetLine3).isEqualTo(streetList[2]); + } + } + + @Entity(name = "TestEntity") + @XmlRootElement + @XmlAccessorType(XmlAccessType.FIELD) + static class TestEntity extends ImmutableObject { + + @XmlTransient @Id long id; + + @XmlElement TestAddress address; + + TestEntity() {} + + TestEntity(Long id, TestAddress address) { + this.id = id; + this.address = address; + } + + @Embeddable + public static class TestAddress extends Address { + + public static class Builder extends Address.Builder {} + } } } diff --git a/core/src/test/resources/google/registry/model/schema.txt b/core/src/test/resources/google/registry/model/schema.txt index 5458a4bb3..a3f17b48b 100644 --- a/core/src/test/resources/google/registry/model/schema.txt +++ b/core/src/test/resources/google/registry/model/schema.txt @@ -5,13 +5,6 @@ class google.registry.model.common.GaeUserIdConverter { @Id long id; com.google.appengine.api.users.User user; } -class google.registry.model.contact.ContactAddress { - java.lang.String city; - java.lang.String countryCode; - java.lang.String state; - java.lang.String zip; - java.util.List street; -} class google.registry.model.contact.ContactAuthInfo { google.registry.model.eppcommon.AuthInfo$PasswordAuth pw; } @@ -21,8 +14,6 @@ class google.registry.model.contact.ContactBase { google.registry.model.contact.ContactPhoneNumber fax; google.registry.model.contact.ContactPhoneNumber voice; google.registry.model.contact.Disclose disclose; - google.registry.model.contact.PostalInfo internationalizedPostalInfo; - google.registry.model.contact.PostalInfo localizedPostalInfo; google.registry.model.transfer.ContactTransferData transferData; java.lang.String contactId; java.lang.String creationClientId; @@ -61,8 +52,6 @@ class google.registry.model.contact.ContactResource { google.registry.model.contact.ContactPhoneNumber fax; google.registry.model.contact.ContactPhoneNumber voice; google.registry.model.contact.Disclose disclose; - google.registry.model.contact.PostalInfo internationalizedPostalInfo; - google.registry.model.contact.PostalInfo localizedPostalInfo; google.registry.model.transfer.ContactTransferData transferData; java.lang.String contactId; java.lang.String creationClientId; @@ -87,12 +76,6 @@ class google.registry.model.contact.Disclose { class google.registry.model.contact.Disclose$PostalInfoChoice { google.registry.model.contact.PostalInfo$Type type; } -class google.registry.model.contact.PostalInfo { - google.registry.model.contact.ContactAddress address; - google.registry.model.contact.PostalInfo$Type type; - java.lang.String name; - java.lang.String org; -} enum google.registry.model.contact.PostalInfo$Type { INTERNATIONALIZED; LOCALIZED; @@ -372,8 +355,6 @@ class google.registry.model.registrar.Registrar { boolean registryLockAllowed; google.registry.model.registrar.Registrar$State state; google.registry.model.registrar.Registrar$Type type; - google.registry.model.registrar.RegistrarAddress internationalizedAddress; - google.registry.model.registrar.RegistrarAddress localizedAddress; java.lang.Long ianaIdentifier; java.lang.String clientCertificate; java.lang.String clientCertificateHash; @@ -414,13 +395,6 @@ enum google.registry.model.registrar.Registrar$Type { REAL; TEST; } -class google.registry.model.registrar.RegistrarAddress { - java.lang.String city; - java.lang.String countryCode; - java.lang.String state; - java.lang.String zip; - java.util.List street; -} class google.registry.model.reporting.DomainTransactionRecord { google.registry.model.reporting.DomainTransactionRecord$TransactionReportField reportField; java.lang.Integer reportAmount;