opendev/gerrit
Code Issues Proposed changes

Revert "Add config option to prevent group updates while migrating groups"

Browse Source 
This reverts commit 809de7e70c.

Reason for revert: Groups are migrated offline with change I530116c8c5a.
Hence, we don't need to prevent any intermediate group updates.

Change-Id: I28113f8dbca7698a2335ae315405e7893636a745
This commit is contained in:
Alice Kober-Sotzek
2018-03-15 17:13:51 +00:00
committed by Edwin Kempin
parent 3afca9eb0d
commit 033b48c396
3 changed files with 43 additions and 121 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
java/com/google/gerrit/server/group/db/GroupsUpdate.java
View File

@@ -43,7 +43,6 @@ import com.google.gerrit.server.account.GroupCache;
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.Future;
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 @@ public class GroupsUpdate {
private final GroupsMigration groupsMigration;
private final GitReferenceUpdated gitRefUpdated;
private final RetryHelper retryHelper;
private final boolean reviewDbUpdatesAreBlocked;
@Inject
GroupsUpdate(
@@ -131,7 +128,6 @@ public class GroupsUpdate {
@GerritPersonIdent PersonIdent serverIdent,
MetaDataUpdate.InternalFactory metaDataUpdateInternalFactory,
GroupsMigration groupsMigration,
@GerritServerConfig Config config,
GitReferenceUpdated gitRefUpdated,
RetryHelper retryHelper,
@Assisted @Nullable IdentifiedUser currentUser) {
@@ -152,7 +148,6 @@ public class GroupsUpdate {
getMetaDataUpdateFactory(
metaDataUpdateInternalFactory, currentUser, serverIdent, auditLogFormatter);
authorIdent = getAuthorIdent(serverIdent, currentUser);
reviewDbUpdatesAreBlocked = config.getBoolean("user", null, "blockReviewDbGroupUpdates", false);
}
private static MetaDataUpdateFactory getMetaDataUpdateFactory(
@@ -276,7 +271,6 @@ public class GroupsUpdate {
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 @@ public class GroupsUpdate {
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 @@ public class GroupsUpdate {
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;

105
javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java
View File

@@ -85,13 +85,11 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
@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 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
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 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
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);
}
}
}

45
javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
View File

@@ -32,7 +32,6 @@ import static java.lang.annotation.ElementType.METHOD;
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.AuthException;
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.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.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.io.IOException;
import java.lang.annotation.Retention;
@@ -1199,38 +1196,6 @@ public class GroupsIT extends AbstractDaemonTest {
allUsers, groupRef(REGISTERED_USERS), RefNames.REFS_GROUPS + "*", true, Permission.READ);
}
@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 {
@@ -1527,16 +1492,6 @@ public class GroupsIT extends AbstractDaemonTest {
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 {}