Change pkey of User to emailAddress (#2505)

Originally, we though that User entities were going to have mutable
email addresses, and thus would require a non-changing primary key. This
proved to not be the case. It'll simplify the User loading/saving code
if we just do everything by email address.

Obviously this doesn't change much functionality, but it prepares us for
removing the id field down the line once the changes propagate.

core/src/main/java/google/registry/model/console/User.java

@@ -14,9 +14,11 @@
 
 package google.registry.model.console;
 
+import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.tools.server.UpdateUserGroupAction.GROUP_UPDATE_QUEUE;
 
 import com.google.cloud.tasks.v2.Task;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.flogger.FluentLogger;
 import google.registry.batch.CloudTasksUtils;
@@ -27,25 +29,25 @@ import google.registry.tools.server.UpdateUserGroupAction;
 import google.registry.tools.server.UpdateUserGroupAction.Mode;
 import google.registry.util.RegistryEnvironment;
 import java.util.Optional;
+import java.util.concurrent.atomic.AtomicLong;
 import javax.persistence.Access;
 import javax.persistence.AccessType;
 import javax.persistence.Embeddable;
 import javax.persistence.Entity;
-import javax.persistence.GeneratedValue;
-import javax.persistence.GenerationType;
 import javax.persistence.Id;
-import javax.persistence.Index;
 import javax.persistence.Table;
 
 /** A console user, either a registry employee or a registrar partner. */
 @Embeddable
 @Entity
-@Table(indexes = {@Index(columnList = "emailAddress", name = "user_email_address_idx")})
+@Table
 public class User extends UserBase {
 
   public static final String IAP_SECURED_WEB_APP_USER_ROLE = "roles/iap.httpsResourceAccessor";
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
+  @VisibleForTesting public static final AtomicLong ID_GENERATOR_FOR_TESTING = new AtomicLong();
+
   /**
    * Grants the user permission to pass IAP.
    *
@@ -114,13 +116,18 @@ public class User extends UserBase {
   }
 
   @Override
-  @Id
-  @GeneratedValue(strategy = GenerationType.IDENTITY)
   @Access(AccessType.PROPERTY)
   public Long getId() {
     return super.getId();
   }
 
+  @Id
+  @Override
+  @Access(AccessType.PROPERTY)
+  public String getEmailAddress() {
+    return super.getEmailAddress();
+  }
+
   @Override
   public Builder asBuilder() {
     return new Builder(clone(this));
@@ -139,5 +146,20 @@ public class User extends UserBase {
     public Builder(User user) {
       super(user);
     }
+
+    @Override
+    public User build() {
+      // Sets the ID temporarily until we can get rid of the non-null constraint (and the field)
+      if (getInstance().getId() == null || getInstance().getId().equals(0L)) {
+        // In tests, we cannot guarantee that the database is fully set up -- so don't use it to
+        // generate a new long
+        if (RegistryEnvironment.get() == RegistryEnvironment.UNITTEST) {
+          getInstance().setId(ID_GENERATOR_FOR_TESTING.getAndIncrement());
+        } else {
+          getInstance().setId(tm().reTransact(tm()::allocateId));
+        }
+      }
+      return super.build();
+    }
   }
 }

core/src/main/java/google/registry/model/console/UserBase.java

@@ -53,8 +53,7 @@ public class UserBase extends UpdateAutoTimestampEntity implements Buildable {
   @Transient private Long id;
 
   /** Email address of the user in question. */
-  @Column(nullable = false)
-  String emailAddress;
+  @Transient String emailAddress;
 
   /** Optional external email address to use for registry lock confirmation emails. */
   @Column String registryLockEmailAddress;
@@ -90,6 +89,10 @@ public class UserBase extends UpdateAutoTimestampEntity implements Buildable {
     this.id = id;
   }
 
+  void setEmailAddress(String emailAddress) {
+    this.emailAddress = emailAddress;
+  }
+
   public String getEmailAddress() {
     return emailAddress;
   }

core/src/main/java/google/registry/model/console/UserDao.java

@@ -14,7 +14,6 @@
 
 package google.registry.model.console;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 
 import java.util.Optional;
@@ -32,23 +31,8 @@ public class UserDao {
                     .findFirst());
   }
 
-  /** Saves the given user, checking that no existing user already exists with this email. */
+  /** Saves the given user, updating it if it already exists. */
   public static void saveUser(User user) {
-    tm().transact(
-            () -> {
-              // Check for an existing user (the unique constraint protects us, but this gives a
-              // nicer exception)
-              Optional<User> maybeSavedUser = loadUser(user.getEmailAddress());
-              if (maybeSavedUser.isPresent()) {
-                User savedUser = maybeSavedUser.get();
-                checkArgument(
-                    savedUser.getId().equals(user.getId()),
-                    String.format(
-                        "Attempted save of User with email address %s and ID %s, user with that"
-                            + " email already exists with ID %s",
-                        user.getEmailAddress(), user.getId(), savedUser.getId()));
-              }
-              tm().put(user);
-            });
+    tm().transact(() -> tm().put(user));
   }
 }

