SONAR-7641 refactor UserIndex

- remove unused method getByLogin()
- refactor tests to prepare removal of _id path (required for ES 2.3)

server/sonar-server/src/main/java/org/sonar/server/user/index/UserIndex.java

@@ -48,31 +48,17 @@ import org.sonar.server.es.EsClient;
 import org.sonar.server.es.EsUtils;
 import org.sonar.server.es.SearchOptions;
 import org.sonar.server.es.SearchResult;
-import org.sonar.server.exceptions.NotFoundException;
-
-import static java.lang.String.format;
 
 @ServerSide
 @ComputeEngineSide
 public class UserIndex {
 
-  /**
-   * Convert an Elasticsearch result (a map) to an {@link UserDoc}. It's
-   * used for {@link org.sonar.server.es.SearchResult}.
-   */
-  private static final Function<Map<String, Object>, UserDoc> DOC_CONVERTER = new NonNullInputFunction<Map<String, Object>, UserDoc>() {
-    @Override
-    protected UserDoc doApply(Map<String, Object> input) {
-      return new UserDoc(input);
-    }
-  };
+  private final EsClient esClient;
 
   public UserIndex(EsClient esClient) {
     this.esClient = esClient;
   }
 
-  private final EsClient esClient;
-
   @CheckForNull
   public UserDoc getNullableByLogin(String login) {
     GetRequestBuilder request = esClient.prepareGet(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, login)
@@ -85,14 +71,6 @@ public class UserIndex {
     return null;
   }
 
-  public UserDoc getByLogin(String login) {
-    UserDoc userDoc = getNullableByLogin(login);
-    if (userDoc == null) {
-      throw new NotFoundException(format("User '%s' not found", login));
-    }
-    return userDoc;
-  }
-
   /**
    * Returns the active users (at most 3) who are associated to the given SCM account. This method can be used
    * to detect user conflicts.
@@ -126,7 +104,7 @@ public class UserIndex {
       .setSearchType(SearchType.SCAN)
       .addSort(SortBuilders.fieldSort(UserIndexDefinition.FIELD_LOGIN).order(SortOrder.ASC))
       .setScroll(TimeValue.timeValueMinutes(EsUtils.SCROLL_TIME_IN_MINUTES))
-      .setSize(10000)
+      .setSize(10_000)
       .setFetchSource(
         new String[] {UserIndexDefinition.FIELD_LOGIN, UserIndexDefinition.FIELD_NAME},
         null)
@@ -146,7 +124,7 @@ public class UserIndex {
     BoolFilterBuilder userFilter = FilterBuilders.boolFilter()
       .must(FilterBuilders.termFilter(UserIndexDefinition.FIELD_ACTIVE, true));
 
-    QueryBuilder query = null;
+    QueryBuilder query;
     if (StringUtils.isEmpty(searchText)) {
       query = QueryBuilders.matchAllQuery();
     } else {
@@ -163,4 +141,15 @@ public class UserIndex {
 
     return new SearchResult<>(request.get(), DOC_CONVERTER);
   }
+
+  /**
+   * Convert an Elasticsearch result (a map) to an {@link UserDoc}. It's
+   * used for {@link org.sonar.server.es.SearchResult}.
+   */
+  private static final Function<Map<String, Object>, UserDoc> DOC_CONVERTER = new NonNullInputFunction<Map<String, Object>, UserDoc>() {
+    @Override
+    protected UserDoc doApply(Map<String, Object> input) {
+      return new UserDoc(input);
+    }
+  };
 }

server/sonar-server/src/test/java/org/sonar/server/computation/issue/ScmAccountToUserLoaderTest.java

@@ -27,9 +27,11 @@ import org.sonar.api.config.Settings;
 import org.sonar.api.utils.log.LogTester;
 import org.sonar.api.utils.log.LoggerLevel;
 import org.sonar.server.es.EsTester;
+import org.sonar.server.user.index.UserDoc;
 import org.sonar.server.user.index.UserIndex;
 import org.sonar.server.user.index.UserIndexDefinition;
 
+import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.fail;
 
@@ -48,30 +50,51 @@ public class ScmAccountToUserLoaderTest {
 
   @Test
   public void load_login_for_scm_account() throws Exception {
-    esTester.putDocuments("users", "user", getClass(), "charlie.json");
+    UserDoc user = new UserDoc()
+      .setLogin("charlie")
+      .setName("Charlie")
+      .setEmail("charlie@hebdo.com")
+      .setActive(true)
+      .setScmAccounts(asList("charlie", "jesuis@charlie.com"));
+    esTester.index(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, "charlie", user.getFields());
+
     UserIndex index = new UserIndex(esTester.client());
-    ScmAccountToUserLoader loader = new ScmAccountToUserLoader(index);
+    ScmAccountToUserLoader underTest = new ScmAccountToUserLoader(index);
 
-    assertThat(loader.load("missing")).isNull();
-    assertThat(loader.load("jesuis@charlie.com")).isEqualTo("charlie");
+    assertThat(underTest.load("missing")).isNull();
+    assertThat(underTest.load("jesuis@charlie.com")).isEqualTo("charlie");
   }
 
   @Test
-  public void warn_if_multiple_users_share_same_scm_account() throws Exception {
-    esTester.putDocuments("users", "user", getClass(), "charlie.json", "charlie_conflict.json");
+  public void warn_if_multiple_users_share_the_same_scm_account() throws Exception {
+    UserDoc user1 = new UserDoc()
+      .setLogin("charlie")
+      .setName("Charlie")
+      .setEmail("charlie@hebdo.com")
+      .setActive(true)
+      .setScmAccounts(asList("charlie", "jesuis@charlie.com"));
+    esTester.index(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, user1.login(), user1.getFields());
+
+    UserDoc user2 = new UserDoc()
+      .setLogin("another.charlie")
+      .setName("Another Charlie")
+      .setActive(true)
+      .setScmAccounts(asList("charlie"));
+    esTester.index(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, user2.login(), user2.getFields());
+
     UserIndex index = new UserIndex(esTester.client());
-    ScmAccountToUserLoader loader = new ScmAccountToUserLoader(index);
+    ScmAccountToUserLoader underTest = new ScmAccountToUserLoader(index);
 
-    assertThat(loader.load("charlie")).isNull();
+    assertThat(underTest.load("charlie")).isNull();
     assertThat(logTester.logs(LoggerLevel.WARN)).contains("Multiple users share the SCM account 'charlie': another.charlie, charlie");
   }
 
   @Test
   public void load_by_multiple_scm_accounts_is_not_supported_yet() {
     UserIndex index = new UserIndex(esTester.client());
-    ScmAccountToUserLoader loader = new ScmAccountToUserLoader(index);
+    ScmAccountToUserLoader underTest = new ScmAccountToUserLoader(index);
     try {
-      loader.loadAll(Collections.<String>emptyList());
+      underTest.loadAll(Collections.<String>emptyList());
       fail();
     } catch (UnsupportedOperationException ignored) {
     }

server/sonar-server/src/test/java/org/sonar/server/user/index/UserIndexTest.java

@@ -19,19 +19,29 @@
  */
 package org.sonar.server.user.index;
 
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
 import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.sonar.api.config.Settings;
 import org.sonar.server.es.EsTester;
 import org.sonar.server.es.SearchOptions;
-import org.sonar.server.exceptions.NotFoundException;
 
+import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.fail;
+import static org.sonar.server.user.index.UserIndexDefinition.INDEX;
+import static org.sonar.server.user.index.UserIndexDefinition.TYPE_USER;
 
 public class UserIndexTest {
 
+  private static final String USER1_LOGIN = "user1";
+  private static final String USER2_LOGIN = "user2";
+  private static final long DATE_1 = 1_500_000_000_000L;
+  private static final long DATE_2 = 1_500_000_000_001L;
+
   @ClassRule
   public static EsTester esTester = new EsTester().addDefinitions(new UserIndexDefinition(new Settings()));
 
@@ -45,17 +55,19 @@ public class UserIndexTest {
 
   @Test
   public void get_nullable_by_login() throws Exception {
-    esTester.putDocuments(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, this.getClass(), "user1.json", "user2.json");
+    UserDoc user1 = newUser(USER1_LOGIN, asList("scmA", "scmB"));
+    esTester.index(INDEX, TYPE_USER, USER1_LOGIN, user1.getFields());
+    esTester.index(INDEX, TYPE_USER, USER2_LOGIN, newUser(USER2_LOGIN, Collections.<String>emptyList()).getFields());
 
-    UserDoc userDoc = index.getNullableByLogin("user1");
+    UserDoc userDoc = index.getNullableByLogin(USER1_LOGIN);
     assertThat(userDoc).isNotNull();
-    assertThat(userDoc.login()).isEqualTo("user1");
-    assertThat(userDoc.name()).isEqualTo("User1");
-    assertThat(userDoc.email()).isEqualTo("user1@mail.com");
+    assertThat(userDoc.login()).isEqualTo(user1.login());
+    assertThat(userDoc.name()).isEqualTo(user1.name());
+    assertThat(userDoc.email()).isEqualTo(user1.email());
     assertThat(userDoc.active()).isTrue();
-    assertThat(userDoc.scmAccounts()).containsOnly("user_1", "u1");
-    assertThat(userDoc.createdAt()).isEqualTo(1500000000000L);
-    assertThat(userDoc.updatedAt()).isEqualTo(1500000000000L);
+    assertThat(userDoc.scmAccounts()).isEqualTo(user1.scmAccounts());
+    assertThat(userDoc.createdAt()).isEqualTo(user1.createdAt());
+    assertThat(userDoc.updatedAt()).isEqualTo(user1.updatedAt());
 
     assertThat(index.getNullableByLogin("")).isNull();
     assertThat(index.getNullableByLogin("unknown")).isNull();
@@ -63,46 +75,27 @@ public class UserIndexTest {
 
   @Test
   public void get_nullable_by_login_should_be_case_sensitive() throws Exception {
-    esTester.putDocuments(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, this.getClass(), "user1.json");
-
-    assertThat(index.getNullableByLogin("user1")).isNotNull();
-    assertThat(index.getNullableByLogin("User1")).isNull();
-  }
-
-  @Test
-  public void get_by_login() throws Exception {
-    esTester.putDocuments(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, this.getClass(), "user1.json", "user2.json");
-
-    UserDoc userDoc = index.getByLogin("user1");
-    assertThat(userDoc).isNotNull();
-    assertThat(userDoc.login()).isEqualTo("user1");
-    assertThat(userDoc.name()).isEqualTo("User1");
-    assertThat(userDoc.email()).isEqualTo("user1@mail.com");
-    assertThat(userDoc.active()).isTrue();
-    assertThat(userDoc.scmAccounts()).containsOnly("user_1", "u1");
-    assertThat(userDoc.createdAt()).isEqualTo(1500000000000L);
-    assertThat(userDoc.updatedAt()).isEqualTo(1500000000000L);
-  }
+    UserDoc user1 = newUser(USER1_LOGIN, asList("scmA", "scmB"));
+    esTester.index(INDEX, TYPE_USER, USER1_LOGIN, user1.getFields());
 
-  @Test
-  public void fail_to_get_by_login_on_unknown_user() {
-    try {
-      index.getByLogin("unknown");
-      fail();
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(NotFoundException.class).hasMessage("User 'unknown' not found");
-    }
+    assertThat(index.getNullableByLogin(USER1_LOGIN)).isNotNull();
+    assertThat(index.getNullableByLogin("UsEr1")).isNull();
   }
 
   @Test
   public void getAtMostThreeActiveUsersForScmAccount() throws Exception {
-    esTester.putDocuments(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, this.getClass(), "user1.json", "user3-with-same-email-as-user1.json", "inactive-user.json");
+    UserDoc user1 = newUser("user1", asList("user_1", "u1"));
+    UserDoc user2 = newUser("user_with_same_email_as_user1", asList("user_2")).setEmail(user1.email());
+    UserDoc user3 = newUser("inactive_user_with_same_scm_as_user1", user1.scmAccounts()).setActive(false);
+    esTester.index(INDEX, TYPE_USER, user1.login(), user1.getFields());
+    esTester.index(INDEX, TYPE_USER, user2.login(), user2.getFields());
+    esTester.index(INDEX, TYPE_USER, user3.login(), user3.getFields());
 
-    assertThat(index.getAtMostThreeActiveUsersForScmAccount("user_1")).extractingResultOf("login").containsOnly("user1");
-    assertThat(index.getAtMostThreeActiveUsersForScmAccount("user1")).extractingResultOf("login").containsOnly("user1");
+    assertThat(index.getAtMostThreeActiveUsersForScmAccount(user1.scmAccounts().get(0))).extractingResultOf("login").containsOnly(user1.login());
+    assertThat(index.getAtMostThreeActiveUsersForScmAccount(user1.login())).extractingResultOf("login").containsOnly(user1.login());
 
     // both users share the same email
-    assertThat(index.getAtMostThreeActiveUsersForScmAccount("user1@mail.com")).extractingResultOf("login").containsOnly("user1", "user3");
+    assertThat(index.getAtMostThreeActiveUsersForScmAccount(user1.email())).extractingResultOf("login").containsOnly(user1.login(), user2.login());
 
     assertThat(index.getAtMostThreeActiveUsersForScmAccount("")).isEmpty();
     assertThat(index.getAtMostThreeActiveUsersForScmAccount("unknown")).isEmpty();
@@ -110,30 +103,50 @@ public class UserIndexTest {
 
   @Test
   public void getAtMostThreeActiveUsersForScmAccount_ignore_inactive_user() throws Exception {
-    esTester.putDocuments(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, this.getClass(), "inactive-user.json");
+    String scmAccount = "scmA";
+    UserDoc user = newUser(USER1_LOGIN, asList(scmAccount)).setActive(false);
+    esTester.index(INDEX, TYPE_USER, user.login(), user.getFields());
 
-    assertThat(index.getAtMostThreeActiveUsersForScmAccount("inactiveUser")).isEmpty();
-    assertThat(index.getAtMostThreeActiveUsersForScmAccount("user_1")).isEmpty();
+    assertThat(index.getAtMostThreeActiveUsersForScmAccount(user.login())).isEmpty();
+    assertThat(index.getAtMostThreeActiveUsersForScmAccount(scmAccount)).isEmpty();
   }
 
   @Test
   public void getAtMostThreeActiveUsersForScmAccount_max_three() throws Exception {
-    esTester.putDocuments(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, this.getClass(), "user1.json",
-      "user2-with-same-email-as-user1.json", "user3-with-same-email-as-user1.json", "user4-with-same-email-as-user1.json");
+    String email = "user@mail.com";
+    UserDoc user1 = newUser("user1", Collections.<String>emptyList()).setEmail(email);
+    UserDoc user2 = newUser("user2", Collections.<String>emptyList()).setEmail(email);
+    UserDoc user3 = newUser("user3", Collections.<String>emptyList()).setEmail(email);
+    UserDoc user4 = newUser("user4", Collections.<String>emptyList()).setEmail(email);
+    esTester.index(INDEX, TYPE_USER, user1.login(), user1.getFields());
+    esTester.index(INDEX, TYPE_USER, user2.login(), user2.getFields());
+    esTester.index(INDEX, TYPE_USER, user3.login(), user3.getFields());
+    esTester.index(INDEX, TYPE_USER, user4.login(), user4.getFields());
 
     // restrict results to 3 users
-    assertThat(index.getAtMostThreeActiveUsersForScmAccount("user1@mail.com")).hasSize(3);
+    assertThat(index.getAtMostThreeActiveUsersForScmAccount(email)).hasSize(3);
   }
 
   @Test
   public void searchUsers() throws Exception {
-    esTester.putDocuments(UserIndexDefinition.INDEX, UserIndexDefinition.TYPE_USER, this.getClass(),
-      "user1.json", "user2.json");
+    esTester.index(INDEX, TYPE_USER, USER1_LOGIN, newUser(USER1_LOGIN, Arrays.asList("user_1", "u1")).getFields());
+    esTester.index(INDEX, TYPE_USER, USER2_LOGIN, newUser(USER2_LOGIN, Collections.<String>emptyList()).getFields());
 
     assertThat(index.search(null, new SearchOptions()).getDocs()).hasSize(2);
     assertThat(index.search("user", new SearchOptions()).getDocs()).hasSize(2);
     assertThat(index.search("ser", new SearchOptions()).getDocs()).hasSize(2);
-    assertThat(index.search("user1", new SearchOptions()).getDocs()).hasSize(1);
-    assertThat(index.search("user2", new SearchOptions()).getDocs()).hasSize(1);
+    assertThat(index.search(USER1_LOGIN, new SearchOptions()).getDocs()).hasSize(1);
+    assertThat(index.search(USER2_LOGIN, new SearchOptions()).getDocs()).hasSize(1);
+  }
+
+  private static UserDoc newUser(String login, List<String> scmAccounts) {
+    return new UserDoc()
+      .setLogin(login)
+      .setName(login.toUpperCase(Locale.ENGLISH))
+      .setEmail(login + "@mail.com")
+      .setActive(true)
+      .setScmAccounts(scmAccounts)
+      .setCreatedAt(DATE_1)
+      .setUpdatedAt(DATE_2);
   }
 }

server/sonar-server/src/test/java/org/sonar/server/user/ws/CreateActionTest.java

@@ -119,7 +119,7 @@ public class CreateActionTest {
       .setParam("password", "1234").execute()
       .assertJson(getClass(), "create_user.json");
 
-    UserDoc user = index.getByLogin("john");
+    UserDoc user = index.getNullableByLogin("john");
     assertThat(user.login()).isEqualTo("john");
     assertThat(user.name()).isEqualTo("John");
     assertThat(user.email()).isEqualTo("john@email.com");
@@ -138,7 +138,7 @@ public class CreateActionTest {
       .setParam("password", "1234").execute()
       .assertJson(getClass(), "create_user.json");
 
-    UserDoc user = index.getByLogin("john");
+    UserDoc user = index.getNullableByLogin("john");
     assertThat(user.login()).isEqualTo("john");
     assertThat(user.name()).isEqualTo("John");
     assertThat(user.email()).isEqualTo("john@email.com");
@@ -164,7 +164,7 @@ public class CreateActionTest {
       .setParam("password", "1234").execute()
       .assertJson(getClass(), "reactivate_user.json");
 
-    assertThat(index.getByLogin("john").active()).isTrue();
+    assertThat(index.getNullableByLogin("john").active()).isTrue();
   }
 
   @Test(expected = ForbiddenException.class)

server/sonar-server/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java

@@ -100,7 +100,7 @@ public class DeactivateActionTest {
       .execute()
       .assertJson(getClass(), "deactivate_user.json");
 
-    UserDoc user = index.getByLogin("john");
+    UserDoc user = index.getNullableByLogin("john");
     assertThat(user.active()).isFalse();
     assertThat(dbClient.userTokenDao().selectByLogin(dbSession, "john")).isEmpty();
   }

server/sonar-server/src/test/resources/org/sonar/server/user/index/UserIndexTest/inactive-user.json

@@ -1,9 +0,0 @@
-{
-  "login": "inactiveUser",
-  "name": "User1",
-  "email": "user1@mail.com",
-  "active": false,
-  "scmAccounts": ["user_1", "u1"],
-  "createdAt": 1500000000000,
-  "updatedAt": 1500000000000
-}

server/sonar-server/src/test/resources/org/sonar/server/user/index/UserIndexTest/user1.json

@@ -1,9 +0,0 @@
-{
-  "login": "user1",
-  "name": "User1",
-  "email": "user1@mail.com",
-  "active": true,
-  "scmAccounts": ["user_1", "u1"],
-  "createdAt": 1500000000000,
-  "updatedAt": 1500000000000
-}

server/sonar-server/src/test/resources/org/sonar/server/user/index/UserIndexTest/user2-with-same-email-as-user1.json

@@ -1,10 +0,0 @@
-{
-  "login": "user2",
-  "name": "User2",
-  "email": "user1@mail.com",
-  "active": true,
-  "scmAccounts": ["u2"],
-  "createdAt": 1500000000000,
-  "updatedAt": 1500000000000
-}
-

server/sonar-server/src/test/resources/org/sonar/server/user/index/UserIndexTest/user2.json

@@ -1,10 +0,0 @@
-{
-  "login": "user2",
-  "name": "User2",
-  "email": "user2@mail.com",
-  "active": true,
-  "scmAccounts": ["u2"],
-  "createdAt": 1500000000000,
-  "updatedAt": 1500000000000
-}
-

server/sonar-server/src/test/resources/org/sonar/server/user/index/UserIndexTest/user3-with-same-email-as-user1.json

@@ -1,10 +0,0 @@
-{
-  "login": "user3",
-  "name": "User3",
-  "email": "user1@mail.com",
-  "active": true,
-  "scmAccounts": ["u3"],
-  "createdAt": 1500000000000,
-  "updatedAt": 1500000000000
-}
-

server/sonar-server/src/test/resources/org/sonar/server/user/index/UserIndexTest/user4-with-same-email-as-user1.json

@@ -1,10 +0,0 @@
-{
-  "login": "user4",
-  "name": "User4",
-  "email": "user1@mail.com",
-  "active": true,
-  "scmAccounts": ["u4"],
-  "createdAt": 1500000000000,
-  "updatedAt": 1500000000000
-}
-