SONAR-6476 SONAr-6477 Apply feedback from PR

diff --git a/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/AddUserAction.java b/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/AddUserAction.java
index 0a8ff5781d8a637ff9921653c673baf5ea82ee5d..8753ff4002525502b7a85d8963c65f4647e16560 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/AddUserAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/AddUserAction.java
@@ -37,8 +37,8 @@ import static org.sonar.core.persistence.MyBatis.closeQuietly;
 
 public class AddUserAction implements UserGroupsWsAction {
 
-  private static final String PARAM_ID = "id";
-  private static final String PARAM_LOGIN = "login";
+  private static final String PARAM_ID = "groupId";
+  private static final String PARAM_LOGIN = "userLogin";
 
   private final DbClient dbClient;
   private final UserSession userSession;
@@ -72,7 +72,7 @@ public class AddUserAction implements UserGroupsWsAction {
     userSession.checkLoggedIn().checkGlobalPermission(GlobalPermissions.SYSTEM_ADMIN);
 
     Long groupId = request.mandatoryParamAsLong(PARAM_ID);
-    String login = request.param(PARAM_LOGIN);
+    String login = request.mandatoryParam(PARAM_LOGIN);
 
     DbSession dbSession = dbClient.openSession(false);
     try {
@@ -82,7 +82,7 @@ public class AddUserAction implements UserGroupsWsAction {
         throw new NotFoundException(String.format("Could not find a user with login '%s'", login));
       }
 
-      if (!userIsAlreadyMemberOf(dbSession, login, group)) {
+      if (userIsNotYetMemberOf(dbSession, login, group)) {
         UserGroupDto userGroup = new UserGroupDto().setGroupId(group.getId()).setUserId(user.getId());
         dbClient.userGroupDao().insert(dbSession, userGroup);
         dbSession.commit();
@@ -95,7 +95,7 @@ public class AddUserAction implements UserGroupsWsAction {
 
   }
 
-  private boolean userIsAlreadyMemberOf(DbSession dbSession, String login, GroupDto group) {
-    return dbClient.groupMembershipDao().selectGroupsByLogins(dbSession, Arrays.asList(login)).get(login).contains(group.getName());
+  private boolean userIsNotYetMemberOf(DbSession dbSession, String login, GroupDto group) {
+    return !dbClient.groupMembershipDao().selectGroupsByLogins(dbSession, Arrays.asList(login)).get(login).contains(group.getName());
   }
 }

diff --git a/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/RemoveUserAction.java b/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/RemoveUserAction.java
index 1e5dcc66d303d9234c7dfd53da4f37346d3c215b..b8fc4d863b108e274e092b450c5056a8222e0096 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/RemoveUserAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/RemoveUserAction.java
@@ -36,8 +36,8 @@ import static org.sonar.core.persistence.MyBatis.closeQuietly;
 
 public class RemoveUserAction implements UserGroupsWsAction {
 
-  private static final String PARAM_ID = "id";
-  private static final String PARAM_LOGIN = "login";
+  private static final String PARAM_ID = "groupId";
+  private static final String PARAM_LOGIN = "userLogin";
 
   private final DbClient dbClient;
   private final UserSession userSession;
@@ -71,7 +71,7 @@ public class RemoveUserAction implements UserGroupsWsAction {
     userSession.checkLoggedIn().checkGlobalPermission(GlobalPermissions.SYSTEM_ADMIN);
 
     Long groupId = request.mandatoryParamAsLong(PARAM_ID);
-    String login = request.param(PARAM_LOGIN);
+    String login = request.mandatoryParam(PARAM_LOGIN);
 
     DbSession dbSession = dbClient.openSession(false);
     try {

diff --git a/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/UsersAction.java b/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/UsersAction.java
index c6c5d185e1fc3262d9f3da5e48a2c5276d357630..eabc744a71610a051bee000d52c178f8220f058b 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/UsersAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/UsersAction.java
@@ -29,12 +29,11 @@ import org.sonar.api.server.ws.WebService.Param;
 import org.sonar.api.utils.Paging;
 import org.sonar.api.utils.text.JsonWriter;
 import org.sonar.core.persistence.DbSession;
-import org.sonar.core.user.GroupDto;
+import org.sonar.core.persistence.MyBatis;
 import org.sonar.core.user.GroupMembershipQuery;
 import org.sonar.core.user.UserMembershipDto;
 import org.sonar.core.user.UserMembershipQuery;
 import org.sonar.server.db.DbClient;
-import org.sonar.server.exceptions.NotFoundException;
 
 public class UsersAction implements UserGroupsWsAction {
 
@@ -54,7 +53,7 @@ public class UsersAction implements UserGroupsWsAction {
   @Override
   public void define(NewController context) {
     NewAction action = context.createAction("users")
-      .setDescription("List the members of a group.")
+      .setDescription("Search for users with membership information with respect to a group.")
       .setHandler(this)
       .setResponseExample(getClass().getResource("example-users.json"))
       .setSince("5.2");
@@ -65,13 +64,11 @@ public class UsersAction implements UserGroupsWsAction {
       .setRequired(true);
 
     action.createParam(PARAM_SELECTED)
-      .setDescription("If specified, only show users who belong to this group (selected) or not (deselected).")
+      .setDescription("If specified, only show users who belong to a group (selected=selected) or only those who do not (selected=deselected).")
       .setPossibleValues(SELECTION_SELECTED, SELECTION_DESELECTED, SELECTION_ALL)
       .setDefaultValue(SELECTION_ALL);
 
-    action.createParam(Param.TEXT_QUERY)
-      .setDescription("If specified, only show users whose name or login contains the query.")
-      .setExampleValue("freddy");
+    action.addSearchQuery("freddy", "names", "logins");
 
     action.addPagingParams(25);
   }
@@ -92,22 +89,19 @@ public class UsersAction implements UserGroupsWsAction {
       .pageSize(pageSize)
       .build();
 
-    DbSession session = dbClient.openSession(false);
+    DbSession dbSession = dbClient.openSession(false);
     try {
-      GroupDto group = dbClient.groupDao().selectById(session, groupId);
-      if (group == null) {
-        throw new NotFoundException(String.format("Unable to find a group with ID '%d'", groupId));
-      }
-      int total = dbClient.groupMembershipDao().countMembers(session, query);
+      dbClient.groupDao().selectById(dbSession, groupId);
+      int total = dbClient.groupMembershipDao().countMembers(dbSession, query);
       Paging paging = Paging.create(pageSize, page, total);
-      List<UserMembershipDto> users = dbClient.groupMembershipDao().selectMembers(session, query, paging.offset(), pageSize);
+      List<UserMembershipDto> users = dbClient.groupMembershipDao().selectMembers(dbSession, query, paging.offset(), paging.pageSize());
 
       JsonWriter json = response.newJsonWriter().beginObject();
       writeMembers(json, users);
       writePaging(json, paging);
       json.endObject().close();
     } finally {
-      session.close();
+      MyBatis.closeQuietly(dbSession);
     }
   }
 
@@ -124,8 +118,8 @@ public class UsersAction implements UserGroupsWsAction {
   }
 
   private void writePaging(JsonWriter json, Paging paging) {
-    json.prop("p", paging.pageIndex())
-      .prop("ps", paging.pageSize())
+    json.prop(Param.PAGE, paging.pageIndex())
+      .prop(Param.PAGE_SIZE, paging.pageSize())
       .prop("total", paging.total());
   }
 

diff --git a/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/AddUserActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/AddUserActionTest.java
index db56d1a1afd69325e6ddb53bbf118dea18a0cb6e..84d69569aefdb6b04852cd4412323251c96f3e97 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/AddUserActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/AddUserActionTest.java
@@ -97,8 +97,8 @@ public class AddUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "add_user")
-      .setParam("id", group.getId().toString())
-      .setParam("login", user.getLogin())
+      .setParam("groupId", group.getId().toString())
+      .setParam("userLogin", user.getLogin())
       .execute()
       .assertNoContent();
 
@@ -116,8 +116,8 @@ public class AddUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "add_user")
-      .setParam("id", admins.getId().toString())
-      .setParam("login", user.getLogin())
+      .setParam("groupId", admins.getId().toString())
+      .setParam("userLogin", user.getLogin())
       .execute()
       .assertNoContent();
 
@@ -134,8 +134,8 @@ public class AddUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "add_user")
-      .setParam("id", users.getId().toString())
-      .setParam("login", user.getLogin())
+      .setParam("groupId", users.getId().toString())
+      .setParam("userLogin", user.getLogin())
       .execute()
       .assertNoContent();
 
@@ -153,8 +153,8 @@ public class AddUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "add_user")
-      .setParam("id", users.getId().toString())
-      .setParam("login", user2.getLogin())
+      .setParam("groupId", users.getId().toString())
+      .setParam("userLogin", user2.getLogin())
       .execute()
       .assertNoContent();
 
@@ -172,8 +172,8 @@ public class AddUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "add_user")
-      .setParam("id", "42")
-      .setParam("login", user.getLogin())
+      .setParam("groupId", "42")
+      .setParam("userLogin", user.getLogin())
       .execute();
   }
 
@@ -186,8 +186,8 @@ public class AddUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "add_user")
-      .setParam("id", group.getId().toString())
-      .setParam("login", "my-admin")
+      .setParam("groupId", group.getId().toString())
+      .setParam("userLogin", "my-admin")
       .execute();
   }
 

diff --git a/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/RemoveUserActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/RemoveUserActionTest.java
index 106671a5e0728a0672a72886068a10fa035b733d..4b87e3daabeace75f44892f27eb997a59cd6589a 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/RemoveUserActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/RemoveUserActionTest.java
@@ -96,8 +96,8 @@ public class RemoveUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "remove_user")
-      .setParam("id", group.getId().toString())
-      .setParam("login", user.getLogin())
+      .setParam("groupId", group.getId().toString())
+      .setParam("userLogin", user.getLogin())
       .execute()
       .assertNoContent();
 
@@ -114,8 +114,8 @@ public class RemoveUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "remove_user")
-      .setParam("id", users.getId().toString())
-      .setParam("login", user.getLogin())
+      .setParam("groupId", users.getId().toString())
+      .setParam("userLogin", user.getLogin())
       .execute()
       .assertNoContent();
 
@@ -134,8 +134,8 @@ public class RemoveUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "remove_user")
-      .setParam("id", admins.getId().toString())
-      .setParam("login", user.getLogin())
+      .setParam("groupId", admins.getId().toString())
+      .setParam("userLogin", user.getLogin())
       .execute()
       .assertNoContent();
 
