opendev/gerrit
Code Issues Proposed changes

Cleanup /groups/ REST endpoints

Browse Source 
UUIDs for groups are the future. Assume the id passed to name a
resource is a URL encoded UUID, and do not use the 'uuid-' prefix.
If we get an invalid UUID but it is an AccountGroup.Id, accept it.

Change-Id: I592ab574c4d6cc7737ee4d32ec379302d199dfaf
This commit is contained in:
Shawn Pearce
2013-01-10 16:03:02 -08:00
parent 3d217418d4
commit 89136bc832
7 changed files with 88 additions and 117 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
Documentation/rest-api.txt
View File

@@ -261,56 +261,45 @@ and accepts the same options as query parameters.
{ {
"Administrators": { "Administrators": {
"kind": "gerritcodereview#group", "kind": "gerritcodereview#group",
"id": "uuid-6a1e70e1a88782771a91808c8af9bbb7a9871389", "id": "6a1e70e1a88782771a91808c8af9bbb7a9871389",
"uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389",
"group_id": 1,
"description": "Gerrit Site Administrators", "description": "Gerrit Site Administrators",
"is_visible_to_all": false, "group_id": 1,
"owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389"
}, },
"Anonymous Users": { "Anonymous Users": {
"kind": "gerritcodereview#group", "kind": "gerritcodereview#group",
"id": "uuid-global%3AAnonymous-Users", "id": "global%3AAnonymous-Users",
"uuid": "global:Anonymous-Users",
"group_id": 2,
"description": "Any user, signed-in or not", "description": "Any user, signed-in or not",
"is_visible_to_all": false, "group_id": 2,
"owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389"
}, },
"MyProject_Committers": { "MyProject_Committers": {
"kind": "gerritcodereview#group", "kind": "gerritcodereview#group",
"id": "uuid-834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7", "id": "834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7",
"uuid": "834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7", "visible_to_all": true,
"group_id": 6, "group_id": 6,
"is_visible_to_all": true, "owner_id": "834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7"
"owner_uuid": "834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7"
}, },
"Non-Interactive Users": { "Non-Interactive Users": {
"kind": "gerritcodereview#group", "kind": "gerritcodereview#group",
"id": "uuid-5057f3cbd3519d6ab69364429a89ffdffba50f73", "id": "5057f3cbd3519d6ab69364429a89ffdffba50f73",
"uuid": "5057f3cbd3519d6ab69364429a89ffdffba50f73",
"group_id": 4,
"description": "Users who perform batch actions on Gerrit", "description": "Users who perform batch actions on Gerrit",
"is_visible_to_all": false, "group_id": 4,
"owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389"
}, },
"Project Owners": { "Project Owners": {
"kind": "gerritcodereview#group", "kind": "gerritcodereview#group",
"id": "uuid-global%3AProject-Owners", "id": "global%3AProject-Owners",
"uuid": "global:Project-Owners",
"group_id": 5,
"description": "Any owner of the project", "description": "Any owner of the project",
"is_visible_to_all": false, "group_id": 5,
"owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389"
}, },
"Registered Users": { "Registered Users": {
"kind": "gerritcodereview#group", "kind": "gerritcodereview#group",
"id": "uuid-global%3ARegistered-Users", "id": "global%3ARegistered-Users",
"uuid": "global:Registered-Users",
"group_id": 3,
"description": "Any signed-in user", "description": "Any signed-in user",
"is_visible_to_all": false, "group_id": 3,
"owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389"
} }
} }
---- ----
@@ -352,12 +341,10 @@ Lists the included groups of a group.
[ [
{ {
"kind": "gerritcodereview#group", "kind": "gerritcodereview#group",
"id": "uuid-7ca042f4d5847936fcb90ca91057673157fd06fc", "id": "7ca042f4d5847936fcb90ca91057673157fd06fc",
"name": "MyProject-Verifiers", "name": "MyProject-Verifiers",
"uuid": "7ca042f4d5847936fcb90ca91057673157fd06fc",
"group_id": 38, "group_id": 38,
"is_visible_to_all": false, "owner_id": "7ca042f4d5847936fcb90ca91057673157fd06fc"
"owner_uuid": "7ca042f4d5847936fcb90ca91057673157fd06fc"
} }
] ]
---- ----

