From 89136bc83205248e6c76bb0c6e589fd5b3c5b0c7 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 10 Jan 2013 16:03:02 -0800 Subject: [PATCH] Cleanup /groups/ REST endpoints 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 --- Documentation/rest-api.txt | 53 +++++++------------ .../gerrit/client/groups/GroupInfo.java | 24 +++++++-- .../google/gerrit/server/group/GetGroup.java | 47 ++++++++-------- .../gerrit/server/group/GetIncludedGroup.java | 9 +--- .../gerrit/server/group/GroupsCollection.java | 34 ++++++------ .../gerrit/server/group/ListGroups.java | 36 +++---------- .../server/group/ListIncludedGroups.java | 2 +- 7 files changed, 88 insertions(+), 117 deletions(-) diff --git a/Documentation/rest-api.txt b/Documentation/rest-api.txt index 08b1d99ca3..afc5673800 100644 --- a/Documentation/rest-api.txt +++ b/Documentation/rest-api.txt @@ -261,56 +261,45 @@ and accepts the same options as query parameters. { "Administrators": { "kind": "gerritcodereview#group", - "id": "uuid-6a1e70e1a88782771a91808c8af9bbb7a9871389", - "uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389", - "group_id": 1, + "id": "6a1e70e1a88782771a91808c8af9bbb7a9871389", "description": "Gerrit Site Administrators", - "is_visible_to_all": false, - "owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" + "group_id": 1, + "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389" }, "Anonymous Users": { "kind": "gerritcodereview#group", - "id": "uuid-global%3AAnonymous-Users", - "uuid": "global:Anonymous-Users", - "group_id": 2, + "id": "global%3AAnonymous-Users", "description": "Any user, signed-in or not", - "is_visible_to_all": false, - "owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" + "group_id": 2, + "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389" }, "MyProject_Committers": { "kind": "gerritcodereview#group", - "id": "uuid-834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7", - "uuid": "834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7", + "id": "834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7", + "visible_to_all": true, "group_id": 6, - "is_visible_to_all": true, - "owner_uuid": "834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7" + "owner_id": "834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7" }, "Non-Interactive Users": { "kind": "gerritcodereview#group", - "id": "uuid-5057f3cbd3519d6ab69364429a89ffdffba50f73", - "uuid": "5057f3cbd3519d6ab69364429a89ffdffba50f73", - "group_id": 4, + "id": "5057f3cbd3519d6ab69364429a89ffdffba50f73", "description": "Users who perform batch actions on Gerrit", - "is_visible_to_all": false, - "owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" + "group_id": 4, + "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389" }, "Project Owners": { "kind": "gerritcodereview#group", - "id": "uuid-global%3AProject-Owners", - "uuid": "global:Project-Owners", - "group_id": 5, + "id": "global%3AProject-Owners", "description": "Any owner of the project", - "is_visible_to_all": false, - "owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" + "group_id": 5, + "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389" }, "Registered Users": { "kind": "gerritcodereview#group", - "id": "uuid-global%3ARegistered-Users", - "uuid": "global:Registered-Users", - "group_id": 3, + "id": "global%3ARegistered-Users", "description": "Any signed-in user", - "is_visible_to_all": false, - "owner_uuid": "6a1e70e1a88782771a91808c8af9bbb7a9871389" + "group_id": 3, + "owner_id": "6a1e70e1a88782771a91808c8af9bbb7a9871389" } } ---- @@ -352,12 +341,10 @@ Lists the included groups of a group. [ { "kind": "gerritcodereview#group", - "id": "uuid-7ca042f4d5847936fcb90ca91057673157fd06fc", + "id": "7ca042f4d5847936fcb90ca91057673157fd06fc", "name": "MyProject-Verifiers", - "uuid": "7ca042f4d5847936fcb90ca91057673157fd06fc", "group_id": 38, - "is_visible_to_all": false, - "owner_uuid": "7ca042f4d5847936fcb90ca91057673157fd06fc" + "owner_id": "7ca042f4d5847936fcb90ca91057673157fd06fc" } ] ---- diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/groups/GroupInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/groups/GroupInfo.java index 5fed85dbce..503035e5ad 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/groups/GroupInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/groups/GroupInfo.java @@ -16,18 +16,32 @@ package com.google.gerrit.client.groups; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gwt.core.client.JavaScriptObject; +import com.google.gwt.http.client.URL; public class GroupInfo extends JavaScriptObject { 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 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 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() { } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java index 769225b0f7..aba56d3776 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java @@ -28,37 +28,36 @@ class GetGroup implements RestReadView { @Override public Object apply(GroupResource resource) throws AuthException, BadRequestException, ResourceConflictException, Exception { - return parse(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; + return new GroupInfo(resource.getControl().getGroup()); } public static class GroupInfo { final String kind = "gerritcodereview#group"; String id; String name; - String uuid; - int groupId; - String description; - boolean isVisibleToAll; - String ownerUuid; + Boolean visibleToAll; - void finish() { - id = Url.encode(GroupsCollection.UUID_PREFIX + uuid); + // These fields are only supplied for internal groups. + 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; } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java index cc5373109b..86879bdf7a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java @@ -14,17 +14,12 @@ 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.server.group.GetGroup.GroupInfo; public class GetIncludedGroup implements RestReadView { - @Override - public GroupInfo apply(final IncludedGroupResource resource) throws AuthException, - BadRequestException, ResourceConflictException, Exception { - return GetGroup.parse(resource.getControl().getGroup()); + public GroupInfo apply(IncludedGroupResource rsrc) { + return new GetGroup.GroupInfo(rsrc.getGroup()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java index 23cb807e1f..01bb5d822f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java @@ -32,8 +32,6 @@ import com.google.inject.Provider; public class GroupsCollection implements RestCollection { - public final static String UUID_PREFIX = "uuid-"; - private final DynamicMap> views; private final Provider list; private final GroupControl.Factory groupControlFactory; @@ -75,30 +73,30 @@ public class GroupsCollection implements return parse(id, groupControlFactory); } - public static GroupResource parse(final String id, - final GroupControl.Factory groupControlFactory) + public static GroupResource parse(String id, GroupControl.Factory factory) throws ResourceNotFoundException { - final String decodedId = Url.decode(id); - final GroupControl ctl; + String uuid = Url.decode(id); + GroupControl ctl; try { - if (decodedId.startsWith(UUID_PREFIX)) { - final String uuid = decodedId.substring(UUID_PREFIX.length()); - ctl = groupControlFactory.controlFor(new AccountGroup.UUID(uuid)); - } else { + ctl = factory.controlFor(new AccountGroup.UUID(uuid)); + } catch (NoSuchGroupException noSuchGroup) { + if (uuid.matches("^[1-9][0-9]*$")) { + // Might be a legacy AccountGroup.Id. try { - ctl = groupControlFactory.controlFor( - new AccountGroup.Id(Integer.parseInt(decodedId))); - } catch (NumberFormatException e) { + ctl = factory.controlFor(AccountGroup.Id.parse(uuid)); + } catch (IllegalArgumentException invalidId) { + throw new ResourceNotFoundException(id); + } catch (NoSuchGroupException e) { throw new ResourceNotFoundException(id); } + } else { + throw new ResourceNotFoundException(id); } - } catch (NoSuchGroupException e) { - throw new ResourceNotFoundException(id); } - if (!ctl.isVisible() && !ctl.isOwner()) { - throw new ResourceNotFoundException(id); + if (ctl.isOwner()) { + return new GroupResource(ctl); } - return new GroupResource(ctl); + throw new ResourceNotFoundException(id); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java index ec03f3a901..a5b43bdfe5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.group; import com.google.common.collect.Maps; +import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.AuthException; 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.account.GroupCache; 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.project.ProjectControl; -import com.google.gerrit.server.util.Url; import com.google.gson.JsonElement; import com.google.gson.reflect.TypeToken; -import com.google.gwtorm.client.KeyUtil; import com.google.inject.Inject; import org.kohsuke.args4j.Option; @@ -127,15 +127,10 @@ public class ListGroups implements RestReadView { if (stdout == null) { final Map output = Maps.newTreeMap(); - for (final AccountGroup g : groupList) { - final GroupInfo info = new GroupInfo(); - 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(); + for (AccountGroup g : groupList) { + GroupInfo info = new GroupInfo(GroupDescriptions.forAccountGroup(g)); output.put(info.name, info); + info.name = null; } return OutputFormat.JSON.newGson().toJsonTree(output, new TypeToken>() {}.getType()); @@ -144,7 +139,7 @@ public class ListGroups implements RestReadView { for (final AccountGroup g : groupList) { formatter.addColumn(g.getName()); if (verboseOutput) { - formatter.addColumn(KeyUtil.decode(g.getGroupUUID().toString())); + formatter.addColumn(g.getGroupUUID().get()); formatter.addColumn( g.getDescription() != null ? g.getDescription() : ""); formatter.addColumn(g.getType().toString()); @@ -152,7 +147,7 @@ public class ListGroups implements RestReadView { groupCache.get(g.getOwnerGroupUUID()); formatter.addColumn( 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.nextLine(); @@ -166,21 +161,4 @@ public class ListGroups implements RestReadView { } } } - - 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); - } - } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java index b948a8b427..3924857401 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java @@ -63,7 +63,7 @@ public class ListIncludedGroups implements RestReadView { try { GroupControl i = controlFactory.controlFor(u.getIncludeUUID()); if (ownerOfParent || i.isVisible()) { - included.add(GetGroup.parse(i.getGroup())); + included.add(new GetGroup.GroupInfo(i.getGroup())); } } catch (NoSuchGroupException notFound) { log.warn(String.format("Group %s no longer available, included into ",