opendev/gerrit
Code Issues Proposed changes

AccountsUpdate: Return AccountState instead of Account

Browse Source 
On insert/update of an account the updated Account instance was
returned. However on insert/update of an account not only account
properties can be set, but also external IDs, watched projectis and
general preferences. To be consistent we should rather return the
updated AccountState which includes external IDs, watched projects and
general preferences in addition to the account.

Change-Id: Iff30984fe39551f88480399b57ce9cb3a90a10e2
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-01-10 13:28:50 +01:00
parent 06e676fc95
commit d845be694f
8 changed files with 77 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -238,14 +238,14 @@ public class AccountManager {
}
if (!accountUpdates.isEmpty()) {
Account account =
AccountState accountState =
accountsUpdateFactory
.create()
.update(
"Update Account on Login",
user.getAccountId(),
AccountUpdater.joinConsumers(accountUpdates));
if (account == null) {
if (accountState == null) {
throw new OrmException("Account " + user.getAccountId() + " has been deleted");
}
}
@@ -266,9 +266,9 @@ public class AccountManager {
boolean isFirstAccount = awaitsFirstAccountCheck.getAndSet(false) && !accounts.hasAnyAccount();
Account account;
AccountState accountState;
try {
account =
accountState =
accountsUpdateFactory
.create()
.insert(
@@ -318,7 +318,7 @@ public class AccountManager {
addGroupMember(db, adminGroupUuid, user);
}
realm.onCreateAccount(who, account);
realm.onCreateAccount(who, accountState.getAccount());
return new AuthResult(newId, extId.key(), true);
}

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

@@ -360,7 +360,7 @@ public class AccountsUpdate {
* @throws OrmException if creating the user branch fails
* @throws ConfigInvalidException if any of the account fields has an invalid value
*/
public Account insert(
public AccountState insert(
String message, Account.Id accountId, Consumer<InternalAccountUpdate.Builder> init)
throws OrmException, IOException, ConfigInvalidException {
return insert(message, accountId, AccountUpdater.fromConsumer(init));
@@ -378,7 +378,7 @@ public class AccountsUpdate {
* @throws OrmException if creating the user branch fails
* @throws ConfigInvalidException if any of the account fields has an invalid value
*/
public Account insert(String message, Account.Id accountId, AccountUpdater updater)
public AccountState insert(String message, Account.Id accountId, AccountUpdater updater)
throws OrmException, IOException, ConfigInvalidException {
return updateAccount(
r -> {
@@ -399,7 +399,8 @@ public class AccountsUpdate {
accountConfig.setAccountUpdate(update);
ExternalIdNotes extIdNotes =
createExternalIdNotes(r, accountConfig.getExternalIdsRev(), accountId, update);
UpdatedAccount updatedAccounts = new UpdatedAccount(message, accountConfig, extIdNotes);
UpdatedAccount updatedAccounts =
new UpdatedAccount(allUsersName, externalIds, message, accountConfig, extIdNotes);
updatedAccounts.setCreated(true);
return updatedAccounts;
});
@@ -418,7 +419,7 @@ public class AccountsUpdate {
* @throws OrmException if updating the user branch fails
* @throws ConfigInvalidException if any of the account fields has an invalid value
*/
public Account update(
public AccountState update(
String message, Account.Id accountId, Consumer<InternalAccountUpdate.Builder> update)
throws OrmException, IOException, ConfigInvalidException {
return update(message, accountId, AccountUpdater.fromConsumer(update));
@@ -438,7 +439,7 @@ public class AccountsUpdate {
* @throws ConfigInvalidException if any of the account fields has an invalid value
*/
@Nullable
public Account update(String message, Account.Id accountId, AccountUpdater updater)
public AccountState update(String message, Account.Id accountId, AccountUpdater updater)
throws OrmException, IOException, ConfigInvalidException {
return updateAccount(
r -> {
@@ -456,7 +457,8 @@ public class AccountsUpdate {
accountConfig.setAccountUpdate(update);
ExternalIdNotes extIdNotes =
createExternalIdNotes(r, accountConfig.getExternalIdsRev(), accountId, update);
UpdatedAccount updatedAccounts = new UpdatedAccount(message, accountConfig, extIdNotes);
UpdatedAccount updatedAccounts =
new UpdatedAccount(allUsersName, externalIds, message, accountConfig, extIdNotes);
return updatedAccounts;
});
}
@@ -468,7 +470,7 @@ public class AccountsUpdate {
return accountConfig;
}
private Account updateAccount(AccountUpdate accountUpdate)
private AccountState updateAccount(AccountUpdate accountUpdate)
throws IOException, ConfigInvalidException, OrmException {
return retryHelper.execute(
ActionType.ACCOUNT_UPDATE,
@@ -508,6 +510,7 @@ public class AccountsUpdate {
beforeCommit.run();
BatchRefUpdate batchRefUpdate = allUsersRepo.getRefDatabase().newBatchUpdate();
if (updatedAccount.isCreated()) {
commitNewAccountConfig(
updatedAccount.getMessage(),
@@ -527,6 +530,7 @@ public class AccountsUpdate {
allUsersRepo,
batchRefUpdate,
updatedAccount.getExternalIdNotes());
RefUpdateUtil.executeChecked(batchRefUpdate, allUsersRepo);
updatedAccount.getExternalIdNotes().updateCaches();
gitRefUpdated.fire(
@@ -599,6 +603,8 @@ public class AccountsUpdate {
}
private static class UpdatedAccount {
private final AllUsersName allUsersName;
private final ExternalIds externalIds;
private final String message;
private final AccountConfig accountConfig;
private final ExternalIdNotes extIdNotes;
@@ -606,8 +612,14 @@ public class AccountsUpdate {
private boolean created;
private UpdatedAccount(
String message, AccountConfig accountConfig, ExternalIdNotes extIdNotes) {
AllUsersName allUsersName,
ExternalIds externalIds,
String message,
AccountConfig accountConfig,
ExternalIdNotes extIdNotes) {
checkState(!Strings.isNullOrEmpty(message), "message for account update must be set");
this.allUsersName = checkNotNull(allUsersName);
this.externalIds = checkNotNull(externalIds);
this.message = checkNotNull(message);
this.accountConfig = checkNotNull(accountConfig);
this.extIdNotes = checkNotNull(extIdNotes);
@@ -621,8 +633,16 @@ public class AccountsUpdate {
return accountConfig;
}
public Account getAccount() {
return accountConfig.getLoadedAccount().get();
public AccountState getAccount() throws IOException {
Account account = accountConfig.getLoadedAccount().get();
return new AccountState(
allUsersName,
account,
extIdNotes.getRevision() != null
? externalIds.byAccount(account.getId(), extIdNotes.getRevision())
: ImmutableSet.of(),
accountConfig.getProjectWatches(),
accountConfig.getGeneralPreferences());
}
public ExternalIdNotes getExternalIdNotes() {

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

@@ -39,7 +39,7 @@ public class SetInactiveFlag {
public Response<?> deactivate(Account.Id accountId)
throws RestApiException, IOException, ConfigInvalidException, OrmException {
AtomicBoolean alreadyInactive = new AtomicBoolean(false);
Account account =
AccountState accountState =
accountsUpdate
.create()
.update(
@@ -52,7 +52,7 @@ public class SetInactiveFlag {
u.setActive(false);
}
});
if (account == null) {
if (accountState == null) {
throw new ResourceNotFoundException("account not found");
}
if (alreadyInactive.get()) {
@@ -64,7 +64,7 @@ public class SetInactiveFlag {
public Response<String> activate(Account.Id accountId)
throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException {
AtomicBoolean alreadyActive = new AtomicBoolean(false);
Account account =
AccountState accountState =
accountsUpdate
.create()
.update(
@@ -77,7 +77,7 @@ public class SetInactiveFlag {
u.setActive(true);
}
});
if (account == null) {
if (accountState == null) {
throw new ResourceNotFoundException("account not found");
}
return alreadyActive.get() ? Response.ok("") : Response.created("");

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

@@ -92,6 +92,7 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.SetHashtagsOp;
@@ -2980,7 +2981,7 @@ class ReceiveCommits {
}
logDebug("Updating full name of caller");
try {
Account account =
AccountState accountState =
accountsUpdate
.create()
.update(
@@ -2991,8 +2992,8 @@ class ReceiveCommits {
u.setFullName(setFullNameTo);
}
});
if (account != null) {
user.getAccount().setFullName(account.getFullName());
if (accountState != null) {
user.getAccount().setFullName(accountState.getAccount().getFullName());
}
} catch (OrmException | IOException | ConfigInvalidException e) {
logWarn("Failed to update full name of caller", e);

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

@@ -22,10 +22,10 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
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.Realm;
import com.google.gerrit.server.permissions.GlobalPermission;
@@ -79,15 +79,15 @@ public class PutName implements RestModifyView<AccountResource, NameInput> {
}
String newName = input.name;
Account account =
AccountState accountState =
accountsUpdate
.create()
.update("Set Full Name via API", user.getAccountId(), u -> u.setFullName(newName));
if (account == null) {
if (accountState == null) {
throw new ResourceNotFoundException("account not found");
}
return Strings.isNullOrEmpty(account.getFullName())
return Strings.isNullOrEmpty(accountState.getAccount().getFullName())
? Response.none()
: Response.ok(account.getFullName());
: Response.ok(accountState.getAccount().getFullName());
}
}

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

@@ -19,10 +19,10 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
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.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -65,7 +65,7 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
public Response<String> apply(IdentifiedUser user, String email)
throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException {
AtomicBoolean alreadyPreferred = new AtomicBoolean(false);
Account account =
AccountState accountState =
accountsUpdate
.create()
.update(
@@ -78,7 +78,7 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
u.setPreferredEmail(email);
}
});
if (account == null) {
if (accountState == null) {
throw new ResourceNotFoundException("account not found");
}
return alreadyPreferred.get() ? Response.ok("") : Response.created("");

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

@@ -20,10 +20,10 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
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.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -68,15 +68,15 @@ public class PutStatus implements RestModifyView<AccountResource, StatusInput> {
}
String newStatus = input.status;
Account account =
AccountState accountState =
accountsUpdate
.create()
.update("Set Status via API", user.getAccountId(), u -> u.setStatus(newStatus));
if (account == null) {
if (accountState == null) {
throw new ResourceNotFoundException("account not found");
}
return Strings.isNullOrEmpty(account.getStatus())
return Strings.isNullOrEmpty(accountState.getAccount().getStatus())
? Response.none()
: Response.ok(account.getStatus());
: Response.ok(accountState.getAccount().getStatus());
}
}

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

@@ -373,14 +373,14 @@ public class AccountIT extends AbstractDaemonTest {
Account.Id accountId = new Account.Id(seq.nextAccountId());
String fullName = "Foo";
ExternalId extId = ExternalId.createEmail(accountId, "foo@example.com");
Account account =
AccountState accountState =
accountsUpdate
.create()
.insert(
"Create Account Atomically",
accountId,
u -> u.setFullName(fullName).addExternalId(extId));
assertThat(account.getFullName()).isEqualTo(fullName);
assertThat(accountState.getAccount().getFullName()).isEqualTo(fullName);
AccountInfo info = gApi.accounts().id(accountId.get()).get();
assertThat(info.name).isEqualTo(fullName);
@@ -401,12 +401,12 @@ public class AccountIT extends AbstractDaemonTest {
public void updateNonExistingAccount() throws Exception {
Account.Id nonExistingAccountId = new Account.Id(999999);
AtomicBoolean consumerCalled = new AtomicBoolean();
Account account =
AccountState accountState =
accountsUpdate
.create()
.update(
"Update Non-Existing Account", nonExistingAccountId, a -> consumerCalled.set(true));
assertThat(account).isNull();
assertThat(accountState).isNull();
assertThat(consumerCalled.get()).isFalse();
}
@@ -416,13 +416,13 @@ public class AccountIT extends AbstractDaemonTest {
assertUserBranchWithoutAccountConfig(anonymousCoward.getId());
String status = "OOO";
Account account =
AccountState accountState =
accountsUpdate
.create()
.update("Set status", anonymousCoward.getId(), u -> u.setStatus(status));
assertThat(account).isNotNull();
assertThat(account.getFullName()).isNull();
assertThat(account.getStatus()).isEqualTo(status);
assertThat(accountState).isNotNull();
assertThat(accountState.getAccount().getFullName()).isNull();
assertThat(accountState.getAccount().getStatus()).isEqualTo(status);
assertUserBranch(anonymousCoward.getId(), null, status);
}
@@ -1942,13 +1942,15 @@ public class AccountIT extends AbstractDaemonTest {
// metaId is set when account is created
AccountsUpdate au = accountsUpdate.create();
Account.Id accountId = new Account.Id(seq.nextAccountId());
Account account = au.insert("Create Test Account", accountId, u -> {});
assertThat(account.getMetaId()).isEqualTo(getMetaId(accountId));
AccountState accountState = au.insert("Create Test Account", accountId, u -> {});
assertThat(accountState.getAccount().getMetaId()).isEqualTo(getMetaId(accountId));
// metaId is set when account is updated
Account updatedAccount = au.update("Set Full Name", accountId, u -> u.setFullName("foo"));
assertThat(account.getMetaId()).isNotEqualTo(updatedAccount.getMetaId());
assertThat(updatedAccount.getMetaId()).isEqualTo(getMetaId(accountId));
AccountState updatedAccountState =
au.update("Set Full Name", accountId, u -> u.setFullName("foo"));
assertThat(accountState.getAccount().getMetaId())
.isNotEqualTo(updatedAccountState.getAccount().getMetaId());
assertThat(updatedAccountState.getAccount().getMetaId()).isEqualTo(getMetaId(accountId));
}
private EmailInput newEmailInput(String email, boolean noConfirmation) {
@@ -2049,11 +2051,12 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(accountInfo.status).isNull();
assertThat(accountInfo.name).isNotEqualTo(fullName);
Account updatedAccount = update.update("Set Full Name", admin.id, u -> u.setFullName(fullName));
AccountState updatedAccountState =
update.update("Set Full Name", admin.id, u -> u.setFullName(fullName));
assertThat(doneBgUpdate.get()).isTrue();
assertThat(updatedAccount.getStatus()).isEqualTo(status);
assertThat(updatedAccount.getFullName()).isEqualTo(fullName);
assertThat(updatedAccountState.getAccount().getStatus()).isEqualTo(status);
assertThat(updatedAccountState.getAccount().getFullName()).isEqualTo(fullName);
accountInfo = gApi.accounts().id(admin.id.get()).get();
assertThat(accountInfo.status).isEqualTo(status);
@@ -2158,7 +2161,7 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(bgCounterA2.get()).isEqualTo(0);
assertThat(gApi.accounts().id(admin.id.get()).get().status).isEqualTo("A-1");
Account updatedAccount =
AccountState updatedAccountState =
update.update(
"Set Status",
admin.id,
@@ -2177,7 +2180,7 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(bgCounterA1.get()).isEqualTo(1);
assertThat(bgCounterA2.get()).isEqualTo(1);
assertThat(updatedAccount.getStatus()).isEqualTo("B-2");
assertThat(updatedAccountState.getAccount().getStatus()).isEqualTo("B-2");
assertThat(accounts.get(admin.id).getAccount().getStatus()).isEqualTo("B-2");
assertThat(gApi.accounts().id(admin.id.get()).get().status).isEqualTo("B-2");
}