@@ -152,8 +152,8 @@ public class RemoveUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "remove_user")
-      .setParam("id", "42")
-      .setParam("login", user.getLogin())
+      .setParam("groupId", "42")
+      .setParam("userLogin", user.getLogin())
       .execute();
   }
 
@@ -166,8 +166,8 @@ public class RemoveUserActionTest {
 
     userSession.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
     tester.newPostRequest("api/usergroups", "remove_user")
-      .setParam("id", group.getId().toString())
-      .setParam("login", "my-admin")
+      .setParam("groupId", group.getId().toString())
+      .setParam("userLogin", "my-admin")
       .execute();
   }
 

diff --git a/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/UsersActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/UsersActionTest.java
index 1540f0318564ce22fc7221689369ccd7006c8ce5..928e0c638a551b8649a5a97e32cb48192ee262d2 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/UsersActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/UsersActionTest.java
@@ -20,11 +20,13 @@
 
 package org.sonar.server.usergroups.ws;
 
+import org.sonar.server.ws.WsTester.TestRequest;
+
 import org.junit.After;
 import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Test;
-import org.sonar.api.server.ws.WebService;
+import org.junit.experimental.categories.Category;
 import org.sonar.api.utils.System2;
 import org.sonar.core.persistence.DbSession;
 import org.sonar.core.persistence.DbTester;
