Remove ID field from User in Java classes and remove UserDao (#2517)

This is the first step in the field removal (second will be removing the
column from SQL once this is deployed).

There's no point in using a UserDao versus just doing the standard
loading-from-DB that we do everywhere else. No need to special-case it.

core/src/test/java/google/registry/model/OteAccountBuilderTest.java

@@ -22,6 +22,7 @@ import static google.registry.persistence.transaction.JpaTransactionManagerExten
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT_HASH;
 import static google.registry.testing.DatabaseHelper.createTld;
+import static google.registry.testing.DatabaseHelper.loadByKeyIfPresent;
 import static google.registry.testing.DatabaseHelper.persistPremiumList;
 import static google.registry.testing.DatabaseHelper.persistSimpleResource;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
@@ -36,10 +37,10 @@ import com.google.common.collect.ImmutableList;
 import google.registry.batch.CloudTasksUtils;
 import google.registry.model.console.RegistrarRole;
 import google.registry.model.console.User;
-import google.registry.model.console.UserDao;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.tld.Tld;
 import google.registry.model.tld.Tld.TldState;
+import google.registry.persistence.VKey;
 import google.registry.persistence.transaction.JpaTestExtensions;
 import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationTestExtension;
 import google.registry.testing.CloudTasksHelper;
@@ -106,7 +107,7 @@ public final class OteAccountBuilderTest {
   }
 
   public static void verifyUser(String registrarId, String email) {
-    Optional<User> maybeUser = UserDao.loadUser(email);
+    Optional<User> maybeUser = loadByKeyIfPresent(VKey.create(User.class, email));
     assertThat(maybeUser).isPresent();
     assertThat(maybeUser.get().getUserRoles().getRegistrarRoles().get(registrarId))
         .isEqualTo(RegistrarRole.ACCOUNT_MANAGER);

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

@@ -1,83 +0,0 @@
-// Copyright 2022 The Nomulus Authors. All Rights Reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-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 google.registry.model.EntityTestCase;
-import org.junit.jupiter.api.Test;
-
-/** Tests for {@link UserDao}. */
-public class UserDaoTest extends EntityTestCase {
-
-  @Test
-  void testSuccess_saveAndRetrieve() {
-    User user1 =
-        new User.Builder()
-            .setEmailAddress("email@email.com")
-            .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.SUPPORT_AGENT).build())
-            .build();
-    User user2 =
-        new User.Builder()
-            .setEmailAddress("foo@bar.com")
-            .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.SUPPORT_AGENT).build())
-            .build();
-    UserDao.saveUser(user1);
-    UserDao.saveUser(user2);
-    assertAboutImmutableObjects()
-        .that(user1)
-        .isEqualExceptFields(UserDao.loadUser("email@email.com").get(), "id", "updateTimestamp");
-    assertAboutImmutableObjects()
-        .that(user2)
-        .isEqualExceptFields(UserDao.loadUser("foo@bar.com").get(), "id", "updateTimestamp");
-  }
-
-  @Test
-  void testSuccess_absentUser() {
-    User user =
-        new User.Builder()
-            .setEmailAddress("email@email.com")
-            .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.SUPPORT_AGENT).build())
-            .build();
-    UserDao.saveUser(user);
-    User fromDb = UserDao.loadUser("email@email.com").get();
-    // nonexistent one should never exist
-    assertThat(UserDao.loadUser("nonexistent@email.com")).isEmpty();
-    // now try deleting the one that does exist
-    tm().transact(() -> tm().delete(fromDb));
-    assertThat(UserDao.loadUser("email@email.com")).isEmpty();
-  }
-
-  @Test
-  void testSuccess_updateUser_sameEmail() {
-    User user1 =
-        new User.Builder()
-            .setEmailAddress("email@email.com")
-            .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.SUPPORT_AGENT).build())
-            .build();
-    User user2 =
-        new User.Builder()
-            .setEmailAddress("email@email.com")
-            .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.FTE).build())
-            .build();
-    UserDao.saveUser(user1);
-    UserDao.saveUser(user2);
-    assertAboutImmutableObjects()
-        .that(user2)
-        .isEqualExceptFields(UserDao.loadUser("email@email.com").get(), "updateTimestamp");
-  }
-}

