diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 54ba802081..6233dbb97e 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -360,8 +360,8 @@ public abstract class AbstractDaemonTest { accountIndexer.index(accountId); } - private void reindexAllGroups() throws OrmException, IOException, ConfigInvalidException { - Iterable allGroups = groups.getAllGroupReferences(db)::iterator; + private void reindexAllGroups() throws IOException, ConfigInvalidException { + Iterable allGroups = groups.getAllGroupReferences()::iterator; for (GroupReference group : allGroups) { groupIndexer.index(group.getUUID()); } diff --git a/java/com/google/gerrit/acceptance/AccountCreator.java b/java/com/google/gerrit/acceptance/AccountCreator.java index dc5e59d1d4..c6e03a8732 100644 --- a/java/com/google/gerrit/acceptance/AccountCreator.java +++ b/java/com/google/gerrit/acceptance/AccountCreator.java @@ -23,7 +23,6 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.ServerInitiated; import com.google.gerrit.server.account.AccountsUpdate; @@ -35,7 +34,6 @@ import com.google.gerrit.server.group.db.GroupsUpdate; import com.google.gerrit.server.group.db.InternalGroupUpdate; import com.google.gerrit.server.ssh.SshKeyCache; import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -57,7 +55,6 @@ import org.eclipse.jgit.errors.ConfigInvalidException; public class AccountCreator { private final Map accounts; - private final SchemaFactory reviewDbProvider; private final Sequences sequences; private final Provider accountsUpdateProvider; private final VersionedAuthorizedKeys.Accessor authorizedKeys; @@ -68,7 +65,6 @@ public class AccountCreator { @Inject AccountCreator( - SchemaFactory schema, Sequences sequences, @ServerInitiated Provider accountsUpdateProvider, VersionedAuthorizedKeys.Accessor authorizedKeys, @@ -77,7 +73,6 @@ public class AccountCreator { SshKeyCache sshKeyCache, @SshEnabled boolean sshEnabled) { accounts = new HashMap<>(); - reviewDbProvider = schema; this.sequences = sequences; this.accountsUpdateProvider = accountsUpdateProvider; this.authorizedKeys = authorizedKeys; @@ -98,51 +93,49 @@ public class AccountCreator { if (account != null) { return account; } - try (ReviewDb db = reviewDbProvider.open()) { - Account.Id id = new Account.Id(sequences.nextAccountId()); + Account.Id id = new Account.Id(sequences.nextAccountId()); - List extIds = new ArrayList<>(2); - String httpPass = null; - if (username != null) { - httpPass = "http-pass"; - extIds.add(ExternalId.createUsername(username, id, httpPass)); - } - - if (email != null) { - extIds.add(ExternalId.createEmail(id, email)); - } - - accountsUpdateProvider - .get() - .insert( - "Create Test Account", - id, - u -> u.setFullName(fullName).setPreferredEmail(email).addExternalIds(extIds)); - - if (groupNames != null) { - for (String n : groupNames) { - AccountGroup.NameKey k = new AccountGroup.NameKey(n); - Optional group = groupCache.get(k); - if (!group.isPresent()) { - throw new NoSuchGroupException(n); - } - addGroupMember(db, group.get().getGroupUUID(), id); - } - } - - KeyPair sshKey = null; - if (sshEnabled && username != null) { - sshKey = genSshKey(); - authorizedKeys.addKey(id, publicKey(sshKey, email)); - sshKeyCache.evict(username); - } - - account = new TestAccount(id, username, email, fullName, sshKey, httpPass); - if (username != null) { - accounts.put(username, account); - } - return account; + List extIds = new ArrayList<>(2); + String httpPass = null; + if (username != null) { + httpPass = "http-pass"; + extIds.add(ExternalId.createUsername(username, id, httpPass)); } + + if (email != null) { + extIds.add(ExternalId.createEmail(id, email)); + } + + accountsUpdateProvider + .get() + .insert( + "Create Test Account", + id, + u -> u.setFullName(fullName).setPreferredEmail(email).addExternalIds(extIds)); + + if (groupNames != null) { + for (String n : groupNames) { + AccountGroup.NameKey k = new AccountGroup.NameKey(n); + Optional group = groupCache.get(k); + if (!group.isPresent()) { + throw new NoSuchGroupException(n); + } + addGroupMember(group.get().getGroupUUID(), id); + } + } + + KeyPair sshKey = null; + if (sshEnabled && username != null) { + sshKey = genSshKey(); + authorizedKeys.addKey(id, publicKey(sshKey, email)); + sshKeyCache.evict(username); + } + + account = new TestAccount(id, username, email, fullName, sshKey, httpPass); + if (username != null) { + accounts.put(username, account); + } + return account; } public TestAccount create(@Nullable String username, String group) throws Exception { @@ -193,12 +186,12 @@ public class AccountCreator { return out.toString(US_ASCII.name()).trim(); } - private void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId) + private void addGroupMember(AccountGroup.UUID groupUuid, Account.Id accountId) throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException { InternalGroupUpdate groupUpdate = InternalGroupUpdate.builder() .setMemberModification(memberIds -> Sets.union(memberIds, ImmutableSet.of(accountId))) .build(); - groupsUpdateProvider.get().updateGroup(db, groupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate); } } diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index 37ef20a768..c0564541a9 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java @@ -36,7 +36,6 @@ import com.google.gerrit.server.util.OneOffRequestContext; import com.google.gerrit.server.util.SocketUtil; import com.google.gerrit.server.util.SystemLog; import com.google.gerrit.testing.FakeEmailSender; -import com.google.gerrit.testing.GroupNoteDbMode; import com.google.gerrit.testing.InMemoryDatabase; import com.google.gerrit.testing.InMemoryRepositoryManager; import com.google.gerrit.testing.NoteDbChecker; @@ -419,7 +418,6 @@ public class GerritServer implements AutoCloseable { cfg.setBoolean("index", null, "reindexAfterRefUpdate", false); NoteDbMode.newNotesMigrationFromEnv().setConfigValues(cfg); - GroupNoteDbMode.get().getGroupsMigration().setConfigValuesIfNotSetYet(cfg); } private static Injector createTestInjector(Daemon daemon) throws Exception { diff --git a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java index 58dfa9445d..83a3874d37 100644 --- a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java +++ b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java @@ -29,7 +29,6 @@ import com.google.gerrit.server.config.TrackingFooters; import com.google.gerrit.server.config.TrackingFootersProvider; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeBundleReader; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.gerrit.server.notedb.GwtormChangeBundleReader; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.schema.DataSourceType; @@ -93,7 +92,6 @@ class InMemoryTestingDatabaseModule extends LifecycleModule { bind(DataSourceType.class).to(InMemoryH2Type.class); install(new NotesMigration.Module()); - install(new GroupsMigration.Module()); TypeLiteral> schemaFactory = new TypeLiteral>() {}; bind(schemaFactory).to(NotesMigrationSchemaFactory.class); diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java index bf1dd35ca3..bac3fd0476 100644 --- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java +++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java @@ -71,7 +71,6 @@ import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier; import com.google.gerrit.server.mail.receive.MailReceiver; import com.google.gerrit.server.mail.send.SmtpEmailSender; import com.google.gerrit.server.mime.MimeUtil2Module; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.patch.DiffExecutorModule; import com.google.gerrit.server.permissions.DefaultPermissionBackendModule; @@ -309,7 +308,6 @@ public class WebAppInitializer extends GuiceServletContextListener implements Fi } modules.add(new DatabaseModule()); modules.add(new NotesMigration.Module()); - modules.add(new GroupsMigration.Module()); modules.add(new DropWizardMetricMaker.ApiModule()); return Guice.createInjector(PRODUCTION, modules); } diff --git a/java/com/google/gerrit/pgm/init/GroupsOnInit.java b/java/com/google/gerrit/pgm/init/GroupsOnInit.java index 4b752e6c0b..1d3e516774 100644 --- a/java/com/google/gerrit/pgm/init/GroupsOnInit.java +++ b/java/com/google/gerrit/pgm/init/GroupsOnInit.java @@ -16,11 +16,8 @@ package com.google.gerrit.pgm.init; import static com.google.common.base.Preconditions.checkArgument; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.common.collect.Sets; -import com.google.common.collect.Streams; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.errors.NoSuchGroupException; @@ -28,8 +25,6 @@ import com.google.gerrit.pgm.init.api.AllUsersNameOnInitProvider; import com.google.gerrit.pgm.init.api.InitFlags; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroupMember; -import com.google.gerrit.reviewdb.client.AccountGroupName; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdentProvider; @@ -41,17 +36,13 @@ import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.db.AuditLogFormatter; import com.google.gerrit.server.group.db.GroupConfig; import com.google.gerrit.server.group.db.GroupNameNotes; -import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.group.db.InternalGroupUpdate; -import com.google.gerrit.server.notedb.GroupsMigration; -import com.google.gwtorm.server.OrmDuplicateKeyException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import java.io.File; import java.io.IOException; import java.nio.file.Path; import java.sql.Timestamp; -import java.util.List; import java.util.stream.Stream; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.internal.storage.file.FileRepository; @@ -74,14 +65,12 @@ public class GroupsOnInit { private final InitFlags flags; private final SitePaths site; private final String allUsers; - private final GroupsMigration groupsMigration; @Inject public GroupsOnInit(InitFlags flags, SitePaths site, AllUsersNameOnInitProvider allUsers) { this.flags = flags; this.site = site; this.allUsers = allUsers.get(); - this.groupsMigration = new GroupsMigration(flags.cfg); } /** @@ -97,15 +86,6 @@ public class GroupsOnInit { */ public InternalGroup getExistingGroup(ReviewDb db, GroupReference groupReference) throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException { - if (groupsMigration.readFromNoteDb()) { - return getExistingGroupFromNoteDb(groupReference); - } - - return getExistingGroupFromReviewDb(db, groupReference); - } - - private InternalGroup getExistingGroupFromNoteDb(GroupReference groupReference) - throws IOException, ConfigInvalidException, NoSuchGroupException { File allUsersRepoPath = getPathToAllUsersRepository(); if (allUsersRepoPath != null) { try (Repository allUsersRepo = new FileRepository(allUsersRepoPath)) { @@ -119,23 +99,6 @@ public class GroupsOnInit { throw new NoSuchGroupException(groupReference.getUUID()); } - private static InternalGroup getExistingGroupFromReviewDb( - ReviewDb db, GroupReference groupReference) throws OrmException, NoSuchGroupException { - String groupName = groupReference.getName(); - AccountGroupName accountGroupName = - db.accountGroupNames().get(new AccountGroup.NameKey(groupName)); - if (accountGroupName == null) { - throw new NoSuchGroupException(groupName); - } - - AccountGroup.Id groupId = accountGroupName.getId(); - AccountGroup group = db.accountGroups().get(groupId); - if (group == null) { - throw new NoSuchGroupException(groupName); - } - return Groups.asInternalGroup(db, group); - } - /** * Returns {@code GroupReference}s for all internal groups. * @@ -147,18 +110,13 @@ public class GroupsOnInit { */ public Stream getAllGroupReferences(ReviewDb db) throws OrmException, IOException, ConfigInvalidException { - if (groupsMigration.readFromNoteDb()) { - File allUsersRepoPath = getPathToAllUsersRepository(); - if (allUsersRepoPath != null) { - try (Repository allUsersRepo = new FileRepository(allUsersRepoPath)) { - return GroupNameNotes.loadAllGroups(allUsersRepo).stream(); - } + File allUsersRepoPath = getPathToAllUsersRepository(); + if (allUsersRepoPath != null) { + try (Repository allUsersRepo = new FileRepository(allUsersRepoPath)) { + return GroupNameNotes.loadAllGroups(allUsersRepo).stream(); } - return Stream.empty(); } - - return Streams.stream(db.accountGroups().all()) - .map(group -> new GroupReference(group.getGroupUUID(), group.getName())); + return Stream.empty(); } /** @@ -176,49 +134,6 @@ public class GroupsOnInit { */ public void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, Account account) throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException { - addGroupMemberInReviewDb(db, groupUuid, account.getId()); - if (!groupsMigration.writeToNoteDb()) { - return; - } - addGroupMemberInNoteDb(groupUuid, account); - } - - private static void addGroupMemberInReviewDb( - ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId) - throws OrmException, NoSuchGroupException { - AccountGroup group = getExistingGroup(db, groupUuid); - AccountGroup.Id groupId = group.getId(); - - if (isMember(db, groupId, accountId)) { - return; - } - - db.accountGroupMembers() - .insert( - ImmutableList.of( - new AccountGroupMember(new AccountGroupMember.Key(accountId, groupId)))); - } - - private static AccountGroup getExistingGroup(ReviewDb db, AccountGroup.UUID groupUuid) - throws OrmException, NoSuchGroupException { - List accountGroups = db.accountGroups().byUUID(groupUuid).toList(); - if (accountGroups.size() == 1) { - return Iterables.getOnlyElement(accountGroups); - } else if (accountGroups.isEmpty()) { - throw new NoSuchGroupException(groupUuid); - } else { - throw new OrmDuplicateKeyException("Duplicate group UUID " + groupUuid); - } - } - - private static boolean isMember(ReviewDb db, AccountGroup.Id groupId, Account.Id accountId) - throws OrmException { - AccountGroupMember.Key key = new AccountGroupMember.Key(accountId, groupId); - return db.accountGroupMembers().get(key) != null; - } - - private void addGroupMemberInNoteDb(AccountGroup.UUID groupUuid, Account account) - throws IOException, ConfigInvalidException, NoSuchGroupException { File allUsersRepoPath = getPathToAllUsersRepository(); if (allUsersRepoPath != null) { try (Repository repository = new FileRepository(allUsersRepoPath)) { diff --git a/java/com/google/gerrit/pgm/util/SiteProgram.java b/java/com/google/gerrit/pgm/util/SiteProgram.java index afabcf6628..b59e085858 100644 --- a/java/com/google/gerrit/pgm/util/SiteProgram.java +++ b/java/com/google/gerrit/pgm/util/SiteProgram.java @@ -28,7 +28,6 @@ import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GerritServerConfigModule; import com.google.gerrit.server.config.SitePath; import com.google.gerrit.server.git.GitRepositoryManagerModule; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.schema.DataSourceModule; import com.google.gerrit.server.schema.DataSourceProvider; @@ -184,7 +183,6 @@ public abstract class SiteProgram extends AbstractProgram { modules.add(new SchemaModule()); modules.add(cfgInjector.getInstance(GitRepositoryManagerModule.class)); modules.add(new NotesMigration.Module()); - modules.add(new GroupsMigration.Module()); try { return Guice.createInjector(PRODUCTION, modules); diff --git a/java/com/google/gerrit/reviewdb/server/DisallowReadFromGroupsReviewDbWrapper.java b/java/com/google/gerrit/reviewdb/server/DisallowReadFromGroupsReviewDbWrapper.java deleted file mode 100644 index 1bfbd37824..0000000000 --- a/java/com/google/gerrit/reviewdb/server/DisallowReadFromGroupsReviewDbWrapper.java +++ /dev/null @@ -1,306 +0,0 @@ -// 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.reviewdb.server; - -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroupById; -import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; -import com.google.gerrit.reviewdb.client.AccountGroupMember; -import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; -import com.google.gerrit.reviewdb.client.AccountGroupName; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.ResultSet; - -public class DisallowReadFromGroupsReviewDbWrapper extends ReviewDbWrapper { - private static final String MSG = "This table has been migrated to NoteDb"; - - private final Groups groups; - private final GroupNames groupNames; - private final GroupMembers groupMembers; - private final GroupMemberAudits groupMemberAudits; - private final ByIds byIds; - private final ByIdAudits byIdAudits; - - public DisallowReadFromGroupsReviewDbWrapper(ReviewDb db) { - super(db); - groups = new Groups(delegate.accountGroups()); - groupNames = new GroupNames(delegate.accountGroupNames()); - groupMembers = new GroupMembers(delegate.accountGroupMembers()); - groupMemberAudits = new GroupMemberAudits(delegate.accountGroupMembersAudit()); - byIds = new ByIds(delegate.accountGroupById()); - byIdAudits = new ByIdAudits(delegate.accountGroupByIdAud()); - } - - @Override - public AccountGroupAccess accountGroups() { - return groups; - } - - @Override - public AccountGroupNameAccess accountGroupNames() { - return groupNames; - } - - @Override - public AccountGroupMemberAccess accountGroupMembers() { - return groupMembers; - } - - @Override - public AccountGroupMemberAuditAccess accountGroupMembersAudit() { - return groupMemberAudits; - } - - @Override - public AccountGroupByIdAccess accountGroupById() { - return byIds; - } - - @Override - public AccountGroupByIdAudAccess accountGroupByIdAud() { - return byIdAudits; - } - - private static class Groups extends AccountGroupAccessWrapper { - protected Groups(AccountGroupAccess delegate) { - super(delegate); - } - - @Override - public ResultSet iterateAllEntities() { - throw new UnsupportedOperationException(MSG); - } - - @SuppressWarnings("deprecation") - @Override - public com.google.common.util.concurrent.CheckedFuture getAsync( - AccountGroup.Id key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet get(Iterable keys) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public AccountGroup get(AccountGroup.Id id) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byUUID(AccountGroup.UUID uuid) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet all() { - throw new UnsupportedOperationException(MSG); - } - } - - private static class GroupNames extends AccountGroupNameAccessWrapper { - protected GroupNames(AccountGroupNameAccess delegate) { - super(delegate); - } - - @Override - public ResultSet iterateAllEntities() { - throw new UnsupportedOperationException(MSG); - } - - @SuppressWarnings("deprecation") - @Override - public com.google.common.util.concurrent.CheckedFuture getAsync( - AccountGroup.NameKey key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet get(Iterable keys) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public AccountGroupName get(AccountGroup.NameKey name) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet all() { - throw new UnsupportedOperationException(MSG); - } - } - - private static class GroupMembers extends AccountGroupMemberAccessWrapper { - protected GroupMembers(AccountGroupMemberAccess delegate) { - super(delegate); - } - - @Override - public ResultSet iterateAllEntities() { - throw new UnsupportedOperationException(MSG); - } - - @SuppressWarnings("deprecation") - @Override - public com.google.common.util.concurrent.CheckedFuture - getAsync(AccountGroupMember.Key key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet get(Iterable keys) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public AccountGroupMember get(AccountGroupMember.Key key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byAccount(Account.Id id) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byGroup(AccountGroup.Id id) { - throw new UnsupportedOperationException(MSG); - } - } - - private static class GroupMemberAudits extends AccountGroupMemberAuditAccessWrapper { - protected GroupMemberAudits(AccountGroupMemberAuditAccess delegate) { - super(delegate); - } - - @Override - public ResultSet iterateAllEntities() { - throw new UnsupportedOperationException(MSG); - } - - @SuppressWarnings("deprecation") - @Override - public com.google.common.util.concurrent.CheckedFuture - getAsync(AccountGroupMemberAudit.Key key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet get(Iterable keys) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public AccountGroupMemberAudit get(AccountGroupMemberAudit.Key key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byGroupAccount( - AccountGroup.Id groupId, Account.Id accountId) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byGroup(AccountGroup.Id groupId) { - throw new UnsupportedOperationException(MSG); - } - } - - private static class ByIds extends AccountGroupByIdAccessWrapper { - protected ByIds(AccountGroupByIdAccess delegate) { - super(delegate); - } - - @Override - public ResultSet iterateAllEntities() { - throw new UnsupportedOperationException(MSG); - } - - @SuppressWarnings("deprecation") - @Override - public com.google.common.util.concurrent.CheckedFuture getAsync( - AccountGroupById.Key key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet get(Iterable keys) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public AccountGroupById get(AccountGroupById.Key key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byIncludeUUID(AccountGroup.UUID uuid) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byGroup(AccountGroup.Id id) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet all() { - throw new UnsupportedOperationException(MSG); - } - } - - private static class ByIdAudits extends AccountGroupByIdAudAccessWrapper { - protected ByIdAudits(AccountGroupByIdAudAccess delegate) { - super(delegate); - } - - @Override - public ResultSet iterateAllEntities() { - throw new UnsupportedOperationException(MSG); - } - - @SuppressWarnings("deprecation") - @Override - public com.google.common.util.concurrent.CheckedFuture - getAsync(AccountGroupByIdAud.Key key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet get(Iterable keys) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public AccountGroupByIdAud get(AccountGroupByIdAud.Key key) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byGroupInclude( - AccountGroup.Id groupId, AccountGroup.UUID incGroupUUID) { - throw new UnsupportedOperationException(MSG); - } - - @Override - public ResultSet byGroup(AccountGroup.Id groupId) { - throw new UnsupportedOperationException(MSG); - } - } -} diff --git a/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java b/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java index ef057ebf9a..aed97784fe 100644 --- a/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java +++ b/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java @@ -53,9 +53,6 @@ public class ReviewDbUtil { if (db instanceof DisallowReadFromChangesReviewDbWrapper) { return unwrapDb(((DisallowReadFromChangesReviewDbWrapper) db).unsafeGetDelegate()); } - if (db instanceof DisallowReadFromGroupsReviewDbWrapper) { - return unwrapDb(((DisallowReadFromGroupsReviewDbWrapper) db).unsafeGetDelegate()); - } return db; } diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java index 79c57484ea..8e00130df9 100644 --- a/java/com/google/gerrit/server/account/AccountManager.java +++ b/java/com/google/gerrit/server/account/AccountManager.java @@ -28,7 +28,6 @@ import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.client.AccountFieldName; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.ServerInitiated; @@ -43,7 +42,6 @@ import com.google.gerrit.server.group.db.InternalGroupUpdate; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.ssh.SshKeyCache; import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -65,7 +63,6 @@ import org.slf4j.LoggerFactory; public class AccountManager { private static final Logger log = LoggerFactory.getLogger(AccountManager.class); - private final SchemaFactory schema; private final Sequences sequences; private final Accounts accounts; private final Provider accountsUpdateProvider; @@ -82,7 +79,6 @@ public class AccountManager { @Inject AccountManager( - SchemaFactory schema, Sequences sequences, @GerritServerConfig Config cfg, Accounts accounts, @@ -95,7 +91,6 @@ public class AccountManager { ExternalIds externalIds, GroupsUpdate.Factory groupsUpdateFactory, SetInactiveFlag setInactiveFlag) { - this.schema = schema; this.sequences = sequences; this.accounts = accounts; this.accountsUpdateProvider = accountsUpdateProvider; @@ -140,51 +135,49 @@ public class AccountManager { throw e; } try { - try (ReviewDb db = schema.open()) { - ExternalId id = externalIds.get(who.getExternalIdKey()); - if (id == null) { - if (who.getUserName().isPresent()) { - ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, who.getUserName().get()); - ExternalId existingId = externalIds.get(key); - if (existingId != null) { - // An inconsistency is detected in the database, having a record for scheme "username:" - // but no record for scheme "gerrit:". Try to recover by linking - // "gerrit:" identity to the existing account. - log.warn( - "User {} already has an account; link new identity to the existing account.", - who.getUserName()); - return link(existingId.accountId(), who); - } + ExternalId id = externalIds.get(who.getExternalIdKey()); + if (id == null) { + if (who.getUserName().isPresent()) { + ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, who.getUserName().get()); + ExternalId existingId = externalIds.get(key); + if (existingId != null) { + // An inconsistency is detected in the database, having a record for scheme "username:" + // but no record for scheme "gerrit:". Try to recover by linking + // "gerrit:" identity to the existing account. + log.warn( + "User {} already has an account; link new identity to the existing account.", + who.getUserName()); + return link(existingId.accountId(), who); } - // New account, automatically create and return. - log.info("External ID not found. Attempting to create new account."); - return create(db, who); } - - Optional accountState = byIdCache.get(id.accountId()); - if (!accountState.isPresent()) { - log.error( - String.format( - "Authentication with external ID %s failed. Account %s doesn't exist.", - id.key().get(), id.accountId().get())); - throw new AccountException("Authentication error, account not found"); - } - - // Account exists - Optional act = updateAccountActiveStatus(who, accountState.get().getAccount()); - if (!act.isPresent()) { - // The account was deleted since we checked for it last time. This should never happen - // since we don't support deletion of accounts. - throw new AccountException("Authentication error, account not found"); - } - if (!act.get().isActive()) { - throw new AccountException("Authentication error, account inactive"); - } - - // return the identity to the caller. - update(who, id); - return new AuthResult(id.accountId(), who.getExternalIdKey(), false); + // New account, automatically create and return. + log.info("External ID not found. Attempting to create new account."); + return create(who); } + + Optional accountState = byIdCache.get(id.accountId()); + if (!accountState.isPresent()) { + log.error( + String.format( + "Authentication with external ID %s failed. Account %s doesn't exist.", + id.key().get(), id.accountId().get())); + throw new AccountException("Authentication error, account not found"); + } + + // Account exists + Optional act = updateAccountActiveStatus(who, accountState.get().getAccount()); + if (!act.isPresent()) { + // The account was deleted since we checked for it last time. This should never happen + // since we don't support deletion of accounts. + throw new AccountException("Authentication error, account not found"); + } + if (!act.get().isActive()) { + throw new AccountException("Authentication error, account inactive"); + } + + // return the identity to the caller. + update(who, id); + return new AuthResult(id.accountId(), who.getExternalIdKey(), false); } catch (OrmException | ConfigInvalidException e) { throw new AccountException("Authentication error", e); } @@ -289,7 +282,7 @@ public class AccountManager { } } - private AuthResult create(ReviewDb db, AuthRequest who) + private AuthResult create(AuthRequest who) throws OrmException, AccountException, IOException, ConfigInvalidException { Account.Id newId = new Account.Id(sequences.nextAccountId()); @@ -349,7 +342,7 @@ public class AccountManager { .getPermission(GlobalCapability.ADMINISTRATE_SERVER); AccountGroup.UUID adminGroupUuid = admin.getRules().get(0).getGroup().getUUID(); - addGroupMember(db, adminGroupUuid, user); + addGroupMember(adminGroupUuid, user); } realm.onCreateAccount(who, accountState.getAccount()); @@ -369,7 +362,7 @@ public class AccountManager { return ExternalId.create(SCHEME_USERNAME, username, accountId); } - private void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, IdentifiedUser user) + private void addGroupMember(AccountGroup.UUID groupUuid, IdentifiedUser user) throws OrmException, IOException, ConfigInvalidException, AccountException { // The user initiated this request by logging in. -> Attribute all modifications to that user. GroupsUpdate groupsUpdate = groupsUpdateFactory.create(user); @@ -379,7 +372,7 @@ public class AccountManager { memberIds -> Sets.union(memberIds, ImmutableSet.of(user.getAccountId()))) .build(); try { - groupsUpdate.updateGroup(db, groupUuid, groupUpdate); + groupsUpdate.updateGroup(groupUuid, groupUpdate); } catch (NoSuchGroupException e) { throw new AccountException(String.format("Group %s not found", groupUuid)); } diff --git a/java/com/google/gerrit/server/account/GroupCacheImpl.java b/java/com/google/gerrit/server/account/GroupCacheImpl.java index 58eaadc7ad..a20aab7d04 100644 --- a/java/com/google/gerrit/server/account/GroupCacheImpl.java +++ b/java/com/google/gerrit/server/account/GroupCacheImpl.java @@ -17,12 +17,10 @@ package com.google.gerrit.server.account; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.query.group.InternalGroupQuery; -import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Module; import com.google.inject.Provider; @@ -166,20 +164,16 @@ public class GroupCacheImpl implements GroupCache { } static class ByUUIDLoader extends CacheLoader> { - private final SchemaFactory schema; private final Groups groups; @Inject - ByUUIDLoader(SchemaFactory sf, Groups groups) { - schema = sf; + ByUUIDLoader(Groups groups) { this.groups = groups; } @Override public Optional load(String uuid) throws Exception { - try (ReviewDb db = schema.open()) { - return groups.getGroup(db, new AccountGroup.UUID(uuid)); - } + return groups.getGroup(new AccountGroup.UUID(uuid)); } } } diff --git a/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java b/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java index d8472a6643..ba81c6a39b 100644 --- a/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java +++ b/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java @@ -23,13 +23,11 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.query.group.InternalGroupQuery; import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Module; import com.google.inject.Provider; @@ -180,20 +178,16 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache { } static class AllExternalLoader extends CacheLoader> { - private final SchemaFactory schema; private final Groups groups; @Inject - AllExternalLoader(SchemaFactory sf, Groups groups) { - schema = sf; + AllExternalLoader(Groups groups) { this.groups = groups; } @Override public ImmutableList load(String key) throws Exception { - try (ReviewDb db = schema.open()) { - return groups.getExternalGroups(db).collect(toImmutableList()); - } + return groups.getExternalGroups().collect(toImmutableList()); } } } diff --git a/java/com/google/gerrit/server/account/InternalGroupBackend.java b/java/com/google/gerrit/server/account/InternalGroupBackend.java index 4547807b73..ea6eb87443 100644 --- a/java/com/google/gerrit/server/account/InternalGroupBackend.java +++ b/java/com/google/gerrit/server/account/InternalGroupBackend.java @@ -20,15 +20,12 @@ import com.google.common.collect.ImmutableList; import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.InternalGroupDescription; import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.group.db.GroupsNoteDbConsistencyChecker; import com.google.gerrit.server.project.ProjectState; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -43,7 +40,6 @@ public class InternalGroupBackend implements GroupBackend { private final GroupControl.Factory groupControlFactory; private final GroupCache groupCache; private final Groups groups; - private final SchemaFactory schema; private final IncludingGroupMembership.Factory groupMembershipFactory; @Inject @@ -51,12 +47,10 @@ public class InternalGroupBackend implements GroupBackend { GroupControl.Factory groupControlFactory, GroupCache groupCache, Groups groups, - SchemaFactory schema, IncludingGroupMembership.Factory groupMembershipFactory) { this.groupControlFactory = groupControlFactory; this.groupCache = groupCache; this.groups = groups; - this.schema = schema; this.groupMembershipFactory = groupMembershipFactory; } @@ -77,13 +71,13 @@ public class InternalGroupBackend implements GroupBackend { @Override public Collection suggest(String name, ProjectState project) { - try (ReviewDb db = schema.open()) { + try { return groups - .getAllGroupReferences(db) + .getAllGroupReferences() .filter(group -> startsWithIgnoreCase(group, name)) .filter(this::isVisible) .collect(toList()); - } catch (OrmException | IOException | ConfigInvalidException e) { + } catch (IOException | ConfigInvalidException e) { return ImmutableList.of(); } } diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index 9536d55815..d84065204a 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -155,7 +155,6 @@ import com.google.gerrit.server.mail.send.ReplacePatchSetSender; import com.google.gerrit.server.mail.send.SetAssigneeSender; import com.google.gerrit.server.mime.FileTypeRegistry; import com.google.gerrit.server.mime.MimeUtilFileTypeRegistry; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.gerrit.server.notedb.NoteDbModule; import com.google.gerrit.server.patch.PatchListCacheImpl; import com.google.gerrit.server.patch.PatchScriptFactory; @@ -204,14 +203,11 @@ import org.eclipse.jgit.transport.PreUploadHook; public class GerritGlobalModule extends FactoryModule { private final Config cfg; private final AuthModule authModule; - private final GroupsMigration groupsMigration; @Inject - GerritGlobalModule( - @GerritServerConfig Config cfg, AuthModule authModule, GroupsMigration groupsMigration) { + GerritGlobalModule(@GerritServerConfig Config cfg, AuthModule authModule) { this.cfg = cfg; this.authModule = authModule; - this.groupsMigration = groupsMigration; } @Override @@ -307,7 +303,6 @@ public class GerritGlobalModule extends FactoryModule { install(new com.google.gerrit.server.restapi.access.Module()); install(new ConfigRestModule()); install(new com.google.gerrit.server.restapi.change.Module()); - install(new com.google.gerrit.server.group.Module(groupsMigration)); install(new com.google.gerrit.server.restapi.account.Module()); install(new com.google.gerrit.server.restapi.project.Module()); install(new com.google.gerrit.server.restapi.group.Module()); diff --git a/java/com/google/gerrit/server/group/DbGroupAuditListener.java b/java/com/google/gerrit/server/group/DbGroupAuditListener.java deleted file mode 100644 index 8b533486b1..0000000000 --- a/java/com/google/gerrit/server/group/DbGroupAuditListener.java +++ /dev/null @@ -1,265 +0,0 @@ -// Copyright (C) 2014 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; - -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.gerrit.reviewdb.server.ReviewDbUtil.unwrapDb; - -import com.google.common.base.Joiner; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; -import com.google.gerrit.reviewdb.client.AccountGroupMember; -import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; -import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.account.AccountCache; -import com.google.gerrit.server.account.AccountState; -import com.google.gerrit.server.account.GroupCache; -import com.google.gerrit.server.account.UniversalGroupBackend; -import com.google.gerrit.server.audit.group.GroupAuditEvent; -import com.google.gerrit.server.audit.group.GroupAuditListener; -import com.google.gerrit.server.audit.group.GroupMemberAuditEvent; -import com.google.gerrit.server.audit.group.GroupSubgroupAuditEvent; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.ResultSet; -import com.google.gwtorm.server.SchemaFactory; -import com.google.inject.Inject; -import java.sql.Timestamp; -import java.text.MessageFormat; -import java.util.ArrayList; -import java.util.List; -import java.util.Optional; -import org.slf4j.Logger; - -class DbGroupAuditListener implements GroupAuditListener { - private static final Logger log = org.slf4j.LoggerFactory.getLogger(DbGroupAuditListener.class); - - private final SchemaFactory schema; - private final AccountCache accountCache; - private final GroupCache groupCache; - private final UniversalGroupBackend groupBackend; - - @Inject - DbGroupAuditListener( - SchemaFactory schema, - AccountCache accountCache, - GroupCache groupCache, - UniversalGroupBackend groupBackend) { - this.schema = schema; - this.accountCache = accountCache; - this.groupCache = groupCache; - this.groupBackend = groupBackend; - } - - @Override - public void onAddMembers(GroupMemberAuditEvent event) { - Optional updatedGroup = groupCache.get(event.getUpdatedGroup()); - if (!updatedGroup.isPresent()) { - logFailToLoadUpdatedGroup(event); - return; - } - - InternalGroup group = updatedGroup.get(); - try (ReviewDb db = unwrapDb(schema.open())) { - db.accountGroupMembersAudit().insert(toAccountGroupMemberAudits(event, group.getId())); - } catch (OrmException e) { - logOrmException( - "Cannot log add accounts to group event performed by user", event, group.getName(), e); - } - } - - @Override - public void onDeleteMembers(GroupMemberAuditEvent event) { - Optional updatedGroup = groupCache.get(event.getUpdatedGroup()); - if (!updatedGroup.isPresent()) { - logFailToLoadUpdatedGroup(event); - return; - } - - InternalGroup group = updatedGroup.get(); - List auditInserts = new ArrayList<>(); - List auditUpdates = new ArrayList<>(); - try (ReviewDb db = unwrapDb(schema.open())) { - for (Account.Id accountId : event.getModifiedMembers()) { - AccountGroupMemberAudit audit = null; - ResultSet audits = - db.accountGroupMembersAudit().byGroupAccount(group.getId(), accountId); - for (AccountGroupMemberAudit a : audits) { - if (a.isActive()) { - audit = a; - break; - } - } - - if (audit != null) { - audit.removed(event.getActor(), event.getTimestamp()); - auditUpdates.add(audit); - continue; - } - AccountGroupMember.Key key = new AccountGroupMember.Key(accountId, group.getId()); - audit = - new AccountGroupMemberAudit( - new AccountGroupMember(key), event.getActor(), event.getTimestamp()); - audit.removedLegacy(); - auditInserts.add(audit); - } - db.accountGroupMembersAudit().update(auditUpdates); - db.accountGroupMembersAudit().insert(auditInserts); - } catch (OrmException e) { - logOrmException( - "Cannot log delete accounts from group event performed by user", - event, - group.getName(), - e); - } - } - - @Override - public void onAddSubgroups(GroupSubgroupAuditEvent event) { - Optional updatedGroup = groupCache.get(event.getUpdatedGroup()); - if (!updatedGroup.isPresent()) { - logFailToLoadUpdatedGroup(event); - return; - } - - InternalGroup group = updatedGroup.get(); - try (ReviewDb db = unwrapDb(schema.open())) { - db.accountGroupByIdAud().insert(toAccountGroupByIdAudits(event, group.getId())); - } catch (OrmException e) { - logOrmException( - "Cannot log add groups to group event performed by user", event, group.getName(), e); - } - } - - @Override - public void onDeleteSubgroups(GroupSubgroupAuditEvent event) { - Optional updatedGroup = groupCache.get(event.getUpdatedGroup()); - if (!updatedGroup.isPresent()) { - logFailToLoadUpdatedGroup(event); - return; - } - - InternalGroup group = updatedGroup.get(); - List auditUpdates = new ArrayList<>(); - try (ReviewDb db = unwrapDb(schema.open())) { - for (AccountGroup.UUID uuid : event.getModifiedSubgroups()) { - AccountGroupByIdAud audit = null; - ResultSet audits = - db.accountGroupByIdAud().byGroupInclude(updatedGroup.get().getId(), uuid); - for (AccountGroupByIdAud a : audits) { - if (a.isActive()) { - audit = a; - break; - } - } - - if (audit != null) { - audit.removed(event.getActor(), event.getTimestamp()); - auditUpdates.add(audit); - } - } - db.accountGroupByIdAud().update(auditUpdates); - } catch (OrmException e) { - logOrmException( - "Cannot log delete groups from group event performed by user", event, group.getName(), e); - } - } - - private void logFailToLoadUpdatedGroup(GroupAuditEvent event) { - ImmutableList descriptions = createEventDescriptions(event, "(fail to load group)"); - String message = - createErrorMessage("Fail to load the updated group", event.getActor(), descriptions); - log.error(message); - } - - private void logOrmException( - String header, GroupAuditEvent event, String updatedGroupName, OrmException e) { - ImmutableList descriptions = createEventDescriptions(event, updatedGroupName); - String message = createErrorMessage(header, event.getActor(), descriptions); - log.error(message, e); - } - - private ImmutableList createEventDescriptions( - GroupAuditEvent event, String updatedGroupName) { - ImmutableList.Builder builder = ImmutableList.builder(); - if (event instanceof GroupMemberAuditEvent) { - GroupMemberAuditEvent memberAuditEvent = (GroupMemberAuditEvent) event; - for (Account.Id accountId : memberAuditEvent.getModifiedMembers()) { - String userName = getUserName(accountId).orElse(""); - builder.add( - MessageFormat.format( - "account {0}/{1}, group {2}/{3}", - accountId, userName, event.getUpdatedGroup(), updatedGroupName)); - } - } else if (event instanceof GroupSubgroupAuditEvent) { - GroupSubgroupAuditEvent subgroupAuditEvent = (GroupSubgroupAuditEvent) event; - for (AccountGroup.UUID groupUuid : subgroupAuditEvent.getModifiedSubgroups()) { - String groupName = groupBackend.get(groupUuid).getName(); - builder.add( - MessageFormat.format( - "group {0}/{1}, group {2}/{3}", - groupUuid, groupName, subgroupAuditEvent.getUpdatedGroup(), updatedGroupName)); - } - } - - return builder.build(); - } - - private String createErrorMessage( - String header, Account.Id me, ImmutableList descriptions) { - StringBuilder message = new StringBuilder(header); - message.append(" "); - message.append(me); - message.append("/"); - message.append(getUserName(me).orElse(null)); - message.append(": "); - message.append(Joiner.on("; ").join(descriptions)); - return message.toString(); - } - - private Optional getUserName(Account.Id accountId) { - return accountCache.get(accountId).map(AccountState::getUserName).orElse(Optional.empty()); - } - - private static ImmutableSet toAccountGroupMemberAudits( - GroupMemberAuditEvent event, AccountGroup.Id updatedGroupId) { - Timestamp timestamp = event.getTimestamp(); - Account.Id actor = event.getActor(); - return event - .getModifiedMembers() - .stream() - .map( - member -> - new AccountGroupMemberAudit( - new AccountGroupMemberAudit.Key(member, updatedGroupId, timestamp), actor)) - .collect(toImmutableSet()); - } - - private static ImmutableSet toAccountGroupByIdAudits( - GroupSubgroupAuditEvent event, AccountGroup.Id updatedGroupId) { - Timestamp timestamp = event.getTimestamp(); - Account.Id actor = event.getActor(); - return event - .getModifiedSubgroups() - .stream() - .map( - subgroup -> - new AccountGroupByIdAud( - new AccountGroupByIdAud.Key(updatedGroupId, subgroup, timestamp), actor)) - .collect(toImmutableSet()); - } -} diff --git a/java/com/google/gerrit/server/group/Module.java b/java/com/google/gerrit/server/group/Module.java deleted file mode 100644 index a1d8378bbc..0000000000 --- a/java/com/google/gerrit/server/group/Module.java +++ /dev/null @@ -1,24 +0,0 @@ -package com.google.gerrit.server.group; - -import com.google.gerrit.extensions.config.FactoryModule; -import com.google.gerrit.extensions.registration.DynamicSet; -import com.google.gerrit.server.audit.group.GroupAuditListener; -import com.google.gerrit.server.notedb.GroupsMigration; - -public class Module extends FactoryModule { - private final GroupsMigration groupsMigration; - - public Module(GroupsMigration groupsMigration) { - this.groupsMigration = groupsMigration; - } - - @Override - protected void configure() { - if (!groupsMigration.disableGroupReviewDb()) { - // DbGroupAuditListener is used solely for the ReviewDb audit log. It does not respect - // ReviewDb wrappers that disable reads. Hence, we don't want to bind it if ReviewDb is - // disabled. - DynamicSet.bind(binder(), GroupAuditListener.class).to(DbGroupAuditListener.class); - } - } -} diff --git a/java/com/google/gerrit/server/group/SystemGroupBackend.java b/java/com/google/gerrit/server/group/SystemGroupBackend.java index 91cc11c957..ebbb7a1e37 100644 --- a/java/com/google/gerrit/server/group/SystemGroupBackend.java +++ b/java/com/google/gerrit/server/group/SystemGroupBackend.java @@ -24,7 +24,6 @@ import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.StartupCheck; import com.google.gerrit.server.StartupException; @@ -34,8 +33,6 @@ import com.google.gerrit.server.account.ListGroupMembership; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.project.ProjectState; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -191,13 +188,11 @@ public class SystemGroupBackend extends AbstractGroupBackend { public static class NameCheck implements StartupCheck { private final Config cfg; private final Groups groups; - private final SchemaFactory schema; @Inject - NameCheck(@GerritServerConfig Config cfg, Groups groups, SchemaFactory schema) { + NameCheck(@GerritServerConfig Config cfg, Groups groups) { this.cfg = cfg; this.groups = groups; - this.schema = schema; } @Override @@ -216,14 +211,14 @@ public class SystemGroupBackend extends AbstractGroupBackend { } Optional conflictingGroup; - try (ReviewDb db = schema.open()) { + try { conflictingGroup = groups - .getAllGroupReferences(db) + .getAllGroupReferences() .filter(group -> hasConfiguredName(byLowerCaseConfiguredName, group)) .findAny(); - } catch (OrmException | IOException | ConfigInvalidException ignored) { + } catch (IOException | ConfigInvalidException ignored) { return; } diff --git a/java/com/google/gerrit/server/group/db/Groups.java b/java/com/google/gerrit/server/group/db/Groups.java index bbdc8d2bf6..46fa998513 100644 --- a/java/com/google/gerrit/server/group/db/Groups.java +++ b/java/com/google/gerrit/server/group/db/Groups.java @@ -14,28 +14,15 @@ package com.google.gerrit.server.group.db; -import static com.google.common.collect.ImmutableSet.toImmutableSet; - import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; -import com.google.common.collect.Streams; import com.google.gerrit.common.data.GroupReference; -import com.google.gerrit.common.errors.NoSuchGroupException; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroupById; import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; -import com.google.gerrit.reviewdb.client.AccountGroupMember; import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.group.InternalGroup; -import com.google.gerrit.server.notedb.GroupsMigration; -import com.google.gwtorm.server.OrmDuplicateKeyException; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -48,10 +35,10 @@ import org.eclipse.jgit.lib.Repository; /** * A database accessor for read calls related to groups. * - *

All calls which read group related details from the database (either ReviewDb or NoteDb) are - * gathered here. Other classes should always use this class instead of accessing the database - * directly. There are a few exceptions though: schema classes, wrapper classes, and classes - * executed during init. The latter ones should use {@code GroupsOnInit} instead. + *

All calls which read group related details from the database are gathered here. Other classes + * should always use this class instead of accessing the database directly. There are a few + * exceptions though: schema classes, wrapper classes, and classes executed during init. The latter + * ones should use {@code GroupsOnInit} instead. * *

Most callers should not need to read groups directly from the database; they should use the * {@link com.google.gerrit.server.account.GroupCache GroupCache} instead. @@ -60,18 +47,13 @@ import org.eclipse.jgit.lib.Repository; */ @Singleton public class Groups { - private final GroupsMigration groupsMigration; private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; private final AuditLogReader auditLogReader; @Inject public Groups( - GroupsMigration groupsMigration, - GitRepositoryManager repoManager, - AllUsersName allUsersName, - AuditLogReader auditLogReader) { - this.groupsMigration = groupsMigration; + GitRepositoryManager repoManager, AllUsersName allUsersName, AuditLogReader auditLogReader) { this.repoManager = repoManager; this.allUsersName = allUsersName; this.auditLogReader = auditLogReader; @@ -80,27 +62,16 @@ public class Groups { /** * Returns the {@code InternalGroup} for the specified UUID if it exists. * - * @param db the {@code ReviewDb} instance to use for lookups * @param groupUuid the UUID of the group * @return the found {@code InternalGroup} if it exists, or else an empty {@code Optional} - * @throws OrmDuplicateKeyException if multiple groups are found for the specified UUID - * @throws OrmException if the group couldn't be retrieved from ReviewDb * @throws IOException if the group couldn't be retrieved from NoteDb * @throws ConfigInvalidException if the group couldn't be retrieved from NoteDb */ - public Optional getGroup(ReviewDb db, AccountGroup.UUID groupUuid) - throws OrmException, IOException, ConfigInvalidException { - if (groupsMigration.readFromNoteDb()) { - try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { - return getGroupFromNoteDb(allUsersRepo, groupUuid); - } + public Optional getGroup(AccountGroup.UUID groupUuid) + throws IOException, ConfigInvalidException { + try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { + return getGroupFromNoteDb(allUsersRepo, groupUuid); } - - Optional accountGroup = getGroupFromReviewDb(db, groupUuid); - if (!accountGroup.isPresent()) { - return Optional.empty(); - } - return Optional.of(asInternalGroup(db, accountGroup.get())); } private static Optional getGroupFromNoteDb( @@ -116,130 +87,31 @@ public class Groups { return loadedGroup; } - public static InternalGroup asInternalGroup(ReviewDb db, AccountGroup accountGroup) - throws OrmException { - ImmutableSet members = - getMembersFromReviewDb(db, accountGroup.getId()).collect(toImmutableSet()); - ImmutableSet subgroups = - getSubgroupsFromReviewDb(db, accountGroup.getId()).collect(toImmutableSet()); - return InternalGroup.create(accountGroup, members, subgroups); - } - - /** - * Returns the {@code AccountGroup} for the specified UUID. - * - * @param db the {@code ReviewDb} instance to use for lookups - * @param groupUuid the UUID of the group - * @return the {@code AccountGroup} which has the specified UUID - * @throws OrmDuplicateKeyException if multiple groups are found for the specified UUID - * @throws OrmException if the group couldn't be retrieved from ReviewDb - * @throws NoSuchGroupException if a group with such a UUID doesn't exist - */ - static AccountGroup getExistingGroupFromReviewDb(ReviewDb db, AccountGroup.UUID groupUuid) - throws OrmException, NoSuchGroupException { - Optional group = getGroupFromReviewDb(db, groupUuid); - return group.orElseThrow(() -> new NoSuchGroupException(groupUuid)); - } - - /** - * Returns the {@code AccountGroup} for the specified UUID if it exists. - * - * @param db the {@code ReviewDb} instance to use for lookups - * @param groupUuid the UUID of the group - * @return the found {@code AccountGroup} if it exists, or else an empty {@code Optional} - * @throws OrmDuplicateKeyException if multiple groups are found for the specified UUID - * @throws OrmException if the group couldn't be retrieved from ReviewDb - */ - private static Optional getGroupFromReviewDb( - ReviewDb db, AccountGroup.UUID groupUuid) throws OrmException { - List accountGroups = db.accountGroups().byUUID(groupUuid).toList(); - if (accountGroups.size() == 1) { - return Optional.of(Iterables.getOnlyElement(accountGroups)); - } else if (accountGroups.isEmpty()) { - return Optional.empty(); - } else { - throw new OrmDuplicateKeyException("Duplicate group UUID " + groupUuid); - } - } - /** * Returns {@code GroupReference}s for all internal groups. * - * @param db the {@code ReviewDb} instance to use for lookups * @return a stream of the {@code GroupReference}s of all internal groups - * @throws OrmException if an error occurs while reading from ReviewDb * @throws IOException if an error occurs while reading from NoteDb * @throws ConfigInvalidException if the data in NoteDb is in an incorrect format */ - public Stream getAllGroupReferences(ReviewDb db) - throws OrmException, IOException, ConfigInvalidException { - if (groupsMigration.readFromNoteDb()) { - try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { - return GroupNameNotes.loadAllGroups(allUsersRepo).stream(); - } + public Stream getAllGroupReferences() throws IOException, ConfigInvalidException { + try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { + return GroupNameNotes.loadAllGroups(allUsersRepo).stream(); } - - return Streams.stream(db.accountGroups().all()) - .map(group -> new GroupReference(group.getGroupUUID(), group.getName())); - } - - /** - * Returns the members (accounts) of a group. - * - *

Note: This method doesn't check whether the accounts exist! - * - * @param db the {@code ReviewDb} instance to use for lookups - * @param groupId the ID of the group - * @return a stream of the IDs of the members - * @throws OrmException if an error occurs while reading from ReviewDb - */ - static Stream getMembersFromReviewDb(ReviewDb db, AccountGroup.Id groupId) - throws OrmException { - ResultSet accountGroupMembers = db.accountGroupMembers().byGroup(groupId); - return Streams.stream(accountGroupMembers).map(AccountGroupMember::getAccountId); - } - - /** - * Returns the subgroups of a group. - * - *

This parent group must be an internal group whereas the subgroups can either be internal or - * external groups. - * - *

Note: This method doesn't check whether the subgroups exist! - * - * @param db the {@code ReviewDb} instance to use for lookups - * @param groupId the ID of the group - * @return a stream of the UUIDs of the subgroups - * @throws OrmException if an error occurs while reading from ReviewDb - */ - static Stream getSubgroupsFromReviewDb(ReviewDb db, AccountGroup.Id groupId) - throws OrmException { - ResultSet accountGroupByIds = db.accountGroupById().byGroup(groupId); - return Streams.stream(accountGroupByIds).map(AccountGroupById::getIncludeUUID).distinct(); } /** * Returns all known external groups. External groups are 'known' when they are specified as a * subgroup of an internal group. * - * @param db the {@code ReviewDb} instance to use for lookups * @return a stream of the UUIDs of the known external groups - * @throws OrmException if an error occurs while reading from ReviewDb * @throws IOException if an error occurs while reading from NoteDb * @throws ConfigInvalidException if the data in NoteDb is in an incorrect format */ - public Stream getExternalGroups(ReviewDb db) - throws OrmException, IOException, ConfigInvalidException { - if (groupsMigration.readFromNoteDb()) { - try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { - return getExternalGroupsFromNoteDb(allUsersRepo); - } + public Stream getExternalGroups() throws IOException, ConfigInvalidException { + try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { + return getExternalGroupsFromNoteDb(allUsersRepo); } - - return Streams.stream(db.accountGroupById().all()) - .map(AccountGroupById::getIncludeUUID) - .distinct() - .filter(groupUuid -> !AccountGroup.isInternalGroup(groupUuid)); } private static Stream getExternalGroupsFromNoteDb(Repository allUsersRepo) @@ -259,50 +131,28 @@ public class Groups { /** * Returns the membership audit records for a given group. * - * @param db the {@code ReviewDb} instance to use for lookups * @param repo All-Users repository. * @param groupUuid the UUID of the group * @return the audit records, in arbitrary order; empty if the group does not exist - * @throws OrmException if an error occurs while reading from ReviewDb * @throws IOException if an error occurs while reading from NoteDb * @throws ConfigInvalidException if the group couldn't be retrieved from NoteDb */ - public List getMembersAudit( - ReviewDb db, Repository repo, AccountGroup.UUID groupUuid) - throws OrmException, IOException, ConfigInvalidException { - if (groupsMigration.readFromNoteDb()) { - return auditLogReader.getMembersAudit(repo, groupUuid); - } - Optional group = getGroupFromReviewDb(db, groupUuid); - if (!group.isPresent()) { - return ImmutableList.of(); - } - - return db.accountGroupMembersAudit().byGroup(group.get().getId()).toList(); + public List getMembersAudit(Repository repo, AccountGroup.UUID groupUuid) + throws IOException, ConfigInvalidException { + return auditLogReader.getMembersAudit(repo, groupUuid); } /** * Returns the subgroup audit records for a given group. * - * @param db the {@code ReviewDb} instance to use for lookups * @param repo All-Users repository. * @param groupUuid the UUID of the group * @return the audit records, in arbitrary order; empty if the group does not exist - * @throws OrmException if an error occurs while reading from ReviewDb * @throws IOException if an error occurs while reading from NoteDb * @throws ConfigInvalidException if the group couldn't be retrieved from NoteDb */ - public List getSubgroupsAudit( - ReviewDb db, Repository repo, AccountGroup.UUID groupUuid) - throws OrmException, IOException, ConfigInvalidException { - if (groupsMigration.readFromNoteDb()) { - return auditLogReader.getSubgroupsAudit(repo, groupUuid); - } - Optional group = getGroupFromReviewDb(db, groupUuid); - if (!group.isPresent()) { - return ImmutableList.of(); - } - - return db.accountGroupByIdAud().byGroup(group.get().getId()).toList(); + public List getSubgroupsAudit(Repository repo, AccountGroup.UUID groupUuid) + throws IOException, ConfigInvalidException { + return auditLogReader.getSubgroupsAudit(repo, groupUuid); } } diff --git a/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java b/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java index 2335d3233f..9b862212cc 100644 --- a/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java +++ b/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java @@ -26,7 +26,6 @@ import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.config.AllUsersName; 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.util.ArrayList; import java.util.HashSet; @@ -52,7 +51,6 @@ public class GroupsConsistencyChecker { private final Accounts accounts; private final GitRepositoryManager repoManager; private final GroupsNoteDbConsistencyChecker globalChecker; - private final GroupsMigration groupsMigration; @Inject GroupsConsistencyChecker( @@ -60,22 +58,16 @@ public class GroupsConsistencyChecker { GroupBackend groupBackend, Accounts accounts, GitRepositoryManager repositoryManager, - GroupsNoteDbConsistencyChecker globalChecker, - GroupsMigration groupsMigration) { + GroupsNoteDbConsistencyChecker globalChecker) { this.allUsersName = allUsersName; this.groupBackend = groupBackend; this.accounts = accounts; this.repoManager = repositoryManager; this.globalChecker = globalChecker; - this.groupsMigration = groupsMigration; } /** Checks that all internal group references exist, and that no groups have cycles. */ public List check() throws IOException { - if (!groupsMigration.writeToNoteDb()) { - return new ArrayList<>(); - } - try (Repository repo = repoManager.openRepository(allUsersName)) { GroupsNoteDbConsistencyChecker.Result result = globalChecker.check(repo); if (!result.problems.isEmpty()) { diff --git a/java/com/google/gerrit/server/group/db/GroupsUpdate.java b/java/com/google/gerrit/server/group/db/GroupsUpdate.java index f10409e401..1fad1fbd2c 100644 --- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java +++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java @@ -14,14 +14,10 @@ package com.google.gerrit.server.group.db; -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.gerrit.server.group.db.Groups.getExistingGroupFromReviewDb; - import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.common.base.Throwables; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.gerrit.common.Nullable; @@ -29,12 +25,7 @@ import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroupById; -import com.google.gerrit.reviewdb.client.AccountGroupMember; -import com.google.gerrit.reviewdb.client.AccountGroupName; import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; @@ -51,11 +42,9 @@ import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.RenameGroupOp; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.index.group.GroupIndexer; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.gerrit.server.update.RefUpdateUtil; import com.google.gerrit.server.update.RetryHelper; import com.google.gwtorm.server.OrmDuplicateKeyException; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; @@ -75,10 +64,10 @@ import org.eclipse.jgit.lib.Repository; /** * A database accessor for write calls related to groups. * - *

All calls which write group related details to the database (either ReviewDb or NoteDb) are - * gathered here. Other classes should always use this class instead of accessing the database - * directly. There are a few exceptions though: schema classes, wrapper classes, and classes - * executed during init. The latter ones should use {@code GroupsOnInit} instead. + *

All calls which write group related details to the database are gathered here. Other classes + * should always use this class instead of accessing the database directly. There are a few + * exceptions though: schema classes, wrapper classes, and classes executed during init. The latter + * ones should use {@code GroupsOnInit} instead. * *

If not explicitly stated, all methods of this class refer to internal groups. */ @@ -109,7 +98,6 @@ public class GroupsUpdate { private final AuditLogFormatter auditLogFormatter; private final PersonIdent authorIdent; private final MetaDataUpdateFactory metaDataUpdateFactory; - private final GroupsMigration groupsMigration; private final GitReferenceUpdated gitRefUpdated; private final RetryHelper retryHelper; @@ -127,7 +115,6 @@ public class GroupsUpdate { @GerritServerId String serverId, @GerritPersonIdent PersonIdent serverIdent, MetaDataUpdate.InternalFactory metaDataUpdateInternalFactory, - GroupsMigration groupsMigration, GitReferenceUpdated gitRefUpdated, RetryHelper retryHelper, @Assisted @Nullable IdentifiedUser currentUser) { @@ -138,7 +125,6 @@ public class GroupsUpdate { this.indexer = indexer; this.auditService = auditService; this.renameGroupOpFactory = renameGroupOpFactory; - this.groupsMigration = groupsMigration; this.gitRefUpdated = gitRefUpdated; this.retryHelper = retryHelper; this.currentUser = currentUser; @@ -184,37 +170,18 @@ public class GroupsUpdate { /** * Creates the specified group for the specified members (accounts). * - * @param db the {@code ReviewDb} instance to update * @param groupCreation an {@code InternalGroupCreation} which specifies all mandatory properties * of the group * @param groupUpdate an {@code InternalGroupUpdate} which specifies optional properties of the * group. If this {@code InternalGroupUpdate} updates a property which was already specified * by the {@code InternalGroupCreation}, the value of this {@code InternalGroupUpdate} wins. - * @throws OrmException if an error occurs while reading/writing from/to ReviewDb * @throws OrmDuplicateKeyException if a group with the chosen name already exists * @throws IOException if indexing fails, or an error occurs while reading/writing from/to NoteDb * @return the created {@code InternalGroup} */ public InternalGroup createGroup( - ReviewDb db, InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) - throws OrmException, IOException, ConfigInvalidException { - if (!groupsMigration.disableGroupReviewDb()) { - if (!groupUpdate.getUpdatedOn().isPresent()) { - // Set updatedOn to a specific value so that the same timestamp is used for ReviewDb and - // NoteDb. - groupUpdate = groupUpdate.toBuilder().setUpdatedOn(TimeUtil.nowTs()).build(); - } - - InternalGroup createdGroupInReviewDb = - createGroupInReviewDb(ReviewDbUtil.unwrapDb(db), groupCreation, groupUpdate); - - if (!groupsMigration.writeToNoteDb()) { - updateCachesOnGroupCreation(createdGroupInReviewDb); - dispatchAuditEventsOnGroupCreation(createdGroupInReviewDb); - return createdGroupInReviewDb; - } - } - + InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) + throws OrmDuplicateKeyException, IOException, ConfigInvalidException { InternalGroup createdGroup = createGroupInNoteDbWithRetry(groupCreation, groupUpdate); updateCachesOnGroupCreation(createdGroup); dispatchAuditEventsOnGroupCreation(createdGroup); @@ -224,67 +191,32 @@ public class GroupsUpdate { /** * Updates the specified group. * - * @param db the {@code ReviewDb} instance to update * @param groupUuid the UUID of the group to update * @param groupUpdate an {@code InternalGroupUpdate} which indicates the desired updates on the * group - * @throws OrmException if an error occurs while reading/writing from/to ReviewDb - * @throws com.google.gwtorm.server.OrmDuplicateKeyException if the new name of the group is used - * by another group + * @throws OrmDuplicateKeyException if the new name of the group is used by another group * @throws IOException if indexing fails, or an error occurs while reading/writing from/to NoteDb * @throws NoSuchGroupException if the specified group doesn't exist */ - public void updateGroup(ReviewDb db, AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) - throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException { + public void updateGroup(AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) + throws OrmDuplicateKeyException, IOException, NoSuchGroupException, ConfigInvalidException { Optional updatedOn = groupUpdate.getUpdatedOn(); if (!updatedOn.isPresent()) { - // Set updatedOn to a specific value so that the same timestamp is used for ReviewDb and - // NoteDb. This timestamp is also used by audit events. updatedOn = Optional.of(TimeUtil.nowTs()); groupUpdate = groupUpdate.toBuilder().setUpdatedOn(updatedOn.get()).build(); } - UpdateResult result = updateGroupInDb(db, groupUuid, groupUpdate); + UpdateResult result = updateGroupInDb(groupUuid, groupUpdate); updateCachesOnGroupUpdate(result); dispatchAuditEventsOnGroupUpdate(result, updatedOn.get()); } @VisibleForTesting - public UpdateResult updateGroupInDb( - ReviewDb db, AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) - throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException { - UpdateResult reviewDbUpdateResult = null; - if (!groupsMigration.disableGroupReviewDb()) { - AccountGroup group = getExistingGroupFromReviewDb(ReviewDbUtil.unwrapDb(db), groupUuid); - reviewDbUpdateResult = updateGroupInReviewDb(ReviewDbUtil.unwrapDb(db), group, groupUpdate); - - if (!groupsMigration.writeToNoteDb()) { - return reviewDbUpdateResult; - } - } - + public UpdateResult updateGroupInDb(AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) + throws OrmDuplicateKeyException, NoSuchGroupException, IOException, ConfigInvalidException { Optional noteDbUpdateResult = updateGroupInNoteDbWithRetry(groupUuid, groupUpdate); - return noteDbUpdateResult.orElse(reviewDbUpdateResult); - } - - private InternalGroup createGroupInReviewDb( - ReviewDb db, InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) - throws OrmException { - - AccountGroupName gn = new AccountGroupName(groupCreation.getNameKey(), groupCreation.getId()); - // first insert the group name to validate that the group name hasn't - // already been used to create another group - db.accountGroupNames().insert(ImmutableList.of(gn)); - - Timestamp createdOn = groupUpdate.getUpdatedOn().orElseGet(TimeUtil::nowTs); - AccountGroup group = createAccountGroup(groupCreation, createdOn); - UpdateResult updateResult = updateGroupInReviewDb(db, group, groupUpdate); - return InternalGroup.create( - group, - updateResult.getAddedMembers(), - updateResult.getAddedSubgroups(), - updateResult.getRefState()); + return noteDbUpdateResult.orElse(null); } public static AccountGroup createAccountGroup( @@ -308,157 +240,9 @@ public class GroupsUpdate { groupUpdate.getVisibleToAll().ifPresent(group::setVisibleToAll); } - private UpdateResult updateGroupInReviewDb( - ReviewDb db, AccountGroup group, InternalGroupUpdate groupUpdate) throws OrmException { - AccountGroup.NameKey originalName = group.getNameKey(); - applyUpdate(group, groupUpdate); - AccountGroup.NameKey updatedName = group.getNameKey(); - - // The name must be inserted first so that we stop early for already used names. - updateNameInReviewDb(db, group.getId(), originalName, updatedName); - db.accountGroups().upsert(ImmutableList.of(group)); - - ImmutableSet originalMembers = - Groups.getMembersFromReviewDb(db, group.getId()).collect(toImmutableSet()); - ImmutableSet updatedMembers = - ImmutableSet.copyOf(groupUpdate.getMemberModification().apply(originalMembers)); - ImmutableSet originalSubgroups = - Groups.getSubgroupsFromReviewDb(db, group.getId()).collect(toImmutableSet()); - ImmutableSet updatedSubgroups = - ImmutableSet.copyOf(groupUpdate.getSubgroupModification().apply(originalSubgroups)); - - Set addedMembers = - addGroupMembersInReviewDb(db, group.getId(), originalMembers, updatedMembers); - Set deletedMembers = - deleteGroupMembersInReviewDb(db, group.getId(), originalMembers, updatedMembers); - Set addedSubgroups = - addSubgroupsInReviewDb(db, group.getId(), originalSubgroups, updatedSubgroups); - Set deletedSubgroups = - deleteSubgroupsInReviewDb(db, group.getId(), originalSubgroups, updatedSubgroups); - - UpdateResult.Builder resultBuilder = - UpdateResult.builder() - .setGroupUuid(group.getGroupUUID()) - .setGroupId(group.getId()) - .setGroupName(group.getNameKey()) - .setAddedMembers(addedMembers) - .setDeletedMembers(deletedMembers) - .setAddedSubgroups(addedSubgroups) - .setDeletedSubgroups(deletedSubgroups); - if (!Objects.equals(originalName, updatedName)) { - resultBuilder.setPreviousGroupName(originalName); - } - return resultBuilder.build(); - } - - private static void updateNameInReviewDb( - ReviewDb db, - AccountGroup.Id groupId, - AccountGroup.NameKey originalName, - AccountGroup.NameKey updatedName) - throws OrmException { - try { - AccountGroupName id = new AccountGroupName(updatedName, groupId); - db.accountGroupNames().insert(ImmutableList.of(id)); - } catch (OrmException e) { - AccountGroupName other = db.accountGroupNames().get(updatedName); - if (other != null) { - // If we are using this identity, don't report the exception. - if (other.getId().equals(groupId)) { - return; - } - } - throw e; - } - db.accountGroupNames().deleteKeys(ImmutableList.of(originalName)); - } - - private static Set addGroupMembersInReviewDb( - ReviewDb db, - AccountGroup.Id groupId, - ImmutableSet originalMembers, - ImmutableSet updatedMembers) - throws OrmException { - Set accountIds = Sets.difference(updatedMembers, originalMembers); - if (accountIds.isEmpty()) { - return accountIds; - } - - ImmutableSet newMembers = toAccountGroupMembers(groupId, accountIds); - db.accountGroupMembers().insert(newMembers); - return accountIds; - } - - private static Set deleteGroupMembersInReviewDb( - ReviewDb db, - AccountGroup.Id groupId, - ImmutableSet originalMembers, - ImmutableSet updatedMembers) - throws OrmException { - Set accountIds = Sets.difference(originalMembers, updatedMembers); - if (accountIds.isEmpty()) { - return accountIds; - } - - ImmutableSet membersToRemove = toAccountGroupMembers(groupId, accountIds); - db.accountGroupMembers().delete(membersToRemove); - return accountIds; - } - - private static ImmutableSet toAccountGroupMembers( - AccountGroup.Id groupId, Set accountIds) { - return accountIds - .stream() - .map(accountId -> new AccountGroupMember.Key(accountId, groupId)) - .map(AccountGroupMember::new) - .collect(toImmutableSet()); - } - - private static Set addSubgroupsInReviewDb( - ReviewDb db, - AccountGroup.Id parentGroupId, - ImmutableSet originalSubgroups, - ImmutableSet updatedSubgroups) - throws OrmException { - Set subgroupUuids = Sets.difference(updatedSubgroups, originalSubgroups); - if (subgroupUuids.isEmpty()) { - return subgroupUuids; - } - - ImmutableSet newSubgroups = toAccountGroupByIds(parentGroupId, subgroupUuids); - db.accountGroupById().insert(newSubgroups); - return subgroupUuids; - } - - private static Set deleteSubgroupsInReviewDb( - ReviewDb db, - AccountGroup.Id parentGroupId, - ImmutableSet originalSubgroups, - ImmutableSet updatedSubgroups) - throws OrmException { - Set subgroupUuids = Sets.difference(originalSubgroups, updatedSubgroups); - if (subgroupUuids.isEmpty()) { - return subgroupUuids; - } - - ImmutableSet subgroupsToRemove = - toAccountGroupByIds(parentGroupId, subgroupUuids); - db.accountGroupById().delete(subgroupsToRemove); - return subgroupUuids; - } - - private static ImmutableSet toAccountGroupByIds( - AccountGroup.Id parentGroupId, Set subgroupUuids) { - return subgroupUuids - .stream() - .map(subgroupUuid -> new AccountGroupById.Key(parentGroupId, subgroupUuid)) - .map(AccountGroupById::new) - .collect(toImmutableSet()); - } - private InternalGroup createGroupInNoteDbWithRetry( InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) - throws IOException, ConfigInvalidException, OrmException { + throws IOException, ConfigInvalidException, OrmDuplicateKeyException { try { return retryHelper.execute( RetryHelper.ActionType.GROUP_UPDATE, @@ -514,14 +298,11 @@ public class GroupsUpdate { private Optional updateGroupInNoteDb( AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) - throws IOException, ConfigInvalidException, OrmDuplicateKeyException, NoSuchGroupException { + throws IOException, ConfigInvalidException, OrmDuplicateKeyException { try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { GroupConfig groupConfig = GroupConfig.loadForGroup(allUsersRepo, groupUuid); groupConfig.setGroupUpdate(groupUpdate, auditLogFormatter); if (!groupConfig.getLoadedGroup().isPresent()) { - if (groupsMigration.readFromNoteDb()) { - throw new NoSuchGroupException(groupUuid); - } return Optional.empty(); } diff --git a/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java b/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java index 47dad947c1..c90bece1b4 100644 --- a/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java +++ b/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java @@ -24,14 +24,11 @@ import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.index.SiteIndexer; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.group.db.GroupsNoteDbConsistencyChecker; import com.google.gerrit.server.index.IndexExecutor; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -52,18 +49,15 @@ import org.slf4j.LoggerFactory; public class AllGroupsIndexer extends SiteIndexer { private static final Logger log = LoggerFactory.getLogger(AllGroupsIndexer.class); - private final SchemaFactory schemaFactory; private final ListeningExecutorService executor; private final GroupCache groupCache; private final Groups groups; @Inject AllGroupsIndexer( - SchemaFactory schemaFactory, @IndexExecutor(BATCH) ListeningExecutorService executor, GroupCache groupCache, Groups groups) { - this.schemaFactory = schemaFactory; this.executor = executor; this.groupCache = groupCache; this.groups = groups; @@ -77,7 +71,7 @@ public class AllGroupsIndexer extends SiteIndexer uuids; try { uuids = collectGroups(progress); - } catch (OrmException | IOException | ConfigInvalidException e) { + } catch (IOException | ConfigInvalidException e) { log.error("Error collecting groups", e); return new SiteIndexer.Result(sw, false, 0, 0); } @@ -133,13 +127,10 @@ public class AllGroupsIndexer extends SiteIndexer collectGroups(ProgressMonitor progress) - throws OrmException, IOException, ConfigInvalidException { + throws IOException, ConfigInvalidException { progress.beginTask("Collecting groups", ProgressMonitor.UNKNOWN); - try (ReviewDb db = schemaFactory.open()) { - return groups - .getAllGroupReferences(db) - .map(GroupReference::getUUID) - .collect(toImmutableList()); + try { + return groups.getAllGroupReferences().map(GroupReference::getUUID).collect(toImmutableList()); } finally { progress.endTask(); } diff --git a/java/com/google/gerrit/server/index/group/StalenessChecker.java b/java/com/google/gerrit/server/index/group/StalenessChecker.java index 94e1be718d..7900287f4b 100644 --- a/java/com/google/gerrit/server/index/group/StalenessChecker.java +++ b/java/com/google/gerrit/server/index/group/StalenessChecker.java @@ -21,7 +21,6 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -47,27 +46,20 @@ public class StalenessChecker { private final GitRepositoryManager repoManager; private final IndexConfig indexConfig; private final AllUsersName allUsers; - private final GroupsMigration groupsMigration; @Inject StalenessChecker( GroupIndexCollection indexes, GitRepositoryManager repoManager, IndexConfig indexConfig, - AllUsersName allUsers, - GroupsMigration groupsMigration) { + AllUsersName allUsers) { this.indexes = indexes; this.repoManager = repoManager; this.indexConfig = indexConfig; this.allUsers = allUsers; - this.groupsMigration = groupsMigration; } public boolean isStale(AccountGroup.UUID uuid) throws IOException { - if (!groupsMigration.readFromNoteDb()) { - return false; // This class only treats staleness for groups in NoteDb. - } - GroupIndex i = indexes.getSearchIndex(); if (i == null) { return false; // No index; caller couldn't do anything if it is stale. diff --git a/java/com/google/gerrit/server/notedb/GroupsMigration.java b/java/com/google/gerrit/server/notedb/GroupsMigration.java deleted file mode 100644 index 923ab9e689..0000000000 --- a/java/com/google/gerrit/server/notedb/GroupsMigration.java +++ /dev/null @@ -1,79 +0,0 @@ -// 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.notedb; - -import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS; -import static com.google.gerrit.server.notedb.NotesMigration.DISABLE_REVIEW_DB; -import static com.google.gerrit.server.notedb.NotesMigration.READ; -import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB; -import static com.google.gerrit.server.notedb.NotesMigration.WRITE; - -import com.google.gerrit.server.config.GerritServerConfig; -import com.google.inject.AbstractModule; -import com.google.inject.Inject; -import com.google.inject.Singleton; -import java.util.Set; -import org.eclipse.jgit.lib.Config; - -@Singleton -public class GroupsMigration { - public static class Module extends AbstractModule { - @Override - public void configure() { - bind(GroupsMigration.class); - } - } - - private final boolean writeToNoteDb; - private final boolean readFromNoteDb; - private final boolean disableGroupReviewDb; - - @Inject - public GroupsMigration(@GerritServerConfig Config cfg) { - // TODO(aliceks): Remove these flags and the ReviewDb code for groups. - this( - true, - cfg.getBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, true), - cfg.getBoolean(SECTION_NOTE_DB, GROUPS.key(), DISABLE_REVIEW_DB, true)); - } - - public GroupsMigration( - boolean writeToNoteDb, boolean readFromNoteDb, boolean disableGroupReviewDb) { - this.writeToNoteDb = writeToNoteDb; - this.readFromNoteDb = readFromNoteDb; - this.disableGroupReviewDb = disableGroupReviewDb; - } - - public boolean writeToNoteDb() { - return writeToNoteDb; - } - - public boolean readFromNoteDb() { - return readFromNoteDb; - } - - public boolean disableGroupReviewDb() { - return disableGroupReviewDb; - } - - public void setConfigValuesIfNotSetYet(Config cfg) { - Set subsections = cfg.getSubsections(SECTION_NOTE_DB); - if (!subsections.contains(GROUPS.key())) { - cfg.setBoolean(SECTION_NOTE_DB, GROUPS.key(), WRITE, writeToNoteDb()); - cfg.setBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, readFromNoteDb()); - cfg.setBoolean(SECTION_NOTE_DB, GROUPS.key(), DISABLE_REVIEW_DB, disableGroupReviewDb()); - } - } -} diff --git a/java/com/google/gerrit/server/restapi/account/CreateAccount.java b/java/com/google/gerrit/server/restapi/account/CreateAccount.java index 31bf93f13f..404b3d3462 100644 --- a/java/com/google/gerrit/server/restapi/account/CreateAccount.java +++ b/java/com/google/gerrit/server/restapi/account/CreateAccount.java @@ -36,7 +36,6 @@ import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.UserInitiated; import com.google.gerrit.server.account.AccountExternalIdCreator; @@ -67,7 +66,6 @@ public class CreateAccount implements RestModifyView groupsUpdate, OutgoingEmailValidator validator, @Assisted String username) { - this.db = db; this.seq = seq; this.groupsCollection = groupsCollection; this.authorizedKeys = authorizedKeys; @@ -202,6 +198,6 @@ public class CreateAccount implements RestModifyView Sets.union(memberIds, ImmutableSet.of(accountId))) .build(); - groupsUpdate.get().updateGroup(db, groupUuid, groupUpdate); + groupsUpdate.get().updateGroup(groupUuid, groupUpdate); } } diff --git a/java/com/google/gerrit/server/restapi/group/AddMembers.java b/java/com/google/gerrit/server/restapi/group/AddMembers.java index f65b29f7ef..9ddafe3191 100644 --- a/java/com/google/gerrit/server/restapi/group/AddMembers.java +++ b/java/com/google/gerrit/server/restapi/group/AddMembers.java @@ -29,7 +29,6 @@ import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.UserInitiated; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountException; @@ -92,7 +91,6 @@ public class AddMembers implements RestModifyView { private final AccountResolver accountResolver; private final AccountCache accountCache; private final AccountLoader.Factory infoFactory; - private final Provider db; private final Provider groupsUpdateProvider; @Inject @@ -103,7 +101,6 @@ public class AddMembers implements RestModifyView { AccountResolver accountResolver, AccountCache accountCache, AccountLoader.Factory infoFactory, - Provider db, @UserInitiated Provider groupsUpdateProvider) { this.accountManager = accountManager; this.authType = authConfig.getAuthType(); @@ -111,7 +108,6 @@ public class AddMembers implements RestModifyView { this.accountResolver = accountResolver; this.accountCache = accountCache; this.infoFactory = infoFactory; - this.db = db; this.groupsUpdateProvider = groupsUpdateProvider; } @@ -186,7 +182,7 @@ public class AddMembers implements RestModifyView { InternalGroupUpdate.builder() .setMemberModification(memberIds -> Sets.union(memberIds, newMemberIds)) .build(); - groupsUpdateProvider.get().updateGroup(db.get(), groupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate); } private Optional createAccountByLdap(String user) throws IOException { diff --git a/java/com/google/gerrit/server/restapi/group/AddSubgroups.java b/java/com/google/gerrit/server/restapi/group/AddSubgroups.java index e29bb7c053..e11f3896b9 100644 --- a/java/com/google/gerrit/server/restapi/group/AddSubgroups.java +++ b/java/com/google/gerrit/server/restapi/group/AddSubgroups.java @@ -28,7 +28,6 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.UserInitiated; import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.group.GroupResource; @@ -75,18 +74,15 @@ public class AddSubgroups implements RestModifyView { } private final GroupsCollection groupsCollection; - private final Provider db; private final Provider groupsUpdateProvider; private final GroupJson json; @Inject public AddSubgroups( GroupsCollection groupsCollection, - Provider db, @UserInitiated Provider groupsUpdateProvider, GroupJson json) { this.groupsCollection = groupsCollection; - this.db = db; this.groupsUpdateProvider = groupsUpdateProvider; this.json = json; } @@ -128,7 +124,7 @@ public class AddSubgroups implements RestModifyView { InternalGroupUpdate.builder() .setSubgroupModification(subgroupUuids -> Sets.union(subgroupUuids, newSubgroupUuids)) .build(); - groupsUpdateProvider.get().updateGroup(db.get(), parentGroupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroup(parentGroupUuid, groupUpdate); } static class PutSubgroup implements RestModifyView { diff --git a/java/com/google/gerrit/server/restapi/group/CreateGroup.java b/java/com/google/gerrit/server/restapi/group/CreateGroup.java index a0c88f2eb4..79f9688699 100644 --- a/java/com/google/gerrit/server/restapi/group/CreateGroup.java +++ b/java/com/google/gerrit/server/restapi/group/CreateGroup.java @@ -34,7 +34,6 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.Sequences; @@ -75,7 +74,6 @@ public class CreateGroup implements RestModifyView private final Provider self; private final PersonIdent serverIdent; - private final ReviewDb db; private final Provider groupsUpdateProvider; private final GroupCache groupCache; private final GroupsCollection groups; @@ -91,7 +89,6 @@ public class CreateGroup implements RestModifyView CreateGroup( Provider self, @GerritPersonIdent PersonIdent serverIdent, - ReviewDb db, @UserInitiated Provider groupsUpdateProvider, GroupCache groupCache, GroupsCollection groups, @@ -104,7 +101,6 @@ public class CreateGroup implements RestModifyView Sequences sequences) { this.self = self; this.serverIdent = serverIdent; - this.db = db; this.groupsUpdateProvider = groupsUpdateProvider; this.groupCache = groupCache; this.groups = groups; @@ -222,7 +218,7 @@ public class CreateGroup implements RestModifyView groupUpdateBuilder.setMemberModification( members -> ImmutableSet.copyOf(createGroupArgs.initialMembers)); try { - return groupsUpdateProvider.get().createGroup(db, groupCreation, groupUpdateBuilder.build()); + return groupsUpdateProvider.get().createGroup(groupCreation, groupUpdateBuilder.build()); } catch (OrmDuplicateKeyException e) { throw new ResourceConflictException( "group '" + createGroupArgs.getGroupName() + "' already exists"); diff --git a/java/com/google/gerrit/server/restapi/group/DeleteMembers.java b/java/com/google/gerrit/server/restapi/group/DeleteMembers.java index d92c5217e8..3685469d54 100644 --- a/java/com/google/gerrit/server/restapi/group/DeleteMembers.java +++ b/java/com/google/gerrit/server/restapi/group/DeleteMembers.java @@ -25,7 +25,6 @@ import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.UserInitiated; import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.group.GroupResource; @@ -46,16 +45,12 @@ import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class DeleteMembers implements RestModifyView { private final AccountsCollection accounts; - private final Provider db; private final Provider groupsUpdateProvider; @Inject DeleteMembers( - AccountsCollection accounts, - Provider db, - @UserInitiated Provider groupsUpdateProvider) { + AccountsCollection accounts, @UserInitiated Provider groupsUpdateProvider) { this.accounts = accounts; - this.db = db; this.groupsUpdateProvider = groupsUpdateProvider; } @@ -93,7 +88,7 @@ public class DeleteMembers implements RestModifyView { InternalGroupUpdate.builder() .setMemberModification(memberIds -> Sets.difference(memberIds, accountIds)) .build(); - groupsUpdateProvider.get().updateGroup(db.get(), groupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate); } @Singleton diff --git a/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java b/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java index 6a5ac5d41e..0eba8c7a25 100644 --- a/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java +++ b/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java @@ -25,7 +25,6 @@ import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.UserInitiated; import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.group.GroupResource; @@ -45,16 +44,13 @@ import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class DeleteSubgroups implements RestModifyView { private final GroupsCollection groupsCollection; - private final Provider db; private final Provider groupsUpdateProvider; @Inject DeleteSubgroups( GroupsCollection groupsCollection, - Provider db, @UserInitiated Provider groupsUpdateProvider) { this.groupsCollection = groupsCollection; - this.db = db; this.groupsUpdateProvider = groupsUpdateProvider; } @@ -96,7 +92,7 @@ public class DeleteSubgroups implements RestModifyView { .setSubgroupModification( subgroupUuids -> Sets.difference(subgroupUuids, removedSubgroupUuids)) .build(); - groupsUpdateProvider.get().updateGroup(db.get(), parentGroupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroup(parentGroupUuid, groupUpdate); } @Singleton diff --git a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java index 51fffbbcad..eb66a3790a 100644 --- a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java +++ b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java @@ -26,7 +26,6 @@ import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupCache; @@ -38,7 +37,6 @@ import com.google.gerrit.server.group.InternalGroupDescription; import com.google.gerrit.server.group.db.Groups; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; -import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.util.ArrayList; @@ -50,7 +48,6 @@ import org.eclipse.jgit.lib.Repository; @Singleton public class GetAuditLog implements RestReadView { - private final Provider db; private final AccountLoader.Factory accountLoaderFactory; private final AllUsersName allUsers; private final GroupCache groupCache; @@ -61,7 +58,6 @@ public class GetAuditLog implements RestReadView { @Inject public GetAuditLog( - Provider db, AccountLoader.Factory accountLoaderFactory, AllUsersName allUsers, GroupCache groupCache, @@ -69,7 +65,6 @@ public class GetAuditLog implements RestReadView { GroupBackend groupBackend, Groups groups, GitRepositoryManager repoManager) { - this.db = db; this.accountLoaderFactory = accountLoaderFactory; this.allUsers = allUsers; this.groupCache = groupCache; @@ -95,7 +90,7 @@ public class GetAuditLog implements RestReadView { try (Repository allUsersRepo = repoManager.openRepository(allUsers)) { for (AccountGroupMemberAudit auditEvent : - groups.getMembersAudit(db.get(), allUsersRepo, group.getGroupUUID())) { + groups.getMembersAudit(allUsersRepo, group.getGroupUUID())) { AccountInfo member = accountLoader.get(auditEvent.getMemberId()); auditEvents.add( @@ -110,7 +105,7 @@ public class GetAuditLog implements RestReadView { } for (AccountGroupByIdAud auditEvent : - groups.getSubgroupsAudit(db.get(), allUsersRepo, group.getGroupUUID())) { + groups.getSubgroupsAudit(allUsersRepo, group.getGroupUUID())) { AccountGroup.UUID includedGroupUUID = auditEvent.getIncludeUUID(); Optional includedGroup = groupCache.get(includedGroupUUID); GroupInfo member; diff --git a/java/com/google/gerrit/server/restapi/group/ListGroups.java b/java/com/google/gerrit/server/restapi/group/ListGroups.java index 91aee6dd92..0dbd7b6b6e 100644 --- a/java/com/google/gerrit/server/restapi/group/ListGroups.java +++ b/java/com/google/gerrit/server/restapi/group/ListGroups.java @@ -33,7 +33,6 @@ import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountResource; @@ -83,7 +82,6 @@ public class ListGroups implements RestReadView { private final GroupBackend groupBackend; private final Groups groups; private final GroupsCollection groupsCollection; - private final Provider db; private EnumSet options = EnumSet.noneOf(ListGroupsOption.class); private boolean visibleToAll; @@ -232,8 +230,7 @@ public class ListGroups implements RestReadView { final GroupsCollection groupsCollection, GroupJson json, GroupBackend groupBackend, - Groups groups, - Provider db) { + Groups groups) { this.groupCache = groupCache; this.groupControlFactory = groupControlFactory; this.genericGroupControlFactory = genericGroupControlFactory; @@ -244,7 +241,6 @@ public class ListGroups implements RestReadView { this.groupBackend = groupBackend; this.groups = groups; this.groupsCollection = groupsCollection; - this.db = db; } public void setOptions(EnumSet options) { @@ -316,8 +312,7 @@ public class ListGroups implements RestReadView { return groupInfos; } - private Stream getAllExistingGroups() - throws OrmException, IOException, ConfigInvalidException { + private Stream getAllExistingGroups() throws IOException, ConfigInvalidException { if (!projects.isEmpty()) { return projects .stream() @@ -325,7 +320,7 @@ public class ListGroups implements RestReadView { .flatMap(Collection::stream) .distinct(); } - return groups.getAllGroupReferences(db.get()); + return groups.getAllGroupReferences(); } private List suggestGroups() throws OrmException, BadRequestException { @@ -388,7 +383,7 @@ public class ListGroups implements RestReadView { Pattern pattern = getRegexPattern(); Stream foundGroups = groups - .getAllGroupReferences(db.get()) + .getAllGroupReferences() .filter(group -> isRelevant(pattern, group)) .map(this::loadGroup) .flatMap(Streams::stream) diff --git a/java/com/google/gerrit/server/restapi/group/PutDescription.java b/java/com/google/gerrit/server/restapi/group/PutDescription.java index 6a68c7a6d8..d407f69d17 100644 --- a/java/com/google/gerrit/server/restapi/group/PutDescription.java +++ b/java/com/google/gerrit/server/restapi/group/PutDescription.java @@ -23,7 +23,6 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.UserInitiated; import com.google.gerrit.server.group.GroupResource; import com.google.gerrit.server.group.db.GroupsUpdate; @@ -38,13 +37,10 @@ import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PutDescription implements RestModifyView { - private final Provider db; private final Provider groupsUpdateProvider; @Inject - PutDescription( - Provider db, @UserInitiated Provider groupsUpdateProvider) { - this.db = db; + PutDescription(@UserInitiated Provider groupsUpdateProvider) { this.groupsUpdateProvider = groupsUpdateProvider; } @@ -69,7 +65,7 @@ public class PutDescription implements RestModifyView { - private final Provider db; private final Provider groupsUpdateProvider; @Inject - PutName(Provider db, @UserInitiated Provider groupsUpdateProvider) { - this.db = db; + PutName(@UserInitiated Provider groupsUpdateProvider) { this.groupsUpdateProvider = groupsUpdateProvider; } @Override public String apply(GroupResource rsrc, NameInput input) throws NotInternalGroupException, AuthException, BadRequestException, - ResourceConflictException, ResourceNotFoundException, OrmException, IOException, + ResourceConflictException, ResourceNotFoundException, IOException, ConfigInvalidException { GroupDescription.Internal internalGroup = rsrc.asInternalGroup().orElseThrow(NotInternalGroupException::new); @@ -74,13 +70,13 @@ public class PutName implements RestModifyView { } private void renameGroup(GroupDescription.Internal group, String newName) - throws ResourceConflictException, ResourceNotFoundException, OrmException, IOException, + throws ResourceConflictException, ResourceNotFoundException, IOException, ConfigInvalidException { AccountGroup.UUID groupUuid = group.getGroupUUID(); InternalGroupUpdate groupUpdate = InternalGroupUpdate.builder().setName(new AccountGroup.NameKey(newName)).build(); try { - groupsUpdateProvider.get().updateGroup(db.get(), groupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate); } catch (NoSuchGroupException e) { throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); } catch (OrmDuplicateKeyException e) { diff --git a/java/com/google/gerrit/server/restapi/group/PutOptions.java b/java/com/google/gerrit/server/restapi/group/PutOptions.java index ab2ae1a2a4..29b87d2015 100644 --- a/java/com/google/gerrit/server/restapi/group/PutOptions.java +++ b/java/com/google/gerrit/server/restapi/group/PutOptions.java @@ -22,7 +22,6 @@ import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.UserInitiated; import com.google.gerrit.server.group.GroupResource; import com.google.gerrit.server.group.db.GroupsUpdate; @@ -36,12 +35,10 @@ import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class PutOptions implements RestModifyView { - private final Provider db; private final Provider groupsUpdateProvider; @Inject - PutOptions(Provider db, @UserInitiated Provider groupsUpdateProvider) { - this.db = db; + PutOptions(@UserInitiated Provider groupsUpdateProvider) { this.groupsUpdateProvider = groupsUpdateProvider; } @@ -67,7 +64,7 @@ public class PutOptions implements RestModifyView { private final GroupsCollection groupsCollection; private final Provider groupsUpdateProvider; - private final Provider db; private final GroupJson json; @Inject PutOwner( GroupsCollection groupsCollection, @UserInitiated Provider groupsUpdateProvider, - Provider db, GroupJson json) { this.groupsCollection = groupsCollection; this.groupsUpdateProvider = groupsUpdateProvider; - this.db = db; this.json = json; } @@ -77,7 +73,7 @@ public class PutOwner implements RestModifyView { InternalGroupUpdate groupUpdate = InternalGroupUpdate.builder().setOwnerGroupUUID(owner.getGroupUUID()).build(); try { - groupsUpdateProvider.get().updateGroup(db.get(), groupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate); } catch (NoSuchGroupException e) { throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid)); } diff --git a/java/com/google/gerrit/server/schema/NoGroupsReviewDbWrapper.java b/java/com/google/gerrit/server/schema/NoGroupsReviewDbWrapper.java deleted file mode 100644 index 33c4d770a4..0000000000 --- a/java/com/google/gerrit/server/schema/NoGroupsReviewDbWrapper.java +++ /dev/null @@ -1,202 +0,0 @@ -// 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.schema; - -import com.google.common.collect.ImmutableList; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroupById; -import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; -import com.google.gerrit.reviewdb.client.AccountGroupMember; -import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; -import com.google.gerrit.reviewdb.client.AccountGroupName; -import com.google.gerrit.reviewdb.server.AccountGroupAccess; -import com.google.gerrit.reviewdb.server.AccountGroupByIdAccess; -import com.google.gerrit.reviewdb.server.AccountGroupByIdAudAccess; -import com.google.gerrit.reviewdb.server.AccountGroupMemberAccess; -import com.google.gerrit.reviewdb.server.AccountGroupMemberAuditAccess; -import com.google.gerrit.reviewdb.server.AccountGroupNameAccess; -import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.reviewdb.server.ReviewDbWrapper; -import com.google.gwtorm.server.ListResultSet; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.ResultSet; - -/** - * Wrapper for ReviewDb that never calls the underlying groups tables. - * - *

See {@link NotesMigrationSchemaFactory} for discussion. - */ -public class NoGroupsReviewDbWrapper extends ReviewDbWrapper { - private static ResultSet empty() { - return new ListResultSet<>(ImmutableList.of()); - } - - private final AccountGroupAccess groups; - private final AccountGroupNameAccess groupNames; - private final AccountGroupMemberAccess members; - private final AccountGroupMemberAuditAccess memberAudits; - private final AccountGroupByIdAccess byIds; - private final AccountGroupByIdAudAccess byIdAudits; - - protected NoGroupsReviewDbWrapper(ReviewDb db) { - super(db); - this.groups = new Groups(this, delegate); - this.groupNames = new GroupNames(this, delegate); - this.members = new Members(this, delegate); - this.memberAudits = new MemberAudits(this, delegate); - this.byIds = new ByIds(this, delegate); - this.byIdAudits = new ByIdAudits(this, delegate); - } - - @Override - public AccountGroupAccess accountGroups() { - return groups; - } - - @Override - public AccountGroupNameAccess accountGroupNames() { - return groupNames; - } - - @Override - public AccountGroupMemberAccess accountGroupMembers() { - return members; - } - - @Override - public AccountGroupMemberAuditAccess accountGroupMembersAudit() { - return memberAudits; - } - - @Override - public AccountGroupByIdAudAccess accountGroupByIdAud() { - return byIdAudits; - } - - @Override - public AccountGroupByIdAccess accountGroupById() { - return byIds; - } - - private static class Groups extends AbstractDisabledAccess - implements AccountGroupAccess { - private Groups(ReviewDbWrapper wrapper, ReviewDb db) { - super(wrapper, db.accountGroups()); - } - - @Override - public ResultSet byUUID(AccountGroup.UUID uuid) throws OrmException { - return empty(); - } - - @Override - public ResultSet all() throws OrmException { - return empty(); - } - } - - private static class GroupNames - extends AbstractDisabledAccess - implements AccountGroupNameAccess { - private GroupNames(ReviewDbWrapper wrapper, ReviewDb db) { - super(wrapper, db.accountGroupNames()); - } - - @Override - public ResultSet all() throws OrmException { - return empty(); - } - } - - private static class Members - extends AbstractDisabledAccess - implements AccountGroupMemberAccess { - private Members(ReviewDbWrapper wrapper, ReviewDb db) { - super(wrapper, db.accountGroupMembers()); - } - - @Override - public ResultSet byAccount(Account.Id id) throws OrmException { - return empty(); - } - - @Override - public ResultSet byGroup(AccountGroup.Id id) throws OrmException { - return empty(); - } - } - - private static class MemberAudits - extends AbstractDisabledAccess - implements AccountGroupMemberAuditAccess { - private MemberAudits(ReviewDbWrapper wrapper, ReviewDb db) { - super(wrapper, db.accountGroupMembersAudit()); - } - - @Override - public ResultSet byGroupAccount( - AccountGroup.Id groupId, com.google.gerrit.reviewdb.client.Account.Id accountId) - throws OrmException { - return empty(); - } - - @Override - public ResultSet byGroup(AccountGroup.Id groupId) throws OrmException { - return empty(); - } - } - - private static class ByIds extends AbstractDisabledAccess - implements AccountGroupByIdAccess { - private ByIds(ReviewDbWrapper wrapper, ReviewDb db) { - super(wrapper, db.accountGroupById()); - } - - @Override - public ResultSet byIncludeUUID(AccountGroup.UUID uuid) throws OrmException { - return empty(); - } - - @Override - public ResultSet byGroup(AccountGroup.Id id) throws OrmException { - return empty(); - } - - @Override - public ResultSet all() throws OrmException { - return empty(); - } - } - - private static class ByIdAudits - extends AbstractDisabledAccess - implements AccountGroupByIdAudAccess { - private ByIdAudits(ReviewDbWrapper wrapper, ReviewDb db) { - super(wrapper, db.accountGroupByIdAud()); - } - - @Override - public ResultSet byGroupInclude( - AccountGroup.Id groupId, AccountGroup.UUID incGroupUUID) throws OrmException { - return empty(); - } - - @Override - public ResultSet byGroup(AccountGroup.Id groupId) throws OrmException { - return empty(); - } - } -} diff --git a/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java b/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java index 9bc8b61b9f..0d95610499 100644 --- a/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java +++ b/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java @@ -15,9 +15,7 @@ package com.google.gerrit.server.schema; import com.google.gerrit.reviewdb.server.DisallowReadFromChangesReviewDbWrapper; -import com.google.gerrit.reviewdb.server.DisallowReadFromGroupsReviewDbWrapper; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; @@ -28,16 +26,12 @@ import com.google.inject.Singleton; public class NotesMigrationSchemaFactory implements SchemaFactory { private final SchemaFactory delegate; private final NotesMigration migration; - private final GroupsMigration groupsMigration; @Inject NotesMigrationSchemaFactory( - @ReviewDbFactory SchemaFactory delegate, - NotesMigration migration, - GroupsMigration groupsMigration) { + @ReviewDbFactory SchemaFactory delegate, NotesMigration migration) { this.delegate = delegate; this.migration = migration; - this.groupsMigration = groupsMigration; } @Override @@ -73,23 +67,12 @@ public class NotesMigrationSchemaFactory implements SchemaFactory { db = new NoChangesReviewDbWrapper(db); } - if (groupsMigration.readFromNoteDb() && groupsMigration.disableGroupReviewDb()) { - // Disable writes to group tables in ReviewDb (ReviewDb access for groups are No-Ops). - db = new NoGroupsReviewDbWrapper(db); - } - // Second create the wrappers which can be removed by ReviewDbUtil#unwrapDb(ReviewDb). if (migration.readChanges()) { // If reading changes from NoteDb is configured, changes should not be read from ReviewDb. // Make sure that any attempt to read a change from ReviewDb anyway fails with an exception. db = new DisallowReadFromChangesReviewDbWrapper(db); } - - if (groupsMigration.readFromNoteDb()) { - // If reading groups from NoteDb is configured, groups should not be read from ReviewDb. - // Make sure that any attempt to read a group from ReviewDb anyway fails with an exception. - db = new DisallowReadFromGroupsReviewDbWrapper(db); - } return db; } } diff --git a/java/com/google/gerrit/server/schema/SchemaCreator.java b/java/com/google/gerrit/server/schema/SchemaCreator.java index ba68b2cca9..88b3e2879a 100644 --- a/java/com/google/gerrit/server/schema/SchemaCreator.java +++ b/java/com/google/gerrit/server/schema/SchemaCreator.java @@ -14,13 +14,11 @@ package com.google.gerrit.server.schema; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroupName; import com.google.gerrit.reviewdb.client.CurrentSchemaVersion; import com.google.gerrit.reviewdb.client.SystemConfig; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -40,12 +38,10 @@ import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.db.AuditLogFormatter; import com.google.gerrit.server.group.db.GroupConfig; import com.google.gerrit.server.group.db.GroupNameNotes; -import com.google.gerrit.server.group.db.GroupsUpdate; import com.google.gerrit.server.group.db.InternalGroupCreation; import com.google.gerrit.server.group.db.InternalGroupUpdate; import com.google.gerrit.server.index.group.GroupIndex; import com.google.gerrit.server.index.group.GroupIndexCollection; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.update.RefUpdateUtil; import com.google.gwtorm.jdbc.JdbcExecutor; @@ -73,7 +69,6 @@ public class SchemaCreator { private final PersonIdent serverUser; private final DataSourceType dataSourceType; private final GroupIndexCollection indexCollection; - private final GroupsMigration groupsMigration; private final String serverId; private final Config config; @@ -91,7 +86,6 @@ public class SchemaCreator { @GerritPersonIdent PersonIdent au, DataSourceType dst, GroupIndexCollection ic, - GroupsMigration gm, @GerritServerId String serverId, @GerritServerConfig Config config, MetricMaker metricMaker, @@ -106,7 +100,6 @@ public class SchemaCreator { au, dst, ic, - gm, serverId, config, metricMaker, @@ -123,7 +116,6 @@ public class SchemaCreator { @GerritPersonIdent PersonIdent au, DataSourceType dst, GroupIndexCollection ic, - GroupsMigration gm, String serverId, Config config, MetricMaker metricMaker, @@ -137,7 +129,6 @@ public class SchemaCreator { serverUser = au; dataSourceType = dst; indexCollection = ic; - groupsMigration = gm; this.serverId = serverId; this.config = config; @@ -176,25 +167,24 @@ public class SchemaCreator { allUsersName, metricMaker); try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { - createAdminsGroup(db, seqs, allUsersRepo, admins); - createBatchUsersGroup(db, seqs, allUsersRepo, batchUsers, admins.getUUID()); + createAdminsGroup(seqs, allUsersRepo, admins); + createBatchUsersGroup(seqs, allUsersRepo, batchUsers, admins.getUUID()); } dataSourceType.getIndexScript().run(db); } private void createAdminsGroup( - ReviewDb db, Sequences seqs, Repository allUsersRepo, GroupReference groupReference) + Sequences seqs, Repository allUsersRepo, GroupReference groupReference) throws OrmException, IOException, ConfigInvalidException { InternalGroupCreation groupCreation = getGroupCreation(seqs, groupReference); InternalGroupUpdate groupUpdate = InternalGroupUpdate.builder().setDescription("Gerrit Site Administrators").build(); - createGroup(db, allUsersRepo, groupCreation, groupUpdate); + createGroup(allUsersRepo, groupCreation, groupUpdate); } private void createBatchUsersGroup( - ReviewDb db, Sequences seqs, Repository allUsersRepo, GroupReference groupReference, @@ -207,35 +197,16 @@ public class SchemaCreator { .setOwnerGroupUUID(adminsGroupUuid) .build(); - createGroup(db, allUsersRepo, groupCreation, groupUpdate); + createGroup(allUsersRepo, groupCreation, groupUpdate); } private void createGroup( - ReviewDb db, - Repository allUsersRepo, - InternalGroupCreation groupCreation, - InternalGroupUpdate groupUpdate) + Repository allUsersRepo, InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) throws OrmException, ConfigInvalidException, IOException { - InternalGroup groupInReviewDb = createGroupInReviewDb(db, groupCreation, groupUpdate); - - if (!groupsMigration.writeToNoteDb()) { - index(groupInReviewDb); - return; - } - InternalGroup createdGroup = createGroupInNoteDb(allUsersRepo, groupCreation, groupUpdate); index(createdGroup); } - private static InternalGroup createGroupInReviewDb( - ReviewDb db, InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) - throws OrmException { - AccountGroup group = GroupsUpdate.createAccountGroup(groupCreation, groupUpdate); - db.accountGroupNames().insert(ImmutableList.of(new AccountGroupName(group))); - db.accountGroups().insert(ImmutableList.of(group)); - return InternalGroup.create(group, ImmutableSet.of(), ImmutableSet.of()); - } - private InternalGroup createGroupInNoteDb( Repository allUsersRepo, InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) throws ConfigInvalidException, IOException, OrmDuplicateKeyException { diff --git a/java/com/google/gerrit/sshd/commands/RenameGroupCommand.java b/java/com/google/gerrit/sshd/commands/RenameGroupCommand.java index 9e334e6b50..cd9fbda2c8 100644 --- a/java/com/google/gerrit/sshd/commands/RenameGroupCommand.java +++ b/java/com/google/gerrit/sshd/commands/RenameGroupCommand.java @@ -23,7 +23,6 @@ import com.google.gerrit.server.restapi.group.GroupsCollection; import com.google.gerrit.server.restapi.group.PutName; import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.SshCommand; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import java.io.IOException; import org.eclipse.jgit.errors.ConfigInvalidException; @@ -53,7 +52,7 @@ public class RenameGroupCommand extends SshCommand { NameInput input = new NameInput(); input.name = newGroupName; putName.apply(rsrc, input); - } catch (RestApiException | OrmException | IOException | ConfigInvalidException e) { + } catch (RestApiException | IOException | ConfigInvalidException e) { throw die(e); } } diff --git a/java/com/google/gerrit/testing/GroupNoteDbMode.java b/java/com/google/gerrit/testing/GroupNoteDbMode.java deleted file mode 100644 index 86e92b8234..0000000000 --- a/java/com/google/gerrit/testing/GroupNoteDbMode.java +++ /dev/null @@ -1,78 +0,0 @@ -// 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.testing; - -import static com.google.common.base.Preconditions.checkArgument; - -import com.google.common.base.Enums; -import com.google.common.base.Strings; -import com.google.gerrit.server.notedb.GroupsMigration; - -public enum GroupNoteDbMode { - /** NoteDb is disabled, groups are only in ReviewDb */ - OFF(new GroupsMigration(false, false, false)), - - /** Writing new groups to NoteDb is enabled. */ - WRITE(new GroupsMigration(true, false, false)), - - /** - * Reading/writing groups from/to NoteDb is enabled. Trying to read groups from ReviewDb throws an - * exception. - */ - READ_WRITE(new GroupsMigration(true, true, false)), - - /** - * All group tables in ReviewDb are entirely disabled. Trying to read groups from ReviewDb throws - * an exception. Reading groups through an unwrapped ReviewDb instance writing groups to ReviewDb - * is a No-Op. - */ - ON(new GroupsMigration(true, true, true)); - - private static final String ENV_VAR = "GERRIT_NOTEDB_GROUPS"; - private static final String SYS_PROP = "gerrit.notedb.groups"; - - public static GroupNoteDbMode get() { - String value = System.getenv(ENV_VAR); - if (Strings.isNullOrEmpty(value)) { - value = System.getProperty(SYS_PROP); - } - if (Strings.isNullOrEmpty(value)) { - return OFF; - } - value = value.toUpperCase().replace("-", "_"); - GroupNoteDbMode mode = Enums.getIfPresent(GroupNoteDbMode.class, value).orNull(); - if (!Strings.isNullOrEmpty(System.getenv(ENV_VAR))) { - checkArgument( - mode != null, "Invalid value for env variable %s: %s", ENV_VAR, System.getenv(ENV_VAR)); - } else { - checkArgument( - mode != null, - "Invalid value for system property %s: %s", - SYS_PROP, - System.getProperty(SYS_PROP)); - } - return mode; - } - - private final GroupsMigration groupsMigration; - - private GroupNoteDbMode(GroupsMigration groupsMigration) { - this.groupsMigration = groupsMigration; - } - - public GroupsMigration getGroupsMigration() { - return groupsMigration; - } -} diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupIndexerIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupIndexerIT.java index 389efb4f58..7af057ff0c 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupIndexerIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupIndexerIT.java @@ -15,11 +15,6 @@ package com.google.gerrit.acceptance.api.group; import static com.google.common.truth.Truth.assertWithMessage; -import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS; -import static com.google.gerrit.server.notedb.NotesMigration.DISABLE_REVIEW_DB; -import static com.google.gerrit.server.notedb.NotesMigration.READ; -import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB; -import static com.google.gerrit.server.notedb.NotesMigration.WRITE; import static com.google.gerrit.truth.ListSubject.assertThat; import static com.google.gerrit.truth.OptionalSubject.assertThat; @@ -29,7 +24,6 @@ import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.common.GroupInfo; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ServerInitiated; import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.group.InternalGroup; @@ -48,27 +42,15 @@ import java.io.IOException; import java.util.List; import java.util.Optional; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.Config; import org.junit.Rule; import org.junit.Test; public class GroupIndexerIT { - private static Config createPureNoteDbConfig() { - Config config = new Config(); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), WRITE, true); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, true); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), DISABLE_REVIEW_DB, true); - return config; - } - - @Rule - public InMemoryTestEnvironment testEnvironment = - new InMemoryTestEnvironment(GroupIndexerIT::createPureNoteDbConfig); + @Rule public InMemoryTestEnvironment testEnvironment = new InMemoryTestEnvironment(); @Inject private GroupIndexer groupIndexer; @Inject private GerritApi gApi; @Inject private GroupCache groupCache; - @Inject private ReviewDb db; @Inject @ServerInitiated private GroupsUpdate groupsUpdate; @Inject private Provider groupQueryProvider; @@ -176,7 +158,7 @@ public class GroupIndexerIT { private void updateGroupWithoutCacheOrIndex( AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException { - groupsUpdate.updateGroupInDb(db, groupUuid, groupUpdate); + groupsUpdate.updateGroupInDb(groupUuid, groupUpdate); } private static OptionalSubject assertThatGroup( diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java index 4e9c37bce9..87a566e237 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java @@ -15,9 +15,7 @@ package com.google.gerrit.acceptance.api.group; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; -import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; @@ -32,11 +30,7 @@ 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.lib.Config; import org.eclipse.jgit.lib.RefRename; import org.eclipse.jgit.lib.RefUpdate; @@ -53,17 +47,6 @@ import org.junit.Test; @Sandboxed @NoHttpd public class GroupsConsistencyIT extends AbstractDaemonTest { - - @ConfigSuite.Config - public static Config noteDbConfig() { - Config config = new Config(); - config.setBoolean(NotesMigration.SECTION_NOTE_DB, GROUPS.key(), NotesMigration.WRITE, true); - config.setBoolean(NotesMigration.SECTION_NOTE_DB, GROUPS.key(), NotesMigration.READ, true); - return config; - } - - @Inject private GroupsMigration groupsMigration; - private GroupInfo gAdmin; private GroupInfo g1; private GroupInfo g2; @@ -72,7 +55,6 @@ public class GroupsConsistencyIT extends AbstractDaemonTest { @Before public void basicSetup() throws Exception { - assume().that(groupsInNoteDb()).isTrue(); allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); String name1 = createGroup("g1"); @@ -87,10 +69,6 @@ public class GroupsConsistencyIT extends AbstractDaemonTest { this.gAdmin = gApi.groups().id("Administrators").detail(); } - private boolean groupsInNoteDb() { - return groupsMigration.writeToNoteDb(); - } - @Test public void allGood() throws Exception { assertThat(check()).isEmpty(); diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index 0e5da1284b..c176acdc87 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -16,18 +16,12 @@ package com.google.gerrit.acceptance.api.group; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.GitUtil.deleteRef; import static com.google.gerrit.acceptance.GitUtil.fetch; import static com.google.gerrit.acceptance.api.group.GroupAssert.assertGroupInfo; import static com.google.gerrit.acceptance.rest.account.AccountAssert.assertAccountInfos; import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; -import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS; -import static com.google.gerrit.server.notedb.NotesMigration.DISABLE_REVIEW_DB; -import static com.google.gerrit.server.notedb.NotesMigration.READ; -import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB; -import static com.google.gerrit.server.notedb.NotesMigration.WRITE; import static java.lang.annotation.ElementType.METHOD; import static java.lang.annotation.RetentionPolicy.RUNTIME; import static java.util.stream.Collectors.toList; @@ -85,7 +79,6 @@ import com.google.gerrit.server.group.db.InternalGroupUpdate; import com.google.gerrit.server.index.group.GroupIndexer; import com.google.gerrit.server.index.group.StalenessChecker; import com.google.gerrit.server.util.MagicBranch; -import com.google.gerrit.testing.ConfigSuite; import com.google.gerrit.testing.TestTimeUtil; import com.google.inject.Inject; import java.io.IOException; @@ -119,21 +112,6 @@ import org.junit.Test; @NoHttpd public class GroupsIT extends AbstractDaemonTest { - @ConfigSuite.Config - public static Config noteDbConfig() { - Config config = new Config(); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), WRITE, true); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, true); - return config; - } - - @ConfigSuite.Config - public static Config disableReviewDb() { - Config config = noteDbConfig(); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), DISABLE_REVIEW_DB, true); - return config; - } - @Inject private Groups groups; @Inject @ServerInitiated private GroupsUpdate groupsUpdate; @Inject private GroupIncludeCache groupIncludeCache; @@ -732,7 +710,7 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void listAllGroups() throws Exception { List expectedGroups = - groups.getAllGroupReferences(db).map(GroupReference::getName).sorted().collect(toList()); + groups.getAllGroupReferences().map(GroupReference::getName).sorted().collect(toList()); assertThat(expectedGroups.size()).isAtLeast(2); assertThat(gApi.groups().list().getAsMap().keySet()) .containsExactlyElementsIn(expectedGroups) @@ -908,8 +886,6 @@ public class GroupsIT extends AbstractDaemonTest { @Sandboxed @IgnoreGroupInconsistencies public void getAuditLogAfterDeletingASubgroup() throws Exception { - assume().that(readGroupsFromNoteDb()).isTrue(); - GroupInfo parentGroup = gApi.groups().create(name("parent-group")).get(); // Creates a subgroup and adds it to "parent-group" as a subgroup. @@ -973,7 +949,6 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void pushToGroupBranchIsRejectedForAllUsersRepo() throws Exception { - assume().that(groupsInNoteDb()).isTrue(); // branch only exists when groups are in NoteDb assertPushToGroupBranch( allUsers, RefNames.refsGroups(adminGroupUuid()), "group update not allowed"); } @@ -989,7 +964,6 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void pushToGroupNamesBranchIsRejectedForAllUsersRepo() throws Exception { - assume().that(groupsInNoteDb()).isTrue(); // branch only exists when groups are in NoteDb // refs/meta/group-names isn't usually available for fetch, so grant ACCESS_DATABASE allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); assertPushToGroupBranch(allUsers, RefNames.REFS_GROUPNAMES, "group update not allowed"); @@ -1114,8 +1088,6 @@ public class GroupsIT extends AbstractDaemonTest { @Test @IgnoreGroupInconsistencies public void cannotCreateGroupNamesBranch() throws Exception { - assume().that(groupsInNoteDb()).isTrue(); - // Use ProjectResetter to restore the group names ref try (ProjectResetter resetter = projectResetter @@ -1155,7 +1127,6 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void cannotDeleteGroupBranch() throws Exception { - assume().that(groupsInNoteDb()).isTrue(); testCannotDeleteGroupBranch(RefNames.REFS_GROUPS + "*", RefNames.refsGroups(adminGroupUuid())); } @@ -1168,8 +1139,6 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void cannotDeleteGroupNamesBranch() throws Exception { - assume().that(groupsInNoteDb()).isTrue(); - // refs/meta/group-names is only visible with ACCESS_DATABASE allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); @@ -1199,8 +1168,6 @@ public class GroupsIT extends AbstractDaemonTest { @Test @IgnoreGroupInconsistencies public void stalenessChecker() throws Exception { - assume().that(readGroupsFromNoteDb()).isTrue(); - // Newly created group is not stale GroupInfo groupInfo = gApi.groups().create(name("foo")).get(); AccountGroup.UUID groupUuid = new AccountGroup.UUID(groupInfo.id); @@ -1246,8 +1213,6 @@ public class GroupsIT extends AbstractDaemonTest { @Test @Sandboxed public void groupsOfUserCanBeListedInSlaveMode() throws Exception { - assume().that(readGroupsFromNoteDb()).isTrue(); - GroupInput groupInput = new GroupInput(); groupInput.name = name("contributors"); groupInput.members = ImmutableList.of(user.username); @@ -1267,11 +1232,8 @@ public class GroupsIT extends AbstractDaemonTest { @GerritConfig(name = "index.autoReindexIfStale", value = "false") @IgnoreGroupInconsistencies public void reindexGroupsInSlaveMode() throws Exception { - assume().that(readGroupsFromNoteDb()).isTrue(); - assume().that(cfg.getBoolean(SECTION_NOTE_DB, GROUPS.key(), DISABLE_REVIEW_DB, false)).isTrue(); - List expectedGroups = - groups.getAllGroupReferences(db).map(GroupReference::getUUID).collect(toList()); + groups.getAllGroupReferences().map(GroupReference::getUUID).collect(toList()); assertThat(expectedGroups.size()).isAtLeast(2); // Restart the server as slave, on startup of the slave all groups are indexed. @@ -1303,7 +1265,7 @@ public class GroupsIT extends AbstractDaemonTest { // Update a group without updating the cache or index, // then run the reindexer -> only the updated group is reindexed. groupsUpdate.updateGroupInDb( - db, groupUuid, InternalGroupUpdate.builder().setDescription("bar").build()); + groupUuid, InternalGroupUpdate.builder().setDescription("bar").build()); slaveGroupIndexer.run(); groupIndexedCounter.assertReindexOf(groupUuid); @@ -1328,10 +1290,8 @@ public class GroupsIT extends AbstractDaemonTest { @GerritConfig(name = "index.autoReindexIfStale", value = "false") @IgnoreGroupInconsistencies public void disabledReindexGroupsOnStartupSlaveMode() throws Exception { - assume().that(readGroupsFromNoteDb()).isTrue(); - List expectedGroups = - groups.getAllGroupReferences(db).map(GroupReference::getUUID).collect(toList()); + groups.getAllGroupReferences().map(GroupReference::getUUID).collect(toList()); assertThat(expectedGroups.size()).isAtLeast(2); restartAsSlave(); @@ -1360,8 +1320,6 @@ public class GroupsIT extends AbstractDaemonTest { private void pushToGroupBranchForReviewAndSubmit( Project.NameKey project, String groupRef, String expectedError) throws Exception { - assume().that(groupsInNoteDb()).isTrue(); // branch only exists when groups are in NoteDb - grantLabel( "Code-Review", -2, 2, project, RefNames.REFS_GROUPS + "*", false, REGISTERED_USERS, false); grant(project, RefNames.REFS_GROUPS + "*", Permission.SUBMIT, false, REGISTERED_USERS); @@ -1484,14 +1442,6 @@ public class GroupsIT extends AbstractDaemonTest { } } - private boolean groupsInNoteDb() { - return cfg.getBoolean(SECTION_NOTE_DB, GROUPS.key(), WRITE, false); - } - - private boolean readGroupsFromNoteDb() { - return groupsInNoteDb() && cfg.getBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, false); - } - @Target({METHOD}) @Retention(RUNTIME) private @interface IgnoreGroupInconsistencies {} diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsUpdateIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsUpdateIT.java index e7ddcaec47..44be241c30 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsUpdateIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsUpdateIT.java @@ -15,17 +15,11 @@ package com.google.gerrit.acceptance.api.group; import static com.google.common.truth.Truth8.assertThat; -import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS; -import static com.google.gerrit.server.notedb.NotesMigration.DISABLE_REVIEW_DB; -import static com.google.gerrit.server.notedb.NotesMigration.READ; -import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB; -import static com.google.gerrit.server.notedb.NotesMigration.WRITE; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ServerInitiated; import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.group.db.GroupsUpdate; @@ -39,27 +33,14 @@ import java.io.IOException; import java.util.Set; import java.util.stream.Stream; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.Config; import org.junit.Rule; import org.junit.Test; public class GroupsUpdateIT { - - private static Config createPureNoteDbConfig() { - Config config = new Config(); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), WRITE, true); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, true); - config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), DISABLE_REVIEW_DB, true); - return config; - } - - @Rule - public InMemoryTestEnvironment testEnvironment = - new InMemoryTestEnvironment(GroupsUpdateIT::createPureNoteDbConfig); + @Rule public InMemoryTestEnvironment testEnvironment = new InMemoryTestEnvironment(); @Inject @ServerInitiated private Provider groupsUpdateProvider; @Inject private Groups groups; - @Inject private ReviewDb reviewDb; @Test public void groupCreationIsRetriedWhenFailedDueToConcurrentNameModification() throws Exception { @@ -100,17 +81,16 @@ public class GroupsUpdateIT { private void createGroup(InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) throws OrmException, IOException, ConfigInvalidException { - groupsUpdateProvider.get().createGroup(reviewDb, groupCreation, groupUpdate); + groupsUpdateProvider.get().createGroup(groupCreation, groupUpdate); } private void updateGroup(AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) throws Exception { - groupsUpdateProvider.get().updateGroup(reviewDb, groupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate); } - private Stream getAllGroupNames() - throws OrmException, IOException, ConfigInvalidException { - return groups.getAllGroupReferences(reviewDb).map(GroupReference::getName); + private Stream getAllGroupNames() throws IOException, ConfigInvalidException { + return groups.getAllGroupReferences().map(GroupReference::getName); } private static InternalGroupCreation getGroupCreation(String groupName, String groupUuid) { @@ -145,7 +125,7 @@ public class GroupsUpdateIT { InternalGroupCreation groupCreation = getGroupCreation(groupName, groupName + "-UUID"); InternalGroupUpdate groupUpdate = InternalGroupUpdate.builder().build(); try { - groupsUpdateProvider.get().createGroup(reviewDb, groupCreation, groupUpdate); + groupsUpdateProvider.get().createGroup(groupCreation, groupUpdate); } catch (OrmException | IOException | ConfigInvalidException e) { throw new IllegalStateException(e); } diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java index 2bff3f98c4..f1b65d453e 100644 --- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java +++ b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java @@ -359,7 +359,7 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests { AccountGroup.UUID groupUuid = new AccountGroup.UUID(group1.id); InternalGroupUpdate groupUpdate = InternalGroupUpdate.builder().setDescription(newDescription).build(); - groupsUpdateProvider.get().updateGroupInDb(db, groupUuid, groupUpdate); + groupsUpdateProvider.get().updateGroupInDb(groupUuid, groupUpdate); assertQuery("description:" + group1.description, group1); assertQuery("description:" + newDescription); diff --git a/javatests/com/google/gerrit/server/schema/SchemaUpdaterTest.java b/javatests/com/google/gerrit/server/schema/SchemaUpdaterTest.java index 047b933391..ed94c97152 100644 --- a/javatests/com/google/gerrit/server/schema/SchemaUpdaterTest.java +++ b/javatests/com/google/gerrit/server/schema/SchemaUpdaterTest.java @@ -33,7 +33,6 @@ import com.google.gerrit.server.config.GerritServerId; import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.group.SystemGroupBackend; -import com.google.gerrit.server.notedb.GroupsMigration; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.testing.InMemoryDatabase; import com.google.gerrit.testing.InMemoryH2Type; @@ -119,7 +118,6 @@ public class SchemaUpdaterTest { bind(SystemGroupBackend.class); install(new NotesMigration.Module()); - install(new GroupsMigration.Module()); bind(MetricMaker.class).to(DisabledMetricMaker.class); } }) diff --git a/javatests/com/google/gerrit/server/schema/Schema_150_to_151_Test.java b/javatests/com/google/gerrit/server/schema/Schema_150_to_151_Test.java index b817e71925..a58d36d688 100644 --- a/javatests/com/google/gerrit/server/schema/Schema_150_to_151_Test.java +++ b/javatests/com/google/gerrit/server/schema/Schema_150_to_151_Test.java @@ -18,17 +18,20 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; import com.google.gerrit.common.TimeUtil; -import com.google.gerrit.extensions.api.groups.GroupInput; -import com.google.gerrit.extensions.common.GroupInfo; -import com.google.gerrit.extensions.restapi.TopLevelResource; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup.Id; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.restapi.group.CreateGroup; +import com.google.gerrit.reviewdb.server.ReviewDbWrapper; +import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.Sequences; +import com.google.gerrit.server.account.GroupUUID; import com.google.gerrit.testing.InMemoryTestEnvironment; import com.google.gerrit.testing.TestUpdateUI; import com.google.gwtorm.jdbc.JdbcSchema; import com.google.inject.Inject; +import com.google.inject.Provider; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; @@ -37,6 +40,7 @@ import java.time.Instant; import java.time.LocalDateTime; import java.time.Month; import java.time.ZoneOffset; +import org.eclipse.jgit.lib.PersonIdent; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -46,14 +50,22 @@ public class Schema_150_to_151_Test { @Rule public InMemoryTestEnvironment testEnv = new InMemoryTestEnvironment(); - @Inject private CreateGroup.Factory createGroupFactory; @Inject private Schema_151 schema151; @Inject private ReviewDb db; + @Inject private IdentifiedUser currentUser; + @Inject private @GerritPersonIdent Provider serverIdent; + @Inject private Sequences seq; private Connection connection; private PreparedStatement createdOnRetrieval; private PreparedStatement createdOnUpdate; private PreparedStatement auditEntryDeletion; + private JdbcSchema jdbcSchema; + + @Before + public void unwrapDb() { + jdbcSchema = ReviewDbWrapper.unwrapJbdcSchema(db); + } @Before public void setUp() throws Exception { @@ -87,7 +99,7 @@ public class Schema_150_to_151_Test { @Test public void createdOnIsPopulatedForGroupsCreatedAfterAudit() throws Exception { Timestamp testStartTime = TimeUtil.nowTs(); - AccountGroup.Id groupId = createGroup("Group for schema migration"); + AccountGroup.Id groupId = createGroupInReviewDb("Group for schema migration"); setCreatedOnToVeryOldTimestamp(groupId); schema151.migrateData(db, new TestUpdateUI()); @@ -98,7 +110,7 @@ public class Schema_150_to_151_Test { @Test public void createdOnIsPopulatedForGroupsCreatedBeforeAudit() throws Exception { - AccountGroup.Id groupId = createGroup("Ancient group for schema migration"); + AccountGroup.Id groupId = createGroupInReviewDb("Ancient group for schema migration"); setCreatedOnToVeryOldTimestamp(groupId); removeAuditEntriesFor(groupId); @@ -108,12 +120,16 @@ public class Schema_150_to_151_Test { assertThat(createdOn).isEqualTo(AccountGroup.auditCreationInstantTs()); } - private AccountGroup.Id createGroup(String name) throws Exception { - GroupInput groupInput = new GroupInput(); - groupInput.name = name; - GroupInfo groupInfo = - createGroupFactory.create(name).apply(TopLevelResource.INSTANCE, groupInput); - return new Id(groupInfo.groupId); + private AccountGroup.Id createGroupInReviewDb(String name) throws Exception { + AccountGroup group = + new AccountGroup( + new AccountGroup.NameKey(name), + new AccountGroup.Id(seq.nextGroupId()), + GroupUUID.make(name, serverIdent.get()), + TimeUtil.nowTs()); + storeInReviewDb(group); + addMembersInReviewDb(group.getId(), currentUser.getAccountId()); + return group.getId(); } private Timestamp getCreatedOn(Id groupId) throws Exception { @@ -138,4 +154,69 @@ public class Schema_150_to_151_Test { auditEntryDeletion.setInt(1, groupId.get()); auditEntryDeletion.executeUpdate(); } + + private void storeInReviewDb(AccountGroup... groups) throws Exception { + try (PreparedStatement stmt = + jdbcSchema + .getConnection() + .prepareStatement( + "INSERT INTO account_groups" + + " (group_uuid," + + " group_id," + + " name," + + " description," + + " created_on," + + " owner_group_uuid," + + " visible_to_all) VALUES (?, ?, ?, ?, ?, ?, ?)")) { + for (AccountGroup group : groups) { + stmt.setString(1, group.getGroupUUID().get()); + stmt.setInt(2, group.getId().get()); + stmt.setString(3, group.getName()); + stmt.setString(4, group.getDescription()); + stmt.setTimestamp(5, group.getCreatedOn()); + stmt.setString(6, group.getOwnerGroupUUID().get()); + stmt.setString(7, group.isVisibleToAll() ? "Y" : "N"); + stmt.addBatch(); + } + stmt.executeBatch(); + } + } + + private void addMembersInReviewDb(AccountGroup.Id groupId, Account.Id... memberIds) + throws Exception { + try (PreparedStatement addMemberStmt = + jdbcSchema + .getConnection() + .prepareStatement( + "INSERT INTO account_group_members" + + " (group_id," + + " account_id) VALUES (" + + groupId.get() + + ", ?)"); + PreparedStatement addMemberAuditStmt = + jdbcSchema + .getConnection() + .prepareStatement( + "INSERT INTO account_group_members_audit" + + " (group_id," + + " account_id," + + " added_by," + + " added_on) VALUES (" + + groupId.get() + + ", ?, " + + currentUser.getAccountId().get() + + ", ?)")) { + Timestamp addedOn = TimeUtil.nowTs(); + for (Account.Id memberId : memberIds) { + addMemberStmt.setInt(1, memberId.get()); + addMemberStmt.addBatch(); + + addMemberAuditStmt.setInt(1, memberId.get()); + addMemberAuditStmt.setTimestamp(2, addedOn); + addMemberAuditStmt.addBatch(); + } + addMemberStmt.executeBatch(); + addMemberAuditStmt.executeBatch(); + } + } } diff --git a/javatests/com/google/gerrit/server/schema/Schema_166_to_167_WithGroupsInNoteDbTest.java b/javatests/com/google/gerrit/server/schema/Schema_166_to_167_WithGroupsInNoteDbTest.java index 032b590ff7..26913e5c2b 100644 --- a/javatests/com/google/gerrit/server/schema/Schema_166_to_167_WithGroupsInNoteDbTest.java +++ b/javatests/com/google/gerrit/server/schema/Schema_166_to_167_WithGroupsInNoteDbTest.java @@ -87,7 +87,6 @@ public class Schema_166_to_167_WithGroupsInNoteDbTest { // disableReviewDb == true) InternalGroup internalGroup = groupsUpdate.createGroup( - db, InternalGroupCreation.builder() .setNameKey(new AccountGroup.NameKey("users")) .setGroupUUID(new AccountGroup.UUID("users")) diff --git a/javatests/com/google/gerrit/server/schema/Schema_166_to_167_WithGroupsInReviewDbTest.java b/javatests/com/google/gerrit/server/schema/Schema_166_to_167_WithGroupsInReviewDbTest.java index 04c0de0a6b..1dbeeed634 100644 --- a/javatests/com/google/gerrit/server/schema/Schema_166_to_167_WithGroupsInReviewDbTest.java +++ b/javatests/com/google/gerrit/server/schema/Schema_166_to_167_WithGroupsInReviewDbTest.java @@ -39,6 +39,7 @@ import com.google.gerrit.extensions.common.GroupAuditEventInfo.GroupMemberAuditE import com.google.gerrit.extensions.common.GroupAuditEventInfo.Type; import com.google.gerrit.extensions.common.GroupAuditEventInfo.UserMemberAuditEventInfo; import com.google.gerrit.extensions.common.GroupInfo; +import com.google.gerrit.extensions.common.GroupOptionsInfo; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.RestApiException; @@ -49,8 +50,10 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbWrapper; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.Sequences; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupMembership; +import com.google.gerrit.server.account.GroupUUID; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerId; import com.google.gerrit.server.config.GerritServerIdProvider; @@ -89,10 +92,8 @@ import java.util.Set; import java.util.concurrent.TimeUnit; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevSort; @@ -129,6 +130,7 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { @Inject private GroupBundle.Factory groupBundleFactory; @Inject private GroupBackend groupBackend; @Inject private DynamicSet backends; + @Inject private Sequences seq; private JdbcSchema jdbcSchema; @@ -169,6 +171,7 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { groupInput.name = "verifiers"; groupInput.description = "old"; GroupInfo group1 = gApi.groups().create(groupInput).get(); + storeInReviewDb(group1); // Update group only in ReviewDb AccountGroup group1InReviewDb = getFromReviewDb(new AccountGroup.Id(group1.groupId)); @@ -177,6 +180,7 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { // Create a second group in NoteDb and ReviewDb GroupInfo group2 = gApi.groups().create("contributors").get(); + storeInReviewDb(group2); executeSchemaMigration(schema167, group1, group2); @@ -193,8 +197,9 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { @Test public void adminGroupIsMigratedToNoteDb() throws Exception { - // Administrators group is automatically created for all Gerrit servers. + // Administrators group is automatically created for all Gerrit servers (NoteDb only). GroupInfo adminGroup = gApi.groups().id("Administrators").get(); + storeInReviewDb(adminGroup); executeSchemaMigration(schema167, adminGroup); @@ -206,8 +211,9 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { @Test public void nonInteractiveUsersGroupIsMigratedToNoteDb() throws Exception { - // 'Non-Interactive Users' group is automatically created for all Gerrit servers. + // 'Non-Interactive Users' group is automatically created for all Gerrit servers (NoteDb only). GroupInfo nonInteractiveUsersGroup = gApi.groups().id("Non-Interactive Users").get(); + storeInReviewDb(nonInteractiveUsersGroup); executeSchemaMigration(schema167, nonInteractiveUsersGroup); @@ -219,6 +225,11 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { @Test public void groupsAreConsistentAfterMigrationToNoteDb() throws Exception { + // Administrators group are automatically created for all Gerrit servers (NoteDb only). + GroupInfo adminGroup = gApi.groups().id("Administrators").get(); + GroupInfo nonInteractiveUsersGroup = gApi.groups().id("Non-Interactive Users").get(); + storeInReviewDb(adminGroup, nonInteractiveUsersGroup); + AccountGroup group1 = newGroup().setName("verifiers").build(); AccountGroup group2 = newGroup().setName("contributors").build(); storeInReviewDb(group1, group2); @@ -344,7 +355,7 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { storeInReviewDb(group); Account.Id member1 = new Account.Id(23456); Account.Id member2 = new Account.Id(93483); - addMembersInReviewDb(group, member1, member2); + addMembersInReviewDb(group.getId(), member1, member2); executeSchemaMigration(schema167, group); @@ -370,29 +381,26 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { public void logFormatWithAccountsAndGerritGroups() throws Exception { AccountInfo user1 = createAccount("user1"); AccountInfo user2 = createAccount("user2"); - GroupInfo group1 = gApi.groups().create("group1").get(); - GroupInfo group2 = gApi.groups().create("group2").get(); - GroupInfo group3 = gApi.groups().create("group3").get(); + + AccountGroup group1 = createInReviewDb("group1"); + AccountGroup group2 = createInReviewDb("group2"); + AccountGroup group3 = createInReviewDb("group3"); // Add some accounts try (TempClockStep step = TestTimeUtil.freezeClock()) { - gApi.groups() - .id(group1.id) - .addMembers(Integer.toString(user1._accountId), Integer.toString(user2._accountId)); + addMembersInReviewDb( + group1.getId(), new Account.Id(user1._accountId), new Account.Id(user2._accountId)); } TimeUtil.nowTs(); // Add some Gerrit groups try (TempClockStep step = TestTimeUtil.freezeClock()) { - gApi.groups().id(group1.id).addGroups(group2.id, group3.id); + addSubgroupsInReviewDb(group1.getId(), group2.getGroupUUID(), group3.getGroupUUID()); } - AccountGroup.UUID group1Uuid = new AccountGroup.UUID(group1.id); - deleteGroupRefs(group1Uuid); - executeSchemaMigration(schema167, group1, group2, group3); - GroupBundle noteDbBundle = readGroupBundleFromNoteDb(group1Uuid); + GroupBundle noteDbBundle = readGroupBundleFromNoteDb(group1.getGroupUUID()); ImmutableList log = log(group1); assertThat(log).hasSize(4); @@ -438,14 +446,15 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { .isEqualTo( "Update group\n" + "\n" - + ("Add-group: " + group2.name + " <" + group2.id + ">\n") - + ("Add-group: " + group3.name + " <" + group3.id + ">")); + + ("Add-group: " + group2.getName() + " <" + group2.getGroupUUID().get() + ">\n") + + ("Add-group: " + group3.getName() + " <" + group3.getGroupUUID().get() + ">")); assertThat(log.get(3)).author().name().isEqualTo(currentUser.getName()); assertThat(log.get(3)).author().email().isEqualTo(currentUser.getAccountId() + "@" + serverId); assertThat(log.get(3)).committer().hasSameDateAs(log.get(3).author); // Verify that audit log is correctly read by Gerrit - List auditEvents = gApi.groups().id(group1.id).auditLog(); + List auditEvents = + gApi.groups().id(group1.getGroupUUID().get()).auditLog(); assertThat(auditEvents).hasSize(5); AccountInfo currentUserInfo = gApi.accounts().id(currentUser.getAccountId().get()).get(); assertMemberAuditEvent( @@ -462,27 +471,22 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { auditEvents.get(0), Type.ADD_GROUP, currentUser.getAccountId(), - group2, - group3); + toGroupInfo(group2), + toGroupInfo(group3)); } @Test public void logFormatWithSystemGroups() throws Exception { - GroupInfo group = gApi.groups().create("group1").get(); + AccountGroup group = createInReviewDb("group"); try (TempClockStep step = TestTimeUtil.freezeClock()) { - gApi.groups() - .id(group.id) - .addGroups( - SystemGroupBackend.ANONYMOUS_USERS.get(), SystemGroupBackend.REGISTERED_USERS.get()); + addSubgroupsInReviewDb( + group.getId(), SystemGroupBackend.ANONYMOUS_USERS, SystemGroupBackend.REGISTERED_USERS); } - AccountGroup.UUID groupUuid = new AccountGroup.UUID(group.id); - deleteGroupRefs(groupUuid); - executeSchemaMigration(schema167, group); - GroupBundle noteDbBundle = readGroupBundleFromNoteDb(groupUuid); + GroupBundle noteDbBundle = readGroupBundleFromNoteDb(group.getGroupUUID()); ImmutableList log = log(group); assertThat(log).hasSize(3); @@ -523,7 +527,8 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { assertThat(log.get(2)).committer().hasSameDateAs(log.get(2).author); // Verify that audit log is correctly read by Gerrit - List auditEvents = gApi.groups().id(group.id).auditLog(); + List auditEvents = + gApi.groups().id(group.getGroupUUID().get()).auditLog(); assertThat(auditEvents).hasSize(3); AccountInfo currentUserInfo = gApi.accounts().id(currentUser.getAccountId().get()).get(); assertMemberAuditEvent( @@ -539,20 +544,17 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { @Test public void logFormatWithExternalGroup() throws Exception { - GroupInfo group = gApi.groups().create("group").get(); + AccountGroup group = createInReviewDb("group"); backends.add(new TestGroupBackend()); AccountGroup.UUID subgroupUuid = TestGroupBackend.createUuuid("foo"); assertThat(groupBackend.handles(subgroupUuid)).isTrue(); - addSubgroupsInReviewDb(new AccountGroup.Id(group.groupId), subgroupUuid); - - AccountGroup.UUID groupUuid = new AccountGroup.UUID(group.id); - deleteGroupRefs(groupUuid); + addSubgroupsInReviewDb(group.getId(), subgroupUuid); executeSchemaMigration(schema167, group); - GroupBundle noteDbBundle = readGroupBundleFromNoteDb(groupUuid); + GroupBundle noteDbBundle = readGroupBundleFromNoteDb(group.getGroupUUID()); ImmutableList log = log(group); assertThat(log).hasSize(3); @@ -599,7 +601,8 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { assertThat(log.get(2)).committer().hasSameDateAs(log.get(2).author); // Verify that audit log is correctly read by Gerrit - List auditEvents = gApi.groups().id(group.id).auditLog(); + List auditEvents = + gApi.groups().id(group.getGroupUUID().get()).auditLog(); assertThat(auditEvents).hasSize(2); AccountInfo currentUserInfo = gApi.accounts().id(currentUser.getAccountId().get()).get(); assertMemberAuditEvent( @@ -613,19 +616,16 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { @Test public void logFormatWithNonExistingExternalGroup() throws Exception { - GroupInfo group = gApi.groups().create("group").get(); + AccountGroup group = createInReviewDb("group"); AccountGroup.UUID subgroupUuid = new AccountGroup.UUID("notExisting:foo"); assertThat(groupBackend.handles(subgroupUuid)).isFalse(); - addSubgroupsInReviewDb(new AccountGroup.Id(group.groupId), subgroupUuid); - - AccountGroup.UUID groupUuid = new AccountGroup.UUID(group.id); - deleteGroupRefs(groupUuid); + addSubgroupsInReviewDb(group.getId(), subgroupUuid); executeSchemaMigration(schema167, group); - GroupBundle noteDbBundle = readGroupBundleFromNoteDb(groupUuid); + GroupBundle noteDbBundle = readGroupBundleFromNoteDb(group.getGroupUUID()); ImmutableList log = log(group); assertThat(log).hasSize(3); @@ -665,7 +665,8 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { assertThat(log.get(2)).committer().hasSameDateAs(log.get(2).author); // Verify that audit log is correctly read by Gerrit - List auditEvents = gApi.groups().id(group.id).auditLog(); + List auditEvents = + gApi.groups().id(group.getGroupUUID().get()).auditLog(); assertThat(auditEvents).hasSize(2); AccountInfo currentUserInfo = gApi.accounts().id(currentUser.getAccountId().get()).get(); assertMemberAuditEvent( @@ -681,6 +682,25 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { return TestGroup.builder(); } + private AccountGroup createInReviewDb(String groupName) throws Exception { + AccountGroup group = + new AccountGroup( + new AccountGroup.NameKey(groupName), + new AccountGroup.Id(seq.nextGroupId()), + GroupUUID.make(groupName, serverIdent), + TimeUtil.nowTs()); + storeInReviewDb(group); + addMembersInReviewDb(group.getId(), currentUser.getAccountId()); + return group; + } + + private void storeInReviewDb(GroupInfo... groups) throws Exception { + storeInReviewDb( + Arrays.stream(groups) + .map(Schema_166_to_167_WithGroupsInReviewDbTest::toAccountGroup) + .toArray(AccountGroup[]::new)); + } + private void storeInReviewDb(AccountGroup... groups) throws Exception { try (PreparedStatement stmt = jdbcSchema @@ -772,21 +792,41 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { } } - private void addMembersInReviewDb(AccountGroup group, Account.Id... memberIds) throws Exception { - try (PreparedStatement stmt = - jdbcSchema - .getConnection() - .prepareStatement( - "INSERT INTO account_group_members" - + " (group_id," - + " account_id) VALUES (" - + group.getId().get() - + ", ?)")) { + private void addMembersInReviewDb(AccountGroup.Id groupId, Account.Id... memberIds) + throws Exception { + try (PreparedStatement addMemberStmt = + jdbcSchema + .getConnection() + .prepareStatement( + "INSERT INTO account_group_members" + + " (group_id," + + " account_id) VALUES (" + + groupId.get() + + ", ?)"); + PreparedStatement addMemberAuditStmt = + jdbcSchema + .getConnection() + .prepareStatement( + "INSERT INTO account_group_members_audit" + + " (group_id," + + " account_id," + + " added_by," + + " added_on) VALUES (" + + groupId.get() + + ", ?, " + + currentUser.getAccountId().get() + + ", ?)")) { + Timestamp addedOn = TimeUtil.nowTs(); for (Account.Id memberId : memberIds) { - stmt.setInt(1, memberId.get()); - stmt.addBatch(); + addMemberStmt.setInt(1, memberId.get()); + addMemberStmt.addBatch(); + + addMemberAuditStmt.setInt(1, memberId.get()); + addMemberAuditStmt.setTimestamp(2, addedOn); + addMemberAuditStmt.addBatch(); } - stmt.executeBatch(); + addMemberStmt.executeBatch(); + addMemberAuditStmt.executeBatch(); } } @@ -841,21 +881,6 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { } } - private void deleteGroupRefs(AccountGroup.UUID groupUuid) throws Exception { - try (Repository repo = gitRepoManager.openRepository(allUsersName)) { - String refName = RefNames.refsGroups(groupUuid); - RefUpdate ru = repo.updateRef(refName); - ru.setForceUpdate(true); - Ref oldRef = repo.exactRef(refName); - if (oldRef == null) { - return; - } - ru.setExpectedOldObjectId(oldRef.getObjectId()); - ru.setNewObjectId(ObjectId.zeroId()); - assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED); - } - } - private void executeSchemaMigration(SchemaVersion schema, AccountGroup... groupsToVerify) throws Exception { executeSchemaMigration( @@ -892,12 +917,12 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { assertThat(GroupBundle.compareWithAudits(expectedReviewDbBundle, noteDbBundle)).isEmpty(); } - private ImmutableList log(GroupInfo g) throws Exception { + private ImmutableList log(AccountGroup group) throws Exception { ImmutableList.Builder result = ImmutableList.builder(); List commitDates = new ArrayList<>(); try (Repository allUsersRepo = gitRepoManager.openRepository(allUsersName); RevWalk rw = new RevWalk(allUsersRepo)) { - Ref ref = allUsersRepo.exactRef(RefNames.refsGroups(new AccountGroup.UUID(g.id))); + Ref ref = allUsersRepo.exactRef(RefNames.refsGroups(group.getGroupUUID())); if (ref != null) { rw.sort(RevSort.REVERSE); rw.setRetainBody(true); @@ -1030,6 +1055,35 @@ public class Schema_166_to_167_WithGroupsInReviewDbTest { return groupInfo; } + private static AccountGroup toAccountGroup(GroupInfo info) { + AccountGroup group = + new AccountGroup( + new AccountGroup.NameKey(info.name), + new AccountGroup.Id(info.groupId), + new AccountGroup.UUID(info.id), + info.createdOn); + group.setDescription(info.description); + if (info.ownerId != null) { + group.setOwnerGroupUUID(new AccountGroup.UUID(info.ownerId)); + } + group.setVisibleToAll( + info.options != null && info.options.visibleToAll != null && info.options.visibleToAll); + return group; + } + + private static GroupInfo toGroupInfo(AccountGroup group) { + GroupInfo groupInfo = new GroupInfo(); + groupInfo.id = group.getGroupUUID().get(); + groupInfo.groupId = group.getId().get(); + groupInfo.name = group.getName(); + groupInfo.createdOn = group.getCreatedOn(); + groupInfo.description = group.getDescription(); + groupInfo.owner = group.getOwnerGroupUUID().get(); + groupInfo.options = new GroupOptionsInfo(); + groupInfo.options.visibleToAll = group.isVisibleToAll() ? true : null; + return groupInfo; + } + private static class TestGroupBackend implements GroupBackend { static final String PREFIX = "testbackend:";