Evict initial members of group created by SSH command from AccountCache

When a user is added to a group, the new group membership gets not
active as long as the old user information stays in the AccountCache.
This is why the user needs to be evicted from the AccountCache when
he is added to a group. Then the user information together with the
new group membership is reloaded from the database.

If a new group membership is assigned in the WebUI the user is
properly evicted from the AccountCache and the new group membership
is immediately active.

If a group gets created by an SSH command and initial members get
added to the new group, the initial members are not evicted from the
AccountCache. This is why the new group membership does not get
active immediatley if the user information is already in the
AccountCache.

This change now ensures that the initial members of a group created
by an SSH command get evicted from the AccountCache. The fix is done
in such a way that there is now only one implementation of the group
creation functionality which is used from both WebUI and SSH command.

Change-Id: I605277d564d3e2d45d6366afbf3af48cc04458d5
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
Bug: issue 814

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/CreateGroup.java

@@ -18,72 +18,34 @@ import com.google.gerrit.common.errors.NameAlreadyUsedException;
 import com.google.gerrit.httpd.rpc.Handler;
 import com.google.gerrit.reviewdb.Account;
 import com.google.gerrit.reviewdb.AccountGroup;
-import com.google.gerrit.reviewdb.AccountGroupMember;
-import com.google.gerrit.reviewdb.AccountGroupMemberAudit;
-import com.google.gerrit.reviewdb.AccountGroupName;
-import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountCache;
-import com.google.gwtorm.client.OrmDuplicateKeyException;
+import com.google.gerrit.server.account.PerformCreateGroup;
+import com.google.gerrit.server.account.PerformCreateGroup;
 import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 
