From 56b361c1785c7e13386e9469ff558c861471d3ba Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Mon, 5 Jan 2015 18:02:04 +0100 Subject: [PATCH] SONAR-5830 Change the way to persist scm accounts in db in order to improve the search --- .../org/sonar/server/user/UserUpdater.java | 12 ++++++-- .../user/index/UserResultSetIterator.java | 5 +++- .../server/user/UserServiceMediumTest.java | 24 +++++++++++++-- .../sonar/server/user/UserUpdaterTest.java | 30 ++++++++++++++----- .../org/sonar/server/user/db/UserDaoTest.java | 2 +- ..._user_when_scm_account_is_already_used.xml | 6 ++++ .../user/UserUpdaterTest/update_user.xml | 2 +- .../user/db/UserDaoTest/select_by_login.xml | 2 +- .../UserResultSetIteratorTest/shared.xml | 4 +-- .../java/org/sonar/core/user/UserDaoTest.java | 8 ++--- 10 files changed, 72 insertions(+), 23 deletions(-) create mode 100644 server/sonar-server/src/test/resources/org/sonar/server/user/UserUpdaterTest/fail_to_update_user_when_scm_account_is_already_used.xml 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 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 { 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"); @@ -526,6 +526,20 @@ public class UserUpdaterTest { assertThat(dto.getCryptedPassword()).isEqualTo("650d2261c98361e2f67f90ce5c65a95e7d8ea2fg"); } + @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"); @@ -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 @@ + + + + + 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 @@ - 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 @@ - 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 @@ 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); -- 2.39.5