Remove ofy support from Address (#1759)

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 {
 

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")),

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 <a href=
  * "http://tools.ietf.org/html/rfc5733">RFC5733</a>.
  */
-@Embed
 @Embeddable
 @XmlType(propOrder = {"name", "org", "address", "type"})
 public class PostalInfo extends ImmutableObject

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,41 +56,63 @@ 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.
+   *
+   * <p>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 <em>only</em> used for persistence
+   * purpose and should not be directly used in Java.
+   *
+   * <p>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:
+   *
+   * <ul>
+   *   <li>When creating an {@link Address} directly in java, using the {@link Builder}: The {@link
+   *       Builder#setStreet(ImmutableList)} sets both the list and the fields.
+   *   <li>When unmarshalling from XML: The list will be set based on the content of the XML.
+   *       Afterwards, {@link #afterUnmarshal(Unmarshaller, Object)}} will be called to set the
+   *       fields.
+   *   <li>When loading from the database: The fields will be set by the values in SQL. Afterwards,
+   *       {@link #postLoad()} will be called to set the list.
+   * </ul>
+   *
+   * 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.
+   *
+   * <p>Schema validation will enforce the 3-line limit.
+   */
   @XmlJavaTypeAdapter(NormalizedStringAdapter.class)
   @Transient
-  List<String> street;
+  protected List<String> 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<String> getStreet() {
-    if (street == null && streetLine1 != null) {
-      return ImmutableList.of(streetLine1, nullToEmpty(streetLine2), nullToEmpty(streetLine3));
-    } else {
     return nullToEmptyImmutableCopy(street);
   }
-  }
 
   public String getCity() {
     return city;
@@ -139,7 +158,13 @@ public class Address extends ImmutableObject implements Jsonifiable, UnsafeSeria
     public Builder<T> setStreet(ImmutableList<String> 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.
-   *
-   * <p>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<String> street) {
-    mapStreetListToIndividualFields(street);
-  }
-
   /**
    * Sets {@link #street} after loading the entity from Cloud SQL.
    *
-   * <p>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.
+   * <p>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
    *
    * <p>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<String> street) {
-    if (street == null || street.size() == 0) {
+    if (street == null || street.isEmpty()) {
       return;
     }
     streetLine1 = street.get(0);

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")),

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 {
 

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 =
+      "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"yes\"?>\n"
+          + "<testEntity>\n"
+          + "    <address>\n"
+          + "        <street>123 W 14th St</street>\n"
+          + "        <street>8th Fl</street>\n"
+          + "        <street>Rm 8</street>\n"
+          + "        <city>New York</city>\n"
+          + "        <sp>NY</sp>\n"
+          + "        <pc>10011</pc>\n"
+          + "        <cc>US</cc>\n"
+          + "    </address>\n"
+          + "</testEntity>\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<TestAddress> {}
+    }
   }
 }

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<java.lang.String> 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<java.lang.String> street;
-}
 class google.registry.model.reporting.DomainTransactionRecord {
   google.registry.model.reporting.DomainTransactionRecord$TransactionReportField reportField;
   java.lang.Integer reportAmount;