From: Simon Brandhof Date: Fri, 3 Jul 2015 09:17:15 +0000 (+0200) Subject: Fix stability of issue tracking X-Git-Tag: 5.2-RC1~1228 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=7f218017d505877feb4415b0568dde3fb8492172;p=sonarqube.git Fix stability of issue tracking --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/DebtAggregator.java b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/DebtAggregator.java index c8c410a6503..68ac76489ab 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/DebtAggregator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/DebtAggregator.java @@ -26,7 +26,6 @@ import javax.annotation.CheckForNull; import javax.annotation.Nullable; import org.sonar.api.measures.CoreMetrics; import org.sonar.core.issue.DefaultIssue; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.component.Component; import org.sonar.server.computation.debt.Characteristic; import org.sonar.server.computation.debt.DebtModelHolder; @@ -56,7 +55,7 @@ public class DebtAggregator extends IssueVisitor { } @Override - public void beforeComponent(Component component, Tracking tracking) { + public void beforeComponent(Component component) { currentDebt = new Debt(); debtsByComponentRef.put(component.getRef(), currentDebt); diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueAssigner.java b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueAssigner.java index 7f49d983154..937a8d217d7 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueAssigner.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueAssigner.java @@ -28,7 +28,6 @@ import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; import org.sonar.batch.protocol.output.BatchReport; import org.sonar.core.issue.DefaultIssue; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.batch.BatchReportReader; import org.sonar.server.computation.component.Component; import org.sonar.server.source.index.SourceLineDoc; @@ -62,21 +61,12 @@ public class IssueAssigner extends IssueVisitor { this.defaultAssigne = defaultAssigne; } - @Override - public void beforeComponent(Component component, Tracking tracking) { - // optimization - do not load data if there are no new issues - if (!tracking.getUnmatchedRaws().isEmpty()) { - scmChangesets = loadScmChangesetsFromReport(component); - if (scmChangesets == null) { - scmChangesets = loadScmChangesetsFromIndex(component); - } - computeLastCommitDateAndAuthor(); - } - } - @Override public void onIssue(Component component, DefaultIssue issue) { if (issue.isNew()) { + // optimization - do not load SCM data of this component if there are no new issues + loadScmChangesetsIfNeeded(component); + String scmAuthor = guessScmAuthor(issue.line()); issue.setAuthorLogin(scmAuthor); if (scmAuthor != null) { @@ -90,6 +80,16 @@ public class IssueAssigner extends IssueVisitor { } } + private void loadScmChangesetsIfNeeded(Component component) { + if (scmChangesets == null) { + scmChangesets = loadScmChangesetsFromReport(component); + if (scmChangesets == null) { + scmChangesets = loadScmChangesetsFromIndex(component); + } + computeLastCommitDateAndAuthor(); + } + } + @Override public void afterComponent(Component component) { lastCommitDate = 0L; diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueCounter.java b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueCounter.java index 8c27aefac35..957ed281c27 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueCounter.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueCounter.java @@ -27,7 +27,6 @@ import java.util.Map; import javax.annotation.Nullable; import org.sonar.api.issue.Issue; import org.sonar.core.issue.DefaultIssue; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.component.Component; import org.sonar.server.computation.measure.Measure; import org.sonar.server.computation.measure.MeasureRepository; @@ -106,7 +105,7 @@ public class IssueCounter extends IssueVisitor { } @Override - public void beforeComponent(Component component, Tracking tracking) { + public void beforeComponent(Component component) { // TODO optimization no need to instantiate counter if no open issues currentCounters = new Counters(); countersByComponentRef.put(component.getRef(), currentCounters); diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueVisitor.java b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueVisitor.java index 6b3768dc26b..d5e53dc4fb9 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueVisitor.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueVisitor.java @@ -20,7 +20,6 @@ package org.sonar.server.computation.issue; import org.sonar.core.issue.DefaultIssue; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.component.Component; public abstract class IssueVisitor { @@ -29,7 +28,7 @@ public abstract class IssueVisitor { * This method is called for each component before processing its issues. * The component does not necessarily have issues. */ - public void beforeComponent(Component component, Tracking tracking) { + public void beforeComponent(Component component) { } diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueVisitors.java b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueVisitors.java index 51ddee18a05..1df195a01a3 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueVisitors.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/IssueVisitors.java @@ -20,7 +20,6 @@ package org.sonar.server.computation.issue; import org.sonar.core.issue.DefaultIssue; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.component.Component; public class IssueVisitors { @@ -31,9 +30,9 @@ public class IssueVisitors { this.visitors = visitors; } - public void beforeComponent(Component component, Tracking tracking) { + public void beforeComponent(Component component) { for (IssueVisitor visitor : visitors) { - visitor.beforeComponent(component, tracking); + visitor.beforeComponent(component); } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/NewDebtAggregator.java b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/NewDebtAggregator.java index d26b324ff14..c1d48f631b7 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/NewDebtAggregator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/NewDebtAggregator.java @@ -28,7 +28,6 @@ import java.util.Map; import org.sonar.api.measures.CoreMetrics; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.db.IssueChangeDto; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.component.Component; import org.sonar.server.computation.measure.Measure; import org.sonar.server.computation.measure.MeasureRepository; @@ -61,7 +60,7 @@ public class NewDebtAggregator extends IssueVisitor { } @Override - public void beforeComponent(Component component, Tracking tracking) { + public void beforeComponent(Component component) { currentSum = new DebtSum(); sumsByComponentRef.put(component.getRef(), currentSum); List changes = dbClient.issueChangeDao().selectChangelogOfNonClosedIssuesByComponent(component.getUuid()); diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/TrackerBaseInputFactory.java b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/TrackerBaseInputFactory.java index 8f5feb2573c..cf7da885f81 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/TrackerBaseInputFactory.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/TrackerBaseInputFactory.java @@ -19,6 +19,8 @@ */ package org.sonar.server.computation.issue; +import com.google.common.base.Objects; +import java.util.Collections; import java.util.List; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.tracking.Input; @@ -58,7 +60,7 @@ public class TrackerBaseInputFactory { DbSession session = dbClient.openSession(false); try { List hashes = dbClient.fileSourceDao().selectLineHashes(session, component.getUuid()); - return new LineHashSequence(hashes); + return new LineHashSequence(Objects.firstNonNull(hashes, Collections.emptyList())); } finally { MyBatis.closeQuietly(session); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/step/IntegrateIssuesStep.java b/server/sonar-server/src/main/java/org/sonar/server/computation/step/IntegrateIssuesStep.java index c1c234d32e2..2a8eb12123b 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/step/IntegrateIssuesStep.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/step/IntegrateIssuesStep.java @@ -75,22 +75,23 @@ public class IntegrateIssuesStep implements ComputationStep { } private void processIssues(Component component) { - Tracking tracking = tracker.track(component); DiskCache.DiskAppender cacheAppender = issueCache.newAppender(); try { - issueVisitors.beforeComponent(component, tracking); + Tracking tracking = tracker.track(component); + issueVisitors.beforeComponent(component); fillNewOpenIssues(component, tracking, cacheAppender); fillExistingOpenIssues(component, tracking, cacheAppender); closeUnmatchedBaseIssues(component, tracking, cacheAppender); issueVisitors.afterComponent(component); + } catch (Exception e) { + Loggers.get(getClass()).error(String.format("Fail to process issues of %s", component.getKey()), e); } finally { cacheAppender.close(); } } private void fillNewOpenIssues(Component component, Tracking tracking, DiskCache.DiskAppender cacheAppender) { - Set issues = tracking.getUnmatchedRaws(); - for (DefaultIssue issue : issues) { + for (DefaultIssue issue : tracking.getUnmatchedRaws()) { issueLifecycle.initNewOpenIssue(issue); process(component, issue, cacheAppender); } @@ -113,7 +114,6 @@ public class IntegrateIssuesStep implements ComputationStep { private void closeUnmatchedBaseIssues(Component component, Tracking tracking, DiskCache.DiskAppender cacheAppender) { for (DefaultIssue issue : tracking.getUnmatchedBases()) { - Loggers.get(getClass()).info("--- close base " + issue); // TODO should replace flag "beingClosed" by express call to transition "automaticClose" issue.setBeingClosed(true); // TODO manual issues -> was updater.setResolution(newIssue, Issue.RESOLUTION_REMOVED, changeContext);. Is it a problem ? diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/issue/DebtAggregatorTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/issue/DebtAggregatorTest.java index 2542ac0c175..44af4c3436e 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/issue/DebtAggregatorTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/issue/DebtAggregatorTest.java @@ -93,7 +93,7 @@ public class DebtAggregatorTest { DefaultIssue unresolvedWithoutDebt = new DefaultIssue().setRuleKey(rule.getKey()); DefaultIssue resolved = new DefaultIssue().setDebt(Duration.create(50)).setResolution(RESOLUTION_FIXED).setRuleKey(rule.getKey()); - underTest.beforeComponent(file, mock(Tracking.class)); + underTest.beforeComponent(file); underTest.onIssue(file, unresolved1); underTest.onIssue(file, unresolved2); underTest.onIssue(file, unresolvedWithoutDebt); @@ -118,10 +118,10 @@ public class DebtAggregatorTest { DefaultIssue fileIssue = new DefaultIssue().setDebt(Duration.create(10)).setRuleKey(rule.getKey()); DefaultIssue projectIssue = new DefaultIssue().setDebt(Duration.create(30)).setRuleKey(rule.getKey()); - underTest.beforeComponent(file, mock(Tracking.class)); + underTest.beforeComponent(file); underTest.onIssue(file, fileIssue); underTest.afterComponent(file); - underTest.beforeComponent(project, mock(Tracking.class)); + underTest.beforeComponent(project); underTest.onIssue(project, projectIssue); underTest.afterComponent(project); diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/issue/IssueAssignerTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/issue/IssueAssignerTest.java index d5f8ab5a2e8..edc5a305279 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/issue/IssueAssignerTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/issue/IssueAssignerTest.java @@ -26,7 +26,6 @@ import org.junit.Rule; import org.junit.Test; import org.sonar.api.config.Settings; import org.sonar.batch.protocol.output.BatchReport; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.batch.BatchReportReaderRule; import org.sonar.server.computation.component.Component; import org.sonar.server.computation.component.DumbComponent; @@ -67,43 +66,43 @@ public class IssueAssignerTest { .addChangesetIndexByLine(1) .build()); - underTest.beforeComponent(file, mock(Tracking.class)); -// underTest.onIssue(file, issue); -// sut.init("ANY_UUID", 123_456_789, reportReader); -// -// assertThat(sut.lineAuthor(1)).isEqualTo("charb"); -// assertThat(sut.lineAuthor(2)).isEqualTo("charb"); -// assertThat(sut.lineAuthor(3)).isEqualTo("wolinski"); -// // compute last author -// assertThat(sut.lineAuthor(4)).isEqualTo("wolinski"); -// assertThat(sut.lineAuthor(null)).isEqualTo("wolinski"); + underTest.beforeComponent(file); + // underTest.onIssue(file, issue); + // sut.init("ANY_UUID", 123_456_789, reportReader); + // + // assertThat(sut.lineAuthor(1)).isEqualTo("charb"); + // assertThat(sut.lineAuthor(2)).isEqualTo("charb"); + // assertThat(sut.lineAuthor(3)).isEqualTo("wolinski"); + // // compute last author + // assertThat(sut.lineAuthor(4)).isEqualTo("wolinski"); + // assertThat(sut.lineAuthor(null)).isEqualTo("wolinski"); } -// @Test -// public void line_author_from_index() throws Exception { -// esTester.putDocuments(SourceLineIndexDefinition.INDEX, SourceLineIndexDefinition.TYPE, -// newSourceLine("cabu", "123-456-789", 123_456_789, 1), -// newSourceLine("cabu", "123-456-789", 123_456_789, 2), -// newSourceLine("cabu", "123-123-789", 123_456_789, 3), -// newSourceLine("wolinski", "987-654-321", 987_654_321, 4), -// newSourceLine("cabu", "123-456-789", 123_456_789, 5) -// ); -// -// sut.init("DEFAULT_UUID", 123, reportReader); -// -// assertThat(sut.lineAuthor(1)).isEqualTo("cabu"); -// assertThat(sut.lineAuthor(2)).isEqualTo("cabu"); -// assertThat(sut.lineAuthor(3)).isEqualTo("cabu"); -// assertThat(sut.lineAuthor(4)).isEqualTo("wolinski"); -// assertThat(sut.lineAuthor(5)).isEqualTo("cabu"); -// assertThat(sut.lineAuthor(6)).isEqualTo("wolinski"); -// } -// -// @Test(expected = IllegalStateException.class) -// public void fail_when_component_ref_is_not_filled() { -// sut.init("ANY_UUID", null, reportReader); -// sut.lineAuthor(0); -// } + // @Test + // public void line_author_from_index() throws Exception { + // esTester.putDocuments(SourceLineIndexDefinition.INDEX, SourceLineIndexDefinition.TYPE, + // newSourceLine("cabu", "123-456-789", 123_456_789, 1), + // newSourceLine("cabu", "123-456-789", 123_456_789, 2), + // newSourceLine("cabu", "123-123-789", 123_456_789, 3), + // newSourceLine("wolinski", "987-654-321", 987_654_321, 4), + // newSourceLine("cabu", "123-456-789", 123_456_789, 5) + // ); + // + // sut.init("DEFAULT_UUID", 123, reportReader); + // + // assertThat(sut.lineAuthor(1)).isEqualTo("cabu"); + // assertThat(sut.lineAuthor(2)).isEqualTo("cabu"); + // assertThat(sut.lineAuthor(3)).isEqualTo("cabu"); + // assertThat(sut.lineAuthor(4)).isEqualTo("wolinski"); + // assertThat(sut.lineAuthor(5)).isEqualTo("cabu"); + // assertThat(sut.lineAuthor(6)).isEqualTo("wolinski"); + // } + // + // @Test(expected = IllegalStateException.class) + // public void fail_when_component_ref_is_not_filled() { + // sut.init("ANY_UUID", null, reportReader); + // sut.lineAuthor(0); + // } private BatchReport.Changesets.Changeset.Builder newChangeset(String author, String revision, long date) { return BatchReport.Changesets.Changeset.newBuilder() diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/issue/IssueCounterTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/issue/IssueCounterTest.java index 67988ec1f5a..d4f2994eda1 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/issue/IssueCounterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/issue/IssueCounterTest.java @@ -26,7 +26,6 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.sonar.core.issue.DefaultIssue; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.batch.BatchReportReaderRule; import org.sonar.server.computation.batch.TreeRootHolderRule; import org.sonar.server.computation.component.Component; @@ -104,7 +103,6 @@ public class IssueCounterTest { @Rule public PeriodsHolderRule periodsHolder = new PeriodsHolderRule(); - Tracking tracking = mock(Tracking.class); MetricRepository metricRepository = mock(MetricRepository.class); MeasureRepository measureRepository; IssueCounter sut; @@ -122,21 +120,21 @@ public class IssueCounterTest { periodsHolder.setPeriods(); // bottom-up traversal -> from files to project - sut.beforeComponent(FILE1, tracking); + sut.beforeComponent(FILE1); sut.onIssue(FILE1, createIssue(null, STATUS_OPEN, BLOCKER)); sut.onIssue(FILE1, createIssue(RESOLUTION_FIXED, STATUS_CLOSED, MAJOR)); sut.onIssue(FILE1, createIssue(RESOLUTION_FALSE_POSITIVE, STATUS_RESOLVED, MAJOR)); sut.afterComponent(FILE1); - sut.beforeComponent(FILE2, tracking); + sut.beforeComponent(FILE2); sut.onIssue(FILE2, createIssue(null, STATUS_CONFIRMED, BLOCKER)); sut.onIssue(FILE2, createIssue(null, STATUS_CONFIRMED, MAJOR)); sut.afterComponent(FILE2); - sut.beforeComponent(FILE3, tracking); + sut.beforeComponent(FILE3); sut.afterComponent(FILE3); - sut.beforeComponent(PROJECT, tracking); + sut.beforeComponent(PROJECT); sut.afterComponent(PROJECT); // count by status @@ -163,18 +161,18 @@ public class IssueCounterTest { periodsHolder.setPeriods(); // bottom-up traversal -> from files to project - sut.beforeComponent(FILE1, tracking); + sut.beforeComponent(FILE1); sut.onIssue(FILE1, createIssue(null, STATUS_OPEN, BLOCKER)); // this resolved issue is ignored sut.onIssue(FILE1, createIssue(RESOLUTION_FIXED, STATUS_CLOSED, MAJOR)); sut.afterComponent(FILE1); - sut.beforeComponent(FILE2, tracking); + sut.beforeComponent(FILE2); sut.onIssue(FILE2, createIssue(null, STATUS_CONFIRMED, BLOCKER)); sut.onIssue(FILE2, createIssue(null, STATUS_CONFIRMED, MAJOR)); sut.afterComponent(FILE2); - sut.beforeComponent(PROJECT, tracking); + sut.beforeComponent(PROJECT); sut.afterComponent(PROJECT); assertThat(measureRepository.getRawMeasure(FILE1, BLOCKER_ISSUES_METRIC).get().getIntValue()).isEqualTo(1); @@ -195,7 +193,7 @@ public class IssueCounterTest { Period period = newPeriod(3, 1500000000000L); periodsHolder.setPeriods(period); - sut.beforeComponent(FILE1, tracking); + sut.beforeComponent(FILE1); // created before -> existing issues sut.onIssue(FILE1, createIssueAt(null, STATUS_OPEN, BLOCKER, period.getSnapshotDate() - 1000000L)); // created during the first analysis starting the period -> existing issues @@ -205,10 +203,10 @@ public class IssueCounterTest { sut.onIssue(FILE1, createIssueAt(RESOLUTION_FIXED, STATUS_CLOSED, MAJOR, period.getSnapshotDate() + 200000L)); sut.afterComponent(FILE1); - sut.beforeComponent(FILE2, tracking); + sut.beforeComponent(FILE2); sut.afterComponent(FILE2); - sut.beforeComponent(PROJECT, tracking); + sut.beforeComponent(PROJECT); sut.afterComponent(PROJECT); assertVariation(FILE1, NEW_ISSUES_METRIC, period.getIndex(), 1); diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/issue/NewDebtAggregatorTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/issue/NewDebtAggregatorTest.java index cf864b8a8e0..f8f217394b9 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/issue/NewDebtAggregatorTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/issue/NewDebtAggregatorTest.java @@ -25,7 +25,6 @@ import org.mockito.Mockito; import org.sonar.api.measures.CoreMetrics; import org.sonar.api.utils.Duration; import org.sonar.core.issue.DefaultIssue; -import org.sonar.core.issue.tracking.Tracking; import org.sonar.server.computation.component.Component; import org.sonar.server.computation.component.DumbComponent; import org.sonar.server.computation.measure.Measure; @@ -78,7 +77,7 @@ public class NewDebtAggregatorTest { when(calculator.calculate(same(unresolved2), anyList(), same(PERIOD))).thenReturn(3L); verifyNoMoreInteractions(calculator); - underTest.beforeComponent(file, mock(Tracking.class)); + underTest.beforeComponent(file); underTest.onIssue(file, unresolved1); underTest.onIssue(file, unresolved2); underTest.onIssue(file, unresolvedWithoutDebt); @@ -105,7 +104,7 @@ public class NewDebtAggregatorTest { DefaultIssue unresolved = new DefaultIssue().setDebt(Duration.create(10)); verifyZeroInteractions(calculator); - underTest.beforeComponent(file, mock(Tracking.class)); + underTest.beforeComponent(file); underTest.onIssue(file, unresolved); underTest.afterComponent(file); diff --git a/sonar-core/src/main/java/org/sonar/core/issue/tracking/BlockRecognizer.java b/sonar-core/src/main/java/org/sonar/core/issue/tracking/BlockRecognizer.java index 55138a34995..70ac173b3c4 100644 --- a/sonar-core/src/main/java/org/sonar/core/issue/tracking/BlockRecognizer.java +++ b/sonar-core/src/main/java/org/sonar/core/issue/tracking/BlockRecognizer.java @@ -123,15 +123,14 @@ class BlockRecognizer { for (RAW raw : raws) { for (BASE base : bases) { if (result.containsUnmatchedBase(base) && base.getRuleKey().equals(raw.getRuleKey())) { - result.associateRawToBase(raw, base); - result.markRawAsAssociated(raw); + result.match(raw, base); break; } } } } - private static Multimap groupByLine(Collection trackables, BlockHashSequence hashSequence) { + private static Multimap groupByLine(Iterable trackables, BlockHashSequence hashSequence) { Multimap result = LinkedHashMultimap.create(); for (T trackable : trackables) { Integer line = trackable.getLine(); diff --git a/sonar-core/src/main/java/org/sonar/core/issue/tracking/Tracker.java b/sonar-core/src/main/java/org/sonar/core/issue/tracking/Tracker.java index e7cd196bbca..9709ca8886e 100644 --- a/sonar-core/src/main/java/org/sonar/core/issue/tracking/Tracker.java +++ b/sonar-core/src/main/java/org/sonar/core/issue/tracking/Tracker.java @@ -23,9 +23,7 @@ import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Multimap; -import java.util.ArrayList; import java.util.Collection; -import java.util.HashSet; import java.util.Objects; import javax.annotation.Nonnull; import org.apache.commons.lang.StringUtils; @@ -75,7 +73,6 @@ public class Tracker { baseSearch.put(factory.create(base), base); } - Collection trackedRaws = new ArrayList<>(); for (RAW raw : tracking.getUnmatchedRaws()) { SearchKey rawKey = factory.create(raw); Collection bases = baseSearch.get(rawKey); @@ -83,21 +80,19 @@ public class Tracker { // TODO taking the first one. Could be improved if there are more than 2 issues on the same line. // Message could be checked to take the best one. BASE match = bases.iterator().next(); - tracking.associateRawToBase(raw, match); + tracking.match(raw, match); baseSearch.remove(rawKey, match); - trackedRaws.add(raw); } } - tracking.markRawsAsAssociated(trackedRaws); } private void relocateManualIssues(Input rawInput, Input baseInput, Tracking tracking) { // FIXME copy of Set if required to avoid concurrent modifications (see tracking.associateManualIssueToLine()) - Iterable manualIssues = from(new HashSet<>(tracking.getUnmatchedBases())).filter(IsManual.INSTANCE); + Iterable manualIssues = from(tracking.getUnmatchedBases()).filter(IsManual.INSTANCE); for (BASE base : manualIssues) { if (base.getLine() == null) { // no need to relocate. Location is unchanged. - tracking.associateManualIssueToLine(base, null); + tracking.keepManualIssueOpen(base, null); } else { String baseHash = base.getLineHash(); if (Strings.isNullOrEmpty(baseHash)) { @@ -106,11 +101,11 @@ public class Tracker { if (!Strings.isNullOrEmpty(baseHash)) { int[] rawLines = rawInput.getLineHashSequence().getLinesForHash(baseHash); if (rawLines.length == 1) { - tracking.associateManualIssueToLine(base, rawLines[0]); + tracking.keepManualIssueOpen(base, rawLines[0]); } else if (rawLines.length == 0 && rawInput.getLineHashSequence().hasLine(base.getLine())) { // still valid (???). We didn't manage to correctly detect code move, so the // issue is kept at the same location, even if code changes - tracking.associateManualIssueToLine(base, base.getLine()); + tracking.keepManualIssueOpen(base, base.getLine()); } // TODO if hash found multiple times, , pick the closest line } diff --git a/sonar-core/src/main/java/org/sonar/core/issue/tracking/Tracking.java b/sonar-core/src/main/java/org/sonar/core/issue/tracking/Tracking.java index 579c6f51623..33b126a6300 100644 --- a/sonar-core/src/main/java/org/sonar/core/issue/tracking/Tracking.java +++ b/sonar-core/src/main/java/org/sonar/core/issue/tracking/Tracking.java @@ -20,47 +20,61 @@ package org.sonar.core.issue.tracking; import com.google.common.base.Objects; +import com.google.common.base.Predicate; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.Iterables; import com.google.common.collect.Multimap; import java.util.Collection; -import java.util.Collections; import java.util.IdentityHashMap; import java.util.Map; -import java.util.Set; import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; import javax.annotation.Nullable; public class Tracking { /** - * Tracked issues -> a raw issue is associated to a base issue + * Matched issues -> a raw issue is associated to a base issue */ private final IdentityHashMap rawToBase = new IdentityHashMap<>(); + private final IdentityHashMap baseToRaw = new IdentityHashMap<>(); - /** - * The raw issues that are not associated to a base issue. - */ - private final Set unmatchedRaws = Collections.newSetFromMap(new IdentityHashMap()); + private final Collection raws; + private final Collection bases; - /** - * IdentityHashSet of the base issues that are not associated to a raw issue. - */ - private final Set unmatchedBases = Collections.newSetFromMap(new IdentityHashMap()); + private final Predicate unmatchedRawPredicate = new Predicate() { + @Override + public boolean apply(@Nonnull RAW raw) { + return !rawToBase.containsKey(raw); + } + }; + + private final Predicate unmatchedBasePredicate = new Predicate() { + @Override + public boolean apply(@Nonnull BASE raw) { + return !baseToRaw.containsKey(raw); + } + }; /** * The manual issues that are still valid (related source code still exists). They * are grouped by line. Lines start with 1. The key 0 references the manual * issues that do not relate to a line. */ - private final Multimap openManualIssues = ArrayListMultimap.create(); + private final Multimap openManualIssuesByLine = ArrayListMultimap.create(); public Tracking(Input rawInput, Input baseInput) { - this.unmatchedRaws.addAll(rawInput.getIssues()); - this.unmatchedBases.addAll(baseInput.getIssues()); + this.raws = rawInput.getIssues(); + this.bases = baseInput.getIssues(); } - public Set getUnmatchedRaws() { - return unmatchedRaws; + /** + * Returns an Iterable to be traversed when matching issues. That means + * that the traversal does not fail if method {@link #match(Trackable, Trackable)} + * is called. + */ + public Iterable getUnmatchedRaws() { + return Iterables.filter(raws, unmatchedRawPredicate); } public Map getMatchedRaws() { @@ -73,58 +87,42 @@ public class Tracking { } /** - * The base issues that are not matched by a raw issue and that need to be closed. Manual + * The base issues that are not matched by a raw issue and that need to be closed. */ - public Set getUnmatchedBases() { - return unmatchedBases; + public Iterable getUnmatchedBases() { + return Iterables.filter(bases, unmatchedBasePredicate); } boolean containsUnmatchedBase(BASE base) { - return unmatchedBases.contains(base); + return !baseToRaw.containsKey(base); } - void associateRawToBase(RAW raw, BASE base) { + void match(RAW raw, BASE base) { rawToBase.put(raw, base); - if (!unmatchedBases.remove(base)) { - throw new IllegalStateException(String.format("Fail to associate base issue %s to %s among %s", base, raw, this)); - } - } - - void markRawAsAssociated(RAW raw) { - if (!unmatchedRaws.remove(raw)) { - throw new IllegalStateException(String.format("Fail to mark issue as associated: %s among %s", raw, this)); - } - } - - void markRawsAsAssociated(Collection c) { - // important : do not use unmatchedRaws.removeAll(Collection) as it's buggy. See: - // http://stackoverflow.com/questions/19682542/why-identityhashmap-keyset-removeallkeys-does-not-use-identity-is-it-a-bug/19682543#19682543 - // http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6588783 - for (RAW raw : c) { - markRawAsAssociated(raw); - } + baseToRaw.put(base, raw); } boolean isComplete() { - return unmatchedRaws.isEmpty() || unmatchedBases.isEmpty(); + return rawToBase.size() == raws.size(); } public Multimap getOpenManualIssuesByLine() { - return openManualIssues; + return openManualIssuesByLine; } - void associateManualIssueToLine(BASE manualIssue, @Nullable Integer line) { - openManualIssues.put(line, manualIssue); - unmatchedBases.remove(manualIssue); + void keepManualIssueOpen(BASE manualIssue, @Nullable Integer line) { + openManualIssuesByLine.put(line, manualIssue); + baseToRaw.put(manualIssue, null); } @Override public String toString() { return Objects.toStringHelper(this) .add("rawToBase", rawToBase) - .add("unmatchedRaws", unmatchedRaws) - .add("unmatchedBases", unmatchedBases) - .add("openManualIssues", openManualIssues) + .add("baseToRaw", baseToRaw) + .add("raws", raws) + .add("bases", bases) + .add("openManualIssuesByLine", openManualIssuesByLine) .toString(); } }