core/src/main/java/google/registry/model/console/UserUpdateHistory.java

@@ -42,6 +42,9 @@ public class UserUpdateHistory extends ConsoleUpdateHistory {
   @Column(nullable = false, name = "userId")
   Long id;
 
+  @Column(nullable = false, name = "emailAddress")
+  String emailAddress;
+
   public UserBase getUser() {
     return user;
   }
@@ -49,6 +52,7 @@ public class UserUpdateHistory extends ConsoleUpdateHistory {
   @PostLoad
   void postLoad() {
     user.setId(id);
+    user.setEmailAddress(emailAddress);
   }
 
   /** Creates a {@link VKey} instance for this entity. */
@@ -80,6 +84,7 @@ public class UserUpdateHistory extends ConsoleUpdateHistory {
     public Builder setUser(User user) {
       getInstance().user = user;
       getInstance().id = user.getId();
+      getInstance().emailAddress = user.getEmailAddress();
       return this;
     }
   }

core/src/test/java/google/registry/model/console/UserDaoTest.java

@@ -17,7 +17,6 @@ package google.registry.model.console;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import google.registry.model.EntityTestCase;
 import org.junit.jupiter.api.Test;
@@ -64,7 +63,7 @@ public class UserDaoTest extends EntityTestCase {
   }
 
   @Test
-  void testFailure_sameEmail() {
+  void testSuccess_updateUser_sameEmail() {
     User user1 =
         new User.Builder()
             .setEmailAddress("email@email.com")
@@ -73,12 +72,12 @@ public class UserDaoTest extends EntityTestCase {
     User user2 =
         new User.Builder()
             .setEmailAddress("email@email.com")
-            .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.SUPPORT_AGENT).build())
+            .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.FTE).build())
             .build();
     UserDao.saveUser(user1);
-    assertThrows(IllegalArgumentException.class, () -> UserDao.saveUser(user2));
+    UserDao.saveUser(user2);
     assertAboutImmutableObjects()
-        .that(user1)
-        .isEqualExceptFields(UserDao.loadUser("email@email.com").get(), "id", "updateTimestamp");
+        .that(user2)
+        .isEqualExceptFields(UserDao.loadUser("email@email.com").get(), "updateTimestamp");
   }
 }

core/src/test/java/google/registry/tools/GetUserCommandTest.java

@@ -28,6 +28,7 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
 
   @BeforeEach
   void beforeEach() {
+    User.ID_GENERATOR_FOR_TESTING.set(0L);
     UserDao.saveUser(
         new User.Builder()
             .setEmailAddress("johndoe@theregistrar.com")
@@ -52,7 +53,7 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
         """
         User: {
             emailAddress=fte@google.com
-            id=2
+            id=1
             registryLockEmailAddress=null
             registryLockPasswordHash=null
             registryLockPasswordSalt=null
@@ -75,7 +76,7 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
         """
         User: {
             emailAddress=johndoe@theregistrar.com
-            id=1
+            id=0
             registryLockEmailAddress=null
             registryLockPasswordHash=null
             registryLockPasswordSalt=null
@@ -90,7 +91,7 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
         }
         User: {
             emailAddress=fte@google.com
-            id=2
+            id=1
             registryLockEmailAddress=null
             registryLockPasswordHash=null
             registryLockPasswordSalt=null
@@ -113,7 +114,7 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
         """
         User: {
             emailAddress=johndoe@theregistrar.com
-            id=1
+            id=0
             registryLockEmailAddress=null
             registryLockPasswordHash=null
             registryLockPasswordSalt=null