diff --git a/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java b/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java index eb05d4ffa7..2c166d08cb 100644 --- a/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java +++ b/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java @@ -78,5 +78,13 @@ public class ConsistencyCheckInfo { public String toString() { return status.name() + ": " + message; } + + public static ConsistencyProblemInfo warning(String fmt, Object... args) { + return new ConsistencyProblemInfo(Status.WARNING, String.format(fmt, args)); + } + + public static ConsistencyProblemInfo error(String fmt, Object... args) { + return new ConsistencyProblemInfo(Status.ERROR, String.format(fmt, args)); + } } } diff --git a/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java b/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java index 6accb83297..541697a186 100644 --- a/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java +++ b/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java @@ -14,14 +14,12 @@ package com.google.gerrit.server.group.db; -import com.google.common.collect.BiMap; -import com.google.common.collect.HashBiMap; -import com.google.gerrit.common.data.GroupReference; +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.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroup.NameKey; -import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.config.AllUsersName; @@ -29,9 +27,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.notedb.GroupsMigration; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; @@ -41,34 +37,34 @@ import java.util.Set; import javax.inject.Inject; import javax.inject.Singleton; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.ObjectLoader; -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.RevWalk; -/** Checks internal consistency of NoteDb storage of groups. */ +/** + * Checks individual groups for oddities, such as cycles, non-existent subgroups, etc. Only works if + * we are writing to NoteDb. + */ @Singleton public class GroupsConsistencyChecker { + private final AllUsersName allUsersName; private final GroupBackend groupBackend; private final Accounts accounts; - private final AllUsersName allUsersName; private final GitRepositoryManager repoManager; + private final GroupsNoteDbConsistencyChecker globalChecker; private final GroupsMigration groupsMigration; @Inject GroupsConsistencyChecker( - Accounts accounts, - GroupBackend groupBackend, AllUsersName allUsersName, + GroupBackend groupBackend, + Accounts accounts, GitRepositoryManager repositoryManager, + GroupsNoteDbConsistencyChecker globalChecker, GroupsMigration groupsMigration) { - this.accounts = accounts; - this.groupBackend = groupBackend; this.allUsersName = allUsersName; + this.groupBackend = groupBackend; + this.accounts = accounts; this.repoManager = repositoryManager; + this.globalChecker = globalChecker; this.groupsMigration = groupsMigration; } @@ -79,89 +75,17 @@ public class GroupsConsistencyChecker { } try (Repository repo = repoManager.openRepository(allUsersName)) { - return check(repo); - } - } - - public List check(Repository repo) throws IOException { - // Get all refs in an attempt to avoid seeing half committed group updates. - Map refs = repo.getAllRefs(); - - List problems = new ArrayList<>(); - Map byUUID = new HashMap<>(); - BiMap nameMap = HashBiMap.create(); - - readGroups(repo, refs, problems, byUUID); - readGroupNames(repo, refs, problems, nameMap); - - // The sequential IDs are not keys in NoteDb, so no need to check them. - - // No use continuing if we couldn't read the data. - if (!problems.isEmpty()) { - return problems; - } - - problems = checkGlobalConsistency(byUUID, nameMap); - if (!problems.isEmpty()) { - return problems; - } - - // Check subgroups and members. - for (InternalGroup g : byUUID.values()) { - problems.addAll(checkGroup(g, byUUID)); - } - return problems; - } - - /** Check invariants of the group refs with the groupname refs. */ - private List checkGlobalConsistency( - Map byUUID, BiMap nameMap) { - List problems = new ArrayList<>(); - - // Check consistency between the data coming from different refs. - for (AccountGroup.UUID uuid : byUUID.keySet()) { - if (!nameMap.containsKey(uuid)) { - problems.add(error("group %s has no entry in name map", uuid)); - continue; + GroupsNoteDbConsistencyChecker.Result result = globalChecker.check(repo); + if (!result.problems.isEmpty()) { + return result.problems; } - String noteName = nameMap.get(uuid); - String groupRefName = byUUID.get(uuid).getName(); - if (!Objects.equals(noteName, groupRefName)) { - problems.add( - error( - "inconsistent name for group %s (name map %s vs. group ref %s)", - uuid, noteName, groupRefName)); + for (InternalGroup g : result.uuidToGroupMap.values()) { + result.problems.addAll(checkGroup(g, result.uuidToGroupMap)); } - } - for (AccountGroup.UUID uuid : nameMap.keySet()) { - if (!byUUID.containsKey(uuid)) { - problems.add( - error( - "name map has entry (%s, %s), entry missing as group ref", - uuid, nameMap.get(uuid))); - } + return result.problems; } - - // No use delving further into inconsistent data. - if (!problems.isEmpty()) { - return problems; - } - - // Check ids. - Map groupById = new HashMap<>(); - for (InternalGroup g : byUUID.values()) { - InternalGroup before = groupById.get(g.getId()); - if (before != null) { - problems.add( - error( - "shared group id %s for %s (%s) and %s (%s)", - g.getId(), before.getName(), before.getGroupUUID(), g.getName(), g.getGroupUUID())); - } - groupById.put(g.getId(), g); - } - return problems; } /** Checks the metadata for a single group for problems. */ @@ -207,78 +131,6 @@ public class GroupsConsistencyChecker { return problems; } - private void readGroupNames( - Repository repo, - Map refs, - List problems, - BiMap result) - throws IOException { - Ref ref = refs.get(RefNames.REFS_GROUPNAMES); - if (ref == null) { - problems.add( - new ConsistencyProblemInfo( - groupsMigration.readFromNoteDb() - ? ConsistencyProblemInfo.Status.ERROR - : ConsistencyProblemInfo.Status.WARNING, - String.format("ref %s does not exist", RefNames.REFS_GROUPNAMES))); - return; - } - - try (RevWalk rw = new RevWalk(repo)) { - NoteMap nm = NoteMap.read(rw.getObjectReader(), rw.parseCommit(ref.getObjectId())); - for (Note note : nm) { - ObjectLoader ld = rw.getObjectReader().open(note.getData()); - byte[] data = ld.getCachedBytes(); - - GroupReference gRef; - try { - gRef = GroupNameNotes.getFromNoteData(data); - } catch (ConfigInvalidException e) { - problems.add( - error( - "notename entry %s: %s does not parse: %s", - note, new String(data, StandardCharsets.UTF_8), e.getMessage())); - continue; - } - - ObjectId nameKey = GroupNameNotes.getNoteKey(new NameKey(gRef.getName())); - if (!Objects.equals(nameKey, note)) { - problems.add(error("notename entry %s does not match name %s", note, gRef.getName())); - } - - // We trust SHA1 to have no collisions, so no need to check uniqueness of name. - result.put(gRef.getUUID(), gRef.getName()); - } - } - } - - private void readGroups( - Repository repo, - Map refs, - List problems, - Map byUUID) - throws IOException { - for (Map.Entry entry : refs.entrySet()) { - if (!entry.getKey().startsWith(RefNames.REFS_GROUPS)) { - continue; - } - - AccountGroup.UUID uuid = AccountGroup.UUID.fromRef(entry.getKey()); - if (uuid == null) { - problems.add(error("null UUID from %s", entry.getKey())); - continue; - } - try { - GroupConfig cfg = - GroupConfig.loadForGroupSnapshot(repo, uuid, entry.getValue().getObjectId()); - - byUUID.put(uuid, cfg.getLoadedGroup().get()); - } catch (ConfigInvalidException e) { - problems.add(error("group %s does not parse: %s", uuid, e.getMessage())); - } - } - } - /** checkCycle walks through root's subgroups recursively, and checks for cycles. */ private List checkCycle( InternalGroup root, Map byUUID) { @@ -315,14 +167,4 @@ public class GroupsConsistencyChecker { } return problems; } - - private ConsistencyProblemInfo warning(String fmt, Object... args) { - return new ConsistencyProblemInfo( - ConsistencyProblemInfo.Status.WARNING, String.format(fmt, args)); - } - - private ConsistencyProblemInfo error(String fmt, Object... args) { - return new ConsistencyProblemInfo( - ConsistencyProblemInfo.Status.ERROR, String.format(fmt, args)); - } } diff --git a/java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java b/java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java new file mode 100644 index 0000000000..265aba04da --- /dev/null +++ b/java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java @@ -0,0 +1,227 @@ +// 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.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.error; +import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.warning; + +import com.google.common.collect.BiMap; +import com.google.common.collect.HashBiMap; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.data.GroupReference; +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.InternalGroup; +import com.google.gerrit.server.notedb.GroupsMigration; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import javax.inject.Inject; +import javax.inject.Singleton; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectLoader; +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.RevWalk; + +/** Check the referential integrity of NoteDb group storage. */ +@Singleton +public class GroupsNoteDbConsistencyChecker { + private final GroupsMigration groupsMigration; + + /** + * The result of a consistency check. The UUID map is only non-null if no problems were detected. + */ + public static class Result { + public List problems; + + @Nullable public Map uuidToGroupMap; + } + + @Inject + GroupsNoteDbConsistencyChecker(GroupsMigration groupsMigration) { + this.groupsMigration = groupsMigration; + } + + /** + * Checks for problems with the given All-Users repo. Returns null if we are not writing to + * NoteDb. + */ + @Nullable + public Result check(Repository repo) throws IOException { + if (!groupsMigration.writeToNoteDb()) { + return null; + } + + Result r = doCheck(repo); + if (!r.problems.isEmpty()) { + r.uuidToGroupMap = null; + } + return r; + } + + @Nullable + private Result doCheck(Repository repo) throws IOException { + Result result = new Result(); + result.problems = new ArrayList<>(); + result.uuidToGroupMap = new HashMap<>(); + + BiMap uuidNameBiMap = HashBiMap.create(); + + // Get all refs in an attempt to avoid seeing half committed group updates. + Map refs = repo.getAllRefs(); + readGroups(repo, refs, result); + readGroupNames(repo, refs, result, uuidNameBiMap); + // The sequential IDs are not keys in NoteDb, so no need to check them. + + if (!result.problems.isEmpty()) { + return result; + } + + // Continue checking if we could read data without problems. + result.problems.addAll(checkGlobalConsistency(result.uuidToGroupMap, uuidNameBiMap)); + + return result; + } + + private void readGroups(Repository repo, Map refs, Result result) + throws IOException { + for (Map.Entry entry : refs.entrySet()) { + if (!entry.getKey().startsWith(RefNames.REFS_GROUPS)) { + continue; + } + + AccountGroup.UUID uuid = AccountGroup.UUID.fromRef(entry.getKey()); + if (uuid == null) { + result.problems.add(error("null UUID from %s", entry.getKey())); + continue; + } + try { + GroupConfig cfg = + GroupConfig.loadForGroupSnapshot(repo, uuid, entry.getValue().getObjectId()); + result.uuidToGroupMap.put(uuid, cfg.getLoadedGroup().get()); + } catch (ConfigInvalidException e) { + result.problems.add(error("group %s does not parse: %s", uuid, e.getMessage())); + } + } + } + + private void readGroupNames( + Repository repo, + Map refs, + Result result, + BiMap uuidNameBiMap) + throws IOException { + Ref ref = refs.get(RefNames.REFS_GROUPNAMES); + if (ref == null) { + String msg = String.format("ref %s does not exist", RefNames.REFS_GROUPNAMES); + result.problems.add(groupsMigration.readFromNoteDb() ? error(msg) : warning(msg)); + return; + } + + try (RevWalk rw = new RevWalk(repo)) { + RevCommit c = rw.parseCommit(ref.getObjectId()); + NoteMap nm = NoteMap.read(rw.getObjectReader(), c); + + for (Note note : nm) { + ObjectLoader ld = rw.getObjectReader().open(note.getData()); + byte[] data = ld.getCachedBytes(); + + GroupReference gRef; + try { + gRef = GroupNameNotes.getFromNoteData(data); + } catch (ConfigInvalidException e) { + result.problems.add( + error( + "notename entry %s: %s does not parse: %s", + note, new String(data, StandardCharsets.UTF_8), e.getMessage())); + continue; + } + + ObjectId nameKey = GroupNameNotes.getNoteKey(new AccountGroup.NameKey(gRef.getName())); + if (!Objects.equals(nameKey, note)) { + result.problems.add( + error("notename entry %s does not match name %s", note, gRef.getName())); + } + + // We trust SHA1 to have no collisions, so no need to check uniqueness of name. + uuidNameBiMap.put(gRef.getUUID(), gRef.getName()); + } + } + } + + /** Check invariants of the group refs with the group name refs. */ + private List checkGlobalConsistency( + Map uuidToGroupMap, + BiMap uuidNameBiMap) { + List problems = new ArrayList<>(); + + // Check consistency between the data coming from different refs. + for (AccountGroup.UUID uuid : uuidToGroupMap.keySet()) { + if (!uuidNameBiMap.containsKey(uuid)) { + problems.add(error("group %s has no entry in name map", uuid)); + continue; + } + + String noteName = uuidNameBiMap.get(uuid); + String groupRefName = uuidToGroupMap.get(uuid).getName(); + if (!Objects.equals(noteName, groupRefName)) { + problems.add( + error( + "inconsistent name for group %s (name map %s vs. group ref %s)", + uuid, noteName, groupRefName)); + } + } + + for (AccountGroup.UUID uuid : uuidNameBiMap.keySet()) { + if (!uuidToGroupMap.containsKey(uuid)) { + problems.add( + error( + "name map has entry (%s, %s), entry missing as group ref", + uuid, uuidNameBiMap.get(uuid))); + } + } + + if (problems.isEmpty()) { + // Check ids. + Map groupById = new HashMap<>(); + for (InternalGroup g : uuidToGroupMap.values()) { + InternalGroup before = groupById.get(g.getId()); + if (before != null) { + problems.add( + error( + "shared group id %s for %s (%s) and %s (%s)", + g.getId(), + before.getName(), + before.getGroupUUID(), + g.getName(), + g.getGroupUUID())); + } + groupById.put(g.getId(), g); + } + } + + return problems; + } +}