]> source.dussan.org Git - sonarqube.git/commitdiff
Fix quality flaws
authorSimon Brandhof <simon.brandhof@sonarsource.com>
Mon, 8 Dec 2014 10:50:57 +0000 (11:50 +0100)
committerSimon Brandhof <simon.brandhof@sonarsource.com>
Mon, 8 Dec 2014 11:01:57 +0000 (12:01 +0100)
13 files changed:
server/sonar-server/src/main/java/org/sonar/server/db/ResultSetIterator.java
server/sonar-server/src/main/java/org/sonar/server/es/BulkIndexer.java
server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationDao.java
server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationIndexer.java
server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueDoc.java
server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndexDefinition.java
server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueNormalizer.java
server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueResultSetIterator.java
server/sonar-server/src/main/java/org/sonar/server/util/ProgressLogger.java
server/sonar-server/src/test/java/org/sonar/server/issue/IssueTesting.java
server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationDaoTest.java
server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationIndexerTest.java
server/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceMediumTest.java

index 4c7a8bcfdfb57228c51a9a8ceb94e50a165b28de..d4a73ecadbf9b6dd56b1be38b7876ed64a3cef8b 100644 (file)
@@ -69,7 +69,6 @@ public abstract class ResultSetIterator<E> implements Iterator<E>, Closeable {
     try {
       return read(rs);
     } catch (SQLException e) {
-      // TODO add SQL request to context
       throw new IllegalStateException("Fail to read result set row", e);
     }
   }