core/src/test/java/google/registry/testing/DatabaseHelper.java

@@ -73,7 +73,6 @@ import google.registry.model.billing.BillingRecurrence;
 import google.registry.model.common.DnsRefreshRequest;
 import google.registry.model.console.GlobalRole;
 import google.registry.model.console.User;
-import google.registry.model.console.UserDao;
 import google.registry.model.console.UserRoles;
 import google.registry.model.contact.Contact;
 import google.registry.model.contact.ContactAuthInfo;
@@ -1014,20 +1013,33 @@ public final class DatabaseHelper {
     return tm().transact(() -> tm().loadByEntity(resource));
   }
 
+  public static User loadExistingUser(String emailAddress) {
+    return loadByKey(VKey.create(User.class, emailAddress));
+  }
+
   /** Persists an admin {@link User} with the given email address if it doesn't already exist. */
   public static User createAdminUser(String emailAddress) {
-    Optional<User> existingUser = UserDao.loadUser(emailAddress);
-    if (existingUser.isPresent()) {
-      return existingUser.get();
-    }
-    User user =
-        new User.Builder()
-            .setEmailAddress(emailAddress)
-            .setUserRoles(
-                new UserRoles.Builder().setGlobalRole(GlobalRole.FTE).setIsAdmin(true).build())
-            .build();
-    tm().transact(() -> tm().put(user));
-    return UserDao.loadUser(emailAddress).get();
+    // Reload the user to pick up the update time
+    return loadByEntity(
+        tm().transact(
+                () -> {
+                  VKey<User> key = VKey.create(User.class, emailAddress);
+                  Optional<User> existingUser = tm().loadByKeyIfPresent(key);
+                  if (existingUser.isPresent()) {
+                    return existingUser.get();
+                  }
+                  User user =
+                      new User.Builder()
+                          .setEmailAddress(emailAddress)
+                          .setUserRoles(
+                              new UserRoles.Builder()
+                                  .setGlobalRole(GlobalRole.FTE)
+                                  .setIsAdmin(true)
+                                  .build())
+                          .build();
+                  tm().put(user);
+                  return user;
+                }));
   }
 
   /** Returns all the history entries that are parented off the given EppResource. */

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

@@ -16,6 +16,7 @@ package google.registry.tools;
 
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.console.User.IAP_SECURED_WEB_APP_USER_ROLE;
+import static google.registry.testing.DatabaseHelper.loadExistingUser;
 import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -28,7 +29,6 @@ import com.google.common.net.MediaType;
 import google.registry.model.console.GlobalRole;
 import google.registry.model.console.RegistrarRole;
 import google.registry.model.console.User;
-import google.registry.model.console.UserDao;
 import google.registry.testing.DatabaseHelper;
 import google.registry.tools.server.UpdateUserGroupAction;
 import java.util.Optional;
@@ -93,14 +93,14 @@ public class CreateUserCommandTest extends CommandTestCase<CreateUserCommand> {
         "user@example.test",
         "--registry_lock_email_address",
         "registrylockemail@otherexample.test");
-    assertThat(UserDao.loadUser("user@example.test").get().getRegistryLockEmailAddress())
+    assertThat(loadExistingUser("user@example.test").getRegistryLockEmailAddress())
         .hasValue("registrylockemail@otherexample.test");
   }
 
   @Test
   void testSuccess_admin() throws Exception {
     runCommandForced("--email", "user@example.test", "--admin", "true");
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().isAdmin()).isTrue();
+    assertThat(loadExistingUser("user@example.test").getUserRoles().isAdmin()).isTrue();
     verify(iamClient).addBinding("user@example.test", IAP_SECURED_WEB_APP_USER_ROLE);
     verifyNoMoreInteractions(iamClient);
     verifyNoInteractions(connection);
