opendev/gerrit
Code Issues Proposed changes

AccountsUpdate#update: Return Optional<AccountState> instead of AccountState

Browse Source 
This makes it more explicit that callers must handle the case where the
returned AccountState is absent.

Change-Id: Id301678f97d2a8827c3a23d85cbc5f6da97abc7c
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-01-12 10:52:25 +01:00
parent 5c37073931
commit d40d81482d
8 changed files with 103 additions and 112 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
java/com/google/gerrit/server/account/AccountManager.java
View File

@@ -238,16 +238,14 @@ public class AccountManager {
} }
if (!accountUpdates.isEmpty()) { if (!accountUpdates.isEmpty()) {
AccountState accountState = accountsUpdateFactory
accountsUpdateFactory .create()
.create() .update(
.update( "Update Account on Login",
"Update Account on Login", user.getAccountId(),
user.getAccountId(), AccountUpdater.joinConsumers(accountUpdates))
AccountUpdater.joinConsumers(accountUpdates)); .orElseThrow(
if (accountState == null) { () -> new OrmException("Account " + user.getAccountId() + " has been deleted"));
throw new OrmException("Account " + user.getAccountId() + " has been deleted");
}
} }
} }

48
java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -378,23 +378,24 @@ public class AccountsUpdate {
public AccountState insert(String message, Account.Id accountId, AccountUpdater updater) public AccountState insert(String message, Account.Id accountId, AccountUpdater updater)
throws OrmException, IOException, ConfigInvalidException { throws OrmException, IOException, ConfigInvalidException {
return updateAccount( return updateAccount(
r -> { r -> {
AccountConfig accountConfig = read(r, accountId); AccountConfig accountConfig = read(r, accountId);
Account account = Account account =
accountConfig.getNewAccount(new Timestamp(committerIdent.getWhen().getTime())); accountConfig.getNewAccount(new Timestamp(committerIdent.getWhen().getTime()));
AccountState accountState = AccountState.forAccount(allUsersName, account); AccountState accountState = AccountState.forAccount(allUsersName, account);
InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder(); InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder();
updater.update(accountState, updateBuilder); updater.update(accountState, updateBuilder);
InternalAccountUpdate update = updateBuilder.build(); InternalAccountUpdate update = updateBuilder.build();
accountConfig.setAccountUpdate(update); accountConfig.setAccountUpdate(update);
ExternalIdNotes extIdNotes = ExternalIdNotes extIdNotes =
createExternalIdNotes(r, accountConfig.getExternalIdsRev(), accountId, update); createExternalIdNotes(r, accountConfig.getExternalIdsRev(), accountId, update);
UpdatedAccount updatedAccounts = UpdatedAccount updatedAccounts =
new UpdatedAccount(allUsersName, externalIds, message, accountConfig, extIdNotes); new UpdatedAccount(allUsersName, externalIds, message, accountConfig, extIdNotes);
updatedAccounts.setCreated(true); updatedAccounts.setCreated(true);
return updatedAccounts; return updatedAccounts;
}); })
.get();
} }
/** /**
@@ -405,12 +406,12 @@ public class AccountsUpdate {
* @param message commit message for the account update, must not be {@code null or empty} * @param message commit message for the account update, must not be {@code null or empty}
* @param accountId ID of the account * @param accountId ID of the account
* @param update consumer to update the account, only invoked if the account exists * @param update consumer to update the account, only invoked if the account exists
* @return the updated account, {@code null} if the account doesn't exist * @return the updated account, {@link Optional#empty()} if the account doesn't exist
* @throws IOException if updating the user branch fails due to an IO error * @throws IOException if updating the user branch fails due to an IO error
* @throws OrmException if updating the user branch fails * @throws OrmException if updating the user branch fails
* @throws ConfigInvalidException if any of the account fields has an invalid value * @throws ConfigInvalidException if any of the account fields has an invalid value
*/ */
public AccountState update( public Optional<AccountState> update(
String message, Account.Id accountId, Consumer<InternalAccountUpdate.Builder> update) String message, Account.Id accountId, Consumer<InternalAccountUpdate.Builder> update)
throws OrmException, IOException, ConfigInvalidException { throws OrmException, IOException, ConfigInvalidException {
return update(message, accountId, AccountUpdater.fromConsumer(update)); return update(message, accountId, AccountUpdater.fromConsumer(update));
@@ -424,13 +425,12 @@ public class AccountsUpdate {
* @param message commit message for the account update, must not be {@code null or empty} * @param message commit message for the account update, must not be {@code null or empty}
* @param accountId ID of the account * @param accountId ID of the account
* @param updater updater to update the account, only invoked if the account exists * @param updater updater to update the account, only invoked if the account exists
* @return the updated account, {@code null} if the account doesn't exist * @return the updated account, {@link Optional#empty} if the account doesn't exist
* @throws IOException if updating the user branch fails due to an IO error * @throws IOException if updating the user branch fails due to an IO error
* @throws OrmException if updating the user branch fails * @throws OrmException if updating the user branch fails
* @throws ConfigInvalidException if any of the account fields has an invalid value * @throws ConfigInvalidException if any of the account fields has an invalid value
*/ */
@Nullable public Optional<AccountState> update(String message, Account.Id accountId, AccountUpdater updater)
public AccountState update(String message, Account.Id accountId, AccountUpdater updater)
throws OrmException, IOException, ConfigInvalidException { throws OrmException, IOException, ConfigInvalidException {
return updateAccount( return updateAccount(
r -> { r -> {
@@ -461,7 +461,7 @@ public class AccountsUpdate {
return accountConfig; return accountConfig;
} }
private AccountState updateAccount(AccountUpdate accountUpdate) private Optional<AccountState> updateAccount(AccountUpdate accountUpdate)
throws IOException, ConfigInvalidException, OrmException { throws IOException, ConfigInvalidException, OrmException {
return retryHelper.execute( return retryHelper.execute(
ActionType.ACCOUNT_UPDATE, ActionType.ACCOUNT_UPDATE,
@@ -469,11 +469,11 @@ public class AccountsUpdate {
try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
UpdatedAccount updatedAccount = accountUpdate.update(allUsersRepo); UpdatedAccount updatedAccount = accountUpdate.update(allUsersRepo);
if (updatedAccount == null) { if (updatedAccount == null) {
return null; return Optional.empty();
} }
commit(allUsersRepo, updatedAccount); commit(allUsersRepo, updatedAccount);
return updatedAccount.getAccount(); return Optional.of(updatedAccount.getAccount());
} }
}); });
} }

58
java/com/google/gerrit/server/account/SetInactiveFlag.java
View File

@@ -39,22 +39,19 @@ public class SetInactiveFlag {
public Response<?> deactivate(Account.Id accountId) public Response<?> deactivate(Account.Id accountId)
throws RestApiException, IOException, ConfigInvalidException, OrmException { throws RestApiException, IOException, ConfigInvalidException, OrmException {
AtomicBoolean alreadyInactive = new AtomicBoolean(false); AtomicBoolean alreadyInactive = new AtomicBoolean(false);
AccountState accountState = accountsUpdate
accountsUpdate .create()
.create() .update(
.update( "Deactivate Account via API",
"Deactivate Account via API", accountId,
accountId, (a, u) -> {
(a, u) -> { if (!a.getAccount().isActive()) {
if (!a.getAccount().isActive()) { alreadyInactive.set(true);
alreadyInactive.set(true); } else {
} else { u.setActive(false);
u.setActive(false); }
} })
}); .orElseThrow(() -> new ResourceNotFoundException("account not found"));
if (accountState == null) {
throw new ResourceNotFoundException("account not found");
}
if (alreadyInactive.get()) { if (alreadyInactive.get()) {
throw new ResourceConflictException("account not active"); throw new ResourceConflictException("account not active");
} }
@@ -64,22 +61,19 @@ public class SetInactiveFlag {
public Response<String> activate(Account.Id accountId) public Response<String> activate(Account.Id accountId)
throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException { throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException {
AtomicBoolean alreadyActive = new AtomicBoolean(false); AtomicBoolean alreadyActive = new AtomicBoolean(false);
AccountState accountState = accountsUpdate
accountsUpdate .create()
.create() .update(
.update( "Activate Account via API",
"Activate Account via API", accountId,
accountId, (a, u) -> {
(a, u) -> { if (a.getAccount().isActive()) {
if (a.getAccount().isActive()) { alreadyActive.set(true);
alreadyActive.set(true); } else {
} else { u.setActive(true);
u.setActive(true); }
} })
}); .orElseThrow(() -> new ResourceNotFoundException("account not found"));
if (accountState == null) {
throw new ResourceNotFoundException("account not found");
}
return alreadyActive.get() ? Response.ok("") : Response.created(""); return alreadyActive.get() ? Response.ok("") : Response.created("");
} }
} }

8
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -2981,7 +2981,7 @@ class ReceiveCommits {
} }
logDebug("Updating full name of caller"); logDebug("Updating full name of caller");
try { try {
AccountState accountState = Optional<AccountState> accountState =
accountsUpdate accountsUpdate
.create() .create()
.update( .update(
@@ -2992,9 +2992,9 @@ class ReceiveCommits {
u.setFullName(setFullNameTo); u.setFullName(setFullNameTo);
} }
}); });
if (accountState != null) { accountState
user.getAccount().setFullName(accountState.getAccount().getFullName()); .map(AccountState::getAccount)
} .ifPresent(a -> user.getAccount().setFullName(a.getFullName()));
} catch (OrmException | IOException | ConfigInvalidException e) { } catch (OrmException | IOException | ConfigInvalidException e) {
logWarn("Failed to update full name of caller", e); logWarn("Failed to update full name of caller", e);
} }

6
java/com/google/gerrit/server/restapi/account/PutName.java
View File

@@ -82,10 +82,8 @@ public class PutName implements RestModifyView<AccountResource, NameInput> {
AccountState accountState = AccountState accountState =
accountsUpdate accountsUpdate
.create() .create()
.update("Set Full Name via API", user.getAccountId(), u -> u.setFullName(newName)); .update("Set Full Name via API", user.getAccountId(), u -> u.setFullName(newName))
if (accountState == null) { .orElseThrow(() -> new ResourceNotFoundException("account not found"));
throw new ResourceNotFoundException("account not found");
}
return Strings.isNullOrEmpty(accountState.getAccount().getFullName()) return Strings.isNullOrEmpty(accountState.getAccount().getFullName())
? Response.none() ? Response.none()
: Response.ok(accountState.getAccount().getFullName()); : Response.ok(accountState.getAccount().getFullName());

30
java/com/google/gerrit/server/restapi/account/PutPreferred.java
View File

@@ -22,7 +22,6 @@ import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountResource; import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend;
@@ -65,22 +64,19 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
public Response<String> apply(IdentifiedUser user, String email) public Response<String> apply(IdentifiedUser user, String email)
throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException { throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException {
AtomicBoolean alreadyPreferred = new AtomicBoolean(false); AtomicBoolean alreadyPreferred = new AtomicBoolean(false);
AccountState accountState = accountsUpdate
accountsUpdate .create()
.create() .update(
.update( "Set Preferred Email via API",
"Set Preferred Email via API", user.getAccountId(),
user.getAccountId(), (a, u) -> {
(a, u) -> { if (email.equals(a.getAccount().getPreferredEmail())) {
if (email.equals(a.getAccount().getPreferredEmail())) { alreadyPreferred.set(true);
alreadyPreferred.set(true); } else {
} else { u.setPreferredEmail(email);
u.setPreferredEmail(email); }
} })
}); .orElseThrow(() -> new ResourceNotFoundException("account not found"));
if (accountState == null) {
throw new ResourceNotFoundException("account not found");
}
return alreadyPreferred.get() ? Response.ok("") : Response.created(""); return alreadyPreferred.get() ? Response.ok("") : Response.created("");
} }
} }

6
java/com/google/gerrit/server/restapi/account/PutStatus.java
View File

@@ -71,10 +71,8 @@ public class PutStatus implements RestModifyView<AccountResource, StatusInput> {
AccountState accountState = AccountState accountState =
accountsUpdate accountsUpdate
.create() .create()
.update("Set Status via API", user.getAccountId(), u -> u.setStatus(newStatus)); .update("Set Status via API", user.getAccountId(), u -> u.setStatus(newStatus))
if (accountState == null) { .orElseThrow(() -> new ResourceNotFoundException("account not found"));
throw new ResourceNotFoundException("account not found");
}
return Strings.isNullOrEmpty(accountState.getAccount().getStatus()) return Strings.isNullOrEmpty(accountState.getAccount().getStatus())
? Response.none() ? Response.none()
: Response.ok(accountState.getAccount().getStatus()); : Response.ok(accountState.getAccount().getStatus());

41
javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.api.accounts;
import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage; import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.acceptance.GitUtil.deleteRef; import static com.google.gerrit.acceptance.GitUtil.deleteRef;
import static com.google.gerrit.acceptance.GitUtil.fetch; import static com.google.gerrit.acceptance.GitUtil.fetch;
import static com.google.gerrit.gpg.PublicKeyStore.REFS_GPG_KEYS; import static com.google.gerrit.gpg.PublicKeyStore.REFS_GPG_KEYS;
@@ -401,12 +402,12 @@ public class AccountIT extends AbstractDaemonTest {
public void updateNonExistingAccount() throws Exception { public void updateNonExistingAccount() throws Exception {
Account.Id nonExistingAccountId = new Account.Id(999999); Account.Id nonExistingAccountId = new Account.Id(999999);
AtomicBoolean consumerCalled = new AtomicBoolean(); AtomicBoolean consumerCalled = new AtomicBoolean();
AccountState accountState = Optional<AccountState> accountState =
accountsUpdate accountsUpdate
.create() .create()
.update( .update(
"Update Non-Existing Account", nonExistingAccountId, a -> consumerCalled.set(true)); "Update Non-Existing Account", nonExistingAccountId, a -> consumerCalled.set(true));
assertThat(accountState).isNull(); assertThat(accountState).isEmpty();
assertThat(consumerCalled.get()).isFalse(); assertThat(consumerCalled.get()).isFalse();
} }
@@ -416,13 +417,14 @@ public class AccountIT extends AbstractDaemonTest {
assertUserBranchWithoutAccountConfig(anonymousCoward.getId()); assertUserBranchWithoutAccountConfig(anonymousCoward.getId());
String status = "OOO"; String status = "OOO";
AccountState accountState = Optional<AccountState> accountState =
accountsUpdate accountsUpdate
.create() .create()
.update("Set status", anonymousCoward.getId(), u -> u.setStatus(status)); .update("Set status", anonymousCoward.getId(), u -> u.setStatus(status));
assertThat(accountState).isNotNull(); assertThat(accountState).isPresent();
assertThat(accountState.getAccount().getFullName()).isNull(); Account account = accountState.get().getAccount();
assertThat(accountState.getAccount().getStatus()).isEqualTo(status); assertThat(account.getFullName()).isNull();
assertThat(account.getStatus()).isEqualTo(status);
assertUserBranch(anonymousCoward.getId(), null, status); assertUserBranch(anonymousCoward.getId(), null, status);
} }
@@ -1946,11 +1948,12 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(accountState.getAccount().getMetaId()).isEqualTo(getMetaId(accountId)); assertThat(accountState.getAccount().getMetaId()).isEqualTo(getMetaId(accountId));
// metaId is set when account is updated // metaId is set when account is updated
AccountState updatedAccountState = Optional<AccountState> updatedAccountState =
au.update("Set Full Name", accountId, u -> u.setFullName("foo")); au.update("Set Full Name", accountId, u -> u.setFullName("foo"));
assertThat(accountState.getAccount().getMetaId()) assertThat(updatedAccountState.isPresent()).isTrue();
.isNotEqualTo(updatedAccountState.getAccount().getMetaId()); Account updatedAccount = updatedAccountState.get().getAccount();
assertThat(updatedAccountState.getAccount().getMetaId()).isEqualTo(getMetaId(accountId)); assertThat(accountState.getAccount().getMetaId()).isNotEqualTo(updatedAccount.getMetaId());
assertThat(updatedAccount.getMetaId()).isEqualTo(getMetaId(accountId));
} }
private EmailInput newEmailInput(String email, boolean noConfirmation) { private EmailInput newEmailInput(String email, boolean noConfirmation) {
@@ -2051,12 +2054,14 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(accountInfo.status).isNull(); assertThat(accountInfo.status).isNull();
assertThat(accountInfo.name).isNotEqualTo(fullName); assertThat(accountInfo.name).isNotEqualTo(fullName);
AccountState updatedAccountState = Optional<AccountState> updatedAccountState =
update.update("Set Full Name", admin.id, u -> u.setFullName(fullName)); update.update("Set Full Name", admin.id, u -> u.setFullName(fullName));
assertThat(doneBgUpdate.get()).isTrue(); assertThat(doneBgUpdate.get()).isTrue();
assertThat(updatedAccountState.getAccount().getStatus()).isEqualTo(status); assertThat(updatedAccountState.isPresent()).isTrue();
assertThat(updatedAccountState.getAccount().getFullName()).isEqualTo(fullName); Account updatedAccount = updatedAccountState.get().getAccount();
assertThat(updatedAccount.getStatus()).isEqualTo(status);
assertThat(updatedAccount.getFullName()).isEqualTo(fullName);
accountInfo = gApi.accounts().id(admin.id.get()).get(); accountInfo = gApi.accounts().id(admin.id.get()).get();
assertThat(accountInfo.status).isEqualTo(status); assertThat(accountInfo.status).isEqualTo(status);
@@ -2161,7 +2166,7 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(bgCounterA2.get()).isEqualTo(0); assertThat(bgCounterA2.get()).isEqualTo(0);
assertThat(gApi.accounts().id(admin.id.get()).get().status).isEqualTo("A-1"); assertThat(gApi.accounts().id(admin.id.get()).get().status).isEqualTo("A-1");
AccountState updatedAccountState = Optional<AccountState> updatedAccountState =
update.update( update.update(
"Set Status", "Set Status",
admin.id, admin.id,
@@ -2180,7 +2185,8 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(bgCounterA1.get()).isEqualTo(1); assertThat(bgCounterA1.get()).isEqualTo(1);
assertThat(bgCounterA2.get()).isEqualTo(1); assertThat(bgCounterA2.get()).isEqualTo(1);
assertThat(updatedAccountState.getAccount().getStatus()).isEqualTo("B-2"); assertThat(updatedAccountState.isPresent()).isTrue();
assertThat(updatedAccountState.get().getAccount().getStatus()).isEqualTo("B-2");
assertThat(accounts.get(admin.id).get().getAccount().getStatus()).isEqualTo("B-2"); assertThat(accounts.get(admin.id).get().getAccount().getStatus()).isEqualTo("B-2");
assertThat(gApi.accounts().id(admin.id.get()).get().status).isEqualTo("B-2"); assertThat(gApi.accounts().id(admin.id.get()).get().status).isEqualTo("B-2");
} }
@@ -2241,7 +2247,7 @@ public class AccountIT extends AbstractDaemonTest {
ExternalId extIdB1 = ExternalId.create("foo", "B-1", accountId); ExternalId extIdB1 = ExternalId.create("foo", "B-1", accountId);
ExternalId extIdB2 = ExternalId.create("foo", "B-2", accountId); ExternalId extIdB2 = ExternalId.create("foo", "B-2", accountId);
AccountState updatedAccount = Optional<AccountState> updatedAccount =
update.update( update.update(
"Update External ID", "Update External ID",
accountId, accountId,
@@ -2260,7 +2266,8 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(bgCounterA1.get()).isEqualTo(1); assertThat(bgCounterA1.get()).isEqualTo(1);
assertThat(bgCounterA2.get()).isEqualTo(1); assertThat(bgCounterA2.get()).isEqualTo(1);
assertThat(updatedAccount.getExternalIds()).containsExactly(extIdB2); assertThat(updatedAccount.isPresent()).isTrue();
assertThat(updatedAccount.get().getExternalIds()).containsExactly(extIdB2);
assertThat(accounts.get(accountId).get().getExternalIds()).containsExactly(extIdB2); assertThat(accounts.get(accountId).get().getExternalIds()).containsExactly(extIdB2);
assertThat( assertThat(
gApi.accounts() gApi.accounts()