24
gerrit-gwtui/src/main/java/com/google/gerrit/client/groups/GroupInfo.java
View File

@@ -16,18 +16,32 @@ package com.google.gerrit.client.groups;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gwt.core.client.JavaScriptObject; import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.http.client.URL;
public class GroupInfo extends JavaScriptObject { public class GroupInfo extends JavaScriptObject {
public final AccountGroup.Id getGroupId() { public final AccountGroup.Id getGroupId() {
return new AccountGroup.Id(groupId()); return new AccountGroup.Id(group_id());
} }
public final native int groupId() /*-{ return this.group_id; }-*/; public final AccountGroup.UUID getGroupUUID() {
return new AccountGroup.UUID(URL.decodePathSegment(id()));
}
public final native String id() /*-{ return this.id; }-*/;
public final native String name() /*-{ return this.name; }-*/; public final native String name() /*-{ return this.name; }-*/;
public final native String uuid() /*-{ return this.uuid; }-*/; public final native boolean isVisibleToAll() /*-{ return this['visible_to_all'] ? true : false; }-*/;
public final native String description() /*-{ return this.description; }-*/; public final native String description() /*-{ return this.description; }-*/;
public final native boolean isVisibleToAll() /*-{ return this['is_visible_to_all'] ? true : false; }-*/;
public final native String ownerUuid() /*-{ return this.owner_uuid; }-*/; private final native int group_id() /*-{ return this.group_id; }-*/;
private final native String owner_uuid() /*-{ return this.owner_uuid; }-*/;
public final AccountGroup.UUID getOwnerUUID() {
String owner = owner_uuid();
if (owner != null) {
return new AccountGroup.UUID(URL.decodePathSegment(owner));
}
return null;
}
protected GroupInfo() { protected GroupInfo() {
} }

47
gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java
View File

@@ -28,37 +28,36 @@ class GetGroup implements RestReadView<GroupResource> {
@Override @Override
public Object apply(GroupResource resource) throws AuthException, public Object apply(GroupResource resource) throws AuthException,
BadRequestException, ResourceConflictException, Exception { BadRequestException, ResourceConflictException, Exception {
return parse(resource.getControl().getGroup()); return new GroupInfo(resource.getControl().getGroup());
}
public static GroupInfo parse(final GroupDescription.Basic group) {
GroupInfo info = new GroupInfo();
info.name = group.getName();
info.uuid = group.getGroupUUID().get();
info.isVisibleToAll = group.isVisibleToAll();
if (group instanceof GroupDescription.Internal) {
final AccountGroup internalGroup =
((GroupDescription.Internal) group).getAccountGroup();
info.groupId = internalGroup.getId().get();
info.description = Strings.emptyToNull(internalGroup.getDescription());
info.ownerUuid = internalGroup.getOwnerGroupUUID().get();
}
info.finish();
return info;
} }
public static class GroupInfo { public static class GroupInfo {
final String kind = "gerritcodereview#group"; final String kind = "gerritcodereview#group";
String id; String id;
String name; String name;
String uuid; Boolean visibleToAll;
int groupId;
String description;
boolean isVisibleToAll;
String ownerUuid;
void finish() { // These fields are only supplied for internal groups.
id = Url.encode(GroupsCollection.UUID_PREFIX + uuid); String description;
Integer groupId;
String ownerId;
GroupInfo(GroupDescription.Basic group) {
id = Url.encode(group.getGroupUUID().get());
name = Strings.emptyToNull(group.getName());
visibleToAll = group.isVisibleToAll() ? true : null;
if (group instanceof GroupDescription.Internal) {
set(((GroupDescription.Internal) group).getAccountGroup());
}
}
private void set(AccountGroup d) {
description = Strings.emptyToNull(d.getDescription());
groupId = d.getId().get();
ownerId = d.getOwnerGroupUUID() != null
? Url.encode(d.getOwnerGroupUUID().get())
: null;
} }
} }
} }

9
gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java
View File

@@ -14,17 +14,12 @@
package com.google.gerrit.server.group; package com.google.gerrit.server.group;
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.RestReadView; import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.group.GetGroup.GroupInfo; import com.google.gerrit.server.group.GetGroup.GroupInfo;
public class GetIncludedGroup implements RestReadView<IncludedGroupResource> { public class GetIncludedGroup implements RestReadView<IncludedGroupResource> {
@Override @Override
public GroupInfo apply(final IncludedGroupResource resource) throws AuthException, public GroupInfo apply(IncludedGroupResource rsrc) {
BadRequestException, ResourceConflictException, Exception { return new GetGroup.GroupInfo(rsrc.getGroup());
return GetGroup.parse(resource.getControl().getGroup());
} }
} }

30
gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java
View File

@@ -32,8 +32,6 @@ import com.google.inject.Provider;
public class GroupsCollection implements public class GroupsCollection implements
RestCollection<TopLevelResource, GroupResource> { RestCollection<TopLevelResource, GroupResource> {
public final static String UUID_PREFIX = "uuid-";
private final DynamicMap<RestView<GroupResource>> views; private final DynamicMap<RestView<GroupResource>> views;
private final Provider<ListGroups> list; private final Provider<ListGroups> list;
private final GroupControl.Factory groupControlFactory; private final GroupControl.Factory groupControlFactory;
@@ -75,31 +73,31 @@ public class GroupsCollection implements
return parse(id, groupControlFactory); return parse(id, groupControlFactory);
} }
public static GroupResource parse(final String id, public static GroupResource parse(String id, GroupControl.Factory factory)
final GroupControl.Factory groupControlFactory)
throws ResourceNotFoundException { throws ResourceNotFoundException {
final String decodedId = Url.decode(id); String uuid = Url.decode(id);
final GroupControl ctl; GroupControl ctl;
try { try {
if (decodedId.startsWith(UUID_PREFIX)) { ctl = factory.controlFor(new AccountGroup.UUID(uuid));
final String uuid = decodedId.substring(UUID_PREFIX.length()); } catch (NoSuchGroupException noSuchGroup) {
ctl = groupControlFactory.controlFor(new AccountGroup.UUID(uuid)); if (uuid.matches("^[1-9][0-9]*$")) {
} else { // Might be a legacy AccountGroup.Id.
try { try {
ctl = groupControlFactory.controlFor( ctl = factory.controlFor(AccountGroup.Id.parse(uuid));
new AccountGroup.Id(Integer.parseInt(decodedId))); } catch (IllegalArgumentException invalidId) {
} catch (NumberFormatException e) {
throw new ResourceNotFoundException(id); throw new ResourceNotFoundException(id);
}
}
} catch (NoSuchGroupException e) { } catch (NoSuchGroupException e) {
throw new ResourceNotFoundException(id); throw new ResourceNotFoundException(id);
} }
if (!ctl.isVisible() && !ctl.isOwner()) { } else {
throw new ResourceNotFoundException(id); throw new ResourceNotFoundException(id);
} }
}
if (ctl.isOwner()) {
return new GroupResource(ctl); return new GroupResource(ctl);
} }
throw new ResourceNotFoundException(id);
}
@Override @Override
public DynamicMap<RestView<GroupResource>> views() { public DynamicMap<RestView<GroupResource>> views() {

36
gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.group; package com.google.gerrit.server.group;
import com.google.common.collect.Maps; import com.google.common.collect.Maps;
import com.google.gerrit.common.data.GroupDescriptions;
import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -27,12 +28,11 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.OutputFormat; import com.google.gerrit.server.OutputFormat;
import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.VisibleGroups; import com.google.gerrit.server.account.VisibleGroups;
import com.google.gerrit.server.group.GetGroup.GroupInfo;
import com.google.gerrit.server.ioutil.ColumnFormatter; import com.google.gerrit.server.ioutil.ColumnFormatter;
import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.util.Url;
import com.google.gson.JsonElement; import com.google.gson.JsonElement;
import com.google.gson.reflect.TypeToken; import com.google.gson.reflect.TypeToken;
import com.google.gwtorm.client.KeyUtil;
import com.google.inject.Inject; import com.google.inject.Inject;
import org.kohsuke.args4j.Option; import org.kohsuke.args4j.Option;
@@ -127,15 +127,10 @@ public class ListGroups implements RestReadView<TopLevelResource> {
if (stdout == null) { if (stdout == null) {
final Map<String, GroupInfo> output = Maps.newTreeMap(); final Map<String, GroupInfo> output = Maps.newTreeMap();
for (final AccountGroup g : groupList) { for (AccountGroup g : groupList) {
final GroupInfo info = new GroupInfo(); GroupInfo info = new GroupInfo(GroupDescriptions.forAccountGroup(g));
info.name = g.getName();
info.groupId = g.getId().get();
info.setUuid(g.getGroupUUID());
info.description = g.getDescription();
info.isVisibleToAll = g.isVisibleToAll();
info.ownerUuid = g.getOwnerGroupUUID().get();
output.put(info.name, info); output.put(info.name, info);
info.name = null;
} }
return OutputFormat.JSON.newGson().toJsonTree(output, return OutputFormat.JSON.newGson().toJsonTree(output,
new TypeToken<Map<String, GroupInfo>>() {}.getType()); new TypeToken<Map<String, GroupInfo>>() {}.getType());
@@ -144,7 +139,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
for (final AccountGroup g : groupList) { for (final AccountGroup g : groupList) {
formatter.addColumn(g.getName()); formatter.addColumn(g.getName());
if (verboseOutput) { if (verboseOutput) {
formatter.addColumn(KeyUtil.decode(g.getGroupUUID().toString())); formatter.addColumn(g.getGroupUUID().get());
formatter.addColumn( formatter.addColumn(
g.getDescription() != null ? g.getDescription() : ""); g.getDescription() != null ? g.getDescription() : "");
formatter.addColumn(g.getType().toString()); formatter.addColumn(g.getType().toString());
@@ -152,7 +147,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
groupCache.get(g.getOwnerGroupUUID()); groupCache.get(g.getOwnerGroupUUID());
formatter.addColumn( formatter.addColumn(
owningGroup != null ? owningGroup.getName() : "n/a"); owningGroup != null ? owningGroup.getName() : "n/a");
formatter.addColumn(KeyUtil.decode(g.getOwnerGroupUUID().toString())); formatter.addColumn(g.getOwnerGroupUUID().get());
formatter.addColumn(Boolean.toString(g.isVisibleToAll())); formatter.addColumn(Boolean.toString(g.isVisibleToAll()));
} }
formatter.nextLine(); formatter.nextLine();
@@ -166,21 +161,4 @@ public class ListGroups implements RestReadView<TopLevelResource> {
} }
} }
} }
static class GroupInfo {
final String kind = "gerritcodereview#group";
transient String name;
String id;
String uuid;
int groupId;
String description;
boolean isVisibleToAll;
String ownerUuid;
void setUuid(AccountGroup.UUID u) {
uuid = u.get();
id = Url.encode(GroupsCollection.UUID_PREFIX + uuid);
}
}
} }

2
gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java
View File

@@ -63,7 +63,7 @@ public class ListIncludedGroups implements RestReadView<GroupResource> {
try { try {
GroupControl i = controlFactory.controlFor(u.getIncludeUUID()); GroupControl i = controlFactory.controlFor(u.getIncludeUUID());
if (ownerOfParent || i.isVisible()) { if (ownerOfParent || i.isVisible()) {
included.add(GetGroup.parse(i.getGroup())); included.add(new GetGroup.GroupInfo(i.getGroup()));
} }
} catch (NoSuchGroupException notFound) { } catch (NoSuchGroupException notFound) {
log.warn(String.format("Group %s no longer available, included into ", log.warn(String.format("Group %s no longer available, included into ",