@@ -38,25 +40,23 @@ import org.sonar.server.user.db.GroupDao;
 import org.sonar.server.user.db.UserDao;
 import org.sonar.server.user.db.UserGroupDao;
 import org.sonar.server.ws.WsTester;
+import org.sonar.test.DbTests;
 
+@Category(DbTests.class)
 public class UsersActionTest {
 
   @ClassRule
   public static final DbTester dbTester = new DbTester();
 
-  WebService.Controller controller;
-
-  WsTester tester;
-
+  WsTester wsTester;
   DbClient dbClient;
-
   DbSession session;
 
   @Before
   public void setUp() {
     dbTester.truncateTables();
 
-    System2 system2 = new System2();
+    System2 system2 = System2.INSTANCE;
     UserDao userDao = new UserDao(dbTester.myBatis(), system2);
     GroupDao groupDao = new GroupDao(system2);
     UserGroupDao userGroupDao = new UserGroupDao();
@@ -66,8 +66,7 @@ public class UsersActionTest {
     session = dbClient.openSession(false);
     session.commit();
 
-    tester = new WsTester(new UserGroupsWs(new UsersAction(dbClient)));
-    controller = tester.controller("api/users");
+    wsTester = new WsTester(new UserGroupsWs(new UsersAction(dbClient)));
 
   }
 
@@ -78,17 +77,21 @@ public class UsersActionTest {
 
   @Test(expected = NotFoundException.class)
   public void fail_on_unknown_user() throws Exception {
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("id", "42")
       .setParam("login", "john").execute();
   }
 
+  private TestRequest newUsersRequest() {
+    return wsTester.newGetRequest("api/usergroups", "users");
+  }
+
   @Test
   public void empty_users() throws Exception {
-    GroupDto group = createGroup();
+    GroupDto group = insertGroup();
     session.commit();
 
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("login", "john")
       .setParam("id", group.getId().toString())
       .execute()
@@ -97,13 +100,13 @@ public class UsersActionTest {
 
   @Test
   public void all_users() throws Exception {
-    GroupDto group = createGroup();
-    UserDto groupUser = createUser("ada", "Ada Lovelace");
-    createUser("grace", "Grace Hopper");
+    GroupDto group = insertGroup();
+    UserDto groupUser = insertUser("ada", "Ada Lovelace");
+    insertUser("grace", "Grace Hopper");
     addUserToGroup(groupUser, group);
     session.commit();
 
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("id", group.getId().toString())
       .execute()
       .assertJson(getClass(), "all.json");
@@ -111,13 +114,13 @@ public class UsersActionTest {
 
   @Test
   public void selected_users() throws Exception {
-    GroupDto group = createGroup();
-    UserDto groupUser = createUser("ada", "Ada Lovelace");
-    createUser("grace", "Grace Hopper");
+    GroupDto group = insertGroup();
+    UserDto groupUser = insertUser("ada", "Ada Lovelace");
+    insertUser("grace", "Grace Hopper");
     addUserToGroup(groupUser, group);
     session.commit();
 
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("id", group.getId().toString())
       .setParam("selected", "selected")
       .execute()
@@ -126,13 +129,13 @@ public class UsersActionTest {
 
   @Test
   public void deselected_users() throws Exception {
-    GroupDto group = createGroup();
-    UserDto groupUser = createUser("ada", "Ada Lovelace");
-    createUser("grace", "Grace Hopper");
+    GroupDto group = insertGroup();
+    UserDto groupUser = insertUser("ada", "Ada Lovelace");
+    insertUser("grace", "Grace Hopper");
     addUserToGroup(groupUser, group);
     session.commit();
 
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("id", group.getId().toString())
       .setParam("selected", "deselected")
       .execute()
@@ -141,19 +144,19 @@ public class UsersActionTest {
 
   @Test
   public void paging() throws Exception {
-    GroupDto group = createGroup();
-    UserDto groupUser = createUser("ada", "Ada Lovelace");
-    createUser("grace", "Grace Hopper");
+    GroupDto group = insertGroup();
+    UserDto groupUser = insertUser("ada", "Ada Lovelace");
+    insertUser("grace", "Grace Hopper");
     addUserToGroup(groupUser, group);
     session.commit();
 
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("id", group.getId().toString())
       .setParam("ps", "1")
       .execute()
       .assertJson(getClass(), "all_page1.json");
 
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("id", group.getId().toString())
       .setParam("ps", "1")
       .setParam("p", "2")
@@ -163,31 +166,31 @@ public class UsersActionTest {
 
   @Test
   public void filtering() throws Exception {
-    GroupDto group = createGroup();
-    UserDto groupUser = createUser("ada", "Ada Lovelace");
-    createUser("grace", "Grace Hopper");
+    GroupDto group = insertGroup();
+    UserDto groupUser = insertUser("ada", "Ada Lovelace");
+    insertUser("grace", "Grace Hopper");
     addUserToGroup(groupUser, group);
     session.commit();
 
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("id", group.getId().toString())
       .setParam("q", "ace")
       .execute()
       .assertJson(getClass(), "all.json");
 
-    tester.newGetRequest("api/usergroups", "users")
+    newUsersRequest()
       .setParam("id", group.getId().toString())
       .setParam("q", "love")
       .execute()
       .assertJson(getClass(), "all_ada.json");
   }
 
-  private GroupDto createGroup() {
+  private GroupDto insertGroup() {
     return dbClient.groupDao().insert(session, new GroupDto()
       .setName("sonar-users"));
   }
 
-  private UserDto createUser(String login, String name) {
+  private UserDto insertUser(String login, String name) {
     return dbClient.userDao().insert(session, new UserDto().setLogin(login).setName(name));
   }
 

diff --git a/sonar-core/src/main/java/org/sonar/core/user/GroupMembershipQuery.java b/sonar-core/src/main/java/org/sonar/core/user/GroupMembershipQuery.java
index d0a55a28305d109e425cd7b76cdb8ce3443404e1..1fae7519f9dd2914e7f260f15a735593d94cdbf0 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/core/user/GroupMembershipQuery.java
+++ b/sonar-core/src/main/java/org/sonar/core/user/GroupMembershipQuery.java
@@ -19,14 +19,15 @@
  */
 package org.sonar.core.user;
 
-import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
-import org.apache.commons.lang.StringUtils;
-
+import java.util.Set;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
+import org.apache.commons.lang.StringUtils;
 
-import java.util.Set;
+import static com.google.common.base.Objects.firstNonNull;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
 
 public class GroupMembershipQuery {
 
@@ -139,29 +140,22 @@ public class GroupMembershipQuery {
     }
 
     private void initMembership() {
-      if (membership == null) {
-        membership = GroupMembershipQuery.ANY;
-      } else {
-        Preconditions.checkArgument(AVAILABLE_MEMBERSHIP.contains(membership),
-          "Membership is not valid (got " + membership + "). Availables values are " + AVAILABLE_MEMBERSHIP);
-      }
+      membership = firstNonNull(membership, ANY);
+      checkArgument(AVAILABLE_MEMBERSHIP.contains(membership),
+        "Membership is not valid (got " + membership + "). Availables values are " + AVAILABLE_MEMBERSHIP);
     }
 
     private void initPageSize() {
-      if (pageSize == null) {
-        pageSize = DEFAULT_PAGE_SIZE;
-      }
+      pageSize = firstNonNull(pageSize, DEFAULT_PAGE_SIZE);
     }
 
     private void initPageIndex() {
-      if (pageIndex == null) {
-        pageIndex = DEFAULT_PAGE_INDEX;
-      }
-      Preconditions.checkArgument(pageIndex > 0, "Page index must be greater than 0 (got " + pageIndex + ")");
+      pageIndex = firstNonNull(pageIndex, DEFAULT_PAGE_INDEX);
+      checkArgument(pageIndex > 0, "Page index must be greater than 0 (got " + pageIndex + ")");
     }
 
     public GroupMembershipQuery build() {
-      Preconditions.checkNotNull(login, "User login cant be null.");
+      checkNotNull(login, "User login cant be null.");
       initMembership();
       initPageIndex();
       initPageSize();

diff --git a/sonar-core/src/main/java/org/sonar/core/user/UserMembershipDto.java b/sonar-core/src/main/java/org/sonar/core/user/UserMembershipDto.java
index afb6ca9584a8f6ca5a299b08b2b04df5864a19a9..b7a128f1d7e834d4a9046fe0e557eae866304b3a 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/core/user/UserMembershipDto.java
+++ b/sonar-core/src/main/java/org/sonar/core/user/UserMembershipDto.java
@@ -22,9 +22,6 @@ package org.sonar.core.user;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 
-/**
- * @since 5.2
- */
 public class UserMembershipDto {
 
   private Long id;

diff --git a/sonar-core/src/main/java/org/sonar/core/user/UserMembershipQuery.java b/sonar-core/src/main/java/org/sonar/core/user/UserMembershipQuery.java
index 209b2063c7326ea5c35660e90dcb14bd557a6219..c54ff834be17a67fbe429d2d30108a7b5acf9c77 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/core/user/UserMembershipQuery.java
+++ b/sonar-core/src/main/java/org/sonar/core/user/UserMembershipQuery.java
@@ -19,13 +19,16 @@
  */
 package org.sonar.core.user;
 
-import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import java.util.Set;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import org.apache.commons.lang.StringUtils;
 
+import static com.google.common.base.Objects.firstNonNull;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+
 public class UserMembershipQuery {
 
   public static final int DEFAULT_PAGE_INDEX = 1;
@@ -34,7 +37,7 @@ public class UserMembershipQuery {
   public static final String ANY = "ANY";
   public static final String IN = "IN";
   public static final String OUT = "OUT";
-  public static final Set<String> AVAILABLE_MEMBERSHIP = ImmutableSet.of(ANY, IN, OUT);
+  public static final Set<String> AVAILABLE_MEMBERSHIPS = ImmutableSet.of(ANY, IN, OUT);
 
   private final Long groupId;
   private final String membership;
@@ -137,29 +140,22 @@ public class UserMembershipQuery {
     }
 
     private void initMembership() {
-      if (membership == null) {
-        membership = UserMembershipQuery.ANY;
-      } else {
-        Preconditions.checkArgument(AVAILABLE_MEMBERSHIP.contains(membership),
-          "Membership is not valid (got " + membership + "). Availables values are " + AVAILABLE_MEMBERSHIP);
-      }
+      membership = firstNonNull(membership, ANY);
+      checkArgument(AVAILABLE_MEMBERSHIPS.contains(membership),
+        "Membership is not valid (got " + membership + "). Availables values are " + AVAILABLE_MEMBERSHIPS);
     }
 
     private void initPageSize() {
-      if (pageSize == null) {
-        pageSize = DEFAULT_PAGE_SIZE;
-      }
+      pageSize = firstNonNull(pageSize, DEFAULT_PAGE_SIZE);
     }
 
     private void initPageIndex() {
-      if (pageIndex == null) {
-        pageIndex = DEFAULT_PAGE_INDEX;
-      }
-      Preconditions.checkArgument(pageIndex > 0, "Page index must be greater than 0 (got " + pageIndex + ")");
+      pageIndex = firstNonNull(pageIndex, DEFAULT_PAGE_INDEX);
+      checkArgument(pageIndex > 0, "Page index must be greater than 0 (got " + pageIndex + ")");
     }
 
     public UserMembershipQuery build() {
-      Preconditions.checkNotNull(groupId, "Group ID cant be null.");
+      checkNotNull(groupId, "Group ID cant be null.");
       initMembership();
       initPageIndex();
       initPageSize();

diff --git a/sonar-core/src/test/java/org/sonar/core/user/GroupMembershipDaoTest.java b/sonar-core/src/test/java/org/sonar/core/user/GroupMembershipDaoTest.java
index 8b8992f07f0724baed5ba38165ba97d42991be4f..e2b45132d91f51f88cb46477ad4ae7b34ccb4a77 100644 (file)
--- a/sonar-core/src/test/java/org/sonar/core/user/GroupMembershipDaoTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/user/GroupMembershipDaoTest.java
@@ -23,12 +23,14 @@ package org.sonar.core.user;
 import com.google.common.collect.Multimap;
 import java.util.Arrays;
 import java.util.List;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.sonar.core.persistence.DbSession;
 import org.sonar.core.persistence.DbTester;
+import org.sonar.core.persistence.MyBatis;
 import org.sonar.test.DbTests;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -42,10 +44,18 @@ public class GroupMembershipDaoTest {
 
   private GroupMembershipDao dao;
 
+  private DbSession dbSession;
+
   @Before
   public void setUp() {
     dbTester.truncateTables();
     dao = new GroupMembershipDao(dbTester.myBatis());
+    dbSession = dbTester.myBatis().openSession(false);
+  }
+
+  @After
+  public void tearDown() {
+    MyBatis.closeQuietly(dbSession);
   }
 
   @Test
@@ -155,154 +165,108 @@ public class GroupMembershipDaoTest {
   @Test
   public void count_groups() {
     dbTester.prepareDbUnit(getClass(), "shared.xml");
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
-      // 200 is member of 3 groups
-      assertThat(dao.countGroups(session, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.IN).build(), 200L)).isEqualTo(3);
-      assertThat(dao.countGroups(session, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.OUT).build(), 200L)).isZero();
-      // 201 is member of 1 group on 3
-      assertThat(dao.countGroups(session, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.IN).build(), 201L)).isEqualTo(1);
-      assertThat(dao.countGroups(session, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.OUT).build(), 201L)).isEqualTo(2);
-      // 999 is member of 0 group
-      assertThat(dao.countGroups(session, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.IN).build(), 999L)).isZero();
-      assertThat(dao.countGroups(session, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.OUT).build(), 2999L)).isEqualTo(3);
-    } finally {
-      session.close();
-    }
+
+    // 200 is member of 3 groups
+    assertThat(dao.countGroups(dbSession, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.IN).build(), 200L)).isEqualTo(3);
+    assertThat(dao.countGroups(dbSession, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.OUT).build(), 200L)).isZero();
+    // 201 is member of 1 group on 3
+    assertThat(dao.countGroups(dbSession, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.IN).build(), 201L)).isEqualTo(1);
+    assertThat(dao.countGroups(dbSession, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.OUT).build(), 201L)).isEqualTo(2);
+    // 999 is member of 0 group
+    assertThat(dao.countGroups(dbSession, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.IN).build(), 999L)).isZero();
+    assertThat(dao.countGroups(dbSession, GroupMembershipQuery.builder().login("arthur").membership(GroupMembershipQuery.OUT).build(), 2999L)).isEqualTo(3);
   }
 
   @Test
   public void count_users_by_group() {
     dbTester.prepareDbUnit(getClass(), "shared_plus_empty_group.xml");
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
-      assertThat(dao.countUsersByGroups(session, Arrays.asList(100L, 101L, 102L, 103L))).containsOnly(
-        entry("sonar-users", 2), entry("sonar-reviewers", 1), entry("sonar-administrators", 1), entry("sonar-nobody", 0));
-      assertThat(dao.countUsersByGroups(session, Arrays.asList(100L, 103L))).containsOnly(
-        entry("sonar-administrators", 1), entry("sonar-nobody", 0));
-    } finally {
-      session.close();
-    }
+
+    assertThat(dao.countUsersByGroups(dbSession, Arrays.asList(100L, 101L, 102L, 103L))).containsOnly(
+      entry("sonar-users", 2), entry("sonar-reviewers", 1), entry("sonar-administrators", 1), entry("sonar-nobody", 0));
+    assertThat(dao.countUsersByGroups(dbSession, Arrays.asList(100L, 103L))).containsOnly(
+      entry("sonar-administrators", 1), entry("sonar-nobody", 0));
   }
 
   @Test
   public void count_groups_by_login() {
     dbTester.prepareDbUnit(getClass(), "shared.xml");
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
-      assertThat(dao.selectGroupsByLogins(session, Arrays.<String>asList()).keys()).isEmpty();
-      Multimap<String, String> groupsByLogin = dao.selectGroupsByLogins(session, Arrays.asList("two-hundred", "two-hundred-one", "two-hundred-two"));
-      assertThat(groupsByLogin.get("two-hundred")).containsOnly("sonar-administrators", "sonar-users", "sonar-reviewers");
-      assertThat(groupsByLogin.get("two-hundred-one")).containsOnly("sonar-users");
-      assertThat(groupsByLogin.get("two-hundred-two")).isEmpty();
-    } finally {
-      session.close();
-    }
+
+    assertThat(dao.selectGroupsByLogins(dbSession, Arrays.<String>asList()).keys()).isEmpty();
+    Multimap<String, String> groupsByLogin = dao.selectGroupsByLogins(dbSession, Arrays.asList("two-hundred", "two-hundred-one", "two-hundred-two"));
+    assertThat(groupsByLogin.get("two-hundred")).containsOnly("sonar-administrators", "sonar-users", "sonar-reviewers");
+    assertThat(groupsByLogin.get("two-hundred-one")).containsOnly("sonar-users");
+    assertThat(groupsByLogin.get("two-hundred-two")).isEmpty();
   }
 
   @Test
   public void count_members() {
     dbTester.prepareDbUnit(getClass(), "shared_plus_empty_group.xml");
-    DbSession session = dbTester.myBatis().openSession(false);
 
-    try {
-      // 100 has 1 member and 1 non member
-      assertThat(dao.countMembers(session, UserMembershipQuery.builder().groupId(100L).membership(UserMembershipQuery.IN).build())).isEqualTo(1);
-      assertThat(dao.countMembers(session, UserMembershipQuery.builder().groupId(100L).membership(UserMembershipQuery.OUT).build())).isEqualTo(1);
-      // 101 has 2 members
-      assertThat(dao.countMembers(session, UserMembershipQuery.builder().groupId(101L).membership(UserMembershipQuery.IN).build())).isEqualTo(2);
-      assertThat(dao.countMembers(session, UserMembershipQuery.builder().groupId(101L).membership(UserMembershipQuery.OUT).build())).isZero();
-      // 102 has 1 member and 1 non member
-      assertThat(dao.countMembers(session, UserMembershipQuery.builder().groupId(102L).membership(UserMembershipQuery.IN).build())).isEqualTo(1);
-      assertThat(dao.countMembers(session, UserMembershipQuery.builder().groupId(102L).membership(UserMembershipQuery.OUT).build())).isEqualTo(1);
-      // 103 has no member
-      assertThat(dao.countMembers(session, UserMembershipQuery.builder().groupId(103L).membership(UserMembershipQuery.IN).build())).isZero();
-      assertThat(dao.countMembers(session, UserMembershipQuery.builder().groupId(103L).membership(UserMembershipQuery.OUT).build())).isEqualTo(2);
-    } finally {
-      session.close();
-    }
+    // 100 has 1 member and 1 non member
+    assertThat(dao.countMembers(dbSession, UserMembershipQuery.builder().groupId(100L).membership(UserMembershipQuery.IN).build())).isEqualTo(1);
+    assertThat(dao.countMembers(dbSession, UserMembershipQuery.builder().groupId(100L).membership(UserMembershipQuery.OUT).build())).isEqualTo(1);
+    // 101 has 2 members
+    assertThat(dao.countMembers(dbSession, UserMembershipQuery.builder().groupId(101L).membership(UserMembershipQuery.IN).build())).isEqualTo(2);
+    assertThat(dao.countMembers(dbSession, UserMembershipQuery.builder().groupId(101L).membership(UserMembershipQuery.OUT).build())).isZero();
+    // 102 has 1 member and 1 non member
+    assertThat(dao.countMembers(dbSession, UserMembershipQuery.builder().groupId(102L).membership(UserMembershipQuery.IN).build())).isEqualTo(1);
+    assertThat(dao.countMembers(dbSession, UserMembershipQuery.builder().groupId(102L).membership(UserMembershipQuery.OUT).build())).isEqualTo(1);
+    // 103 has no member
+    assertThat(dao.countMembers(dbSession, UserMembershipQuery.builder().groupId(103L).membership(UserMembershipQuery.IN).build())).isZero();
+    assertThat(dao.countMembers(dbSession, UserMembershipQuery.builder().groupId(103L).membership(UserMembershipQuery.OUT).build())).isEqualTo(2);
   }
 
   @Test
   public void select_group_members_by_query() {
     dbTester.prepareDbUnit(getClass(), "shared_plus_empty_group.xml");
 
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
-      // 100 has 1 member
-      assertThat(dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).membership(UserMembershipQuery.IN).build(), 0, 10)).hasSize(1);
-      // 101 has 2 members
-      assertThat(dao.selectMembers(session, UserMembershipQuery.builder().groupId(101L).membership(UserMembershipQuery.IN).build(), 0, 10)).hasSize(2);
-      // 102 has 1 member
-      assertThat(dao.selectMembers(session, UserMembershipQuery.builder().groupId(102L).membership(UserMembershipQuery.IN).build(), 0, 10)).hasSize(1);
-      // 103 has no member
-      assertThat(dao.selectMembers(session, UserMembershipQuery.builder().groupId(103L).membership(UserMembershipQuery.IN).build(), 0, 10)).isEmpty();
-    } finally {
-      session.close();
-    }
+    // 100 has 1 member
+    assertThat(dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).membership(UserMembershipQuery.IN).build(), 0, 10)).hasSize(1);
+    // 101 has 2 members
+    assertThat(dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(101L).membership(UserMembershipQuery.IN).build(), 0, 10)).hasSize(2);
+    // 102 has 1 member
+    assertThat(dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(102L).membership(UserMembershipQuery.IN).build(), 0, 10)).hasSize(1);
+    // 103 has no member
+    assertThat(dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(103L).membership(UserMembershipQuery.IN).build(), 0, 10)).isEmpty();
   }
 
   @Test
   public void select_users_not_affected_to_a_group_by_query() {
     dbTester.prepareDbUnit(getClass(), "shared_plus_empty_group.xml");
 
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
       // 100 has 1 member
-      assertThat(dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).membership(UserMembershipQuery.OUT).build(), 0, 10)).hasSize(1);
+    assertThat(dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).membership(UserMembershipQuery.OUT).build(), 0, 10)).hasSize(1);
       // 101 has 2 members
