Merge "Revert "Add config option to prevent group updates while migrating groups""
diff --git a/java/com/google/gerrit/server/group/db/GroupsUpdate.java b/java/com/google/gerrit/server/group/db/GroupsUpdate.java
index a3aae83..f10409e 100644
--- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java
+++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java
@@ -43,7 +43,6 @@
import com.google.gerrit.server.account.GroupIncludeCache;
import com.google.gerrit.server.audit.AuditService;
import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -69,7 +68,6 @@
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.BatchRefUpdate;
-import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
@@ -114,7 +112,6 @@
private final GroupsMigration groupsMigration;
private final GitReferenceUpdated gitRefUpdated;
private final RetryHelper retryHelper;
- private final boolean reviewDbUpdatesAreBlocked;
@Inject
GroupsUpdate(
@@ -131,7 +128,6 @@
@GerritPersonIdent PersonIdent serverIdent,
MetaDataUpdate.InternalFactory metaDataUpdateInternalFactory,
GroupsMigration groupsMigration,
- @GerritServerConfig Config config,
GitReferenceUpdated gitRefUpdated,
RetryHelper retryHelper,
@Assisted @Nullable IdentifiedUser currentUser) {
@@ -152,7 +148,6 @@
getMetaDataUpdateFactory(
metaDataUpdateInternalFactory, currentUser, serverIdent, auditLogFormatter);
authorIdent = getAuthorIdent(serverIdent, currentUser);
- reviewDbUpdatesAreBlocked = config.getBoolean("user", null, "blockReviewDbGroupUpdates", false);
}
private static MetaDataUpdateFactory getMetaDataUpdateFactory(
@@ -276,7 +271,6 @@
private InternalGroup createGroupInReviewDb(
ReviewDb db, InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate)
throws OrmException {
- checkIfReviewDbUpdatesAreBlocked();
AccountGroupName gn = new AccountGroupName(groupCreation.getNameKey(), groupCreation.getId());
// first insert the group name to validate that the group name hasn't
@@ -316,8 +310,6 @@
private UpdateResult updateGroupInReviewDb(
ReviewDb db, AccountGroup group, InternalGroupUpdate groupUpdate) throws OrmException {
- checkIfReviewDbUpdatesAreBlocked();
-
AccountGroup.NameKey originalName = group.getNameKey();
applyUpdate(group, groupUpdate);
AccountGroup.NameKey updatedName = group.getNameKey();
@@ -637,12 +629,6 @@
result.getDeletedSubgroups().forEach(groupIncludeCache::evictParentGroupsOf);
}
- private void checkIfReviewDbUpdatesAreBlocked() throws OrmException {
- if (reviewDbUpdatesAreBlocked) {
- throw new OrmException("Updates to groups in ReviewDb are blocked");
- }
- }
-
private void dispatchAuditEventsOnGroupCreation(InternalGroup createdGroup) {
if (currentUser == null) {
return;
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java
index bfe01ef..6da8bab 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java
@@ -85,13 +85,11 @@
@Test
public void basicGroupProperties() throws Exception {
GroupInfo createdGroup = gApi.groups().create(name("group")).get();
- try (BlockReviewDbUpdatesForGroups ctx = new BlockReviewDbUpdatesForGroups()) {
- GroupBundle reviewDbBundle =
- bundleFactory.fromReviewDb(db, new AccountGroup.Id(createdGroup.groupId));
- deleteGroupRefs(reviewDbBundle);
+ GroupBundle reviewDbBundle =
+ bundleFactory.fromReviewDb(db, new AccountGroup.Id(createdGroup.groupId));
+ deleteGroupRefs(reviewDbBundle);
- assertMigratedCleanly(rebuild(reviewDbBundle), reviewDbBundle);
- }
+ assertMigratedCleanly(rebuild(reviewDbBundle), reviewDbBundle);
}
@Test
@@ -109,53 +107,51 @@
gApi.groups().id(group1.id).addGroups(group2.id, SystemGroupBackend.REGISTERED_USERS.get());
}
- try (BlockReviewDbUpdatesForGroups ctx = new BlockReviewDbUpdatesForGroups()) {
- GroupBundle reviewDbBundle =
- bundleFactory.fromReviewDb(db, new AccountGroup.Id(group1.groupId));
- deleteGroupRefs(reviewDbBundle);
+ GroupBundle reviewDbBundle =
+ bundleFactory.fromReviewDb(db, new AccountGroup.Id(group1.groupId));
+ deleteGroupRefs(reviewDbBundle);
- GroupBundle noteDbBundle = rebuild(reviewDbBundle);
- assertMigratedCleanly(noteDbBundle, reviewDbBundle);
+ GroupBundle noteDbBundle = rebuild(reviewDbBundle);
+ assertMigratedCleanly(noteDbBundle, reviewDbBundle);
- ImmutableList<CommitInfo> log = log(group1);
- assertThat(log).hasSize(4);
+ ImmutableList<CommitInfo> log = log(group1);
+ assertThat(log).hasSize(4);
- assertThat(log.get(0)).message().isEqualTo("Create group");
- assertThat(log.get(0)).author().name().isEqualTo(serverIdent.get().getName());
- assertThat(log.get(0)).author().email().isEqualTo(serverIdent.get().getEmailAddress());
- assertThat(log.get(0)).author().date().isEqualTo(noteDbBundle.group().getCreatedOn());
- assertThat(log.get(0)).author().tz().isEqualTo(serverIdent.get().getTimeZoneOffset());
- assertThat(log.get(0)).committer().isEqualTo(log.get(0).author);
+ assertThat(log.get(0)).message().isEqualTo("Create group");
+ assertThat(log.get(0)).author().name().isEqualTo(serverIdent.get().getName());
+ assertThat(log.get(0)).author().email().isEqualTo(serverIdent.get().getEmailAddress());
+ assertThat(log.get(0)).author().date().isEqualTo(noteDbBundle.group().getCreatedOn());
+ assertThat(log.get(0)).author().tz().isEqualTo(serverIdent.get().getTimeZoneOffset());
+ assertThat(log.get(0)).committer().isEqualTo(log.get(0).author);
- assertThat(log.get(1))
- .message()
- .isEqualTo("Update group\n\nAdd: Administrator <" + admin.id + "@" + serverId + ">");
- assertThat(log.get(1)).author().name().isEqualTo(admin.fullName);
- assertThat(log.get(1)).author().email().isEqualTo(admin.id + "@" + serverId);
- assertThat(log.get(1)).committer().hasSameDateAs(log.get(1).author);
+ assertThat(log.get(1))
+ .message()
+ .isEqualTo("Update group\n\nAdd: Administrator <" + admin.id + "@" + serverId + ">");
+ assertThat(log.get(1)).author().name().isEqualTo(admin.fullName);
+ assertThat(log.get(1)).author().email().isEqualTo(admin.id + "@" + serverId);
+ assertThat(log.get(1)).committer().hasSameDateAs(log.get(1).author);
- assertThat(log.get(2))
- .message()
- .isEqualTo(
- "Update group\n"
- + "\n"
- + ("Add: User <" + user.id + "@" + serverId + ">\n")
- + ("Add: User2 <" + user2.id + "@" + serverId + ">"));
- assertThat(log.get(2)).author().name().isEqualTo(admin.fullName);
- assertThat(log.get(2)).author().email().isEqualTo(admin.id + "@" + serverId);
- assertThat(log.get(2)).committer().hasSameDateAs(log.get(2).author);
+ assertThat(log.get(2))
+ .message()
+ .isEqualTo(
+ "Update group\n"
+ + "\n"
+ + ("Add: User <" + user.id + "@" + serverId + ">\n")
+ + ("Add: User2 <" + user2.id + "@" + serverId + ">"));
+ assertThat(log.get(2)).author().name().isEqualTo(admin.fullName);
+ assertThat(log.get(2)).author().email().isEqualTo(admin.id + "@" + serverId);
+ assertThat(log.get(2)).committer().hasSameDateAs(log.get(2).author);
- assertThat(log.get(3))
- .message()
- .isEqualTo(
- "Update group\n"
- + "\n"
- + ("Add-group: " + group2.name + " <" + group2.id + ">\n")
- + ("Add-group: Registered Users <global:Registered-Users>"));
- assertThat(log.get(3)).author().name().isEqualTo(admin.fullName);
- assertThat(log.get(3)).author().email().isEqualTo(admin.id + "@" + serverId);
- assertThat(log.get(3)).committer().hasSameDateAs(log.get(3).author);
- }
+ assertThat(log.get(3))
+ .message()
+ .isEqualTo(
+ "Update group\n"
+ + "\n"
+ + ("Add-group: " + group2.name + " <" + group2.id + ">\n")
+ + ("Add-group: Registered Users <global:Registered-Users>"));
+ assertThat(log.get(3)).author().name().isEqualTo(admin.fullName);
+ assertThat(log.get(3)).author().email().isEqualTo(admin.id + "@" + serverId);
+ assertThat(log.get(3)).committer().hasSameDateAs(log.get(3).author);
}
@Test
@@ -236,19 +232,4 @@
assertThat(commitDates).named("commit timestamps for %s", result).isOrdered();
return result.build();
}
-
- private class BlockReviewDbUpdatesForGroups implements AutoCloseable {
- BlockReviewDbUpdatesForGroups() {
- blockReviewDbUpdates(true);
- }
-
- @Override
- public void close() throws Exception {
- blockReviewDbUpdates(false);
- }
-
- private void blockReviewDbUpdates(boolean block) {
- cfg.setBoolean("user", null, "blockReviewDbGroupUpdates", block);
- }
- }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 8aa03a4..0e5da12 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -32,7 +32,6 @@
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import static java.util.stream.Collectors.toList;
-import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.util.concurrent.AtomicLongMap;
@@ -66,7 +65,6 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Account;
@@ -89,7 +87,6 @@
import com.google.gerrit.server.util.MagicBranch;
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.TestTimeUtil;
-import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.io.IOException;
import java.lang.annotation.Retention;
@@ -1200,38 +1197,6 @@
}
@Test
- @Sandboxed
- public void blockReviewDbUpdatesOnGroupCreation() throws Exception {
- assume().that(groupsInNoteDb()).isFalse();
- try (AutoCloseable ctx = createBlockReviewDbGroupUpdatesContext()) {
- gApi.groups().create(name("foo"));
- fail("Expected RestApiException: Updates to groups in ReviewDb are blocked");
- } catch (RestApiException e) {
- assertWriteGroupToReviewDbBlockedException(e);
- }
- }
-
- @Test
- @Sandboxed
- public void blockReviewDbUpdatesOnGroupUpdate() throws Exception {
- assume().that(groupsInNoteDb()).isFalse();
- String group1 = gApi.groups().create(name("foo")).get().id;
- String group2 = gApi.groups().create(name("bar")).get().id;
- try (AutoCloseable ctx = createBlockReviewDbGroupUpdatesContext()) {
- gApi.groups().id(group1).addGroups(group2);
- fail("Expected RestApiException: Updates to groups in ReviewDb are blocked");
- } catch (RestApiException e) {
- assertWriteGroupToReviewDbBlockedException(e);
- }
- }
-
- private void assertWriteGroupToReviewDbBlockedException(Exception e) throws Exception {
- Throwable t = Throwables.getRootCause(e);
- assertThat(t).isInstanceOf(OrmException.class);
- assertThat(t.getMessage()).isEqualTo("Updates to groups in ReviewDb are blocked");
- }
-
- @Test
@IgnoreGroupInconsistencies
public void stalenessChecker() throws Exception {
assume().that(readGroupsFromNoteDb()).isTrue();
@@ -1527,16 +1492,6 @@
return groupsInNoteDb() && cfg.getBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, false);
}
- private AutoCloseable createBlockReviewDbGroupUpdatesContext() {
- cfg.setBoolean("user", null, "blockReviewDbGroupUpdates", true);
- return new AutoCloseable() {
- @Override
- public void close() {
- cfg.setBoolean("user", null, "blockReviewDbGroupUpdates", false);
- }
- };
- }
-
@Target({METHOD})
@Retention(RUNTIME)
private @interface IgnoreGroupInconsistencies {}
