Merge "Remove project state check from ChangeControl#addPatchSet"
diff --git a/java/com/google/gerrit/common/data/testing/BUILD b/java/com/google/gerrit/common/data/testing/BUILD
new file mode 100644
index 0000000..83f1c06
--- /dev/null
+++ b/java/com/google/gerrit/common/data/testing/BUILD
@@ -0,0 +1,11 @@
+java_library(
+ name = "common-data-test-util",
+ testonly = 1,
+ srcs = glob(["**/*.java"]),
+ visibility = ["//visibility:public"],
+ deps = [
+ "//java/com/google/gerrit/common:server",
+ "//java/com/google/gerrit/reviewdb:server",
+ "//lib:truth",
+ ],
+)
diff --git a/java/com/google/gerrit/common/data/testing/GroupReferenceSubject.java b/java/com/google/gerrit/common/data/testing/GroupReferenceSubject.java
new file mode 100644
index 0000000..1988d66
--- /dev/null
+++ b/java/com/google/gerrit/common/data/testing/GroupReferenceSubject.java
@@ -0,0 +1,48 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// 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 com.google.gerrit.common.data.testing;
+
+import static com.google.common.truth.Truth.assertAbout;
+
+import com.google.common.truth.ComparableSubject;
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.StringSubject;
+import com.google.common.truth.Subject;
+import com.google.common.truth.Truth;
+import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+
+public class GroupReferenceSubject extends Subject<GroupReferenceSubject, GroupReference> {
+
+ public static GroupReferenceSubject assertThat(GroupReference group) {
+ return assertAbout(GroupReferenceSubject::new).that(group);
+ }
+
+ private GroupReferenceSubject(FailureMetadata metadata, GroupReference group) {
+ super(metadata, group);
+ }
+
+ public ComparableSubject<?, AccountGroup.UUID> groupUuid() {
+ isNotNull();
+ GroupReference group = actual();
+ return Truth.assertThat(group.getUUID()).named("groupUuid");
+ }
+
+ public StringSubject name() {
+ isNotNull();
+ GroupReference group = actual();
+ return Truth.assertThat(group.getName()).named("name");
+ }
+}
diff --git a/java/com/google/gerrit/pgm/init/GroupsOnInit.java b/java/com/google/gerrit/pgm/init/GroupsOnInit.java
index 3385244..7fe227d 100644
--- a/java/com/google/gerrit/pgm/init/GroupsOnInit.java
+++ b/java/com/google/gerrit/pgm/init/GroupsOnInit.java
@@ -150,7 +150,7 @@
File allUsersRepoPath = getPathToAllUsersRepository();
if (allUsersRepoPath != null) {
try (Repository allUsersRepo = new FileRepository(allUsersRepoPath)) {
- return GroupNameNotes.loadAllGroupReferences(allUsersRepo).stream();
+ return GroupNameNotes.loadAllGroups(allUsersRepo).stream();
}
}
return Stream.empty();
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index 8ed1ef0..5e60906 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -569,7 +569,8 @@
new Branch.NameKey(ctx.getProject(), refName),
ctx.getIdentifiedUser(),
new NoSshInfo(),
- ctx.getRevWalk())
+ ctx.getRevWalk(),
+ change)
.validate(event);
}
} catch (CommitValidationException e) {
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index 3cf0dc5..3a32f8f 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -345,7 +345,8 @@
origNotes.getChange().getDest(),
ctx.getIdentifiedUser(),
new NoSshInfo(),
- ctx.getRevWalk())
+ ctx.getRevWalk(),
+ origNotes.getChange())
.validate(event);
} catch (CommitValidationException e) {
throw new ResourceConflictException(e.getFullMessage());
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 10b761f..292fa7a 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -1856,7 +1856,8 @@
logDebug("Creating new change for {} even though it is already tracked", name);
}
- if (!validCommit(rp.getRevWalk(), magicBranch.perm, magicBranch.dest, magicBranch.cmd, c)) {
+ if (!validCommit(
+ rp.getRevWalk(), magicBranch.perm, magicBranch.dest, magicBranch.cmd, c, null)) {
// Not a change the user can propose? Abort as early as possible.
newChanges = Collections.emptyList();
logDebug("Aborting early due to invalid commit");
@@ -2434,7 +2435,7 @@
}
PermissionBackend.ForRef perm = permissions.ref(change.getDest().get());
- if (!validCommit(rp.getRevWalk(), perm, change.getDest(), inputCommand, newCommit)) {
+ if (!validCommit(rp.getRevWalk(), perm, change.getDest(), inputCommand, newCommit, change)) {
return false;
}
rp.getRevWalk().parseBody(priorCommit);
@@ -2798,7 +2799,7 @@
}
if (existing.keySet().contains(c)) {
continue;
- } else if (!validCommit(walk, perm, branch, cmd, c)) {
+ } else if (!validCommit(walk, perm, branch, cmd, c, null)) {
break;
}
@@ -2820,7 +2821,8 @@
PermissionBackend.ForRef perm,
Branch.NameKey branch,
ReceiveCommand cmd,
- ObjectId id)
+ ObjectId id,
+ @Nullable Change change)
throws IOException {
if (validCommits.contains(id)) {
@@ -2841,7 +2843,7 @@
? commitValidatorsFactory.forMergedCommits(
project.getNameKey(), perm, user.asIdentifiedUser())
: commitValidatorsFactory.forReceiveCommits(
- perm, branch, user.asIdentifiedUser(), sshInfo, repo, rw);
+ perm, branch, user.asIdentifiedUser(), sshInfo, repo, rw, change);
messages.addAll(validators.validate(receiveEvent));
} catch (CommitValidationException e) {
logDebug("Commit validation failed on {}", c.name());
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java
index a80ff73..05ca98b 100644
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -19,6 +19,7 @@
import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
import static java.util.stream.Collectors.toList;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.FooterConstants;
@@ -30,6 +31,8 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Branch.NameKey;
+import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
@@ -46,6 +49,7 @@
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.git.ValidationError;
import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ProjectCache;
@@ -125,7 +129,8 @@
IdentifiedUser user,
SshInfo sshInfo,
Repository repo,
- RevWalk rw)
+ RevWalk rw,
+ @Nullable Change change)
throws IOException {
NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw);
ProjectState projectState = projectCache.checkedGet(branch.getParentKey());
@@ -138,7 +143,12 @@
new CommitterUploaderValidator(user, perm, canonicalWebUrl),
new SignedOffByValidator(user, perm, projectState),
new ChangeIdValidator(
- projectState, user, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
+ projectState,
+ user,
+ canonicalWebUrl,
+ installCommitMsgHookCommand,
+ sshInfo,
+ change),
new ConfigValidator(branch, user, rw, allUsers, allProjects),
new BannedCommitsValidator(rejectCommits),
new PluginCommitValidationListener(pluginValidators),
@@ -148,11 +158,12 @@
}
public CommitValidators forGerritCommits(
- PermissionBackend.ForRef perm,
- Branch.NameKey branch,
+ ForRef perm,
+ NameKey branch,
IdentifiedUser user,
SshInfo sshInfo,
- RevWalk rw)
+ RevWalk rw,
+ @Nullable Change change)
throws IOException {
ProjectState projectState = projectCache.checkedGet(branch.getParentKey());
return new CommitValidators(
@@ -163,7 +174,12 @@
new AuthorUploaderValidator(user, perm, canonicalWebUrl),
new SignedOffByValidator(user, perm, projectCache.checkedGet(branch.getParentKey())),
new ChangeIdValidator(
- projectState, user, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
+ projectState,
+ user,
+ canonicalWebUrl,
+ installCommitMsgHookCommand,
+ sshInfo,
+ change),
new ConfigValidator(branch, user, rw, allUsers, allProjects),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
@@ -231,6 +247,15 @@
"[%s] invalid "
+ FooterConstants.CHANGE_ID.getName()
+ " line format in commit message footer";
+
+ @VisibleForTesting
+ public static final String CHANGE_ID_MISMATCH_MSG =
+ "[%s] "
+ + FooterConstants.CHANGE_ID.getName()
+ + " in commit message footer does not match"
+ + FooterConstants.CHANGE_ID.getName()
+ + " of target change";
+
private static final Pattern CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN);
private final ProjectState projectState;
@@ -238,18 +263,21 @@
private final String installCommitMsgHookCommand;
private final SshInfo sshInfo;
private final IdentifiedUser user;
+ private final Change change;
public ChangeIdValidator(
ProjectState projectState,
IdentifiedUser user,
String canonicalWebUrl,
String installCommitMsgHookCommand,
- SshInfo sshInfo) {
+ SshInfo sshInfo,
+ Change change) {
this.projectState = projectState;
this.canonicalWebUrl = canonicalWebUrl;
this.installCommitMsgHookCommand = installCommitMsgHookCommand;
this.sshInfo = sshInfo;
this.user = user;
+ this.change = change;
}
@Override
@@ -289,7 +317,12 @@
messages.add(getMissingChangeIdErrorMsg(errMsg, receiveEvent.commit));
throw new CommitValidationException(errMsg, messages);
}
+ if (change != null && !v.equals(change.getKey().get())) {
+ String errMsg = String.format(CHANGE_ID_MISMATCH_MSG, sha1);
+ throw new CommitValidationException(errMsg);
+ }
}
+
return Collections.emptyList();
}
diff --git a/java/com/google/gerrit/server/group/db/GroupNameNotes.java b/java/com/google/gerrit/server/group/db/GroupNameNotes.java
index 775ebd6..87aee8e 100644
--- a/java/com/google/gerrit/server/group/db/GroupNameNotes.java
+++ b/java/com/google/gerrit/server/group/db/GroupNameNotes.java
@@ -21,8 +21,10 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
+import com.google.common.collect.HashMultiset;
import com.google.common.collect.ImmutableBiMap;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Multiset;
import com.google.common.hash.Hashing;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GroupReference;
@@ -32,11 +34,9 @@
import com.google.gwtorm.server.OrmDuplicateKeyException;
import java.io.IOException;
import java.util.Collection;
-import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
-import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.CommitBuilder;
@@ -68,7 +68,7 @@
@VisibleForTesting
static final String UNIQUE_REF_ERROR = "GroupReference collection must contain unique references";
- public static GroupNameNotes loadForRename(
+ public static GroupNameNotes forRename(
Repository repository,
AccountGroup.UUID groupUuid,
AccountGroup.NameKey oldName,
@@ -83,7 +83,7 @@
return groupNameNotes;
}
- public static GroupNameNotes loadForNewGroup(
+ public static GroupNameNotes forNewGroup(
Repository repository, AccountGroup.UUID groupUuid, AccountGroup.NameKey groupName)
throws IOException, ConfigInvalidException, OrmDuplicateKeyException {
checkNotNull(groupName);
@@ -94,18 +94,19 @@
return groupNameNotes;
}
- public static Optional<GroupReference> loadOneGroupReference(
- Repository allUsersRepo, String groupName) throws IOException, ConfigInvalidException {
- Ref ref = allUsersRepo.exactRef(RefNames.REFS_GROUPNAMES);
+ public static Optional<GroupReference> loadGroup(
+ Repository repository, AccountGroup.NameKey groupName)
+ throws IOException, ConfigInvalidException {
+ Ref ref = repository.exactRef(RefNames.REFS_GROUPNAMES);
if (ref == null) {
return Optional.empty();
}
- try (RevWalk revWalk = new RevWalk(allUsersRepo);
+ try (RevWalk revWalk = new RevWalk(repository);
ObjectReader reader = revWalk.getObjectReader()) {
RevCommit notesCommit = revWalk.parseCommit(ref.getObjectId());
NoteMap noteMap = NoteMap.read(reader, notesCommit);
- ObjectId noteDataBlobId = noteMap.get(getNoteKey(new AccountGroup.NameKey(groupName)));
+ ObjectId noteDataBlobId = noteMap.get(getNoteKey(groupName));
if (noteDataBlobId == null) {
return Optional.empty();
}
@@ -113,34 +114,34 @@
}
}
- public static ImmutableSet<GroupReference> loadAllGroupReferences(Repository repository)
+ public static ImmutableList<GroupReference> loadAllGroups(Repository repository)
throws IOException, ConfigInvalidException {
Ref ref = repository.exactRef(RefNames.REFS_GROUPNAMES);
if (ref == null) {
- return ImmutableSet.of();
+ return ImmutableList.of();
}
try (RevWalk revWalk = new RevWalk(repository);
ObjectReader reader = revWalk.getObjectReader()) {
RevCommit notesCommit = revWalk.parseCommit(ref.getObjectId());
NoteMap noteMap = NoteMap.read(reader, notesCommit);
- Set<GroupReference> groupReferences = new LinkedHashSet<>();
+ Multiset<GroupReference> groupReferences = HashMultiset.create();
for (Note note : noteMap) {
GroupReference groupReference = getGroupReference(reader, note.getData());
- boolean result = groupReferences.add(groupReference);
- if (!result) {
+ int numOfOccurrences = groupReferences.add(groupReference, 1);
+ if (numOfOccurrences > 1) {
GroupsNoteDbConsistencyChecker.logConsistencyProblemAsWarning(
"The UUID of group %s (%s) is duplicate in group name notes",
groupReference.getName(), groupReference.getUUID());
}
}
- return ImmutableSet.copyOf(groupReferences);
+ return ImmutableList.copyOf(groupReferences);
}
}
- public static void updateGroupNames(
- Repository allUsersRepo,
+ public static void updateAllGroups(
+ Repository repository,
ObjectInserter inserter,
BatchRefUpdate bru,
Collection<GroupReference> groupReferences,
@@ -153,7 +154,7 @@
RevWalk rw = new RevWalk(reader)) {
// Always start from an empty map, discarding old notes.
NoteMap noteMap = NoteMap.newEmptyMap();
- Ref ref = allUsersRepo.exactRef(RefNames.REFS_GROUPNAMES);
+ Ref ref = repository.exactRef(RefNames.REFS_GROUPNAMES);
RevCommit oldCommit = ref != null ? rw.parseCommit(ref.getObjectId()) : null;
for (Map.Entry<AccountGroup.UUID, String> e : biMap.entrySet()) {
@@ -195,8 +196,8 @@
}
private final AccountGroup.UUID groupUuid;
- private final Optional<AccountGroup.NameKey> oldGroupName;
- private final Optional<AccountGroup.NameKey> newGroupName;
+ private Optional<AccountGroup.NameKey> oldGroupName;
+ private Optional<AccountGroup.NameKey> newGroupName;
private boolean nameConflicting;
@@ -278,6 +279,9 @@
commit.setTreeId(noteMap.writeTree(inserter));
commit.setMessage(getCommitMessage());
+ oldGroupName = Optional.empty();
+ newGroupName = Optional.empty();
+
return true;
}
@@ -311,8 +315,7 @@
return config.toText();
}
- @VisibleForTesting
- public static GroupReference getGroupReference(ObjectReader reader, ObjectId noteDataBlobId)
+ private static GroupReference getGroupReference(ObjectReader reader, ObjectId noteDataBlobId)
throws IOException, ConfigInvalidException {
byte[] noteData = reader.open(noteDataBlobId, OBJ_BLOB).getCachedBytes();
return getFromNoteData(noteData);
diff --git a/java/com/google/gerrit/server/group/db/Groups.java b/java/com/google/gerrit/server/group/db/Groups.java
index f3232be..d0ea9ca 100644
--- a/java/com/google/gerrit/server/group/db/Groups.java
+++ b/java/com/google/gerrit/server/group/db/Groups.java
@@ -192,7 +192,7 @@
throws OrmException, IOException, ConfigInvalidException {
if (groupsMigration.readFromNoteDb()) {
try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
- return GroupNameNotes.loadAllGroupReferences(allUsersRepo).stream();
+ return GroupNameNotes.loadAllGroups(allUsersRepo).stream();
}
}
@@ -298,10 +298,9 @@
.filter(groupUuid -> !AccountGroup.isInternalGroup(groupUuid));
}
- private Stream<AccountGroup.UUID> getExternalGroupsFromNoteDb(Repository allUsersRepo)
+ private static Stream<AccountGroup.UUID> getExternalGroupsFromNoteDb(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- ImmutableSet<GroupReference> allInternalGroups =
- GroupNameNotes.loadAllGroupReferences(allUsersRepo);
+ ImmutableList<GroupReference> allInternalGroups = GroupNameNotes.loadAllGroups(allUsersRepo);
ImmutableSet.Builder<AccountGroup.UUID> allSubgroups = ImmutableSet.builder();
for (GroupReference internalGroup : allInternalGroups) {
Optional<InternalGroup> group = getGroupFromNoteDb(allUsersRepo, internalGroup.getUUID());
diff --git a/java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java b/java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java
index 65ac12b..9f0cb3a 100644
--- a/java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java
+++ b/java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java
@@ -218,7 +218,7 @@
Repository allUsersRepo, InternalGroup group) throws IOException {
List<ConsistencyCheckInfo.ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
- allUsersRepo, group.getName(), group.getGroupUUID());
+ allUsersRepo, group.getNameKey(), group.getGroupUUID());
problems.forEach(GroupsNoteDbConsistencyChecker::logConsistencyProblem);
}
@@ -232,10 +232,10 @@
*/
@VisibleForTesting
static List<ConsistencyProblemInfo> checkWithGroupNameNotes(
- Repository allUsersRepo, String groupName, AccountGroup.UUID groupUUID) throws IOException {
+ Repository allUsersRepo, AccountGroup.NameKey groupName, AccountGroup.UUID groupUUID)
+ throws IOException {
try {
- Optional<GroupReference> groupRef =
- GroupNameNotes.loadOneGroupReference(allUsersRepo, groupName);
+ Optional<GroupReference> groupRef = GroupNameNotes.loadGroup(allUsersRepo, groupName);
if (!groupRef.isPresent()) {
return ImmutableList.of(
@@ -243,7 +243,6 @@
}
AccountGroup.UUID uuid = groupRef.get().getUUID();
- String name = groupRef.get().getName();
List<ConsistencyProblemInfo> problems = new ArrayList<>();
if (!Objects.equals(groupUUID, uuid)) {
@@ -253,9 +252,11 @@
groupName, groupUUID, uuid));
}
- if (!Objects.equals(groupName, name)) {
+ String name = groupName.get();
+ String actualName = groupRef.get().getName();
+ if (!Objects.equals(name, actualName)) {
problems.add(
- warning("group note of name '%s' claims to represent name of '%s'", groupName, name));
+ warning("group note of name '%s' claims to represent name of '%s'", name, actualName));
}
return problems;
} catch (ConfigInvalidException e) {
diff --git a/java/com/google/gerrit/server/group/db/GroupsUpdate.java b/java/com/google/gerrit/server/group/db/GroupsUpdate.java
index d93c8bd..fd5c453 100644
--- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java
+++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java
@@ -482,7 +482,7 @@
try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
AccountGroup.NameKey groupName = groupUpdate.getName().orElseGet(groupCreation::getNameKey);
GroupNameNotes groupNameNotes =
- GroupNameNotes.loadForNewGroup(allUsersRepo, groupCreation.getGroupUUID(), groupName);
+ GroupNameNotes.forNewGroup(allUsersRepo, groupCreation.getGroupUUID(), groupName);
GroupConfig groupConfig = GroupConfig.createForNewGroup(allUsersRepo, groupCreation);
groupConfig.setGroupUpdate(groupUpdate, this::getAccountNameEmail, this::getGroupName);
@@ -515,7 +515,7 @@
if (groupUpdate.getName().isPresent()) {
AccountGroup.NameKey oldName = originalGroup.getNameKey();
AccountGroup.NameKey newName = groupUpdate.getName().get();
- groupNameNotes = GroupNameNotes.loadForRename(allUsersRepo, groupUuid, oldName, newName);
+ groupNameNotes = GroupNameNotes.forRename(allUsersRepo, groupUuid, oldName, newName);
}
commit(allUsersRepo, groupConfig, groupNameNotes);
diff --git a/java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java b/java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java
index 46fd666..9197a01 100644
--- a/java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java
+++ b/java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java
@@ -15,14 +15,10 @@
package com.google.gerrit.server.group.db.testing;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.gerrit.server.group.db.GroupNameNotes.getGroupReference;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Streams;
-import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.extensions.common.CommitInfo;
-import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.CommitUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -31,29 +27,12 @@
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.notes.Note;
-import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevSort;
import org.eclipse.jgit.revwalk.RevWalk;
/** Test utilities for low-level NoteDb groups. */
public class GroupTestUtil {
- public static ImmutableMap<String, String> readNameToUuidMap(Repository repo) throws Exception {
- ImmutableMap.Builder<String, String> result = ImmutableMap.builder();
- try (RevWalk rw = new RevWalk(repo)) {
- Ref ref = repo.exactRef(RefNames.REFS_GROUPNAMES);
- if (ref != null) {
- NoteMap noteMap = NoteMap.read(rw.getObjectReader(), rw.parseCommit(ref.getObjectId()));
- for (Note note : noteMap) {
- GroupReference gr = getGroupReference(rw.getObjectReader(), note.getData());
- result.put(gr.getName(), gr.getUUID().get());
- }
- }
- }
- return result.build();
- }
-
// TODO(dborowitz): Move somewhere even more common.
public static ImmutableList<CommitInfo> log(Repository repo, String refName) throws Exception {
try (RevWalk rw = new RevWalk(repo)) {
diff --git a/java/com/google/gerrit/server/restapi/account/SetPreferences.java b/java/com/google/gerrit/server/restapi/account/SetPreferences.java
index 8584240..9b2b231 100644
--- a/java/com/google/gerrit/server/restapi/account/SetPreferences.java
+++ b/java/com/google/gerrit/server/restapi/account/SetPreferences.java
@@ -14,18 +14,8 @@
package com.google.gerrit.server.restapi.account;
-import static com.google.gerrit.server.git.UserConfigSections.CHANGE_TABLE_COLUMN;
-import static com.google.gerrit.server.git.UserConfigSections.KEY_ID;
-import static com.google.gerrit.server.git.UserConfigSections.KEY_MATCH;
-import static com.google.gerrit.server.git.UserConfigSections.KEY_TARGET;
-import static com.google.gerrit.server.git.UserConfigSections.KEY_TOKEN;
-import static com.google.gerrit.server.git.UserConfigSections.KEY_URL;
-import static com.google.gerrit.server.git.UserConfigSections.URL_ALIAS;
-
import com.google.common.base.Strings;
-import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
-import com.google.gerrit.extensions.client.MenuItem;
import com.google.gerrit.extensions.config.DownloadScheme;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -37,8 +27,6 @@
import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.PreferencesConfig;
-import com.google.gerrit.server.account.VersionedAccountPreferences;
-import com.google.gerrit.server.git.UserConfigSections;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -47,11 +35,7 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
-import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Config;
@Singleton
public class SetPreferences implements RestModifyView<AccountResource, GeneralPreferencesInfo> {
@@ -93,70 +77,6 @@
return cache.get(id).getGeneralPreferences();
}
- public static void storeMyMenus(VersionedAccountPreferences prefs, List<MenuItem> my)
- throws BadRequestException {
- Config cfg = prefs.getConfig();
- if (my != null) {
- unsetSection(cfg, UserConfigSections.MY);
- for (MenuItem item : my) {
- checkRequiredMenuItemField(item.name, "name");
- checkRequiredMenuItemField(item.url, "URL");
-
- set(cfg, item.name, KEY_URL, item.url);
- set(cfg, item.name, KEY_TARGET, item.target);
- set(cfg, item.name, KEY_ID, item.id);
- }
- }
- }
-
- public static void storeMyChangeTableColumns(
- VersionedAccountPreferences prefs, List<String> changeTable) {
- Config cfg = prefs.getConfig();
- if (changeTable != null) {
- unsetSection(cfg, UserConfigSections.CHANGE_TABLE);
- cfg.setStringList(UserConfigSections.CHANGE_TABLE, null, CHANGE_TABLE_COLUMN, changeTable);
- }
- }
-
- private static void set(Config cfg, String section, String key, @Nullable String val) {
- if (val == null || val.trim().isEmpty()) {
- cfg.unset(UserConfigSections.MY, section.trim(), key);
- } else {
- cfg.setString(UserConfigSections.MY, section.trim(), key, val.trim());
- }
- }
-
- private static void unsetSection(Config cfg, String section) {
- cfg.unsetSection(section, null);
- for (String subsection : cfg.getSubsections(section)) {
- cfg.unsetSection(section, subsection);
- }
- }
-
- public static void storeUrlAliases(
- VersionedAccountPreferences prefs, Map<String, String> urlAliases) {
- if (urlAliases != null) {
- Config cfg = prefs.getConfig();
- for (String subsection : cfg.getSubsections(URL_ALIAS)) {
- cfg.unsetSection(URL_ALIAS, subsection);
- }
-
- int i = 1;
- for (Entry<String, String> e : urlAliases.entrySet()) {
- cfg.setString(URL_ALIAS, URL_ALIAS + i, KEY_MATCH, e.getKey());
- cfg.setString(URL_ALIAS, URL_ALIAS + i, KEY_TOKEN, e.getValue());
- i++;
- }
- }
- }
-
- private static void checkRequiredMenuItemField(String value, String name)
- throws BadRequestException {
- if (value == null || value.trim().isEmpty()) {
- throw new BadRequestException(name + " for menu item is required");
- }
- }
-
private void checkDownloadScheme(String downloadScheme) throws BadRequestException {
if (Strings.isNullOrEmpty(downloadScheme)) {
return;
diff --git a/java/com/google/gerrit/server/schema/SchemaCreator.java b/java/com/google/gerrit/server/schema/SchemaCreator.java
index 2004c98..895e75c 100644
--- a/java/com/google/gerrit/server/schema/SchemaCreator.java
+++ b/java/com/google/gerrit/server/schema/SchemaCreator.java
@@ -240,7 +240,7 @@
AccountGroup.NameKey groupName = groupUpdate.getName().orElseGet(groupCreation::getNameKey);
GroupNameNotes groupNameNotes =
- GroupNameNotes.loadForNewGroup(allUsersRepo, groupCreation.getGroupUUID(), groupName);
+ GroupNameNotes.forNewGroup(allUsersRepo, groupCreation.getGroupUUID(), groupName);
commit(allUsersRepo, groupConfig, groupNameNotes);
diff --git a/java/com/google/gerrit/truth/ListSubject.java b/java/com/google/gerrit/truth/ListSubject.java
index bcd8dcf..cccf51b 100644
--- a/java/com/google/gerrit/truth/ListSubject.java
+++ b/java/com/google/gerrit/truth/ListSubject.java
@@ -30,8 +30,7 @@
@SuppressWarnings("unchecked")
public static <S extends Subject<S, E>, E> ListSubject<S, E> assertThat(
List<E> list, Function<E, S> elementAssertThatFunction) {
- // The ListSubjectFactory always returns ListSubjects.
- // -> Casting is appropriate.
+ // The ListSubjectFactory always returns ListSubjects. -> Casting is appropriate.
return (ListSubject<S, E>)
assertAbout(new ListSubjectFactory<>(elementAssertThatFunction)).that(list);
}
@@ -44,11 +43,8 @@
public S element(int index) {
checkArgument(index >= 0, "index(%s) must be >= 0", index);
- // The constructor only accepts lists.
- // -> Casting is appropriate.
- @SuppressWarnings("unchecked")
- List<E> list = (List<E>) actual();
isNotNull();
+ List<E> list = getActualList();
if (index >= list.size()) {
fail("has an element at index " + index);
}
@@ -61,11 +57,23 @@
return element(0);
}
+ public S lastElement() {
+ isNotNull();
+ isNotEmpty();
+ List<E> list = getActualList();
+ return element(list.size() - 1);
+ }
+
+ @SuppressWarnings("unchecked")
+ private List<E> getActualList() {
+ // The constructor only accepts lists. -> Casting is appropriate.
+ return (List<E>) actual();
+ }
+
@SuppressWarnings("unchecked")
@Override
public ListSubject<S, E> named(String s, Object... objects) {
- // This object is returned which is of type ListSubject.
- // -> Casting is appropriate.
+ // This object is returned which is of type ListSubject. -> Casting is appropriate.
return (ListSubject<S, E>) super.named(s, objects);
}
@@ -81,8 +89,7 @@
@SuppressWarnings("unchecked")
@Override
public ListSubject<S, T> createSubject(FailureMetadata failureMetadata, Iterable<?> objects) {
- // The constructor of ListSubject only accepts lists.
- // -> Casting is appropriate.
+ // The constructor of ListSubject only accepts lists. -> Casting is appropriate.
return new ListSubject<>(failureMetadata, (List<T>) objects, elementAssertThatFunction);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 38a9489..f7ca2f2 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -75,9 +75,11 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.git.receive.ReceiveConstants;
+import com.google.gerrit.server.git.validators.CommitValidators.ChangeIdValidator;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.project.testing.Util;
@@ -1321,6 +1323,28 @@
}
@Test
+ public void testPushWithChangedChangeId() throws Exception {
+ PushOneCommit.Result r = pushTo("refs/for/master");
+ r.assertOkStatus();
+ PushOneCommit push =
+ pushFactory.create(
+ db,
+ admin.getIdent(),
+ testRepo,
+ PushOneCommit.SUBJECT
+ + "\n\n"
+ + "Change-Id: I55eab7c7a76e95005fa9cc469aa8f9fc16da9eba\n",
+ "b.txt",
+ "anotherContent",
+ r.getChangeId());
+ r = push.to("refs/changes/" + r.getChange().change().getId().get());
+ r.assertErrorStatus(
+ String.format(
+ ChangeIdValidator.CHANGE_ID_MISMATCH_MSG,
+ r.getCommit().abbreviate(RevId.ABBREV_LEN).name()));
+ }
+
+ @Test
public void pushWithMultipleChangeIds() throws Exception {
testPushWithMultipleChangeIds();
}
diff --git a/javatests/com/google/gerrit/server/group/db/BUILD b/javatests/com/google/gerrit/server/group/db/BUILD
index 1ba0ce9..9816603 100644
--- a/javatests/com/google/gerrit/server/group/db/BUILD
+++ b/javatests/com/google/gerrit/server/group/db/BUILD
@@ -6,12 +6,14 @@
srcs = glob(["*.java"]),
deps = [
"//java/com/google/gerrit/common:server",
+ "//java/com/google/gerrit/common/data/testing:common-data-test-util",
"//java/com/google/gerrit/extensions:api",
"//java/com/google/gerrit/extensions/common/testing:common-test-util",
"//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/group/db/testing",
"//java/com/google/gerrit/testing:gerrit-test-util",
+ "//java/com/google/gerrit/truth",
"//lib:gwtorm",
"//lib:truth",
"//lib/jgit/org.eclipse.jgit:jgit",
diff --git a/javatests/com/google/gerrit/server/group/db/GroupNameNotesTest.java b/javatests/com/google/gerrit/server/group/db/GroupNameNotesTest.java
index d4ddbcf..c059da9 100644
--- a/javatests/com/google/gerrit/server/group/db/GroupNameNotesTest.java
+++ b/javatests/com/google/gerrit/server/group/db/GroupNameNotesTest.java
@@ -18,24 +18,38 @@
import static com.google.common.truth.Truth.assert_;
import static com.google.gerrit.extensions.common.testing.CommitInfoSubject.assertThat;
import static com.google.gerrit.reviewdb.client.RefNames.REFS_GROUPNAMES;
+import static com.google.gerrit.truth.OptionalSubject.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.testing.GroupReferenceSubject;
import com.google.gerrit.extensions.common.CommitInfo;
+import com.google.gerrit.extensions.common.testing.CommitInfoSubject;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.config.AllUsersNameProvider;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.group.db.testing.GroupTestUtil;
import com.google.gerrit.server.update.RefUpdateUtil;
-import com.google.gerrit.testing.GerritBaseTests;
import com.google.gerrit.testing.TestTimeUtil;
+import com.google.gerrit.truth.ListSubject;
+import com.google.gerrit.truth.OptionalSubject;
+import com.google.gwtorm.client.KeyUtil;
+import com.google.gwtorm.server.OrmDuplicateKeyException;
+import com.google.gwtorm.server.StandardKeyEncoder;
+import java.io.IOException;
import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
import java.util.TimeZone;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
@@ -47,16 +61,28 @@
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.NoteMap;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.After;
import org.junit.Before;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.ExpectedException;
-public class GroupNameNotesTest extends GerritBaseTests {
+public class GroupNameNotesTest {
+ static {
+ KeyUtil.setEncoderImpl(new StandardKeyEncoder());
+ }
+
private static final String SERVER_NAME = "Gerrit Server";
private static final String SERVER_EMAIL = "noreply@gerritcodereview.com";
private static final TimeZone TZ = TimeZone.getTimeZone("America/Los_Angeles");
+ @Rule public ExpectedException expectedException = ExpectedException.none();
+
+ private final AccountGroup.UUID groupUuid = new AccountGroup.UUID("users-XYZ");
+ private final AccountGroup.NameKey groupName = new AccountGroup.NameKey("users");
+
private AtomicInteger idCounter;
private Repository repo;
@@ -73,12 +99,311 @@
}
@Test
+ public void newGroupCanBeCreated() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ Optional<GroupReference> groupReference = loadGroup(groupName);
+ assertThatGroup(groupReference).value().groupUuid().isEqualTo(groupUuid);
+ assertThatGroup(groupReference).value().name().isEqualTo(groupName.get());
+ }
+
+ @Test
+ public void uuidOfNewGroupMustNotBeNull() throws Exception {
+ expectedException.expect(NullPointerException.class);
+ GroupNameNotes.forNewGroup(repo, null, groupName);
+ }
+
+ @Test
+ public void nameOfNewGroupMustNotBeNull() throws Exception {
+ expectedException.expect(NullPointerException.class);
+ GroupNameNotes.forNewGroup(repo, groupUuid, null);
+ }
+
+ @Test
+ public void nameOfNewGroupMayBeEmpty() throws Exception {
+ AccountGroup.NameKey emptyName = new AccountGroup.NameKey("");
+ createGroup(groupUuid, emptyName);
+
+ Optional<GroupReference> groupReference = loadGroup(emptyName);
+ assertThatGroup(groupReference).value().name().isEqualTo("");
+ }
+
+ @Test
+ public void newGroupMustNotReuseNameOfAnotherGroup() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.UUID anotherGroupUuid = new AccountGroup.UUID("AnotherGroup");
+ expectedException.expect(OrmDuplicateKeyException.class);
+ expectedException.expectMessage(groupName.get());
+ GroupNameNotes.forNewGroup(repo, anotherGroupUuid, groupName);
+ }
+
+ @Test
+ public void newGroupMayReuseUuidOfAnotherGroup() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ createGroup(groupUuid, anotherName);
+
+ Optional<GroupReference> group1 = loadGroup(groupName);
+ assertThatGroup(group1).value().groupUuid().isEqualTo(groupUuid);
+ Optional<GroupReference> group2 = loadGroup(anotherName);
+ assertThatGroup(group2).value().groupUuid().isEqualTo(groupUuid);
+ }
+
+ @Test
+ public void groupCanBeRenamed() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ renameGroup(groupUuid, groupName, anotherName);
+
+ Optional<GroupReference> groupReference = loadGroup(anotherName);
+ assertThatGroup(groupReference).value().groupUuid().isEqualTo(groupUuid);
+ assertThatGroup(groupReference).value().name().isEqualTo(anotherName.get());
+ }
+
+ @Test
+ public void previousNameOfGroupCannotBeUsedAfterRename() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ renameGroup(groupUuid, groupName, anotherName);
+
+ Optional<GroupReference> group = loadGroup(groupName);
+ assertThatGroup(group).isAbsent();
+ }
+
+ @Test
+ public void groupCannotBeRenamedToNull() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ expectedException.expect(NullPointerException.class);
+ GroupNameNotes.forRename(repo, groupUuid, groupName, null);
+ }
+
+ @Test
+ public void oldNameOfGroupMustBeSpecifiedForRename() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ expectedException.expect(NullPointerException.class);
+ GroupNameNotes.forRename(repo, groupUuid, null, anotherName);
+ }
+
+ @Test
+ public void groupCannotBeRenamedWhenOldNameIsWrong() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.NameKey anotherOldName = new AccountGroup.NameKey("contributors");
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ expectedException.expect(ConfigInvalidException.class);
+ expectedException.expectMessage(anotherOldName.get());
+ GroupNameNotes.forRename(repo, groupUuid, anotherOldName, anotherName);
+ }
+
+ @Test
+ public void groupCannotBeRenamedToNameOfAnotherGroup() throws Exception {
+ createGroup(groupUuid, groupName);
+ AccountGroup.UUID anotherGroupUuid = new AccountGroup.UUID("admins-ABC");
+ AccountGroup.NameKey anotherGroupName = new AccountGroup.NameKey("admins");
+ createGroup(anotherGroupUuid, anotherGroupName);
+
+ expectedException.expect(OrmDuplicateKeyException.class);
+ expectedException.expectMessage(anotherGroupName.get());
+ GroupNameNotes.forRename(repo, groupUuid, groupName, anotherGroupName);
+ }
+
+ @Test
+ public void groupCannotBeRenamedWithoutSpecifiedUuid() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ expectedException.expect(NullPointerException.class);
+ GroupNameNotes.forRename(repo, null, groupName, anotherName);
+ }
+
+ @Test
+ public void groupCannotBeRenamedWhenUuidIsWrong() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.UUID anotherGroupUuid = new AccountGroup.UUID("admins-ABC");
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ expectedException.expect(ConfigInvalidException.class);
+ expectedException.expectMessage(groupUuid.get());
+ GroupNameNotes.forRename(repo, anotherGroupUuid, groupName, anotherName);
+ }
+
+ @Test
+ public void firstGroupCreationCreatesARootCommit() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ Ref ref = repo.exactRef(RefNames.REFS_GROUPNAMES);
+ assertThat(ref.getObjectId()).isNotNull();
+
+ try (RevWalk revWalk = new RevWalk(repo)) {
+ RevCommit revCommit = revWalk.parseCommit(ref.getObjectId());
+ assertThat(revCommit.getParentCount()).isEqualTo(0);
+ }
+ }
+
+ @Test
+ public void furtherGroupCreationAppendsACommit() throws Exception {
+ createGroup(groupUuid, groupName);
+ ImmutableList<CommitInfo> commitsAfterCreation = log();
+
+ AccountGroup.UUID anotherGroupUuid = new AccountGroup.UUID("admins-ABC");
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ createGroup(anotherGroupUuid, anotherName);
+
+ ImmutableList<CommitInfo> commitsAfterFurtherGroup = log();
+ assertThatCommits(commitsAfterFurtherGroup).containsAllIn(commitsAfterCreation);
+ assertThatCommits(commitsAfterFurtherGroup).lastElement().isNotIn(commitsAfterCreation);
+ }
+
+ @Test
+ public void groupRenamingAppendsACommit() throws Exception {
+ createGroup(groupUuid, groupName);
+ ImmutableList<CommitInfo> commitsAfterCreation = log();
+
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ renameGroup(groupUuid, groupName, anotherName);
+
+ ImmutableList<CommitInfo> commitsAfterRename = log();
+ assertThatCommits(commitsAfterRename).containsAllIn(commitsAfterCreation);
+ assertThatCommits(commitsAfterRename).lastElement().isNotIn(commitsAfterCreation);
+ }
+
+ @Test
+ public void newCommitIsNotCreatedForRedundantNameUpdate() throws Exception {
+ createGroup(groupUuid, groupName);
+ ImmutableList<CommitInfo> commitsAfterCreation = log();
+
+ renameGroup(groupUuid, groupName, groupName);
+
+ ImmutableList<CommitInfo> commitsAfterRename = log();
+ assertThatCommits(commitsAfterRename).isEqualTo(commitsAfterCreation);
+ }
+
+ @Test
+ public void newCommitIsNotCreatedWhenCommittingGroupCreationTwice() throws Exception {
+ GroupNameNotes groupNameNotes = GroupNameNotes.forNewGroup(repo, groupUuid, groupName);
+
+ commit(groupNameNotes);
+ ImmutableList<CommitInfo> commitsAfterFirstCommit = log();
+ commit(groupNameNotes);
+ ImmutableList<CommitInfo> commitsAfterSecondCommit = log();
+
+ assertThatCommits(commitsAfterSecondCommit).isEqualTo(commitsAfterFirstCommit);
+ }
+
+ @Test
+ public void newCommitIsNotCreatedWhenCommittingGroupRenamingTwice() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ GroupNameNotes groupNameNotes =
+ GroupNameNotes.forRename(repo, groupUuid, groupName, anotherName);
+
+ commit(groupNameNotes);
+ ImmutableList<CommitInfo> commitsAfterFirstCommit = log();
+ commit(groupNameNotes);
+ ImmutableList<CommitInfo> commitsAfterSecondCommit = log();
+
+ assertThatCommits(commitsAfterSecondCommit).isEqualTo(commitsAfterFirstCommit);
+ }
+
+ @Test
+ public void commitMessageMentionsGroupCreation() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ ImmutableList<CommitInfo> commits = log();
+ assertThatCommits(commits).lastElement().message().contains("Create");
+ assertThatCommits(commits).lastElement().message().contains(groupName.get());
+ }
+
+ @Test
+ public void commitMessageMentionsGroupRenaming() throws Exception {
+ createGroup(groupUuid, groupName);
+
+ AccountGroup.NameKey anotherName = new AccountGroup.NameKey("admins");
+ renameGroup(groupUuid, groupName, anotherName);
+
+ ImmutableList<CommitInfo> commits = log();
+ assertThatCommits(commits).lastElement().message().contains("Rename");
+ assertThatCommits(commits).lastElement().message().contains(groupName.get());
+ assertThatCommits(commits).lastElement().message().contains(anotherName.get());
+ }
+
+ @Test
+ public void nonExistentNotesRefIsEquivalentToNonExistentGroup() throws Exception {
+ Optional<GroupReference> group = loadGroup(groupName);
+
+ assertThatGroup(group).isAbsent();
+ }
+
+ @Test
+ public void nonExistentGroupCannotBeLoaded() throws Exception {
+ createGroup(new AccountGroup.UUID("contributors-MN"), new AccountGroup.NameKey("contributors"));
+ createGroup(groupUuid, groupName);
+
+ Optional<GroupReference> group = loadGroup(new AccountGroup.NameKey("admins"));
+ assertThatGroup(group).isAbsent();
+ }
+
+ @Test
+ public void specificGroupCanBeLoaded() throws Exception {
+ createGroup(new AccountGroup.UUID("contributors-MN"), new AccountGroup.NameKey("contributors"));
+ createGroup(groupUuid, groupName);
+ createGroup(new AccountGroup.UUID("admins-ABC"), new AccountGroup.NameKey("admins"));
+
+ Optional<GroupReference> group = loadGroup(groupName);
+ assertThatGroup(group).value().groupUuid().isEqualTo(groupUuid);
+ }
+
+ @Test
+ public void nonExistentNotesRefIsEquivalentToNotAnyExistingGroups() throws Exception {
+ ImmutableList<GroupReference> allGroups = GroupNameNotes.loadAllGroups(repo);
+
+ assertThat(allGroups).isEmpty();
+ }
+
+ @Test
+ public void allGroupsCanBeLoaded() throws Exception {
+ AccountGroup.UUID groupUuid1 = new AccountGroup.UUID("contributors-MN");
+ AccountGroup.NameKey groupName1 = new AccountGroup.NameKey("contributors");
+ createGroup(groupUuid1, groupName1);
+ AccountGroup.UUID groupUuid2 = new AccountGroup.UUID("admins-ABC");
+ AccountGroup.NameKey groupName2 = new AccountGroup.NameKey("admins");
+ createGroup(groupUuid2, groupName2);
+
+ ImmutableList<GroupReference> allGroups = GroupNameNotes.loadAllGroups(repo);
+
+ GroupReference group1 = new GroupReference(groupUuid1, groupName1.get());
+ GroupReference group2 = new GroupReference(groupUuid2, groupName2.get());
+ assertThat(allGroups).containsExactly(group1, group2);
+ }
+
+ @Test
+ public void loadedGroupsContainGroupsWithDuplicateGroupUuids() throws Exception {
+ createGroup(groupUuid, groupName);
+ AccountGroup.NameKey anotherGroupName = new AccountGroup.NameKey("admins");
+ createGroup(groupUuid, anotherGroupName);
+
+ ImmutableList<GroupReference> allGroups = GroupNameNotes.loadAllGroups(repo);
+
+ GroupReference group1 = new GroupReference(groupUuid, groupName.get());
+ GroupReference group2 = new GroupReference(groupUuid, anotherGroupName.get());
+ assertThat(allGroups).containsExactly(group1, group2);
+ }
+
+ @Test
public void updateGroupNames() throws Exception {
GroupReference g1 = newGroup("a");
GroupReference g2 = newGroup("b");
PersonIdent ident = newPersonIdent();
- updateGroupNames(ident, g1, g2);
+ updateAllGroups(ident, g1, g2);
ImmutableList<CommitInfo> log = log();
assertThat(log).hasSize(1);
@@ -87,11 +412,11 @@
assertThat(log.get(0)).author().matches(ident);
assertThat(log.get(0)).committer().matches(ident);
- assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
+ assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(g1, g2);
// Updating the same set of names is a no-op.
String commit = log.get(0).commit;
- updateGroupNames(newPersonIdent(), g1, g2);
+ updateAllGroups(newPersonIdent(), g1, g2);
log = log();
assertThat(log).hasSize(1);
assertThat(log.get(0)).commit().isEqualTo(commit);
@@ -120,9 +445,9 @@
.copy();
ident = newPersonIdent();
- updateGroupNames(ident, g1, g2);
+ updateAllGroups(ident, g1, g2);
- assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
+ assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(g1, g2);
ImmutableList<CommitInfo> log = log();
assertThat(log).hasSize(2);
@@ -142,13 +467,13 @@
GroupReference g2 = newGroup("b");
PersonIdent ident = newPersonIdent();
- updateGroupNames(ident, g1, g2);
+ updateAllGroups(ident, g1, g2);
- assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
+ assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(g1, g2);
- updateGroupNames(ident);
+ updateAllGroups(ident);
- assertThat(GroupTestUtil.readNameToUuidMap(repo)).isEmpty();
+ assertThat(GroupNameNotes.loadAllGroups(repo)).isEmpty();
ImmutableList<CommitInfo> log = log();
assertThat(log).hasSize(2);
@@ -171,12 +496,46 @@
@Test
public void emptyGroupName() throws Exception {
GroupReference g = newGroup("");
- updateGroupNames(newPersonIdent(), g);
+ updateAllGroups(newPersonIdent(), g);
- assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("", "-1");
+ assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(g);
assertThat(readNameNote(g)).isEqualTo("[group]\n\tuuid = -1\n\tname = \n");
}
+ private void createGroup(AccountGroup.UUID groupUuid, AccountGroup.NameKey groupName)
+ throws Exception {
+ GroupNameNotes groupNameNotes = GroupNameNotes.forNewGroup(repo, groupUuid, groupName);
+ commit(groupNameNotes);
+ }
+
+ private void renameGroup(
+ AccountGroup.UUID groupUuid, AccountGroup.NameKey oldName, AccountGroup.NameKey newName)
+ throws Exception {
+ GroupNameNotes groupNameNotes = GroupNameNotes.forRename(repo, groupUuid, oldName, newName);
+ commit(groupNameNotes);
+ }
+
+ private Optional<GroupReference> loadGroup(AccountGroup.NameKey groupName) throws Exception {
+ return GroupNameNotes.loadGroup(repo, groupName);
+ }
+
+ private void commit(GroupNameNotes groupNameNotes) throws IOException {
+ try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+ groupNameNotes.commit(metaDataUpdate);
+ }
+ }
+
+ private MetaDataUpdate createMetaDataUpdate() {
+ PersonIdent serverIdent = newPersonIdent();
+
+ MetaDataUpdate metaDataUpdate =
+ new MetaDataUpdate(
+ GitReferenceUpdated.DISABLED, new Project.NameKey("Test Repository"), repo);
+ metaDataUpdate.getCommitBuilder().setCommitter(serverIdent);
+ metaDataUpdate.getCommitBuilder().setAuthor(serverIdent);
+ return metaDataUpdate;
+ }
+
private GroupReference newGroup(String name) {
int id = idCounter.incrementAndGet();
return new GroupReference(new AccountGroup.UUID(name + "-" + id), name);
@@ -190,10 +549,10 @@
return GroupNameNotes.getNoteKey(new AccountGroup.NameKey(g.getName()));
}
- private void updateGroupNames(PersonIdent ident, GroupReference... groupRefs) throws Exception {
+ private void updateAllGroups(PersonIdent ident, GroupReference... groupRefs) throws Exception {
try (ObjectInserter inserter = repo.newObjectInserter()) {
BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
- GroupNameNotes.updateGroupNames(repo, inserter, bru, Arrays.asList(groupRefs), ident);
+ GroupNameNotes.updateAllGroups(repo, inserter, bru, Arrays.asList(groupRefs), ident);
inserter.flush();
RefUpdateUtil.executeChecked(bru, repo);
}
@@ -204,7 +563,7 @@
BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
PersonIdent ident = newPersonIdent();
try {
- GroupNameNotes.updateGroupNames(repo, inserter, bru, Arrays.asList(groupRefs), ident);
+ GroupNameNotes.updateAllGroups(repo, inserter, bru, Arrays.asList(groupRefs), ident);
assert_().fail("Expected IllegalArgumentException");
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo(GroupNameNotes.UNIQUE_REF_ERROR);
@@ -225,4 +584,14 @@
return new String(reader.open(noteMap.get(k), OBJ_BLOB).getCachedBytes(), UTF_8);
}
}
+
+ private static OptionalSubject<GroupReferenceSubject, GroupReference> assertThatGroup(
+ Optional<GroupReference> group) {
+ return assertThat(group, GroupReferenceSubject::assertThat);
+ }
+
+ private static ListSubject<CommitInfoSubject, CommitInfo> assertThatCommits(
+ List<CommitInfo> commits) {
+ return ListSubject.assertThat(commits, CommitInfoSubject::assertThat);
+ }
}
diff --git a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java
index 51cf987..048eaba 100644
--- a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java
+++ b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java
@@ -536,7 +536,7 @@
try (ObjectInserter inserter = repo.newObjectInserter()) {
ImmutableList<GroupReference> refs =
ImmutableList.of(GroupReference.forGroup(g1), GroupReference.forGroup(g2));
- GroupNameNotes.updateGroupNames(repo, inserter, bru, refs, newPersonIdent());
+ GroupNameNotes.updateAllGroups(repo, inserter, bru, refs, newPersonIdent());
inserter.flush();
}
@@ -552,7 +552,9 @@
assertMigratedCleanly(reload(g1), b1);
assertMigratedCleanly(reload(g2), b2);
- assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
+ GroupReference group1 = GroupReference.forGroup(g1);
+ GroupReference group2 = GroupReference.forGroup(g2);
+ assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(group1, group2);
}
@Test
diff --git a/javatests/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyCheckerTest.java b/javatests/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyCheckerTest.java
index eff755f..a5b04ee 100644
--- a/javatests/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyCheckerTest.java
+++ b/javatests/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyCheckerTest.java
@@ -30,7 +30,7 @@
public void groupNamesRefIsMissing() throws Exception {
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
- allUsersRepo, "g-1", new AccountGroup.UUID("uuid-1"));
+ allUsersRepo, new AccountGroup.NameKey("g-1"), new AccountGroup.UUID("uuid-1"));
assertThat(problems)
.containsExactly(warning("Group with name 'g-1' doesn't exist in the list of all names"));
}
@@ -40,7 +40,7 @@
updateGroupNamesRef("g-2", "[group]\n\tuuid = uuid-2\n\tname = g-2\n");
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
- allUsersRepo, "g-1", new AccountGroup.UUID("uuid-1"));
+ allUsersRepo, new AccountGroup.NameKey("g-1"), new AccountGroup.UUID("uuid-1"));
assertThat(problems)
.containsExactly(warning("Group with name 'g-1' doesn't exist in the list of all names"));
}
@@ -50,7 +50,7 @@
updateGroupNamesRef("g-1", "[group]\n\tuuid = uuid-1\n\tname = g-1\n");
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
- allUsersRepo, "g-1", new AccountGroup.UUID("uuid-1"));
+ allUsersRepo, new AccountGroup.NameKey("g-1"), new AccountGroup.UUID("uuid-1"));
assertThat(problems).isEmpty();
}
@@ -59,7 +59,7 @@
updateGroupNamesRef("g-1", "[group]\n\tuuid = uuid-2\n\tname = g-1\n");
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
- allUsersRepo, "g-1", new AccountGroup.UUID("uuid-1"));
+ allUsersRepo, new AccountGroup.NameKey("g-1"), new AccountGroup.UUID("uuid-1"));
assertThat(problems)
.containsExactly(
warning(
@@ -72,7 +72,7 @@
updateGroupNamesRef("g-1", "[group]\n\tuuid = uuid-1\n\tname = g-2\n");
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
- allUsersRepo, "g-1", new AccountGroup.UUID("uuid-1"));
+ allUsersRepo, new AccountGroup.NameKey("g-1"), new AccountGroup.UUID("uuid-1"));
assertThat(problems)
.containsExactly(warning("group note of name 'g-1' claims to represent name of 'g-2'"));
}
@@ -82,7 +82,7 @@
updateGroupNamesRef("g-1", "[group]\n\tuuid = uuid-2\n\tname = g-2\n");
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
- allUsersRepo, "g-1", new AccountGroup.UUID("uuid-1"));
+ allUsersRepo, new AccountGroup.NameKey("g-1"), new AccountGroup.UUID("uuid-1"));
assertThat(problems)
.containsExactly(
warning(
@@ -97,7 +97,7 @@
updateGroupNamesRef("g-1", "[invalid");
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
- allUsersRepo, "g-1", new AccountGroup.UUID("uuid-1"));
+ allUsersRepo, new AccountGroup.NameKey("g-1"), new AccountGroup.UUID("uuid-1"));
assertThat(problems)
.containsExactly(
warning(