@@ -109,7 +109,7 @@ public class CreateUserCommandTest extends CommandTestCase<CreateUserCommand> {
   @Test
   void testSuccess_globalRole() throws Exception {
     runCommandForced("--email", "user@example.test", "--global_role", "FTE");
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().getGlobalRole())
+    assertThat(loadExistingUser("user@example.test").getUserRoles().getGlobalRole())
         .isEqualTo(GlobalRole.FTE);
     verify(iamClient).addBinding("user@example.test", IAP_SECURED_WEB_APP_USER_ROLE);
     verifyNoMoreInteractions(iamClient);
@@ -123,7 +123,7 @@ public class CreateUserCommandTest extends CommandTestCase<CreateUserCommand> {
         "user@example.test",
         "--registrar_roles",
         "TheRegistrar=ACCOUNT_MANAGER,NewRegistrar=PRIMARY_CONTACT");
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().getRegistrarRoles())
+    assertThat(loadExistingUser("user@example.test").getUserRoles().getRegistrarRoles())
         .isEqualTo(
             ImmutableMap.of(
                 "TheRegistrar",

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

@@ -24,7 +24,8 @@ import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.net.MediaType;
-import google.registry.model.console.UserDao;
+import google.registry.model.console.User;
+import google.registry.persistence.VKey;
 import google.registry.testing.DatabaseHelper;
 import google.registry.tools.server.UpdateUserGroupAction;
 import java.util.Optional;
@@ -47,9 +48,10 @@ public class DeleteUserCommandTest extends CommandTestCase<DeleteUserCommand> {
   @Test
   void testSuccess_deletesUser() throws Exception {
     DatabaseHelper.createAdminUser("email@example.test");
-    assertThat(UserDao.loadUser("email@example.test")).isPresent();
+    VKey<User> key = VKey.create(User.class, "email@example.test");
+    assertThat(DatabaseHelper.loadByKeyIfPresent(key)).isPresent();
     runCommandForced("--email", "email@example.test");
-    assertThat(UserDao.loadUser("email@example.test")).isEmpty();
+    assertThat(DatabaseHelper.loadByKeyIfPresent(key)).isEmpty();
     verify(iamClient).removeBinding("email@example.test", IAP_SECURED_WEB_APP_USER_ROLE);
     verifyNoMoreInteractions(iamClient);
     verifyNoInteractions(connection);
@@ -59,9 +61,10 @@ public class DeleteUserCommandTest extends CommandTestCase<DeleteUserCommand> {
   void testSuccess_deletesUser_removeFromGroup() throws Exception {
     command.maybeGroupEmailAddress = Optional.of("group@example.test");
     DatabaseHelper.createAdminUser("email@example.test");
-    assertThat(UserDao.loadUser("email@example.test")).isPresent();
+    VKey<User> key = VKey.create(User.class, "email@example.test");
+    assertThat(DatabaseHelper.loadByKeyIfPresent(key)).isPresent();
     runCommandForced("--email", "email@example.test");
-    assertThat(UserDao.loadUser("email@example.test")).isEmpty();
+    assertThat(DatabaseHelper.loadByKeyIfPresent(key)).isEmpty();
     verify(connection)
         .sendPostRequest(
             UpdateUserGroupAction.PATH,

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

@@ -14,12 +14,13 @@
 
 package google.registry.tools;
 
+
 import com.google.common.collect.ImmutableMap;
 import google.registry.model.console.GlobalRole;
 import google.registry.model.console.RegistrarRole;
 import google.registry.model.console.User;
-import google.registry.model.console.UserDao;
 import google.registry.model.console.UserRoles;
+import google.registry.testing.DatabaseHelper;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -28,8 +29,7 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
 
   @BeforeEach
   void beforeEach() {
-    User.ID_GENERATOR_FOR_TESTING.set(0L);
-    UserDao.saveUser(
+    DatabaseHelper.putInDb(
         new User.Builder()
             .setEmailAddress("johndoe@theregistrar.com")
             .setUserRoles(
@@ -37,8 +37,7 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
                     .setRegistrarRoles(
                         ImmutableMap.of("TheRegistrar", RegistrarRole.PRIMARY_CONTACT))
                     .build())
-            .build());
-    UserDao.saveUser(
+            .build(),
         new User.Builder()
             .setEmailAddress("fte@google.com")
             .setUserRoles(
@@ -53,7 +52,6 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
         """
         User: {
             emailAddress=fte@google.com
-            id=1
             registryLockEmailAddress=null
             registryLockPasswordHash=null
             registryLockPasswordSalt=null
@@ -76,7 +74,6 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
         """
         User: {
             emailAddress=johndoe@theregistrar.com
-            id=0
             registryLockEmailAddress=null
             registryLockPasswordHash=null
             registryLockPasswordSalt=null
@@ -91,7 +88,6 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
         }
         User: {
             emailAddress=fte@google.com
-            id=1
             registryLockEmailAddress=null
             registryLockPasswordHash=null
             registryLockPasswordSalt=null
@@ -114,7 +110,6 @@ public class GetUserCommandTest extends CommandTestCase<GetUserCommand> {
         """
         User: {
             emailAddress=johndoe@theregistrar.com
-            id=0
             registryLockEmailAddress=null
             registryLockPasswordHash=null
             registryLockPasswordSalt=null

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

@@ -22,9 +22,11 @@ import static google.registry.model.tld.Tld.TldState.GENERAL_AVAILABILITY;
 import static google.registry.model.tld.Tld.TldState.START_DATE_SUNRISE;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT_HASH;
 import static google.registry.testing.DatabaseHelper.createTld;
+import static google.registry.testing.DatabaseHelper.loadExistingUser;
 import static google.registry.testing.DatabaseHelper.loadRegistrar;
 import static google.registry.testing.DatabaseHelper.persistPremiumList;
 import static google.registry.testing.DatabaseHelper.persistResource;
+import static google.registry.testing.DatabaseHelper.putInDb;
 import static org.joda.money.CurrencyUnit.USD;
 import static org.joda.time.DateTimeZone.UTC;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -39,7 +41,6 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 import google.registry.model.console.GlobalRole;
 import google.registry.model.console.User;
-import google.registry.model.console.UserDao;
 import google.registry.model.console.UserRoles;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.tld.Tld;
@@ -447,12 +448,11 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
 
   @Test
   void testFailure_userExists() {
-    User user =
+    putInDb(
         new User.Builder()
             .setEmailAddress("contact@email.com")
             .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.FTE).build())
-            .build();
-    UserDao.saveUser(user);
+            .build());
     IllegalStateException thrown =
         assertThrows(
             IllegalStateException.class,
@@ -494,12 +494,11 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
 
   @Test
   void testSuccess_userExists_replaceExisting() throws Exception {
-    User user =
+    putInDb(
         new User.Builder()
             .setEmailAddress("contact@email.com")
             .setUserRoles(new UserRoles.Builder().setGlobalRole(GlobalRole.FTE).build())
-            .build();
-    UserDao.saveUser(user);
+            .build());
 
     runCommandForced(
         "--overwrite",
@@ -520,7 +519,7 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
     verifyUser("blobio-5", "contact@email.com");
 
     // verify that the role is completely replaced, e.g., the global role is gone.
-    assertThat(UserDao.loadUser("contact@email.com").get().getUserRoles().getGlobalRole())
+    assertThat(loadExistingUser("contact@email.com").getUserRoles().getGlobalRole())
         .isEqualTo(GlobalRole.NONE);
 
     verifyIapPermission("contact@email.com");

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

@@ -15,13 +15,14 @@
 package google.registry.tools;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.testing.DatabaseHelper.loadExistingUser;
+import static google.registry.testing.DatabaseHelper.putInDb;
 import static org.junit.Assert.assertThrows;
 
 import com.google.common.collect.ImmutableMap;
 import google.registry.model.console.GlobalRole;
 import google.registry.model.console.RegistrarRole;
 import google.registry.model.console.User;
-import google.registry.model.console.UserDao;
 import google.registry.model.console.UserRoles;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -31,7 +32,7 @@ public class UpdateUserCommandTest extends CommandTestCase<UpdateUserCommand> {
 
   @BeforeEach
   void beforeEach() throws Exception {
-    UserDao.saveUser(
+    putInDb(
         new User.Builder()
             .setEmailAddress("user@example.test")
             .setUserRoles(new UserRoles.Builder().build())
@@ -45,41 +46,39 @@ public class UpdateUserCommandTest extends CommandTestCase<UpdateUserCommand> {
         "user@example.test",
         "--registry_lock_email_address",
         "registrylockemail@otherexample.test");
-    assertThat(UserDao.loadUser("user@example.test").get().getRegistryLockEmailAddress())
+    assertThat(loadExistingUser("user@example.test").getRegistryLockEmailAddress())
         .hasValue("registrylockemail@otherexample.test");
   }
 
   @Test
   void testSuccess_removeRegistryLockEmail() throws Exception {
-    UserDao.saveUser(
-        UserDao.loadUser("user@example.test")
-            .get()
+    putInDb(
+        loadExistingUser("user@example.test")
             .asBuilder()
             .setRegistryLockEmailAddress("registrylock@otherexample.test")
             .build());
     runCommandForced("--email", "user@example.test", "--registry_lock_email_address", "");
-    assertThat(UserDao.loadUser("user@example.test").get().getRegistryLockEmailAddress()).isEmpty();
+    assertThat(loadExistingUser("user@example.test").getRegistryLockEmailAddress()).isEmpty();
   }
 
   @Test
   void testSuccess_admin() throws Exception {
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().isAdmin()).isFalse();
+    assertThat(loadExistingUser("user@example.test").getUserRoles().isAdmin()).isFalse();
     runCommandForced("--email", "user@example.test", "--admin", "true");
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().isAdmin()).isTrue();
+    assertThat(loadExistingUser("user@example.test").getUserRoles().isAdmin()).isTrue();
     runCommandForced("--email", "user@example.test", "--admin", "false");
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().isAdmin()).isFalse();
+    assertThat(loadExistingUser("user@example.test").getUserRoles().isAdmin()).isFalse();
   }
 
   @Test
   void testSuccess_registrarRoles() throws Exception {
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().getRegistrarRoles())
-        .isEmpty();
+    assertThat(loadExistingUser("user@example.test").getUserRoles().getRegistrarRoles()).isEmpty();
     runCommandForced(
         "--email",
         "user@example.test",
         "--registrar_roles",
         "TheRegistrar=ACCOUNT_MANAGER,NewRegistrar=PRIMARY_CONTACT");
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().getRegistrarRoles())
+    assertThat(loadExistingUser("user@example.test").getUserRoles().getRegistrarRoles())
         .isEqualTo(
             ImmutableMap.of(
                 "TheRegistrar",
@@ -87,16 +86,15 @@ public class UpdateUserCommandTest extends CommandTestCase<UpdateUserCommand> {
                 "NewRegistrar",
                 RegistrarRole.PRIMARY_CONTACT));
     runCommandForced("--email", "user@example.test", "--registrar_roles", "");
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().getRegistrarRoles())
-        .isEmpty();
+    assertThat(loadExistingUser("user@example.test").getUserRoles().getRegistrarRoles()).isEmpty();
   }
 
   @Test
   void testSuccess_globalRole() throws Exception {
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().getGlobalRole())
+    assertThat(loadExistingUser("user@example.test").getUserRoles().getGlobalRole())
         .isEqualTo(GlobalRole.NONE);
     runCommandForced("--email", "user@example.test", "--global_role", "FTE");
-    assertThat(UserDao.loadUser("user@example.test").get().getUserRoles().getGlobalRole())
+    assertThat(loadExistingUser("user@example.test").getUserRoles().getGlobalRole())
         .isEqualTo(GlobalRole.FTE);
   }