SONAR-5830 Change the way to persist scm accounts in db in order to improve the search

10 files changed, 72 insertions, 23 deletions

diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java
index 77aaf0a3200..dd9c9c5a345 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java
@@ -20,6 +20,7 @@
 
 package org.sonar.server.user;
 
+import com.google.common.base.CharMatcher;
 import com.google.common.base.Predicate;
 import com.google.common.base.Strings;
 import com.google.common.collect.Iterables;
@@ -76,8 +77,7 @@ public class UserUpdater implements ServerComponent {
   }
 
   /**
-   * Retuen true if the user has been reactivated
-   * @return
+   * Return true if the user has been reactivated
    */
   public boolean create(NewUser newUser) {
     UserDto userDto = createNewUserDto(newUser);
@@ -256,13 +256,19 @@ public class UserUpdater implements ServerComponent {
   @CheckForNull
   private static String convertScmAccountsToCsv(@Nullable List<String> scmAccounts) {
     if (scmAccounts != null) {
+      // Remove empty characters
       scmAccounts.removeAll(Arrays.asList(null, ""));
+      // Add one empty character at the begin and at the end of the list to be able to generate a coma at the begin and at the end of the
+      // text
+      scmAccounts.add(0, "");
+      scmAccounts.add("");
       int size = scmAccounts.size();
       StringWriter writer = new StringWriter(size);
       CsvWriter csv = CsvWriter.of(writer);
       csv.values(scmAccounts.toArray(new String[size]));
       csv.close();
-      return writer.toString();
+      // Remove useless line break added by CsvWriter at this end of the text
+      return CharMatcher.BREAKING_WHITESPACE.removeFrom(writer.toString());
     }
     return null;
   }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/index/UserResultSetIterator.java b/server/sonar-server/src/main/java/org/sonar/server/user/index/UserResultSetIterator.java
index 7d8b4525699..d4b655c1001 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/user/index/UserResultSetIterator.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/user/index/UserResultSetIterator.java
@@ -19,6 +19,7 @@
  */
 package org.sonar.server.user.index;
 
+import com.google.common.base.Strings;
 import com.google.common.collect.Maps;
 import org.apache.commons.csv.CSVFormat;
 import org.apache.commons.csv.CSVParser;
@@ -106,7 +107,9 @@ class UserResultSetIterator extends ResultSetIterator<UserDoc> {
       csvParser = new CSVParser(reader, CSVFormat.DEFAULT);
       for (CSVRecord csvRecord : csvParser) {
         for (String aCsvRecord : csvRecord) {
-          result.add(aCsvRecord);
+          if (!Strings.isNullOrEmpty(aCsvRecord)) {
+            result.add(aCsvRecord);
+          }
         }
       }
       return result;
diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/UserServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/UserServiceMediumTest.java
index 5072bdee0f0..e24dbb3321b 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/user/UserServiceMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/user/UserServiceMediumTest.java
@@ -33,6 +33,7 @@ import org.sonar.server.db.DbClient;
 import org.sonar.server.es.EsClient;
 import org.sonar.server.exceptions.ForbiddenException;
 import org.sonar.server.tester.ServerTester;
+import org.sonar.server.user.index.UserDoc;
 import org.sonar.server.user.index.UserIndexDefinition;
 
 import static com.google.common.collect.Lists.newArrayList;
@@ -79,8 +80,27 @@ public class UserServiceMediumTest {
       .setScmAccounts(newArrayList("u1", "u_1")));
 
     assertThat(result).isFalse();
-    assertThat(dbClient.userDao().selectNullableByLogin(session, "user")).isNotNull();
-    assertThat(esClient.prepareGet(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, "user").get().isExists()).isTrue();
+
+    UserDto userDto = dbClient.userDao().selectNullableByLogin(session, "user");
+    assertThat(userDto).isNotNull();
+    assertThat(userDto.getId()).isNotNull();
+    assertThat(userDto.getLogin()).isEqualTo("user");
+    assertThat(userDto.getName()).isEqualTo("User");
+    assertThat(userDto.getEmail()).isEqualTo("user@mail.com");
+    assertThat(userDto.getCryptedPassword()).isNotNull();
+    assertThat(userDto.getSalt()).isNotNull();
+    assertThat(userDto.getScmAccounts()).contains(",u1,u_1,");
+    assertThat(userDto.getCreatedAt()).isNotNull();
+    assertThat(userDto.getUpdatedAt()).isNotNull();
+
+    UserDoc userDoc = service.getNullableByLogin("user");
+    assertThat(userDoc).isNotNull();
+    assertThat(userDoc.login()).isEqualTo("user");
+    assertThat(userDoc.name()).isEqualTo("User");
+    assertThat(userDoc.email()).isEqualTo("user@mail.com");
+    assertThat(userDoc.scmAccounts()).containsOnly("u1", "u_1");
+    assertThat(userDoc.createdAt()).isNotNull();
+    assertThat(userDoc.updatedAt()).isNotNull();
   }
 
   @Test(expected = ForbiddenException.class)
diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java
index 0ae9a1a9b08..cf7a400e93d 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java
@@ -117,7 +117,7 @@ public class UserUpdaterTest {
     assertThat(dto.getLogin()).isEqualTo("user");
     assertThat(dto.getName()).isEqualTo("User");
     assertThat(dto.getEmail()).isEqualTo("user@mail.com");
-    assertThat(dto.getScmAccounts()).contains("u1,u_1");
+    assertThat(dto.getScmAccounts()).isEqualTo(",u1,u_1,");
     assertThat(dto.isActive()).isTrue();
 
     assertThat(dto.getSalt()).isNotNull();
@@ -153,7 +153,7 @@ public class UserUpdaterTest {
       .setPasswordConfirmation("password")
       .setScmAccounts(newArrayList("u1", "", null)));
 
-    assertThat(userDao.selectNullableByLogin(session, "user").getScmAccounts()).doesNotContain(",");
+    assertThat(userDao.selectNullableByLogin(session, "user").getScmAccounts()).isEqualTo(",u1,");
   }
 
   @Test
@@ -458,7 +458,7 @@ public class UserUpdaterTest {
     assertThat(dto.isActive()).isTrue();
     assertThat(dto.getName()).isEqualTo("Marius2");
     assertThat(dto.getEmail()).isEqualTo("marius2@mail.com");
-    assertThat(dto.getScmAccounts()).contains("ma2");
+    assertThat(dto.getScmAccounts()).isEqualTo(",ma2,");
 
     assertThat(dto.getSalt()).isNotEqualTo("79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8365");
     assertThat(dto.getCryptedPassword()).isNotEqualTo("650d2261c98361e2f67f90ce5c65a95e7d8ea2fg");
@@ -481,7 +481,7 @@ public class UserUpdaterTest {
 
     // Following fields has not changed
     assertThat(dto.getEmail()).isEqualTo("marius@lesbronzes.fr");
-    assertThat(dto.getScmAccounts()).contains("ma,marius33");
+    assertThat(dto.getScmAccounts()).isEqualTo(",ma,marius33,");
     assertThat(dto.getSalt()).isEqualTo("79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8365");
     assertThat(dto.getCryptedPassword()).isEqualTo("650d2261c98361e2f67f90ce5c65a95e7d8ea2fg");
   }
@@ -501,13 +501,13 @@ public class UserUpdaterTest {
 
     // Following fields has not changed
     assertThat(dto.getName()).isEqualTo("Marius");
-    assertThat(dto.getScmAccounts()).contains("ma,marius33");
+    assertThat(dto.getScmAccounts()).isEqualTo(",ma,marius33,");
     assertThat(dto.getSalt()).isEqualTo("79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8365");
     assertThat(dto.getCryptedPassword()).isEqualTo("650d2261c98361e2f67f90ce5c65a95e7d8ea2fg");
   }
 
   @Test
-  public void update_only_scm_accounts_email() throws Exception {
+  public void update_only_scm_accounts() throws Exception {
     db.prepareDbUnit(getClass(), "update_user.xml");
     createDefaultGroup();
 
@@ -517,7 +517,7 @@ public class UserUpdaterTest {
     session.clearCache();
 
     UserDto dto = userDao.selectNullableByLogin(session, "marius");
-    assertThat(dto.getScmAccounts()).contains("ma2");
+    assertThat(dto.getScmAccounts()).isEqualTo(",ma2,");
 
     // Following fields has not changed
     assertThat(dto.getName()).isEqualTo("Marius");
@@ -527,6 +527,20 @@ public class UserUpdaterTest {
   }
 
   @Test
+  public void remove_scm_accounts() throws Exception {
+    db.prepareDbUnit(getClass(), "update_user.xml");
+    createDefaultGroup();
+
+    userUpdater.update(UpdateUser.create("marius")
+      .setScmAccounts(null));
+    session.commit();
+    session.clearCache();
+
+    UserDto dto = userDao.selectNullableByLogin(session, "marius");
+    assertThat(dto.getScmAccounts()).isNull();
+  }
+
+  @Test
   public void update_only_user_password() throws Exception {
     db.prepareDbUnit(getClass(), "update_user.xml");
     createDefaultGroup();
@@ -543,7 +557,7 @@ public class UserUpdaterTest {
 
     // Following fields has not changed
     assertThat(dto.getName()).isEqualTo("Marius");
-    assertThat(dto.getScmAccounts()).contains("ma,marius33");
+    assertThat(dto.getScmAccounts()).isEqualTo(",ma,marius33,");
     assertThat(dto.getEmail()).isEqualTo("marius@lesbronzes.fr");
   }
 
diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/db/UserDaoTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/db/UserDaoTest.java
index dcb7ca99dcd..67534bdfe87 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/user/db/UserDaoTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/user/db/UserDaoTest.java
@@ -64,7 +64,7 @@ public class UserDaoTest {
     assertThat(dto.getName()).isEqualTo("Marius");
     assertThat(dto.getEmail()).isEqualTo("marius@lesbronzes.fr");
     assertThat(dto.isActive()).isTrue();
-    assertThat(dto.getScmAccounts()).isEqualTo("ma,marius33");
+    assertThat(dto.getScmAccounts()).isEqualTo(",ma,marius33,");
     assertThat(dto.getSalt()).isEqualTo("79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8365");
     assertThat(dto.getCryptedPassword()).isEqualTo("650d2261c98361e2f67f90ce5c65a95e7d8ea2fg");
     assertThat(dto.getCreatedAt()).isEqualTo(1418215735482L);
diff --git a/server/sonar-server/src/test/resources/org/sonar/server/user/UserUpdaterTest/fail_to_update_user_when_scm_account_is_already_used.xml b/server/sonar-server/src/test/resources/org/sonar/server/user/UserUpdaterTest/fail_to_update_user_when_scm_account_is_already_used.xml
new file mode 100644
index 00000000000..5a257afb338
--- /dev/null
+++ b/server/sonar-server/src/test/resources/org/sonar/server/user/UserUpdaterTest/fail_to_update_user_when_scm_account_is_already_used.xml
@@ -0,0 +1,6 @@
+<dataset>
+
+  <users id="101" login="john" name="John" email="john@email.com" active="[true]" scm_accounts=",jo," created_at="1418215735482" updated_at="1418215735485"
+         salt="79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8365" crypted_password="650d2261c98361e2f67f90ce5c65a95e7d8ea2fg"/>
+
+</dataset>
diff --git a/server/sonar-server/src/test/resources/org/sonar/server/user/UserUpdaterTest/update_user.xml b/server/sonar-server/src/test/resources/org/sonar/server/user/UserUpdaterTest/update_user.xml
index 3c22b1ebf61..87839ca2fe5 100644
--- a/server/sonar-server/src/test/resources/org/sonar/server/user/UserUpdaterTest/update_user.xml
+++ b/server/sonar-server/src/test/resources/org/sonar/server/user/UserUpdaterTest/update_user.xml
@@ -1,6 +1,6 @@
 <dataset>
 
-  <users id="101" login="marius" name="Marius" email="marius@lesbronzes.fr" active="[true]" scm_accounts="ma,marius33" created_at="1418215735482" updated_at="1418215735485"
+  <users id="101" login="marius" name="Marius" email="marius@lesbronzes.fr" active="[true]" scm_accounts=",ma,marius33," created_at="1418215735482" updated_at="1418215735485"
          salt="79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8365" crypted_password="650d2261c98361e2f67f90ce5c65a95e7d8ea2fg"/>
 
 </dataset>
diff --git a/server/sonar-server/src/test/resources/org/sonar/server/user/db/UserDaoTest/select_by_login.xml b/server/sonar-server/src/test/resources/org/sonar/server/user/db/UserDaoTest/select_by_login.xml
index 0b245535234..a1879e0b470 100644
--- a/server/sonar-server/src/test/resources/org/sonar/server/user/db/UserDaoTest/select_by_login.xml
+++ b/server/sonar-server/src/test/resources/org/sonar/server/user/db/UserDaoTest/select_by_login.xml
@@ -1,6 +1,6 @@
 <dataset>
 
-  <users id="101" login="marius" name="Marius" email="marius@lesbronzes.fr" active="[true]" scm_accounts="ma,marius33" created_at="1418215735482" updated_at="1418215735485"
+  <users id="101" login="marius" name="Marius" email="marius@lesbronzes.fr" active="[true]" scm_accounts=",ma,marius33," created_at="1418215735482" updated_at="1418215735485"
          salt="79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8365" crypted_password="650d2261c98361e2f67f90ce5c65a95e7d8ea2fg"/>
   <users id="102" login="sbrandhof" name="Simon Brandhof" email="marius@lesbronzes.fr" active="[true]" scm_accounts="[null]" created_at="1418215735482" updated_at="1418215735485"
          salt="79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8366" crypted_password="650d2261c98361e2f67f90ce5c65a95e7d8ea2fh"/>
diff --git a/server/sonar-server/src/test/resources/org/sonar/server/user/index/UserResultSetIteratorTest/shared.xml b/server/sonar-server/src/test/resources/org/sonar/server/user/index/UserResultSetIteratorTest/shared.xml
index 96ef1d2a812..0163794b9dc 100644
--- a/server/sonar-server/src/test/resources/org/sonar/server/user/index/UserResultSetIteratorTest/shared.xml
+++ b/server/sonar-server/src/test/resources/org/sonar/server/user/index/UserResultSetIteratorTest/shared.xml
@@ -1,14 +1,14 @@
 <dataset>
 
   <users id="1" login="user1" name="User1" email="user1@mail.com" active="[true]"
-         scm_accounts="user_1,u1"
+         scm_accounts=",user_1,u1,"
          created_at="1500000000000"
          updated_at="1500000000000"
       />
 
   <!-- scm accounts with comma -->
   <users id="2" login="user2" name="User2" email="user2@mail.com" active="[true]"
-         scm_accounts="&quot;user,2&quot;,user_2"
+         scm_accounts=",&quot;user,2&quot;,user_2,"
          created_at="1500000000000"
          updated_at="1500000000000"
       />
diff --git a/sonar-core/src/test/java/org/sonar/core/user/UserDaoTest.java b/sonar-core/src/test/java/org/sonar/core/user/UserDaoTest.java
index 1e19d64f40c..3c04d31eab1 100644
--- a/sonar-core/src/test/java/org/sonar/core/user/UserDaoTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/user/UserDaoTest.java
@@ -182,7 +182,7 @@ public class UserDaoTest extends AbstractDaoTestCase {
       .setLogin("john")
       .setName("John")
       .setEmail("jo@hn.com")
-      .setScmAccounts("jo.hn,john2")
+      .setScmAccounts(",jo.hn,john2,")
       .setActive(true)
       .setSalt("1234")
       .setCryptedPassword("abcd")
@@ -198,7 +198,7 @@ public class UserDaoTest extends AbstractDaoTestCase {
     assertThat(user.getName()).isEqualTo("John");
     assertThat(user.getEmail()).isEqualTo("jo@hn.com");
     assertThat(user.isActive()).isTrue();
-    assertThat(user.getScmAccounts()).isEqualTo("jo.hn,john2");
+    assertThat(user.getScmAccounts()).isEqualTo(",jo.hn,john2,");
     assertThat(user.getSalt()).isEqualTo("1234");
     assertThat(user.getCryptedPassword()).isEqualTo("abcd");
     assertThat(user.getCreatedAt()).isEqualTo(date);
@@ -216,7 +216,7 @@ public class UserDaoTest extends AbstractDaoTestCase {
       .setLogin("john")
       .setName("John Doo")
       .setEmail("jodoo@hn.com")
-      .setScmAccounts("jo.hn,john2,johndoo")
+      .setScmAccounts(",jo.hn,john2,johndoo,")
       .setActive(false)
       .setSalt("12345")
       .setCryptedPassword("abcde")
@@ -231,7 +231,7 @@ public class UserDaoTest extends AbstractDaoTestCase {
     assertThat(user.getName()).isEqualTo("John Doo");
     assertThat(user.getEmail()).isEqualTo("jodoo@hn.com");
     assertThat(user.isActive()).isFalse();
-    assertThat(user.getScmAccounts()).isEqualTo("jo.hn,john2,johndoo");
+    assertThat(user.getScmAccounts()).isEqualTo(",jo.hn,john2,johndoo,");
     assertThat(user.getSalt()).isEqualTo("12345");
     assertThat(user.getCryptedPassword()).isEqualTo("abcde");
     assertThat(user.getCreatedAt()).isEqualTo(1418215735482L);