-import java.util.Collections;
-
 class CreateGroup extends Handler<AccountGroup.Id> {
   interface Factory {
-    CreateGroup create(String newName);
+    CreateGroup create(String groupName);
   }
 
-  private final ReviewDb db;
+  private final PerformCreateGroup.Factory performCreateGroupFactory;
   private final IdentifiedUser user;
-  private final AccountCache accountCache;
-
-  private final String name;
+  private final String groupName;
 
   @Inject
-  CreateGroup(final ReviewDb db, final IdentifiedUser user,
-      final AccountCache accountCache,
-
-      @Assisted final String newName) {
-    this.db = db;
+  CreateGroup(final PerformCreateGroup.Factory performCreateGroupFactory,
+      final IdentifiedUser user, @Assisted final String groupName) {
+    this.performCreateGroupFactory = performCreateGroupFactory;
     this.user = user;
-    this.accountCache = accountCache;
-
-    this.name = newName;
+    this.groupName = groupName;
   }
 
   @Override
   public AccountGroup.Id call() throws OrmException, NameAlreadyUsedException {
-    final AccountGroup.NameKey key = new AccountGroup.NameKey(name);
-
-    if (db.accountGroupNames().get(key) != null) {
-      throw new NameAlreadyUsedException();
-    }
-
-    final AccountGroup.Id id = new AccountGroup.Id(db.nextAccountGroupId());
+    final PerformCreateGroup performCreateGroup = performCreateGroupFactory.create();
     final Account.Id me = user.getAccountId();
-
-    final AccountGroup group = new AccountGroup(key, id);
-    db.accountGroups().insert(Collections.singleton(group));
-
-    try {
-      final AccountGroupName n = new AccountGroupName(group);
-      db.accountGroupNames().insert(Collections.singleton(n));
-    } catch (OrmDuplicateKeyException dupeErr) {
-      db.accountGroups().delete(Collections.singleton(group));
-      throw new NameAlreadyUsedException();
-    }
-
-    AccountGroupMember member = new AccountGroupMember(//
-        new AccountGroupMember.Key(me, id));
-
-    db.accountGroupMembersAudit().insert(
-        Collections.singleton(new AccountGroupMemberAudit(member, me)));
-    db.accountGroupMembers().insert(Collections.singleton(member));
-
-    accountCache.evict(me);
-    return id;
+    return performCreateGroup.createGroup(groupName, null, null, me);
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/account/PerformCreateGroup.java

@@ -0,0 +1,118 @@
+// Copyright (C) 2011 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.account;
+
+import com.google.gerrit.common.errors.NameAlreadyUsedException;
+import com.google.gerrit.reviewdb.Account;
+import com.google.gerrit.reviewdb.AccountGroup;
+import com.google.gerrit.reviewdb.AccountGroupMember;
+import com.google.gerrit.reviewdb.AccountGroupMemberAudit;
+import com.google.gerrit.reviewdb.AccountGroupName;
+import com.google.gerrit.reviewdb.ReviewDb;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gwtorm.client.OrmDuplicateKeyException;
+import com.google.gwtorm.client.OrmException;
+import com.google.inject.Inject;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+public class PerformCreateGroup {
+
+  public interface Factory {
+    PerformCreateGroup create();
+  }
+
+  private final ReviewDb db;
+  private final AccountCache accountCache;
+  private final IdentifiedUser currentUser;
+
+  @Inject
+  PerformCreateGroup(final ReviewDb db, final AccountCache accountCache,
+      final IdentifiedUser currentUser) {
+    this.db = db;
+    this.accountCache = accountCache;
+    this.currentUser = currentUser;
+  }
+
+  /**
+   * Creates a new group.
+   *
+   * @param groupName the name for the new group
+   * @param groupDescription the description of the new group, <code>null</code>
+   *        if no description
+   * @param ownerGroupId the group that should own the new group, if
+   *        <code>null</code> the new group will own itself
+   * @param initialMembers initial members to be added to the new group
+   * @return the id of the new group
+   * @throws OrmException is thrown in case of any data store read or write
+   *         error
+   * @throws NameAlreadyUsedException is thrown in case a group with the given
+   *         name already exists
+   */
+  public AccountGroup.Id createGroup(final String groupName,
+      final String groupDescription, final AccountGroup.Id ownerGroupId,
+      final Account.Id... initialMembers) throws OrmException,
+      NameAlreadyUsedException {
+    final AccountGroup.Id groupId =
+        new AccountGroup.Id(db.nextAccountGroupId());
+    final AccountGroup.NameKey nameKey = new AccountGroup.NameKey(groupName);
+    final AccountGroup group = new AccountGroup(nameKey, groupId);
+    if (ownerGroupId != null) {
+      group.setOwnerGroupId(ownerGroupId);
+    }
+    if (groupDescription != null) {
+      group.setDescription(groupDescription);
+    }
+    final AccountGroupName gn = new AccountGroupName(group);
+    // first insert the group name to validate that the group name hasn't
+    // already been used to create another group
+    try {
+      db.accountGroupNames().insert(Collections.singleton(gn));
+    } catch (OrmDuplicateKeyException e) {
+      throw new NameAlreadyUsedException();
+    }
+    db.accountGroups().insert(Collections.singleton(group));
+
+    addMembers(groupId, initialMembers);
+
+    return groupId;
+  }
+
+  private void addMembers(final AccountGroup.Id groupId,
+      final Account.Id... members) throws OrmException {
+    final List<AccountGroupMember> memberships =
+        new ArrayList<AccountGroupMember>();
+    final List<AccountGroupMemberAudit> membershipsAudit =
+        new ArrayList<AccountGroupMemberAudit>();
+    for (Account.Id accountId : members) {
+      final AccountGroupMember membership =
+          new AccountGroupMember(new AccountGroupMember.Key(accountId, groupId));
+      memberships.add(membership);
+
+      final AccountGroupMemberAudit audit =
+          new AccountGroupMemberAudit(membership, currentUser.getAccountId());
+      membershipsAudit.add(audit);
+    }
+    db.accountGroupMembers().insert(memberships);
+    db.accountGroupMembersAudit().insert(membershipsAudit);
+
+    for (Account.Id accountId : members) {
+      accountCache.evict(accountId);
+    }
+  }
+
+}

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java

@@ -21,6 +21,7 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.RequestCleanup;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.account.PerformCreateGroup;
 import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.git.ReceiveCommits;
 import com.google.gerrit.server.mail.AbandonedSender;
@@ -69,5 +70,6 @@ public class GerritRequestModule extends FactoryModule {
     factory(MergedSender.Factory.class);
     factory(MergeFailSender.Factory.class);
     factory(RegisterNewEmailSender.Factory.class);
+    factory(PerformCreateGroup.Factory.class);
   }
 }

gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java

@@ -347,6 +347,10 @@ public abstract class BaseCommand implements Command {
     return new UnloggedFailure(1, "fatal: " + msg);
   }
 
+  protected UnloggedFailure die(Throwable why) {
+    return new UnloggedFailure(1, "fatal: " + why.getMessage(), why);
+  }
+
   private final class TaskThunk implements CancelableRunnable, ProjectRunnable {
     private final CommandRunnable thunk;
     private final Context context;

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/AdminCreateGroup.java

@@ -14,16 +14,13 @@
 
 package com.google.gerrit.sshd.commands;
 
+import com.google.gerrit.common.errors.NameAlreadyUsedException;
 import com.google.gerrit.reviewdb.Account;
 import com.google.gerrit.reviewdb.AccountGroup;
-import com.google.gerrit.reviewdb.AccountGroupMember;
-import com.google.gerrit.reviewdb.AccountGroupMemberAudit;
-import com.google.gerrit.reviewdb.AccountGroupName;
-import com.google.gerrit.reviewdb.ReviewDb;
-import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.PerformCreateGroup;
+import com.google.gerrit.server.account.PerformCreateGroup;
 import com.google.gerrit.sshd.AdminCommand;
 import com.google.gerrit.sshd.BaseCommand;
-import com.google.gwtorm.client.OrmDuplicateKeyException;
 import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 
@@ -32,10 +29,7 @@ import org.kohsuke.args4j.Argument;
 import org.kohsuke.args4j.Option;
 
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Set;
 
 /**
@@ -54,12 +48,6 @@ public class AdminCreateGroup extends BaseCommand {
   @Argument(index = 0, required = true, metaVar = "GROUP", usage = "name of group to be created")
   private String groupName;
 
-  @Inject
-  private IdentifiedUser currentUser;
-
-  @Inject
-  private ReviewDb db;
-
   private final Set<Account.Id> initialMembers = new HashSet<Account.Id>();
 
   @Option(name = "--member", aliases = {"-m"}, metaVar = "USERNAME", usage = "initial set of users to become members of the group")
@@ -67,6 +55,9 @@ public class AdminCreateGroup extends BaseCommand {
     initialMembers.add(id);
   }
 
+  @Inject
+  private PerformCreateGroup.Factory performCreateGroupFactory;
+
   @Override
   public void start(Environment env) throws IOException {
     startThread(new CommandRunnable() {
@@ -79,37 +70,12 @@ public class AdminCreateGroup extends BaseCommand {
   }
 
   private void createGroup() throws OrmException, UnloggedFailure {
-    AccountGroup.Id groupId = new AccountGroup.Id(db.nextAccountGroupId());
-    AccountGroup.NameKey nameKey = new AccountGroup.NameKey(groupName);
-    AccountGroup group = new AccountGroup(nameKey, groupId);
-    if (ownerGroupId != null) {
-      group.setOwnerGroupId(ownerGroupId);
-    }
-    if (groupDescription != null) {
-      group.setDescription(groupDescription);
-    }
-    AccountGroupName gn = new AccountGroupName(group);
-    // first insert the group name to validate that the group name hasn't already been
-    // used to create another group
+    final PerformCreateGroup performCreateGroup =
+        performCreateGroupFactory.create();
     try {
-      db.accountGroupNames().insert(Collections.singleton(gn));
-    } catch (OrmDuplicateKeyException e) {
-      throw die("group '" + groupName + "' already exists");
+      performCreateGroup.createGroup(groupName, groupDescription, ownerGroupId, initialMembers.toArray(new Account.Id[initialMembers.size()]));
+    } catch (NameAlreadyUsedException e) {
+      throw die(e);
     }
-    db.accountGroups().insert(Collections.singleton(group));
-
-    List<AccountGroupMember> memberships = new ArrayList<AccountGroupMember>();
-    List<AccountGroupMemberAudit> membershipsAudit = new ArrayList<AccountGroupMemberAudit>();
-    for (Account.Id accountId : initialMembers) {
-      AccountGroupMember membership =
-          new AccountGroupMember(new AccountGroupMember.Key(accountId, groupId));
-      memberships.add(membership);
-
-      AccountGroupMemberAudit audit =
-          new AccountGroupMemberAudit(membership, currentUser.getAccountId());
-      membershipsAudit.add(audit);
-    }
-    db.accountGroupMembers().insert(memberships);
-    db.accountGroupMembersAudit().insert(membershipsAudit);
   }
 }