From 3c126ddfd4ac2f7346b844e1937f006c6b01eab4 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Mon, 5 Aug 2024 16:36:17 -0400 Subject: [PATCH] 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. --- .../registry/model/OteAccountBuilder.java | 32 ++----- .../google/registry/model/console/User.java | 28 +------ .../registry/model/console/UserBase.java | 20 +---- .../registry/model/console/UserDao.java | 38 --------- .../model/console/UserUpdateHistory.java | 6 -- .../OidcTokenAuthenticationMechanism.java | 6 +- .../tools/CreateOrUpdateUserCommand.java | 5 +- .../registry/tools/CreateUserCommand.java | 6 +- .../registry/tools/DeleteUserCommand.java | 14 ++-- .../google/registry/tools/GetUserCommand.java | 19 +++-- .../registry/tools/UpdateUserCommand.java | 8 +- .../ConsoleRegistrarCreatorAction.java | 3 +- .../registry/model/OteAccountBuilderTest.java | 5 +- .../registry/model/console/UserDaoTest.java | 83 ------------------- .../registry/testing/DatabaseHelper.java | 38 ++++++--- .../registry/tools/CreateUserCommandTest.java | 10 +-- .../registry/tools/DeleteUserCommandTest.java | 13 +-- .../registry/tools/GetUserCommandTest.java | 13 +-- .../registry/tools/SetupOteCommandTest.java | 15 ++-- .../registry/tools/UpdateUserCommandTest.java | 32 ++++--- .../sql/er_diagram/brief_er_diagram.html | 4 +- .../sql/er_diagram/full_er_diagram.html | 4 +- .../sql/schema/db-schema.sql.generated | 2 - 23 files changed, 116 insertions(+), 288 deletions(-) delete mode 100644 core/src/main/java/google/registry/model/console/UserDao.java delete mode 100644 core/src/test/java/google/registry/model/console/UserDaoTest.java diff --git a/core/src/main/java/google/registry/model/OteAccountBuilder.java b/core/src/main/java/google/registry/model/OteAccountBuilder.java index 5fe9183cd..411e9646a 100644 --- a/core/src/main/java/google/registry/model/OteAccountBuilder.java +++ b/core/src/main/java/google/registry/model/OteAccountBuilder.java @@ -32,7 +32,6 @@ import com.google.common.collect.Streams; 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.console.UserRoles; import google.registry.model.pricing.StaticPremiumListPricingEngine; import google.registry.model.registrar.Registrar; @@ -48,9 +47,7 @@ import google.registry.util.CidrAddressBlock; import google.registry.util.RegistryEnvironment; import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Optional; import java.util.function.Function; import java.util.regex.Pattern; @@ -278,22 +275,14 @@ public final class OteAccountBuilder { /** Saves all the OT&E entities we created. */ private void saveAllEntities() { ImmutableList registries = ImmutableList.of(sunriseTld, gaTld, eapTld); - Map existingUsers = new HashMap<>(); - - users.forEach( - user -> - UserDao.loadUser(user.getEmailAddress()) - .ifPresent( - existingUser -> - existingUsers.put(existingUser.getEmailAddress(), existingUser))); - - if (!replaceExisting) { - checkState(existingUsers.isEmpty(), "Found existing users: %s", existingUsers); - } - tm().transact( () -> { if (!replaceExisting) { + ImmutableMap existingUsers = + tm().loadByEntitiesIfPresent(users).stream() + .collect(toImmutableMap(User::getEmailAddress, u -> u)); + checkState(existingUsers.isEmpty(), "Found existing users: %s", existingUsers); + ImmutableList> keys = Streams.concat( registries.stream().map(tld -> Tld.createVKey(tld.getTldStr())), @@ -317,16 +306,7 @@ public final class OteAccountBuilder { tm().putAll(registrars); }); - for (User user : users) { - String email = user.getEmailAddress(); - if (existingUsers.containsKey(email)) { - // Note that other roles for the existing user are reset. We do this instead of simply - // saving the new user is that UserDao does not allow us to save the new user with the same - // email as the existing user. - user = existingUsers.get(email).asBuilder().setUserRoles(user.getUserRoles()).build(); - } - UserDao.saveUser(user); - } + tm().transact(() -> tm().putAll(ImmutableList.copyOf(users))); } private Registrar addAllowedTld(Registrar registrar) { diff --git a/core/src/main/java/google/registry/model/console/User.java b/core/src/main/java/google/registry/model/console/User.java index f690025b1..c3d26cd78 100644 --- a/core/src/main/java/google/registry/model/console/User.java +++ b/core/src/main/java/google/registry/model/console/User.java @@ -15,11 +15,9 @@ package google.registry.model.console; import static com.google.common.base.Preconditions.checkArgument; -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.ImmutableMap; import com.google.common.collect.ImmutableMultimap; import com.google.common.flogger.FluentLogger; @@ -34,7 +32,6 @@ import google.registry.tools.server.UpdateUserGroupAction.Mode; import google.registry.util.RegistryEnvironment; import java.io.IOException; import java.util.Optional; -import java.util.concurrent.atomic.AtomicLong; import javax.annotation.Nullable; import javax.persistence.Access; import javax.persistence.AccessType; @@ -52,8 +49,6 @@ 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. * @@ -163,12 +158,6 @@ public class User extends UserBase { } } - @Override - @Access(AccessType.PROPERTY) - public Long getId() { - return super.getId(); - } - @Id @Override @Access(AccessType.PROPERTY) @@ -183,7 +172,7 @@ public class User extends UserBase { @Override public VKey createVKey() { - return VKey.create(User.class, getId()); + return VKey.create(User.class, getEmailAddress()); } /** Builder for constructing immutable {@link User} objects. */ @@ -194,20 +183,5 @@ 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(); - } } } diff --git a/core/src/main/java/google/registry/model/console/UserBase.java b/core/src/main/java/google/registry/model/console/UserBase.java index 1631e9da1..132b16ab1 100644 --- a/core/src/main/java/google/registry/model/console/UserBase.java +++ b/core/src/main/java/google/registry/model/console/UserBase.java @@ -49,9 +49,6 @@ public class UserBase extends UpdateAutoTimestampEntity implements Buildable { private static final long serialVersionUID = 6936728603828566721L; - /** Autogenerated unique ID of this user. */ - @Transient private Long id; - /** Email address of the user in question. */ @Transient String emailAddress; @@ -71,24 +68,15 @@ public class UserBase extends UpdateAutoTimestampEntity implements Buildable { /** Randomly generated hash salt. */ String registryLockPasswordSalt; - public Long getId() { - return id; - } - /** - * Sets the user ID. + * Sets the user email address. * - *

This should only be used for restoring the user id of an object being loaded in a PostLoad - * method (effectively, when it is still under construction by Hibernate). In all other cases, the - * object should be regarded as immutable and changes should go through a Builder. + *

This should only be used for restoring an object being loaded in a PostLoad method + * (effectively, when it is still under construction by Hibernate). In all other cases, the object + * should be regarded as immutable and changes should go through a Builder. * *

In addition to this special case use, this method must exist to satisfy Hibernate. */ - @SuppressWarnings("unused") - void setId(Long id) { - this.id = id; - } - void setEmailAddress(String emailAddress) { this.emailAddress = emailAddress; } diff --git a/core/src/main/java/google/registry/model/console/UserDao.java b/core/src/main/java/google/registry/model/console/UserDao.java deleted file mode 100644 index 2694f424e..000000000 --- a/core/src/main/java/google/registry/model/console/UserDao.java +++ /dev/null @@ -1,38 +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 google.registry.persistence.transaction.TransactionManagerFactory.tm; - -import java.util.Optional; - -/** Data access object for {@link User} objects to simplify saving and retrieval. */ -public class UserDao { - - /** Retrieves the one user with this email address if it exists. */ - public static Optional loadUser(String emailAddress) { - return tm().transact( - () -> - tm().query("FROM User WHERE emailAddress = :emailAddress", User.class) - .setParameter("emailAddress", emailAddress) - .getResultStream() - .findFirst()); - } - - /** Saves the given user, updating it if it already exists. */ - public static void saveUser(User user) { - tm().transact(() -> tm().put(user)); - } -} diff --git a/core/src/main/java/google/registry/model/console/UserUpdateHistory.java b/core/src/main/java/google/registry/model/console/UserUpdateHistory.java index 510f17ea3..65308dee4 100644 --- a/core/src/main/java/google/registry/model/console/UserUpdateHistory.java +++ b/core/src/main/java/google/registry/model/console/UserUpdateHistory.java @@ -38,10 +38,6 @@ public class UserUpdateHistory extends ConsoleUpdateHistory { UserBase user; - // This field exists so that it's populated in the SQL table - @Column(nullable = false, name = "userId") - Long id; - @Column(nullable = false, name = "emailAddress") String emailAddress; @@ -51,7 +47,6 @@ public class UserUpdateHistory extends ConsoleUpdateHistory { @PostLoad void postLoad() { - user.setId(id); user.setEmailAddress(emailAddress); } @@ -83,7 +78,6 @@ public class UserUpdateHistory extends ConsoleUpdateHistory { public Builder setUser(User user) { getInstance().user = user; - getInstance().id = user.getId(); getInstance().emailAddress = user.getEmailAddress(); return this; } diff --git a/core/src/main/java/google/registry/request/auth/OidcTokenAuthenticationMechanism.java b/core/src/main/java/google/registry/request/auth/OidcTokenAuthenticationMechanism.java index 5f04a5fba..8a1df2bfa 100644 --- a/core/src/main/java/google/registry/request/auth/OidcTokenAuthenticationMechanism.java +++ b/core/src/main/java/google/registry/request/auth/OidcTokenAuthenticationMechanism.java @@ -15,6 +15,7 @@ package google.registry.request.auth; import static com.google.common.base.Preconditions.checkState; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import com.google.api.client.json.webtoken.JsonWebSignature; import com.google.auth.oauth2.TokenVerifier; @@ -23,7 +24,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.flogger.FluentLogger; import google.registry.config.RegistryConfig.Config; import google.registry.model.console.User; -import google.registry.model.console.UserDao; +import google.registry.persistence.VKey; import google.registry.request.auth.AuthModule.IapOidc; import google.registry.request.auth.AuthModule.RegularOidc; import google.registry.request.auth.AuthModule.RegularOidcFallback; @@ -117,7 +118,8 @@ public abstract class OidcTokenAuthenticationMechanism implements Authentication logger.atWarning().log("No email address from the OIDC token:\n%s", token.getPayload()); return AuthResult.NOT_AUTHENTICATED; } - Optional maybeUser = UserDao.loadUser(email); + Optional maybeUser = + tm().transact(() -> tm().loadByKeyIfPresent(VKey.create(User.class, email))); if (maybeUser.isPresent()) { return AuthResult.createUser(maybeUser.get()); } diff --git a/core/src/main/java/google/registry/tools/CreateOrUpdateUserCommand.java b/core/src/main/java/google/registry/tools/CreateOrUpdateUserCommand.java index 02c0bf2d8..62b55e114 100644 --- a/core/src/main/java/google/registry/tools/CreateOrUpdateUserCommand.java +++ b/core/src/main/java/google/registry/tools/CreateOrUpdateUserCommand.java @@ -22,7 +22,6 @@ 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.tools.params.KeyValueMapParameter.StringToRegistrarRoleMap; import java.util.Optional; @@ -95,8 +94,6 @@ public abstract class CreateOrUpdateUserCommand extends ConfirmingCommand { builder.setRegistryLockEmailAddress(registryLockEmailAddress); } } - - User newUser = builder.build(); - UserDao.saveUser(newUser); + tm().put(builder.build()); } } diff --git a/core/src/main/java/google/registry/tools/CreateUserCommand.java b/core/src/main/java/google/registry/tools/CreateUserCommand.java index d3a413840..8a5aeb3d3 100644 --- a/core/src/main/java/google/registry/tools/CreateUserCommand.java +++ b/core/src/main/java/google/registry/tools/CreateUserCommand.java @@ -16,11 +16,12 @@ package google.registry.tools; import static com.google.common.base.Preconditions.checkArgument; import static google.registry.model.console.User.grantIapPermission; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import com.beust.jcommander.Parameters; import google.registry.config.RegistryConfig.Config; import google.registry.model.console.User; -import google.registry.model.console.UserDao; +import google.registry.persistence.VKey; import java.util.Optional; import javax.annotation.Nullable; import javax.inject.Inject; @@ -40,7 +41,8 @@ public class CreateUserCommand extends CreateOrUpdateUserCommand implements Comm @Nullable @Override User getExistingUser(String email) { - checkArgument(UserDao.loadUser(email).isEmpty(), "A user with email %s already exists", email); + checkArgument( + !tm().exists(VKey.create(User.class, email)), "A user with email %s already exists", email); return null; } diff --git a/core/src/main/java/google/registry/tools/DeleteUserCommand.java b/core/src/main/java/google/registry/tools/DeleteUserCommand.java index 8dba6e52f..f6b6aa2f1 100644 --- a/core/src/main/java/google/registry/tools/DeleteUserCommand.java +++ b/core/src/main/java/google/registry/tools/DeleteUserCommand.java @@ -14,15 +14,15 @@ package google.registry.tools; +import static com.google.api.client.util.Preconditions.checkArgument; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; -import static google.registry.util.PreconditionsUtils.checkArgumentPresent; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import google.registry.config.RegistryConfig.Config; import google.registry.model.console.User; -import google.registry.model.console.UserDao; +import google.registry.persistence.VKey; import java.util.Optional; import javax.annotation.Nullable; import javax.inject.Inject; @@ -46,7 +46,9 @@ public class DeleteUserCommand extends ConfirmingCommand implements CommandWithC @Override protected String prompt() { checkArgumentNotNull(email, "Email must be provided"); - checkArgumentPresent(UserDao.loadUser(email), "Email does not correspond to a valid user"); + checkArgument( + tm().transact(() -> tm().exists(VKey.create(User.class, email))), + "Email does not correspond to a valid user"); return String.format("Delete user with email %s?", email); } @@ -54,9 +56,9 @@ public class DeleteUserCommand extends ConfirmingCommand implements CommandWithC protected String execute() throws Exception { tm().transact( () -> { - Optional optionalUser = UserDao.loadUser(email); - checkArgumentPresent(optionalUser, "Email no longer corresponds to a valid user"); - tm().delete(optionalUser.get()); + VKey key = VKey.create(User.class, email); + checkArgument(tm().exists(key), "Email no longer corresponds to a valid user"); + tm().delete(key); }); User.revokeIapPermission(email, maybeGroupEmailAddress, null, connection, iamClient); return String.format("Deleted user with email %s", email); diff --git a/core/src/main/java/google/registry/tools/GetUserCommand.java b/core/src/main/java/google/registry/tools/GetUserCommand.java index 207c0f9b0..22ac81b5b 100644 --- a/core/src/main/java/google/registry/tools/GetUserCommand.java +++ b/core/src/main/java/google/registry/tools/GetUserCommand.java @@ -14,10 +14,12 @@ package google.registry.tools; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; + import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import google.registry.model.console.User; -import google.registry.model.console.UserDao; +import google.registry.persistence.VKey; import java.util.List; /** Command to display one or more users. */ @@ -29,11 +31,14 @@ public class GetUserCommand implements Command { @Override public void run() throws Exception { - for (String emailAddress : mainParameters) { - System.out.println( - UserDao.loadUser(emailAddress) - .map(User::toString) - .orElse(String.format("No user with email address %s", emailAddress))); - } + tm().transact( + () -> { + for (String emailAddress : mainParameters) { + System.out.println( + tm().loadByKeyIfPresent(VKey.create(User.class, emailAddress)) + .map(User::toString) + .orElse(String.format("No user with email address %s", emailAddress))); + } + }); } } diff --git a/core/src/main/java/google/registry/tools/UpdateUserCommand.java b/core/src/main/java/google/registry/tools/UpdateUserCommand.java index 6e836ac2f..802202a79 100644 --- a/core/src/main/java/google/registry/tools/UpdateUserCommand.java +++ b/core/src/main/java/google/registry/tools/UpdateUserCommand.java @@ -14,20 +14,20 @@ package google.registry.tools; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.PreconditionsUtils.checkArgumentPresent; import com.beust.jcommander.Parameters; import google.registry.model.console.User; -import google.registry.model.console.UserDao; -import javax.annotation.Nullable; +import google.registry.persistence.VKey; /** Updates a user, assuming that the user in question already exists. */ @Parameters(separators = " =", commandDescription = "Update a user account") public class UpdateUserCommand extends CreateOrUpdateUserCommand { - @Nullable @Override User getExistingUser(String email) { - return checkArgumentPresent(UserDao.loadUser(email), "User %s not found", email); + return checkArgumentPresent( + tm().loadByKeyIfPresent(VKey.create(User.class, email)), "User %s not found", email); } } diff --git a/core/src/main/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorAction.java b/core/src/main/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorAction.java index b24e11ced..ff04b995e 100644 --- a/core/src/main/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorAction.java +++ b/core/src/main/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorAction.java @@ -30,7 +30,6 @@ import google.registry.batch.CloudTasksUtils; import google.registry.config.RegistryConfig.Config; 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.model.registrar.Registrar; import google.registry.model.registrar.RegistrarAddress; @@ -260,8 +259,8 @@ public final class ConsoleRegistrarCreatorAction extends HtmlAction { "Registrar with client ID %s already exists", registrar.getRegistrarId()); tm().put(registrar); + tm().put(user); }); - UserDao.saveUser(user); User.grantIapPermission( user.getEmailAddress(), maybeGroupEmailAddress, cloudTasksUtils, null, iamClient); data.put("password", password); diff --git a/core/src/test/java/google/registry/model/OteAccountBuilderTest.java b/core/src/test/java/google/registry/model/OteAccountBuilderTest.java index 5747c139c..ce6744a35 100644 --- a/core/src/test/java/google/registry/model/OteAccountBuilderTest.java +++ b/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 maybeUser = UserDao.loadUser(email); + Optional maybeUser = loadByKeyIfPresent(VKey.create(User.class, email)); assertThat(maybeUser).isPresent(); assertThat(maybeUser.get().getUserRoles().getRegistrarRoles().get(registrarId)) .isEqualTo(RegistrarRole.ACCOUNT_MANAGER); diff --git a/core/src/test/java/google/registry/model/console/UserDaoTest.java b/core/src/test/java/google/registry/model/console/UserDaoTest.java deleted file mode 100644 index 4c48b8487..000000000 --- a/core/src/test/java/google/registry/model/console/UserDaoTest.java +++ /dev/null @@ -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"); - } -} diff --git a/core/src/test/java/google/registry/testing/DatabaseHelper.java b/core/src/test/java/google/registry/testing/DatabaseHelper.java index 3d11ed54c..8a33e22e1 100644 --- a/core/src/test/java/google/registry/testing/DatabaseHelper.java +++ b/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 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 key = VKey.create(User.class, emailAddress); + Optional 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. */ diff --git a/core/src/test/java/google/registry/tools/CreateUserCommandTest.java b/core/src/test/java/google/registry/tools/CreateUserCommandTest.java index 43e4588dc..e365067e6 100644 --- a/core/src/test/java/google/registry/tools/CreateUserCommandTest.java +++ b/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 { "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 { @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 { "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", diff --git a/core/src/test/java/google/registry/tools/DeleteUserCommandTest.java b/core/src/test/java/google/registry/tools/DeleteUserCommandTest.java index 02b62c8d8..41e7ec220 100644 --- a/core/src/test/java/google/registry/tools/DeleteUserCommandTest.java +++ b/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 { @Test void testSuccess_deletesUser() throws Exception { DatabaseHelper.createAdminUser("email@example.test"); - assertThat(UserDao.loadUser("email@example.test")).isPresent(); + VKey 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 { 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 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, diff --git a/core/src/test/java/google/registry/tools/GetUserCommandTest.java b/core/src/test/java/google/registry/tools/GetUserCommandTest.java index 3f8465ba3..04de5f4f1 100644 --- a/core/src/test/java/google/registry/tools/GetUserCommandTest.java +++ b/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 { @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 { .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 { """ User: { emailAddress=fte@google.com - id=1 registryLockEmailAddress=null registryLockPasswordHash=null registryLockPasswordSalt=null @@ -76,7 +74,6 @@ public class GetUserCommandTest extends CommandTestCase { """ User: { emailAddress=johndoe@theregistrar.com - id=0 registryLockEmailAddress=null registryLockPasswordHash=null registryLockPasswordSalt=null @@ -91,7 +88,6 @@ public class GetUserCommandTest extends CommandTestCase { } User: { emailAddress=fte@google.com - id=1 registryLockEmailAddress=null registryLockPasswordHash=null registryLockPasswordSalt=null @@ -114,7 +110,6 @@ public class GetUserCommandTest extends CommandTestCase { """ User: { emailAddress=johndoe@theregistrar.com - id=0 registryLockEmailAddress=null registryLockPasswordHash=null registryLockPasswordSalt=null diff --git a/core/src/test/java/google/registry/tools/SetupOteCommandTest.java b/core/src/test/java/google/registry/tools/SetupOteCommandTest.java index 294d299b9..77cb25e7b 100644 --- a/core/src/test/java/google/registry/tools/SetupOteCommandTest.java +++ b/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 { @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 { @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 { 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"); diff --git a/core/src/test/java/google/registry/tools/UpdateUserCommandTest.java b/core/src/test/java/google/registry/tools/UpdateUserCommandTest.java index 80145d59e..f3135eaed 100644 --- a/core/src/test/java/google/registry/tools/UpdateUserCommandTest.java +++ b/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 { @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 { "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 { "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); } diff --git a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html index 3fef6b111..825d607f9 100644 --- a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html @@ -261,7 +261,7 @@ td.section { generated on - 2024-07-31 14:38:11 + 2024-08-01 19:19:25 last flyway file @@ -280,7 +280,7 @@ td.section { generated by SchemaCrawler 16.21.4 generated on - 2024-07-31 14:38:11 + 2024-08-01 19:19:25 diff --git a/db/src/main/resources/sql/er_diagram/full_er_diagram.html b/db/src/main/resources/sql/er_diagram/full_er_diagram.html index 0c462d504..c20d62268 100644 --- a/db/src/main/resources/sql/er_diagram/full_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/full_er_diagram.html @@ -261,7 +261,7 @@ td.section { </tr> <tr> <td class="property_name">generated on</td> - <td class="property_value">2024-07-31 14:38:09</td> + <td class="property_value">2024-08-01 19:19:23</td> </tr> <tr> <td class="property_name">last flyway file</td> @@ -280,7 +280,7 @@ td.section { <text text-anchor="start" x="4494" y="-29.8" font-family="Helvetica,sans-Serif" font-size="14.00">generated by</text> <text text-anchor="start" x="4577" y="-29.8" font-family="Helvetica,sans-Serif" font-size="14.00">SchemaCrawler 16.21.4</text> <text text-anchor="start" x="4493" y="-10.8" font-family="Helvetica,sans-Serif" font-size="14.00">generated on</text> - <text text-anchor="start" x="4577" y="-10.8" font-family="Helvetica,sans-Serif" font-size="14.00">2024-07-31 14:38:09</text> + <text text-anchor="start" x="4577" y="-10.8" font-family="Helvetica,sans-Serif" font-size="14.00">2024-08-01 19:19:23</text> <polygon fill="none" stroke="#888888" points="4490,-4 4490,-44 4726,-44 4726,-4 4490,-4" /> <!-- allocationtoken_a08ccbef --> <g id="node1" class="node"> <title> diff --git a/db/src/main/resources/sql/schema/db-schema.sql.generated b/db/src/main/resources/sql/schema/db-schema.sql.generated index 544fb985a..e9014cfb6 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/db/src/main/resources/sql/schema/db-schema.sql.generated @@ -893,7 +893,6 @@ global_role text not null, is_admin boolean not null, registrar_roles hstore, - id int8, primary key (email_address) ); @@ -905,7 +904,6 @@ history_type text not null, history_url text not null, email_address text not null, - user_id int8 not null, registry_lock_email_address text, registry_lock_password_hash text, registry_lock_password_salt text,