opendev/gerrit
Code Issues Proposed changes

GroupConfig: add a consistency check after loading a group

Browse Source 
Change-Id: I5743ac5262ec2e5ff7c6be17df9c4d4237d9b3fe
This commit is contained in:
Changcheng Xiao
2017-11-27 15:45:14 +01:00
parent 59fecfc6e3
commit 02058e1e78
7 changed files with 244 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
java/com/google/gerrit/server/group/db/GroupNameNotes.java
View File

@@ -196,6 +196,25 @@ public class GroupNameNotes extends VersionedMetaData {
}
}
public static Optional<GroupReference> loadOneGroupReference(
Repository allUsersRepo, String groupName) throws IOException, ConfigInvalidException {
Ref ref = allUsersRepo.exactRef(RefNames.REFS_GROUPNAMES);
if (ref == null) {
return Optional.empty();
}
try (RevWalk revWalk = new RevWalk(allUsersRepo);
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)));
if (noteDataBlobId == null) {
return Optional.empty();
}
return Optional.of(getGroupReference(reader, noteDataBlobId));
}
}
@Override
protected String getRefName() {
return RefNames.REFS_GROUPNAMES;

8
java/com/google/gerrit/server/group/db/Groups.java
View File

@@ -124,7 +124,13 @@ public class Groups {
Repository allUsersRepository, AccountGroup.UUID groupUuid)
throws IOException, ConfigInvalidException {
GroupConfig groupConfig = GroupConfig.loadForGroup(allUsersRepository, groupUuid);
return groupConfig.getLoadedGroup();
Optional<InternalGroup> loadedGroup = groupConfig.getLoadedGroup();
if (loadedGroup.isPresent()) {
// Check consistency with group name notes.
GroupsNoteDbConsistencyChecker.ensureConsistentWithGroupNameNotes(
allUsersRepository, loadedGroup.get());
}
return loadedGroup;
}
public static InternalGroup asInternalGroup(ReviewDb db, AccountGroup accountGroup)

54
java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java
View File

@@ -17,10 +17,13 @@ package com.google.gerrit.server.group.db;
import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.error;
import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.warning;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.RefNames;
@@ -32,6 +35,7 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import javax.inject.Singleton;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
@@ -210,6 +214,56 @@ public class GroupsNoteDbConsistencyChecker {
return problems;
}
public static void ensureConsistentWithGroupNameNotes(
Repository allUsersRepo, InternalGroup group) throws IOException {
List<ConsistencyCheckInfo.ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
allUsersRepo, group.getName(), group.getGroupUUID());
problems.forEach(GroupsNoteDbConsistencyChecker::logConsistencyProblem);
}
/**
* Check group 'uuid' and 'name' read from 'group.config' with group name notes.
*
* @param allUsersRepo 'All-Users' repository.
* @param groupName the name of the group to be checked.
* @param groupUUID the {@code AccountGroup.UUID} of the group to be checked.
* @return a list of {@code ConsistencyProblemInfo} containing the problem details.
*/
@VisibleForTesting
static List<ConsistencyProblemInfo> checkWithGroupNameNotes(
Repository allUsersRepo, String groupName, AccountGroup.UUID groupUUID) throws IOException {
try {
Optional<GroupReference> groupRef =
GroupNameNotes.loadOneGroupReference(allUsersRepo, groupName);
if (!groupRef.isPresent()) {
return ImmutableList.of(
warning("Group with name '%s' doesn't exist in the list of all names", groupName));
}
AccountGroup.UUID uuid = groupRef.get().getUUID();
String name = groupRef.get().getName();
List<ConsistencyProblemInfo> problems = new ArrayList<>();
if (!Objects.equals(groupUUID, uuid)) {
problems.add(
warning(
"group with name '%s' has UUID '%s' in 'group.config' but '%s' in group name notes",
groupName, groupUUID, uuid));
}
if (!Objects.equals(groupName, name)) {
problems.add(
warning("group note of name '%s' claims to represent name of '%s'", groupName, name));
}
return problems;
} catch (ConfigInvalidException e) {
return ImmutableList.of(
warning("fail to check consistency with group name notes: %s", e.getMessage()));
}
}
public static void logConsistencyProblemAsWarning(String fmt, Object... args) {
logConsistencyProblem(warning(fmt, args));
}

1
java/com/google/gerrit/server/group/db/testing/BUILD
View File

@@ -11,5 +11,6 @@ java_library(
"//java/com/google/gerrit/server",
"//lib:guava",
"//lib/jgit/org.eclipse.jgit:jgit",
"//lib/jgit/org.eclipse.jgit.junit:junit",
],
)

47
java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java
View File