index e3ffdc5ab207d7c515bde8cf083a3494114963a8..b08af55a94e42bd2aadc94a1dcb40a7ee43e4e9a 100644 (file)
@@ -52,6 +52,7 @@ public class BulkIndexer implements Startable {
   private static final Logger LOGGER = LoggerFactory.getLogger(BulkIndexer.class);
   private static final long FLUSH_BYTE_SIZE = new ByteSizeValue(5, ByteSizeUnit.MB).bytes();
   private static final String REFRESH_INTERVAL_SETTING = "index.refresh_interval";
+  private static final String ALREADY_STARTED_MESSAGE = "Bulk indexing is already started";
 
   private final EsClient client;
   private final String indexName;
@@ -77,13 +78,13 @@ public class BulkIndexer implements Startable {
    */
 
   public BulkIndexer setLarge(boolean b) {
-    Preconditions.checkState(bulkRequest == null, "Bulk indexing is already started");
+    Preconditions.checkState(bulkRequest == null, ALREADY_STARTED_MESSAGE);
     this.large = b;
     return this;
   }
 
   public BulkIndexer setRefresh(boolean b) {
-    Preconditions.checkState(bulkRequest == null, "Bulk indexing is already started");
+    Preconditions.checkState(bulkRequest == null, ALREADY_STARTED_MESSAGE);
     this.refresh = b;
     return this;
   }
@@ -99,7 +100,7 @@ public class BulkIndexer implements Startable {
 
   @Override
   public void start() {
-    Preconditions.checkState(bulkRequest == null, "Bulk indexing is already started");
+    Preconditions.checkState(bulkRequest == null, ALREADY_STARTED_MESSAGE);
     if (large) {
       largeInitialSettings = Maps.newHashMap();
       Map<String, Object> bulkSettings = Maps.newHashMap();
index de0936f372513649f6e556472bf2d26108df8693..9b5b5861091eab7bae91570be646b486fde290f7 100644 (file)
@@ -41,8 +41,8 @@ public class IssueAuthorizationDao {
   public static final class Dto {
     private final String projectUuid;
     private final long updatedAt;
-    private List<String> users = Lists.newArrayList();
-    private List<String> groups = Lists.newArrayList();
+    private final List<String> users = Lists.newArrayList();
+    private final List<String> groups = Lists.newArrayList();
 
     public Dto(String projectUuid, long updatedAt) {
       this.projectUuid = projectUuid;
@@ -72,10 +72,6 @@ public class IssueAuthorizationDao {
     public List<String> getGroups() {
       return groups;
     }
-
-    boolean hasNoGroupsNorUsers() {
-      return users.isEmpty() && groups.isEmpty();
-    }
   }
 
   private static final String SQL_TEMPLATE =
index 2121a661182d5e6e977d91b4007c3e5d001244e3..04bf850b395d4b31c6c98108ab1ed56107743b9e 100644 (file)
@@ -23,7 +23,6 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import org.apache.commons.dbutils.DbUtils;
 import org.elasticsearch.action.ActionRequest;
-import org.elasticsearch.action.delete.DeleteRequest;
 import org.elasticsearch.action.update.UpdateRequest;
 import org.sonar.core.persistence.DbSession;
 import org.sonar.server.db.DbClient;
@@ -97,22 +96,15 @@ public class IssueAuthorizationIndexer extends BaseIndexer {
 
   private ActionRequest newUpdateRequest(IssueAuthorizationDao.Dto dto) {
     ActionRequest request;
-    if (dto.hasNoGroupsNorUsers()) {
-      // project still exists but there are no permissions
-      // TODO do we really need to delete the document ? Pushing empty groups/users should be enough
-      request = new DeleteRequest(IssueIndexDefinition.INDEX, IssueIndexDefinition.TYPE_AUTHORIZATION, dto.getProjectUuid())
-        .routing(dto.getProjectUuid());
-    } else {
-      Map<String, Object> doc = ImmutableMap.of(
-        IssueIndexDefinition.FIELD_AUTHORIZATION_PROJECT_UUID, dto.getProjectUuid(),
-        IssueIndexDefinition.FIELD_AUTHORIZATION_GROUPS, dto.getGroups(),
-        IssueIndexDefinition.FIELD_AUTHORIZATION_USERS, dto.getUsers(),
-        IssueIndexDefinition.FIELD_AUTHORIZATION_UPDATED_AT, new Date(dto.getUpdatedAt()));
-      request = new UpdateRequest(IssueIndexDefinition.INDEX, IssueIndexDefinition.TYPE_AUTHORIZATION, dto.getProjectUuid())
-        .routing(dto.getProjectUuid())
-        .doc(doc)
-        .upsert(doc);
-    }
+    Map<String, Object> doc = ImmutableMap.of(
+      IssueIndexDefinition.FIELD_AUTHORIZATION_PROJECT_UUID, dto.getProjectUuid(),
+      IssueIndexDefinition.FIELD_AUTHORIZATION_GROUPS, dto.getGroups(),
+      IssueIndexDefinition.FIELD_AUTHORIZATION_USERS, dto.getUsers(),
+      IssueIndexDefinition.FIELD_AUTHORIZATION_UPDATED_AT, new Date(dto.getUpdatedAt()));
+    request = new UpdateRequest(IssueIndexDefinition.INDEX, IssueIndexDefinition.TYPE_AUTHORIZATION, dto.getProjectUuid())
+      .routing(dto.getProjectUuid())
+      .doc(doc)
+      .upsert(doc);
     return request;
   }
 }
index 7db14c4895f989a094537e665af1c364a5378900..796039db8db95e82f6468ceec9d3adf22d6bf3d4 100644 (file)
@@ -254,10 +254,6 @@ public class IssueDoc extends BaseDoc implements Issue {
     setField(IssueIndexDefinition.FIELD_ISSUE_ASSIGNEE, s);
   }
 
-  public void setTechnicalCreationDate(@Nullable Date d) {
-    setField(IssueIndexDefinition.FIELD_ISSUE_TECHNICAL_CREATED_AT, d);
-  }
-
   public void setFuncUpdateDate(@Nullable Date d) {
     setField(IssueIndexDefinition.FIELD_ISSUE_FUNC_UPDATED_AT, d);
   }
index ca7d0d9ed961566e63d8f40b2a43d7e811bbdafd..57ab2fc0456bb18ff510fd8281b276ed9cd4496b 100644 (file)
@@ -46,10 +46,6 @@ public class IssueIndexDefinition implements IndexDefinition {
   public static final String FIELD_ISSUE_ATTRIBUTES = "attributes";
   public static final String FIELD_ISSUE_AUTHOR_LOGIN = "authorLogin";
   public static final String FIELD_ISSUE_COMPONENT_UUID = "component";
-  /**
-   * Technical date
-   */
-  public static final String FIELD_ISSUE_TECHNICAL_CREATED_AT = "createdAt";
   public static final String FIELD_ISSUE_DEBT = "debt";
   public static final String FIELD_ISSUE_EFFORT = "effort";
   public static final String FIELD_ISSUE_FILE_PATH = "filePath";
@@ -115,11 +111,9 @@ public class IssueIndexDefinition implements IndexDefinition {
     issueMapping.setAttribute("_parent", ImmutableMap.of("type", TYPE_AUTHORIZATION));
     issueMapping.setAttribute("_routing", ImmutableMap.of("required", true, "path", FIELD_ISSUE_PROJECT_UUID));
     issueMapping.stringFieldBuilder(FIELD_ISSUE_ACTION_PLAN).build();
-    // TODO do we really sort by assignee ?
     issueMapping.stringFieldBuilder(FIELD_ISSUE_ASSIGNEE).enableSorting().build();
     issueMapping.stringFieldBuilder(FIELD_ISSUE_ATTRIBUTES).build();
     issueMapping.stringFieldBuilder(FIELD_ISSUE_AUTHOR_LOGIN).build();
-    // TODO rename into componentUuid ? or restrict to fileUuid ?
     issueMapping.stringFieldBuilder(FIELD_ISSUE_COMPONENT_UUID).build();
     issueMapping.createLongField(FIELD_ISSUE_DEBT);
     issueMapping.createDoubleField(FIELD_ISSUE_EFFORT);
@@ -133,19 +127,13 @@ public class IssueIndexDefinition implements IndexDefinition {
     issueMapping.stringFieldBuilder(FIELD_ISSUE_MESSAGE).build();
     issueMapping.stringFieldBuilder(FIELD_ISSUE_MODULE_UUID).build();
     issueMapping.createUuidPathField(FIELD_ISSUE_MODULE_PATH);
-    // TODO do we need to sort by project ?
-    // TODO rename into projectUuid ?
     issueMapping.stringFieldBuilder(FIELD_ISSUE_PROJECT_UUID).enableSorting().build();
     issueMapping.stringFieldBuilder(FIELD_ISSUE_REPORTER).build();
     issueMapping.stringFieldBuilder(FIELD_ISSUE_RESOLUTION).build();
     issueMapping.stringFieldBuilder(FIELD_ISSUE_RULE_KEY).build();
     issueMapping.stringFieldBuilder(FIELD_ISSUE_SEVERITY).build();
-    // TODO do we need to sort by severity ?
     issueMapping.createByteField(FIELD_ISSUE_SEVERITY_VALUE);
-    // TODO do we really sort by status ? If yes, then we should sort by "int value", but not by string key
     issueMapping.stringFieldBuilder(FIELD_ISSUE_STATUS).enableSorting().build();
-    // TODO is createdAt required ?
-    issueMapping.createDateTimeField(FIELD_ISSUE_TECHNICAL_CREATED_AT);
     issueMapping.createDateTimeField(FIELD_ISSUE_TECHNICAL_UPDATED_AT);
   }
 }
index 1ff008e91be9c2e87209d9f7e71593c05cecddc9..1a7e3dd6d0a42f4f3e3b59cecdd0d6c6ddf37981 100644 (file)
@@ -42,7 +42,6 @@ public class IssueNormalizer extends BaseNormalizer {
   public static final class IssueField extends Indexable {
 
     public static final IndexField KEY = addSortable(IndexField.Type.STRING, "key");
-    public static final IndexField CREATED_AT = add(IndexField.Type.DATE, "createdAt");
     public static final IndexField UPDATED_AT = add(IndexField.Type.DATE, "updatedAt");
 
     public static final IndexField PROJECT = addSortable(IndexField.Type.STRING, "project");
index 3e0a962b78f289168ee0f33e857e0a551121453b..620884bd84bac8aa6874d60c81909042de91d2cc 100644 (file)
@@ -43,16 +43,15 @@ class IssueResultSetIterator extends ResultSetIterator<IssueDoc> {
     "i.kee",
     "root.uuid",
     "i.updated_at",
-    "i.created_at",
     "i.action_plan_key",
     "i.assignee",
     "i.effort_to_fix",
     "i.issue_attributes",
     "i.line",
     "i.message",
+    "i.resolution",
 
     // column 11
-    "i.resolution",
     "i.severity",
     "i.status",
     "i.technical_debt",
@@ -62,9 +61,9 @@ class IssueResultSetIterator extends ResultSetIterator<IssueDoc> {
     "i.issue_creation_date",
     "i.issue_update_date",
     "r.plugin_name",
+    "r.plugin_rule_key",
 
     // column 21
-    "r.plugin_rule_key",
     "r.language",
     "p.uuid",
     "p.module_uuid",
@@ -107,30 +106,29 @@ class IssueResultSetIterator extends ResultSetIterator<IssueDoc> {
     doc.setKey(key);
     doc.setProjectUuid(projectUuid);
     doc.setTechnicalUpdateDate(new Date(rs.getLong(3)));
-    doc.setTechnicalCreationDate(new Date(rs.getLong(4)));
-    doc.setActionPlanKey(rs.getString(5));
-    doc.setAssignee(rs.getString(6));
-    doc.setEffortToFix(SqlUtil.getDouble(rs, 7));
-    doc.setAttributes(rs.getString(8));
-    doc.setLine(SqlUtil.getInt(rs, 9));
-    doc.setMessage(rs.getString(10));
-    doc.setResolution(rs.getString(11));
-    doc.setSeverity(rs.getString(12));
-    doc.setStatus(rs.getString(13));
-    doc.setDebt(SqlUtil.getLong(rs, 14));
-    doc.setReporter(rs.getString(15));
-    doc.setAuthorLogin(rs.getString(16));
-    doc.setFuncCloseDate(SqlUtil.getDate(rs, 17));
-    doc.setFuncCreationDate(SqlUtil.getDate(rs, 18));
-    doc.setFuncUpdateDate(SqlUtil.getDate(rs, 19));
-    String ruleRepo = rs.getString(20);
-    String ruleKey = rs.getString(21);
+    doc.setActionPlanKey(rs.getString(4));
+    doc.setAssignee(rs.getString(5));
+    doc.setEffortToFix(SqlUtil.getDouble(rs, 6));
+    doc.setAttributes(rs.getString(7));
+    doc.setLine(SqlUtil.getInt(rs, 8));
+    doc.setMessage(rs.getString(9));
+    doc.setResolution(rs.getString(10));
+    doc.setSeverity(rs.getString(11));
+    doc.setStatus(rs.getString(12));
+    doc.setDebt(SqlUtil.getLong(rs, 13));
+    doc.setReporter(rs.getString(14));
+    doc.setAuthorLogin(rs.getString(15));
+    doc.setFuncCloseDate(SqlUtil.getDate(rs, 16));
+    doc.setFuncCreationDate(SqlUtil.getDate(rs, 17));
+    doc.setFuncUpdateDate(SqlUtil.getDate(rs, 18));
+    String ruleRepo = rs.getString(19);
+    String ruleKey = rs.getString(20);
     doc.setRuleKey(RuleKey.of(ruleRepo, ruleKey).toString());
-    doc.setLanguage(rs.getString(22));
-    doc.setComponentUuid(rs.getString(23));
-    doc.setModuleUuid(rs.getString(24));
-    doc.setModuleUuidPath(rs.getString(25));
-    doc.setFilePath(rs.getString(26));
+    doc.setLanguage(rs.getString(21));
+    doc.setComponentUuid(rs.getString(22));
+    doc.setModuleUuid(rs.getString(23));
+    doc.setModuleUuidPath(rs.getString(24));
+    doc.setFilePath(rs.getString(25));
     return doc;
   }
 }
index 97fc702d8b80310821bfd2385668ed5ef77bafe1..de0ba605c9b6c9168b76ff2dbd58ca757b85e6c7 100644 (file)
@@ -86,7 +86,7 @@ public class ProgressLogger {
   }
 
   public void log() {
-    task.run();
+    task.log();
   }
 
   private static class LoggerTimerTask extends TimerTask {
@@ -101,6 +101,10 @@ public class ProgressLogger {
 
     @Override
     public void run() {
+      log();
+    }
+
+    private void log() {
       logger.info(String.format("%d %s processed", counter.get(), pluralLabel));
     }
   }
index 48947ea610aab00170ed5547f722e779542614f0..cef689ede3ebfdb4bc99be4a2b5c05410ca1ef4e 100644 (file)
@@ -85,7 +85,6 @@ public class IssueTesting {
     doc.setFuncCreationDate(DateUtils.parseDate("2014-09-04"));
     doc.setFuncUpdateDate(DateUtils.parseDate("2014-12-04"));
     doc.setFuncCloseDate(null);
-    doc.setTechnicalCreationDate(DateUtils.parseDate("2014-09-04"));
     doc.setTechnicalUpdateDate(DateUtils.parseDate("2014-12-04"));
     return doc;
   }
index 5ba96b58dbbf2a36209bb7a27015516680b2439f..e0312a185c75d7f38ad1270da74e770142defe57 100644 (file)
@@ -64,13 +64,11 @@ public class IssueAuthorizationDaoTest {
     IssueAuthorizationDao.Dto abc = Iterables.find(dtos, new ProjectPredicate("ABC"));
     assertThat(abc.getGroups()).containsOnly("Anyone", "devs");
     assertThat(abc.getUsers()).containsOnly("user1");
-    assertThat(abc.hasNoGroupsNorUsers()).isFalse();
     assertThat(abc.getUpdatedAt()).isNotNull();
 
     IssueAuthorizationDao.Dto def = Iterables.find(dtos, new ProjectPredicate("DEF"));
     assertThat(def.getGroups()).containsOnly("Anyone");
     assertThat(def.getUsers()).containsOnly("user1", "user2");
-    assertThat(def.hasNoGroupsNorUsers()).isFalse();
     assertThat(def.getUpdatedAt()).isNotNull();
   }
 
@@ -96,7 +94,6 @@ public class IssueAuthorizationDaoTest {
 
     assertThat(dtos).hasSize(1);
     IssueAuthorizationDao.Dto abc = Iterables.find(dtos, new ProjectPredicate("ABC"));
-    assertThat(abc.hasNoGroupsNorUsers()).isTrue();
     assertThat(abc.getGroups()).isEmpty();
     assertThat(abc.getUsers()).isEmpty();
     assertThat(abc.getUpdatedAt()).isNotNull();
index bc7bbb49174803c70912c7ab2558e66afdf86f5f..d05db36d9e3477baf6a92020c4770f08b8315b11 100644 (file)
@@ -91,7 +91,10 @@ public class IssueAuthorizationIndexerTest {
     authorization = new IssueAuthorizationDao.Dto("ABC", System.currentTimeMillis());
     indexer.index(Arrays.asList(authorization));
 
-    assertThat(esTester.countDocuments("issues", "authorization")).isZero();
+    List<SearchHit> docs = esTester.getDocuments("issues", "authorization");
+    assertThat(docs).hasSize(1);
+    assertThat((Collection)docs.get(0).sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_USERS)).hasSize(0);
+    assertThat((Collection)docs.get(0).sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_GROUPS)).hasSize(0);
   }
 
   private IssueAuthorizationIndexer createIndexer() {
index 2070e40c6137e523b5f299d7027dbde1bd627873..f0a6aebb57f77f91a9dbfdb1dbf2baba6b319084 100644 (file)
@@ -21,6 +21,8 @@
 package org.sonar.server.permission;
 
 import com.google.common.collect.Maps;
+import org.elasticsearch.action.search.SearchResponse;
+import org.elasticsearch.search.SearchHit;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.ClassRule;
@@ -40,6 +42,7 @@ import org.sonar.server.user.MockUserSession;
 
 import javax.annotation.Nullable;
 
+import java.util.Collection;
 import java.util.Map;
 
 import static org.fest.assertions.Assertions.assertThat;
@@ -136,7 +139,15 @@ public class InternalPermissionServiceMediumTest {
     assertThat(tester.get(RoleDao.class).selectUserPermissions(session, user.getLogin(), project.getId())).isEmpty();
 
     // Check index of issue authorizations
-    assertThat(countIssueAuthorizationDocs()).isEqualTo(0);
+    SearchResponse docs = getAllIndexDocs();
+    assertThat(docs.getHits().getTotalHits()).isEqualTo(1L);
+    SearchHit doc = docs.getHits().getAt(0);
+    assertThat((Collection) doc.sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_USERS)).hasSize(0);
+    assertThat((Collection) doc.sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_GROUPS)).hasSize(0);
+  }
+
+  private SearchResponse getAllIndexDocs() {
+    return tester.get(EsClient.class).prepareSearch(IssueIndexDefinition.INDEX).setTypes(IssueIndexDefinition.TYPE_AUTHORIZATION).get();
   }
 
   @Test
@@ -163,7 +174,12 @@ public class InternalPermissionServiceMediumTest {
     service.removePermission(change);
     session.commit();
     assertThat(tester.get(RoleDao.class).selectGroupPermissions(session, group.getName(), project.getId())).hasSize(0);
-    assertThat(countIssueAuthorizationDocs()).isEqualTo(0);
+
+    SearchResponse docs = getAllIndexDocs();
+    assertThat(docs.getHits().getTotalHits()).isEqualTo(1L);
+    SearchHit doc = docs.getHits().getAt(0);
+    assertThat((Collection) doc.sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_USERS)).hasSize(0);
+    assertThat((Collection) doc.sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_GROUPS)).hasSize(0);
   }
 
   private Map<String, Object> params(@Nullable String login, @Nullable String group, @Nullable String component, String permission) {