-      assertThat(dao.selectMembers(session, UserMembershipQuery.builder().groupId(101L).membership(UserMembershipQuery.OUT).build(), 0, 10)).isEmpty();
+    assertThat(dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(101L).membership(UserMembershipQuery.OUT).build(), 0, 10)).isEmpty();
       // 102 has 1 member
-      assertThat(dao.selectMembers(session, UserMembershipQuery.builder().groupId(102L).membership(UserMembershipQuery.OUT).build(), 0, 10)).hasSize(1);
+    assertThat(dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(102L).membership(UserMembershipQuery.OUT).build(), 0, 10)).hasSize(1);
       // 103 has no member
-      assertThat(dao.selectMembers(session, UserMembershipQuery.builder().groupId(103L).membership(UserMembershipQuery.OUT).build(), 0, 10)).hasSize(2);
-    } finally {
-      session.close();
-    }
+    assertThat(dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(103L).membership(UserMembershipQuery.OUT).build(), 0, 10)).hasSize(2);
   }
 
   @Test
   public void search_by_user_name_or_login() {
     dbTester.prepareDbUnit(getClass(), "shared_plus_empty_group.xml");
 
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
-
-      List<UserMembershipDto> result = dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).memberSearch("admin").build(), 0, 10);
-      assertThat(result).hasSize(2);
+    List<UserMembershipDto> result = dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).memberSearch("admin").build(), 0, 10);
+    assertThat(result).hasSize(2);
 