@@ -23,12 +23,17 @@ 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;
import java.io.IOException;
import org.eclipse.jgit.junit.TestRepository;
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;
@@ -72,5 +77,47 @@ public class GroupTestUtil {
return ImmutableList.of();
}
public static void updateGroupFile(
GitRepositoryManager repoManager,
AllUsersName allUsersName,
PersonIdent serverIdent,
String refName,
String fileName,
String content)
throws Exception {
try (Repository repo = repoManager.openRepository(allUsersName)) {
updateGroupFile(repo, serverIdent, refName, fileName, content);
}
}
public static void updateGroupFile(
Repository allUsersRepo,
PersonIdent serverIdent,
String refName,
String fileName,
String contents)
throws Exception {
try (RevWalk rw = new RevWalk(allUsersRepo)) {
TestRepository<Repository> testRepository = new TestRepository<>(allUsersRepo, rw);
TestRepository.CommitBuilder builder =
testRepository
.branch(refName)
.commit()
.add(fileName, contents)
.message("update group file")
.author(serverIdent)
.committer(serverIdent);
Ref ref = allUsersRepo.exactRef(refName);
if (ref != null) {
RevCommit c = rw.parseCommit(ref.getObjectId());
if (c != null) {
builder.parent(c);
}
}
builder.create();
}
}
private GroupTestUtil() {}
}

25
javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java
View File

@@ -31,20 +31,17 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.group.db.GroupConfig;
import com.google.gerrit.server.group.db.GroupNameNotes;
import com.google.gerrit.server.group.db.testing.GroupTestUtil;
import com.google.gerrit.server.notedb.GroupsMigration;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.testing.ConfigSuite;
import java.util.List;
import javax.inject.Inject;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefRename;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
import org.junit.Test;
@@ -276,22 +273,8 @@ public class GroupsConsistencyIT extends AbstractDaemonTest {
fail(String.format("could not find %s substring '%s' in %s", want, msg, problems));
}
private void updateGroupFile(String refname, String filename, String contents) throws Exception {
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {
Ref ref = repo.exactRef(refname);
RevCommit c = rw.parseCommit(ref.getObjectId());
TestRepository<Repository> testRepository = new TestRepository<>(repo, rw);
testRepository
.branch(refname)
.commit()
.add(filename, contents)
.parent(c)
.message("update group file")
.author(serverIdent.get())
.committer(serverIdent.get())
.create();
}
private void updateGroupFile(String refName, String fileName, String content) throws Exception {
GroupTestUtil.updateGroupFile(
repoManager, allUsers, serverIdent.get(), refName, fileName, content);
}
}

112
javatests/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyCheckerTest.java Normal file
View File

@@ -0,0 +1,112 @@
// Copyright (C) 2017 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.server.group.db;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.warning;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.group.db.testing.GroupTestUtil;
import java.util.List;
import org.junit.Test;
public class GroupsNoteDbConsistencyCheckerTest extends AbstractGroupTest {
@Test
public void groupNamesRefIsMissing() throws Exception {
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
allUsersRepo, "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"));
}
@Test
public void groupNameNoteIsMissing() throws Exception {
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"));
assertThat(problems)
.containsExactly(warning("Group with name 'g-1' doesn't exist in the list of all names"));
}
@Test
public void groupNameNoteIsConsistent() throws Exception {
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"));
assertThat(problems).isEmpty();
}
@Test
public void groupNameNoteHasDifferentUUID() throws Exception {
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"));
assertThat(problems)
.containsExactly(
warning(
"group with name 'g-1' has UUID 'uuid-1' in 'group.config' but 'uuid-2' in group "
+ "name notes"));
}
@Test
public void groupNameNoteHasDifferentName() throws Exception {
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"));
assertThat(problems)
.containsExactly(warning("group note of name 'g-1' claims to represent name of 'g-2'"));
}
@Test
public void groupNameNoteHasDifferentNameAndUUID() throws Exception {
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"));
assertThat(problems)
.containsExactly(
warning(
"group with name 'g-1' has UUID 'uuid-1' in 'group.config' but 'uuid-2' in group "
+ "name notes"),
warning("group note of name 'g-1' claims to represent name of 'g-2'"))
.inOrder();
}
@Test
public void groupNameNoteFailToParse() throws Exception {
updateGroupNamesRef("g-1", "[invalid");
List<ConsistencyProblemInfo> problems =
GroupsNoteDbConsistencyChecker.checkWithGroupNameNotes(
allUsersRepo, "g-1", new AccountGroup.UUID("uuid-1"));
assertThat(problems)
.containsExactly(
warning(
"fail to check consistency with group name notes: Unexpected end of config file"));
}
private void updateGroupNamesRef(String groupName, String content) throws Exception {
String nameKey = GroupNameNotes.getNoteKey(new AccountGroup.NameKey(groupName)).getName();
GroupTestUtil.updateGroupFile(
allUsersRepo, serverIdent, RefNames.REFS_GROUPNAMES, nameKey, content);
}
}