opendev/gerrit
Code Issues Proposed changes

Simplify implementation of group.PutName

Browse Source 
The use of the GroupDetailFactory seems to be a relic of previous
code. Neither a comment nor a test enforces that we keep this
apparent unnecessary conversion and hence we get rid of it.

Change-Id: I397d6f0b78782c6fe48c61b00e9ed379b8f551aa
This commit is contained in:
Alice Kober-Sotzek 2017-07-25 11:00:57 +02:00
parent 59382e31be
commit 6108cc8292
5 changed files with 12 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDetail.java
View File

@ -14,22 +14,16 @@
package com.google.gerrit.common.data;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroupById;
import com.google.gerrit.reviewdb.client.AccountGroupMember;
import java.util.List;
public class GroupDetail {
public AccountGroup group;
public List<AccountGroupMember> members;
public List<AccountGroupById> includes;
public GroupDetail() {}
public void setGroup(AccountGroup g) {
group = g;
}
public void setMembers(List<AccountGroupMember> m) {
members = m;
}

9
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java
View File

@ -34,20 +34,15 @@ public class GroupDetailFactory implements Callable<GroupDetail> {
private final ReviewDb db;
private final GroupControl.Factory groupControl;
private final GroupCache groupCache;
private final AccountGroup.Id groupId;
private GroupControl control;
@Inject
GroupDetailFactory(
ReviewDb db,
GroupControl.Factory groupControl,
GroupCache groupCache,
@Assisted AccountGroup.Id groupId) {
ReviewDb db, GroupControl.Factory groupControl, @Assisted AccountGroup.Id groupId) {
this.db = db;
this.groupControl = groupControl;
this.groupCache = groupCache;
this.groupId = groupId;
}
@ -55,9 +50,7 @@ public class GroupDetailFactory implements Callable<GroupDetail> {
@Override
public GroupDetail call() throws OrmException, NoSuchGroupException {
control = groupControl.validateFor(groupId);
AccountGroup group = groupCache.get(groupId);
GroupDetail detail = new GroupDetail();
detail.setGroup(group);
detail.setMembers(loadMembers());
detail.setIncludes(loadIncludes());
return detail;

4
gerrit-server/src/main/java/com/google/gerrit/server/api/groups/GroupApiImpl.java
View File

@ -16,13 +16,11 @@ package com.google.gerrit.server.api.groups;
import static com.google.gerrit.server.api.ApiUtil.asRestApiException;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.api.groups.GroupApi;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.GroupAuditEventInfo;
import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.GroupOptionsInfo;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.group.AddIncludedGroups;
import com.google.gerrit.server.group.AddMembers;
@ -144,8 +142,6 @@ class GroupApiImpl implements GroupApi {
in.name = name;
try {
putName.apply(rsrc, in);
} catch (NoSuchGroupException e) {
throw new ResourceNotFoundException(name, e);
} catch (Exception e) {
throw asRestApiException("Cannot put group name", e);
}

29
gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java
View File

@ -15,8 +15,7 @@
package com.google.gerrit.server.group;
import com.google.common.base.Strings;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.DefaultInput;
@ -28,7 +27,6 @@ import com.google.gerrit.reviewdb.client.AccountGroupName;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.GroupDetailFactory;
import com.google.gerrit.server.git.RenameGroupOp;
import com.google.gerrit.server.group.PutName.Input;
import com.google.gwtorm.server.OrmException;
@ -36,7 +34,6 @@ import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Collections;
import java.util.Date;
import java.util.TimeZone;
import java.util.concurrent.Future;
@ -50,7 +47,6 @@ public class PutName implements RestModifyView<GroupResource, Input> {
private final Provider<ReviewDb> db;
private final GroupCache groupCache;
private final GroupDetailFactory.Factory groupDetailFactory;
private final RenameGroupOp.Factory renameGroupOpFactory;
private final Provider<IdentifiedUser> currentUser;
@ -58,12 +54,10 @@ public class PutName implements RestModifyView<GroupResource, Input> {
PutName(
Provider<ReviewDb> db,
GroupCache groupCache,
GroupDetailFactory.Factory groupDetailFactory,
RenameGroupOp.Factory renameGroupOpFactory,
Provider<IdentifiedUser> currentUser) {
this.db = db;
this.groupCache = groupCache;
this.groupDetailFactory = groupDetailFactory;
this.renameGroupOpFactory = renameGroupOpFactory;
this.currentUser = currentUser;
}
@ -71,7 +65,7 @@ public class PutName implements RestModifyView<GroupResource, Input> {
@Override
public String apply(GroupResource rsrc, Input input)
throws MethodNotAllowedException, AuthException, BadRequestException,
ResourceConflictException, OrmException, NoSuchGroupException, IOException {
ResourceConflictException, OrmException, IOException {
if (rsrc.toAccountGroup() == null) {
throw new MethodNotAllowedException();
} else if (!rsrc.getControl().isOwner()) {
@ -88,25 +82,25 @@ public class PutName implements RestModifyView<GroupResource, Input> {
return newName;
}
return renameGroup(rsrc.toAccountGroup(), newName).group.getName();
return renameGroup(rsrc.toAccountGroup(), newName);
}
private GroupDetail renameGroup(AccountGroup group, String newName)
throws ResourceConflictException, OrmException, NoSuchGroupException, IOException {
private String renameGroup(AccountGroup group, String newName)
throws ResourceConflictException, OrmException, IOException {
AccountGroup.Id groupId = group.getId();
AccountGroup.NameKey old = group.getNameKey();
AccountGroup.NameKey key = new AccountGroup.NameKey(newName);
try {
AccountGroupName id = new AccountGroupName(key, groupId);
db.get().accountGroupNames().insert(Collections.singleton(id));
db.get().accountGroupNames().insert(ImmutableList.of(id));
} catch (OrmException e) {
AccountGroupName other = db.get().accountGroupNames().get(key);
if (other != null) {
// If we are using this identity, don't report the exception.
//
if (other.getId().equals(groupId)) {
return groupDetailFactory.create(groupId).call();
return newName;
}
// Otherwise, someone else has this identity.
@ -117,12 +111,9 @@ public class PutName implements RestModifyView<GroupResource, Input> {
}
group.setNameKey(key);
db.get().accountGroups().update(Collections.singleton(group));
db.get().accountGroups().update(ImmutableList.of(group));
AccountGroupName priorName = db.get().accountGroupNames().get(old);
if (priorName != null) {
db.get().accountGroupNames().delete(Collections.singleton(priorName));
}
db.get().accountGroupNames().deleteKeys(ImmutableList.of(old));
groupCache.evict(group);
groupCache.evictAfterRename(old, key);
@ -136,6 +127,6 @@ public class PutName implements RestModifyView<GroupResource, Input> {
newName)
.start(0, TimeUnit.MILLISECONDS);
return groupDetailFactory.create(groupId).call();
return newName;
}
}

3
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/RenameGroupCommand.java
View File

@ -14,7 +14,6 @@
package com.google.gerrit.sshd.commands;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
@ -52,7 +51,7 @@ public class RenameGroupCommand extends SshCommand {
PutName.Input input = new PutName.Input();
input.name = newGroupName;
putName.apply(rsrc, input);
} catch (RestApiException | OrmException | IOException | NoSuchGroupException e) {
} catch (RestApiException | OrmException | IOException e) {
throw die(e);
}
}