-      assertThat(result.get(0).getName()).isEqualTo("Admin");
-      assertThat(result.get(1).getName()).isEqualTo("Not Admin");
+    assertThat(result.get(0).getName()).isEqualTo("Admin");
+    assertThat(result.get(1).getName()).isEqualTo("Not Admin");
 
-      result = dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).memberSearch("not").build(), 0, 10);
+    result = dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).memberSearch("not").build(), 0, 10);
       assertThat(result).hasSize(1);
-
-    } finally {
-      session.close();
-    }
   }
 
   @Test
   public void search_by_login_or_name_with_capitalization() {
     dbTester.prepareDbUnit(getClass(), "shared_plus_empty_group.xml");
 
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
-      List<UserMembershipDto> result = dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).memberSearch("admin").build(), 0, 10);
-      assertThat(result).hasSize(2);
+    List<UserMembershipDto> result = dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).memberSearch("admin").build(), 0, 10);
+    assertThat(result).hasSize(2);
 
-      result = dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).memberSearch("AdMiN").build(), 0, 10);
-      assertThat(result).hasSize(2);
-    } finally {
-      session.close();
-    }
+    result = dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).memberSearch("AdMiN").build(), 0, 10);
+    assertThat(result).hasSize(2);
 
   }
 
@@ -310,38 +274,26 @@ public class GroupMembershipDaoTest {
   public void should_be_sorted_by_user_name() {
     dbTester.prepareDbUnit(getClass(), "shared_plus_empty_group.xml");
 
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
-      List<UserMembershipDto> result = dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).build(), 0, 10);
-      assertThat(result).hasSize(2);
-      assertThat(result.get(0).getName()).isEqualTo("Admin");
-      assertThat(result.get(1).getName()).isEqualTo("Not Admin");
-    } finally {
-      session.close();
-    }
+    List<UserMembershipDto> result = dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).build(), 0, 10);
+    assertThat(result).hasSize(2);
+    assertThat(result.get(0).getName()).isEqualTo("Admin");
+    assertThat(result.get(1).getName()).isEqualTo("Not Admin");
   }
 
   @Test
   public void members_should_be_paginated() {
     dbTester.prepareDbUnit(getClass(), "shared_plus_empty_group.xml");
 
-    DbSession session = dbTester.myBatis().openSession(false);
-
-    try {
-      List<UserMembershipDto> result = dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).build(), 0, 2);
-      assertThat(result).hasSize(2);
-      assertThat(result.get(0).getName()).isEqualTo("Admin");
-      assertThat(result.get(1).getName()).isEqualTo("Not Admin");
+    List<UserMembershipDto> result = dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).build(), 0, 2);
+    assertThat(result).hasSize(2);
+    assertThat(result.get(0).getName()).isEqualTo("Admin");
+    assertThat(result.get(1).getName()).isEqualTo("Not Admin");
 
-      result = dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).build(), 1, 2);
-      assertThat(result).hasSize(1);
-      assertThat(result.get(0).getName()).isEqualTo("Not Admin");
+    result = dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).build(), 1, 2);
+    assertThat(result).hasSize(1);
+    assertThat(result.get(0).getName()).isEqualTo("Not Admin");
 
-      result = dao.selectMembers(session, UserMembershipQuery.builder().groupId(100L).build(), 2, 1);
-      assertThat(result).isEmpty();
-    } finally {
-      session.close();
-    }
+    result = dao.selectMembers(dbSession, UserMembershipQuery.builder().groupId(100L).build(), 2, 1);
+    assertThat(result).isEmpty();
   }
 }