From b0ce7751cc6f1f4bb9a6a2d74a278cc31eecc524 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Mon, 8 Dec 2014 11:50:57 +0100 Subject: [PATCH] Fix quality flaws --- .../sonar/server/db/ResultSetIterator.java | 1 - .../java/org/sonar/server/es/BulkIndexer.java | 7 +-- .../issue/index/IssueAuthorizationDao.java | 8 +-- .../index/IssueAuthorizationIndexer.java | 26 ++++------ .../sonar/server/issue/index/IssueDoc.java | 4 -- .../issue/index/IssueIndexDefinition.java | 12 ----- .../server/issue/index/IssueNormalizer.java | 1 - .../issue/index/IssueResultSetIterator.java | 50 +++++++++---------- .../org/sonar/server/util/ProgressLogger.java | 6 ++- .../org/sonar/server/issue/IssueTesting.java | 1 - .../index/IssueAuthorizationDaoTest.java | 3 -- .../index/IssueAuthorizationIndexerTest.java | 5 +- .../InternalPermissionServiceMediumTest.java | 20 +++++++- 13 files changed, 66 insertions(+), 78 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/db/ResultSetIterator.java b/server/sonar-server/src/main/java/org/sonar/server/db/ResultSetIterator.java index 4c7a8bcfdfb..d4a73ecadbf 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/db/ResultSetIterator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/db/ResultSetIterator.java @@ -69,7 +69,6 @@ public abstract class ResultSetIterator implements Iterator, Closeable { try { return read(rs); } catch (SQLException e) { - // TODO add SQL request to context throw new IllegalStateException("Fail to read result set row", e); } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/es/BulkIndexer.java b/server/sonar-server/src/main/java/org/sonar/server/es/BulkIndexer.java index e3ffdc5ab20..b08af55a94e 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/es/BulkIndexer.java +++ b/server/sonar-server/src/main/java/org/sonar/server/es/BulkIndexer.java @@ -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 bulkSettings = Maps.newHashMap(); diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationDao.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationDao.java index de0936f3725..9b5b5861091 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationDao.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationDao.java @@ -41,8 +41,8 @@ public class IssueAuthorizationDao { public static final class Dto { private final String projectUuid; private final long updatedAt; - private List users = Lists.newArrayList(); - private List groups = Lists.newArrayList(); + private final List users = Lists.newArrayList(); + private final List groups = Lists.newArrayList(); public Dto(String projectUuid, long updatedAt) { this.projectUuid = projectUuid; @@ -72,10 +72,6 @@ public class IssueAuthorizationDao { public List getGroups() { return groups; } - - boolean hasNoGroupsNorUsers() { - return users.isEmpty() && groups.isEmpty(); - } } private static final String SQL_TEMPLATE = diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationIndexer.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationIndexer.java index 2121a661182..04bf850b395 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationIndexer.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueAuthorizationIndexer.java @@ -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 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 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; } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueDoc.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueDoc.java index 7db14c4895f..796039db8db 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueDoc.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueDoc.java @@ -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); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndexDefinition.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndexDefinition.java index ca7d0d9ed96..57ab2fc0456 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndexDefinition.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndexDefinition.java @@ -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); } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueNormalizer.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueNormalizer.java index 1ff008e91be..1a7e3dd6d0a 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueNormalizer.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueNormalizer.java @@ -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"); diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueResultSetIterator.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueResultSetIterator.java index 3e0a962b78f..620884bd84b 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueResultSetIterator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueResultSetIterator.java @@ -43,16 +43,15 @@ class IssueResultSetIterator extends ResultSetIterator { "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 { "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 { 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; } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/util/ProgressLogger.java b/server/sonar-server/src/main/java/org/sonar/server/util/ProgressLogger.java index 97fc702d8b8..de0ba605c9b 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/util/ProgressLogger.java +++ b/server/sonar-server/src/main/java/org/sonar/server/util/ProgressLogger.java @@ -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)); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueTesting.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueTesting.java index 48947ea610a..cef689ede3e 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueTesting.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueTesting.java @@ -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; } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationDaoTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationDaoTest.java index 5ba96b58dbb..e0312a185c7 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationDaoTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationDaoTest.java @@ -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(); diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationIndexerTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationIndexerTest.java index bc7bbb49174..d05db36d9e3 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationIndexerTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueAuthorizationIndexerTest.java @@ -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 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() { diff --git a/server/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceMediumTest.java index 2070e40c613..f0a6aebb57f 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceMediumTest.java @@ -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 params(@Nullable String login, @Nullable String group, @Nullable String component, String permission) { -- 2.39.5