opendev/gerrit
Code Issues Proposed changes

Make Git history of user branches more useful by specific commit messages

Browse Source 
At the moment the Git history of a user branch is not very useful since
it always reads as "Create account", "Update account", "Update account",
"Update account" etc. Make the commit messages more specific to the
actual update. For this callers of AccountsUpdate are now required to
provide a commit message for account creations and account updates.
Having specific commit message also makes debugging easier. E.g. if
there is bad account update it is easier to identify the code that was
doing it.

Change-Id: Ifca4479d1c61b4231c1d78d0eecb4a77a01a464a
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-12-13 14:53:06 +01:00
parent 84d9297298
commit d42596d8e9
16 changed files with 148 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/acceptance/AccountCreator.java
View File

@@ -117,7 +117,9 @@ public class AccountCreator {
}
externalIdsUpdate.create().insert(extIds);
accountsUpdate.create().insert(id, u -> u.setFullName(fullName).setPreferredEmail(email));
accountsUpdate
.create()
.insert("Create Test Account", id, u -> u.setFullName(fullName).setPreferredEmail(email));
if (groupNames != null) {
for (String n : groupNames) {

9
java/com/google/gerrit/server/account/AccountConfig.java
View File

@@ -199,9 +199,14 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
}
if (revision != null) {
commit.setMessage("Update account\n");
if (Strings.isNullOrEmpty(commit.getMessage())) {
commit.setMessage("Update account\n");
}
} else {
commit.setMessage("Create account\n");
if (Strings.isNullOrEmpty(commit.getMessage())) {
commit.setMessage("Create account\n");
}
commit.setAuthor(new PersonIdent(commit.getAuthor(), registeredOn));
commit.setCommitter(new PersonIdent(commit.getCommitter(), registeredOn));
}

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

@@ -240,7 +240,10 @@ public class AccountManager {
Account account =
accountsUpdateFactory
.create()
.update(user.getAccountId(), AccountUpdater.joinConsumers(accountUpdates));
.update(
"Update Account on Login",
user.getAccountId(),
AccountUpdater.joinConsumers(accountUpdates));
if (account == null) {
throw new OrmException("Account " + user.getAccountId() + " has been deleted");
}
@@ -265,7 +268,9 @@ public class AccountManager {
AccountsUpdate accountsUpdate = accountsUpdateFactory.create();
account =
accountsUpdate.insert(
newId, u -> u.setFullName(who.getDisplayName()).setPreferredEmail(extId.email()));
"Create Account on First Login",
newId,
u -> u.setFullName(who.getDisplayName()).setPreferredEmail(extId.email()));
ExternalId existingExtId = externalIds.get(extId.key());
if (existingExtId != null && !existingExtId.accountId().equals(extId.accountId())) {
@@ -415,6 +420,10 @@ public class AccountManager {
accountsUpdateFactory
.create()
.update(
"Set Preferred Email on Linking External ID\n"
+ "\n"
+ "An external ID with email was added and the account didn't have a\n"
+ "preferred email yet.",
to,
(a, u) -> {
if (a.getPreferredEmail() == null) {
@@ -423,7 +432,6 @@ public class AccountManager {
});
}
}
return new AuthResult(to, who.getExternalIdKey(), false);
}
@@ -503,6 +511,10 @@ public class AccountManager {
accountsUpdateFactory
.create()
.update(
"Clear Preferred Email on Unlinking External ID\n"
+ "\n"
+ "The preferred email is cleared because the corresponding external ID\n"
+ "was removed.",
from,
(a, u) -> {
if (a.getPreferredEmail() != null) {

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

@@ -15,8 +15,10 @@
package com.google.gerrit.server.account;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.Runnables;
import com.google.gerrit.common.Nullable;
@@ -270,6 +272,7 @@ public class AccountsUpdate {
/**
* Inserts a new account.
*
* @param message commit message for the account creation, must not be {@code null or empty}
* @param accountId ID of the new account
* @param init consumer to populate the new account
* @return the newly created account
@@ -278,14 +281,16 @@ 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(Account.Id accountId, Consumer<InternalAccountUpdate.Builder> init)
public Account insert(
String message, Account.Id accountId, Consumer<InternalAccountUpdate.Builder> init)
throws OrmException, IOException, ConfigInvalidException {
return insert(accountId, AccountUpdater.fromConsumer(init));
return insert(message, accountId, AccountUpdater.fromConsumer(init));
}
/**
* Inserts a new account.
*
* @param message commit message for the account creation, must not be {@code null or empty}
* @param accountId ID of the new account
* @param updater updater to populate the new account
* @return the newly created account
@@ -294,7 +299,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(Account.Id accountId, AccountUpdater updater)
public Account insert(String message, Account.Id accountId, AccountUpdater updater)
throws OrmException, IOException, ConfigInvalidException {
return updateAccount(
r -> {
@@ -304,7 +309,7 @@ public class AccountsUpdate {
updater.update(account, updateBuilder);
accountConfig.setAccountUpdate(updateBuilder.build());
UpdatedAccount updatedAccounts = new UpdatedAccount(accountConfig);
UpdatedAccount updatedAccounts = new UpdatedAccount(message, accountConfig);
updatedAccounts.setCreated(true);
return updatedAccounts;
});
@@ -315,6 +320,7 @@ public class AccountsUpdate {
*
* <p>Changing the registration date of an account is not supported.
*
* @param message commit message for the account update, must not be {@code null or empty}
* @param accountId ID of the account
* @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
@@ -322,9 +328,10 @@ 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(Account.Id accountId, Consumer<InternalAccountUpdate.Builder> update)
public Account update(
String message, Account.Id accountId, Consumer<InternalAccountUpdate.Builder> update)
throws OrmException, IOException, ConfigInvalidException {
return update(accountId, AccountUpdater.fromConsumer(update));
return update(message, accountId, AccountUpdater.fromConsumer(update));
}
/**
@@ -332,6 +339,7 @@ public class AccountsUpdate {
*
* <p>Changing the registration date of an account is not supported.
*
* @param message commit message for the account update, must not be {@code null or empty}
* @param accountId ID of the account
* @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
@@ -340,7 +348,7 @@ public class AccountsUpdate {
* @throws ConfigInvalidException if any of the account fields has an invalid value
*/
@Nullable
public Account update(Account.Id accountId, AccountUpdater updater)
public Account update(String message, Account.Id accountId, AccountUpdater updater)
throws OrmException, IOException, ConfigInvalidException {
return updateAccount(
r -> {
@@ -353,7 +361,7 @@ public class AccountsUpdate {
InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder();
updater.update(account.get(), updateBuilder);
accountConfig.setAccountUpdate(updateBuilder.build());
UpdatedAccount updatedAccounts = new UpdatedAccount(accountConfig);
UpdatedAccount updatedAccounts = new UpdatedAccount(message, accountConfig);
return updatedAccounts;
});
}
@@ -454,9 +462,17 @@ public class AccountsUpdate {
private void commit(Repository allUsersRepo, UpdatedAccount updatedAccount) throws IOException {
BatchRefUpdate batchRefUpdate = allUsersRepo.getRefDatabase().newBatchUpdate();
if (updatedAccount.isCreated()) {
commitNewAccountConfig(allUsersRepo, batchRefUpdate, updatedAccount.getAccountConfig());
commitNewAccountConfig(
updatedAccount.getMessage(),
allUsersRepo,
batchRefUpdate,
updatedAccount.getAccountConfig());
} else {
commitAccountConfig(allUsersRepo, batchRefUpdate, updatedAccount.getAccountConfig());
commitAccountConfig(
updatedAccount.getMessage(),
allUsersRepo,
batchRefUpdate,
updatedAccount.getAccountConfig());
}
RefUpdateUtil.executeChecked(batchRefUpdate, allUsersRepo);
gitRefUpdated.fire(
@@ -464,36 +480,48 @@ public class AccountsUpdate {
}
private void commitNewAccountConfig(
Repository allUsersRepo, BatchRefUpdate batchRefUpdate, AccountConfig accountConfig)
String message,
Repository allUsersRepo,
BatchRefUpdate batchRefUpdate,
AccountConfig accountConfig)
throws IOException {
// When creating a new account we must allow empty commits so that the user branch gets created
// with an empty commit when no account properties are set and hence no 'account.config' file
// will be created.
commitAccountConfig(allUsersRepo, batchRefUpdate, accountConfig, true);
commitAccountConfig(message, allUsersRepo, batchRefUpdate, accountConfig, true);
}
private void commitAccountConfig(
Repository allUsersRepo, BatchRefUpdate batchRefUpdate, AccountConfig accountConfig)
String message,
Repository allUsersRepo,
BatchRefUpdate batchRefUpdate,
AccountConfig accountConfig)
throws IOException {
commitAccountConfig(allUsersRepo, batchRefUpdate, accountConfig, false);
commitAccountConfig(message, allUsersRepo, batchRefUpdate, accountConfig, false);
}
private void commitAccountConfig(
String message,
Repository allUsersRepo,
BatchRefUpdate batchRefUpdate,
AccountConfig accountConfig,
boolean allowEmptyCommit)
throws IOException {
try (MetaDataUpdate md = createMetaDataUpdate(allUsersRepo, batchRefUpdate)) {
try (MetaDataUpdate md = createMetaDataUpdate(message, allUsersRepo, batchRefUpdate)) {
md.setAllowEmpty(allowEmptyCommit);
accountConfig.commit(md);
}
}
private MetaDataUpdate createMetaDataUpdate(
Repository allUsersRepo, BatchRefUpdate batchRefUpdate) {
String message, Repository allUsersRepo, BatchRefUpdate batchRefUpdate) {
MetaDataUpdate metaDataUpdate =
metaDataUpdateInternalFactory.get().create(allUsersName, allUsersRepo, batchRefUpdate);
if (!message.endsWith("\n")) {
message = message + "\n";
}
metaDataUpdate.getCommitBuilder().setMessage(message);
metaDataUpdate.getCommitBuilder().setCommitter(committerIdent);
metaDataUpdate.getCommitBuilder().setAuthor(authorIdent);
return metaDataUpdate;
@@ -506,19 +534,18 @@ public class AccountsUpdate {
}
private static class UpdatedAccount {
private final String message;
private final AccountConfig accountConfig;
private boolean created;
private UpdatedAccount(AccountConfig accountConfig) {
private UpdatedAccount(String message, AccountConfig accountConfig) {
checkState(!Strings.isNullOrEmpty(message), "message for account update must be set");
this.message = checkNotNull(message);
this.accountConfig = checkNotNull(accountConfig);
}
public void setCreated(boolean created) {
this.created = created;
}
public boolean isCreated() {
return created;
public String getMessage() {
return message;
}
public AccountConfig getAccountConfig() {
@@ -528,5 +555,13 @@ public class AccountsUpdate {
public Account getAccount() {
return accountConfig.getLoadedAccount().get();
}
public void setCreated(boolean created) {
this.created = created;
}
public boolean isCreated() {
return created;
}
}
}

5
java/com/google/gerrit/server/account/CreateAccount.java
View File

@@ -172,7 +172,10 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
accountsUpdate
.create()
.insert(id, u -> u.setFullName(input.name).setPreferredEmail(input.email));
.insert(
"Create Account via API",
id,
u -> u.setFullName(input.name).setPreferredEmail(input.email));
for (AccountGroup.UUID groupUuid : groups) {
try {

4
java/com/google/gerrit/server/account/PutName.java
View File

@@ -77,7 +77,9 @@ public class PutName implements RestModifyView<AccountResource, NameInput> {
String newName = input.name;
Account account =
accountsUpdate.create().update(user.getAccountId(), u -> u.setFullName(newName));
accountsUpdate
.create()
.update("Set Full Name via API", user.getAccountId(), u -> u.setFullName(newName));
if (account == null) {
throw new ResourceNotFoundException("account not found");
}

1
java/com/google/gerrit/server/account/PutPreferred.java
View File

@@ -67,6 +67,7 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
accountsUpdate
.create()
.update(
"Set Preferred Email via API",
user.getAccountId(),
(a, u) -> {
if (email.equals(a.getPreferredEmail())) {

4
java/com/google/gerrit/server/account/PutStatus.java
View File

@@ -67,7 +67,9 @@ public class PutStatus implements RestModifyView<AccountResource, StatusInput> {
String newStatus = input.status;
Account account =
accountsUpdate.create().update(user.getAccountId(), u -> u.setStatus(newStatus));
accountsUpdate
.create()
.update("Set Status via API", user.getAccountId(), u -> u.setStatus(newStatus));
if (account == null) {
throw new ResourceNotFoundException("account not found");
}

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

@@ -43,6 +43,7 @@ public class SetInactiveFlag {
accountsUpdate
.create()
.update(
"Deactivate Account via API",
accountId,
(a, u) -> {
if (!a.isActive()) {
@@ -67,6 +68,7 @@ public class SetInactiveFlag {
accountsUpdate
.create()
.update(
"Activate Account via API",
accountId,
(a, u) -> {
if (a.isActive()) {

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

@@ -2932,6 +2932,7 @@ class ReceiveCommits {
accountsUpdate
.create()
.update(
"Set Full Name on Receive Commits",
user.getAccountId(),
(a, u) -> {
if (Strings.isNullOrEmpty(a.getFullName())) {

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

@@ -324,12 +324,36 @@ public class AccountIT extends AbstractDaemonTest {
assertUserBranchWithoutAccountConfig(anonymousCoward.getId());
}
@Test
public void commitMessageOnAccountUpdates() throws Exception {
AccountsUpdate au = accountsUpdate.create();
Account.Id accountId = new Account.Id(seq.nextAccountId());
au.insert("Create Test Account", accountId, u -> {});
assertLastCommitMessageOfUserBranch(accountId, "Create Test Account");
au.update("Set Status", accountId, u -> u.setStatus("Foo"));
assertLastCommitMessageOfUserBranch(accountId, "Set Status");
}
private void assertLastCommitMessageOfUserBranch(Account.Id accountId, String expectedMessage)
throws Exception {
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {
Ref exactRef = repo.exactRef(RefNames.refsUsers(accountId));
assertThat(rw.parseCommit(exactRef.getObjectId()).getShortMessage())
.isEqualTo(expectedMessage);
}
}
@Test
public void updateNonExistingAccount() throws Exception {
Account.Id nonExistingAccountId = new Account.Id(999999);
AtomicBoolean consumerCalled = new AtomicBoolean();
Account account =
accountsUpdate.create().update(nonExistingAccountId, a -> consumerCalled.set(true));
accountsUpdate
.create()
.update(
"Update Non-Existing Account", nonExistingAccountId, a -> consumerCalled.set(true));
assertThat(account).isNull();
assertThat(consumerCalled.get()).isFalse();
}
@@ -341,7 +365,9 @@ public class AccountIT extends AbstractDaemonTest {
String status = "OOO";
Account account =
accountsUpdate.create().update(anonymousCoward.getId(), u -> u.setStatus(status));
accountsUpdate
.create()
.update("Set status", anonymousCoward.getId(), u -> u.setStatus(status));
assertThat(account).isNotNull();
assertThat(account.getFullName()).isNull();
assertThat(account.getStatus()).isEqualTo(status);
@@ -871,7 +897,9 @@ public class AccountIT extends AbstractDaemonTest {
String prefix = "foo.preferred";
String prefEmail = prefix + "@example.com";
TestAccount foo = accountCreator.create(name("foo"));
accountsUpdate.create().update(foo.id, u -> u.setPreferredEmail(prefEmail));
accountsUpdate
.create()
.update("Set Preferred Email", foo.id, u -> u.setPreferredEmail(prefEmail));
// verify that the account is still found when using the preferred email to lookup the account
ImmutableSet<Account.Id> accountsByPrefEmail = emails.getAccountFor(prefEmail);
@@ -1347,7 +1375,9 @@ public class AccountIT extends AbstractDaemonTest {
String userRef = RefNames.refsUsers(foo.id);
String noEmail = "no.email";
accountsUpdate.create().update(foo.id, u -> u.setPreferredEmail(noEmail));
accountsUpdate
.create()
.update("Set Preferred Email", foo.id, u -> u.setPreferredEmail(noEmail));
accountIndexedCounter.clear();
grant(allUsers, userRef, Permission.PUSH, false, REGISTERED_USERS);
@@ -1829,11 +1859,11 @@ 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(accountId, u -> {});
Account account = au.insert("Create Test Account", accountId, u -> {});
assertThat(account.getMetaId()).isEqualTo(getMetaId(accountId));
// metaId is set when account is updated
Account updatedAccount = au.update(accountId, u -> u.setFullName("foo"));
Account updatedAccount = au.update("Set Full Name", accountId, u -> u.setFullName("foo"));
assertThat(account.getMetaId()).isNotEqualTo(updatedAccount.getMetaId());
assertThat(updatedAccount.getMetaId()).isEqualTo(getMetaId(accountId));
}
@@ -1923,7 +1953,7 @@ public class AccountIT extends AbstractDaemonTest {
() -> {
if (!doneBgUpdate.getAndSet(true)) {
try {
accountsUpdate.create().update(admin.id, u -> u.setStatus(status));
accountsUpdate.create().update("Set Status", admin.id, u -> u.setStatus(status));
} catch (IOException | ConfigInvalidException | OrmException e) {
// Ignore, the successful update of the account is asserted later
}
@@ -1934,7 +1964,7 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(accountInfo.status).isNull();
assertThat(accountInfo.name).isNotEqualTo(fullName);
Account updatedAccount = update.update(admin.id, u -> u.setFullName(fullName));
Account updatedAccount = update.update("Set Full Name", admin.id, u -> u.setFullName(fullName));
assertThat(doneBgUpdate.get()).isTrue();
assertThat(updatedAccount.getStatus()).isEqualTo(status);
@@ -1974,7 +2004,10 @@ public class AccountIT extends AbstractDaemonTest {
try {
accountsUpdate
.create()
.update(admin.id, u -> u.setStatus(status.get(bgCounter.getAndAdd(1))));
.update(
"Set Status",
admin.id,
u -> u.setStatus(status.get(bgCounter.getAndAdd(1))));
} catch (IOException | ConfigInvalidException | OrmException e) {
// Ignore, the expected exception is asserted later
}
@@ -1985,7 +2018,7 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(accountInfo.name).isNotEqualTo(fullName);
try {
update.update(admin.id, u -> u.setFullName(fullName));
update.update("Set Full Name", admin.id, u -> u.setFullName(fullName));
fail("expected LockFailureException");
} catch (LockFailureException e) {
// Ignore, expected

4
javatests/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
View File

@@ -116,7 +116,9 @@ public class GerritPublicKeyCheckerTest {
schemaCreator.create(db);
userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId();
// Note: does not match any key in TestKeys.
accountsUpdate.create().update(userId, u -> u.setPreferredEmail("user@example.com"));
accountsUpdate
.create()
.update("Set Preferred Email", userId, u -> u.setPreferredEmail("user@example.com"));
user = reloadUser();
requestContext.setContext(

1
javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
View File

@@ -542,6 +542,7 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
accountsUpdate
.create()
.update(
"Update Test Account",
id,
u -> {
u.setFullName(fullName).setPreferredEmail(email).setActive(active);

3
javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -224,7 +224,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId();
String email = "user@example.com";
externalIdsUpdate.create().insert(ExternalId.createEmail(userId, email));
accountsUpdate.create().update(userId, u -> u.setPreferredEmail(email));
accountsUpdate.create().update("Set Preferred Email", userId, u -> u.setPreferredEmail(email));
user = userFactory.create(userId);
requestContext.setContext(newRequestContext(userId));
}
@@ -2729,6 +2729,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
accountsUpdate
.create()
.update(
"Update Test Account",
id,
u -> {
u.setFullName(fullName).setPreferredEmail(email).setActive(active);

1
javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
View File

@@ -409,6 +409,7 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
accountsUpdate
.create()
.update(
"Update Test Account",
id,
u -> {
u.setFullName(fullName).setPreferredEmail(email).setActive(active);

1
javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java
View File

@@ -264,6 +264,7 @@ public abstract class AbstractQueryProjectsTest extends GerritServerTests {
accountsUpdate
.create()
.update(
"Update Test Account",
id,
u -> {
u.setFullName(fullName).setPreferredEmail(email).setActive(active);