From: Duarte Meneses Date: Mon, 5 Feb 2018 16:49:04 +0000 (+0100) Subject: SONAR-10257 apply feedback X-Git-Tag: 7.5~1719 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=4991e63dbe3edb7327588c080e88c3de5a6ec3f5;p=sonarqube.git SONAR-10257 apply feedback --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfo.java b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfo.java index 1297b674699..387d88c1ead 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfo.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfo.java @@ -22,7 +22,8 @@ package org.sonar.server.computation.task.projectanalysis.scm; import java.util.Map; /** - * Represents the Scm information for a specific file. + * Represents changeset information for a file. If SCM information is present, it will be the author, revision and date fetched from SCM + * for every line. Otherwise, it's a date that corresponds the the analysis date in which the line was modified. */ public interface ScmInfo { diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfoImpl.java b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfoImpl.java index ca116018d67..feb4ea4e8a3 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfoImpl.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfoImpl.java @@ -22,15 +22,11 @@ package org.sonar.server.computation.task.projectanalysis.scm; import java.util.Collections; import java.util.Comparator; import java.util.Map; -import java.util.stream.Collectors; -import javax.annotation.CheckForNull; import javax.annotation.concurrent.Immutable; import static com.google.common.base.Preconditions.checkState; @Immutable public class ScmInfoImpl implements ScmInfo { - - @CheckForNull private final Changeset latestChangeset; private final Map lineChangesets; @@ -41,9 +37,8 @@ public class ScmInfoImpl implements ScmInfo { } private static Changeset computeLatestChangeset(Map lineChangesets) { - return lineChangesets.values().stream() - .collect(Collectors.maxBy(Comparator.comparing(Changeset::getDate))) - .orElse(null); + return lineChangesets.values().stream().max(Comparator.comparingLong(Changeset::getDate)) + .orElseThrow(() -> new IllegalStateException("Expecting at least one Changeset to be present")); } @Override diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfoRepositoryImpl.java b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfoRepositoryImpl.java index 5dfdf4b7a7b..24e714a5683 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfoRepositoryImpl.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/scm/ScmInfoRepositoryImpl.java @@ -75,15 +75,17 @@ public class ScmInfoRepositoryImpl implements ScmInfoRepository { private Optional getScmInfoForComponent(Component component) { ScannerReport.Changesets changesets = scannerReportReader.readChangesets(component.getReportAttributes().getRef()); - if (changesets != null) { - if (changesets.getChangesetCount() == 0) { - return generateAndMergeDb(component, changesets.getCopyFromPrevious()); - } - return getScmInfoFromReport(component, changesets); + if (changesets == null) { + LOGGER.trace("No SCM info for file '{}'", component.getKey()); + // SCM not available. It might have been available before - don't keep author and revision. + return generateAndMergeDb(component, false); } - LOGGER.trace("No SCM info for file '{}'", component.getKey()); - return generateAndMergeDb(component, false); + // will be empty if the flag "copy from previous" is set, or if the file is empty. + if (changesets.getChangesetCount() == 0) { + return generateAndMergeDb(component, changesets.getCopyFromPrevious()); + } + return getScmInfoFromReport(component, changesets); } private static Optional getScmInfoFromReport(Component file, ScannerReport.Changesets changesets) { @@ -92,6 +94,9 @@ public class ScmInfoRepositoryImpl implements ScmInfoRepository { } private Optional generateScmInfoForAllFile(Component file) { + if (file.getFileAttributes().getLines() == 0) { + return Optional.empty(); + } Set newOrChangedLines = IntStream.rangeClosed(1, file.getFileAttributes().getLines()).boxed().collect(Collectors.toSet()); return Optional.of(GeneratedScmInfo.create(analysisMetadata.getAnalysisDate(), newOrChangedLines)); } @@ -106,13 +111,13 @@ public class ScmInfoRepositoryImpl implements ScmInfoRepository { return Changeset.newChangesetBuilder().setDate(changeset.getDate()).build(); } - private Optional generateAndMergeDb(Component file, boolean copyFromPrevious) { + private Optional generateAndMergeDb(Component file, boolean keepAuthorAndRevision) { Optional dbInfoOpt = scmInfoDbLoader.getScmInfo(file); if (!dbInfoOpt.isPresent()) { return generateScmInfoForAllFile(file); } - ScmInfo scmInfo = copyFromPrevious ? dbInfoOpt.get() : removeAuthorAndRevision(dbInfoOpt.get()); + ScmInfo scmInfo = keepAuthorAndRevision ? dbInfoOpt.get() : removeAuthorAndRevision(dbInfoOpt.get()); boolean fileUnchanged = file.getStatus() == Status.SAME && sourceHashRepository.getRawSourceHash(file).equals(dbInfoOpt.get().fileHash()); if (fileUnchanged) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiff.java b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiff.java index 4490afd1a4f..61b6763df88 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiff.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiff.java @@ -22,5 +22,11 @@ package org.sonar.server.computation.task.projectanalysis.source; import org.sonar.server.computation.task.projectanalysis.component.Component; public interface SourceLinesDiff { + /** + * Creates a diff between the file in the database and the file in the report using Myers' algorithm, and links matching lines between + * both files. + * @return an array with one entry for each line in the left side. Those entries point either to a line in the right side, or to 0, + * in which case it means the line was added. + */ int[] getMatchingLines(Component component); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiffFinder.java b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiffFinder.java index e60fa0b62fe..f0bc058c695 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiffFinder.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiffFinder.java @@ -19,33 +19,30 @@ */ package org.sonar.server.computation.task.projectanalysis.source; +import difflib.myers.DifferentiationFailedException; import difflib.myers.MyersDiff; import difflib.myers.PathNode; import java.util.List; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; public class SourceLinesDiffFinder { + private static final Logger LOG = Loggers.get(SourceLinesDiffFinder.class); + private final List left; + private final List right; - private final List database; - private final List report; - - public SourceLinesDiffFinder(List database, List report) { - this.database = database; - this.report = report; + public SourceLinesDiffFinder(List left, List right) { + this.left = left; + this.right = right; } - /** - * Creates a diff between the file in the database and the file in the report using Myers' algorithm, and links matching lines between - * both files. - * @return an array with one entry for each line in the report. Those entries point either to a line in the database, or to 0, - * in which case it means the line was added. - */ public int[] findMatchingLines() { - int[] index = new int[report.size()]; + int[] index = new int[right.size()]; - int dbLine = database.size(); - int reportLine = report.size(); + int dbLine = left.size(); + int reportLine = right.size(); try { - PathNode node = MyersDiff.buildPath(database.toArray(), report.toArray()); + PathNode node = MyersDiff.buildPath(left.toArray(), right.toArray()); while (node.prev != null) { PathNode prevNode = node.prev; @@ -65,7 +62,8 @@ public class SourceLinesDiffFinder { } node = prevNode; } - } catch (Exception e) { + } catch (DifferentiationFailedException e) { + LOG.error("Error finding matching lines", e); return index; } return index; diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java index 7e2a89645e8..2b23343f6dd 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java @@ -42,7 +42,6 @@ import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; -import org.elasticsearch.index.query.RangeQueryBuilder; import org.elasticsearch.indices.TermsLookup; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregationBuilders; @@ -548,7 +547,7 @@ public class IssueIndex { .format(DateUtils.DATETIME_FORMAT) .timeZone(DateTimeZone.forOffsetMillis(system.getDefaultTimeZone().getRawOffset())) // ES dateHistogram bounds are inclusive while createdBefore parameter is exclusive - .extendedBounds(new ExtendedBounds(startInclusive ? startTime : startTime + 1, endTime - 1L)); + .extendedBounds(new ExtendedBounds(startInclusive ? startTime : (startTime + 1), endTime - 1L)); addEffortAggregationIfNeeded(query, dateHistogram); return Optional.of(dateHistogram); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiffFinderTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiffFinderTest.java index 06c110aeb03..99394391a66 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiffFinderTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/task/projectanalysis/source/SourceLinesDiffFinderTest.java @@ -29,7 +29,6 @@ public class SourceLinesDiffFinderTest { @Test public void shouldFindNothingWhenContentAreIdentical() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -47,12 +46,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(1, 2, 3, 4, 5); - } @Test public void shouldFindNothingWhenContentAreIdentical2() { - List database = new ArrayList<>(); database.add("package sample;\n"); database.add("\n"); @@ -79,12 +76,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(1, 2, 3, 4, 0, 0, 0, 0, 0, 0, 5, 6, 7); - } @Test public void shouldDetectWhenStartingWithModifiedLines() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -100,12 +95,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(0, 0, 3, 4); - } @Test public void shouldDetectWhenEndingWithModifiedLines() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -121,12 +114,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(1, 2, 0, 0); - } @Test public void shouldDetectModifiedLinesInMiddleOfTheFile() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -146,12 +137,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(1, 2, 0, 0, 5, 6); - } @Test public void shouldDetectNewLinesAtBeginningOfFile() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -167,12 +156,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(0, 0, 1, 2, 3); - } @Test public void shouldDetectNewLinesInMiddleOfFile() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -190,12 +177,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(1, 2, 0, 0, 3, 4); - } @Test public void shouldDetectNewLinesAtEndOfFile() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -211,12 +196,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(1, 2, 3, 0, 0); - } @Test public void shouldIgnoreDeletedLinesAtEndOfFile() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -232,12 +215,10 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(1, 2, 3); - } @Test public void shouldIgnoreDeletedLinesInTheMiddleOfFile() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -259,7 +240,6 @@ public class SourceLinesDiffFinderTest { @Test public void shouldIgnoreDeletedLinesAtTheStartOfTheFile() { - List database = new ArrayList<>(); database.add("line - 0"); database.add("line - 1"); @@ -273,7 +253,5 @@ public class SourceLinesDiffFinderTest { int[] diff = new SourceLinesDiffFinder(database, report).findMatchingLines(); assertThat(diff).containsExactly(3, 4); - } - } diff --git a/tests/src/test/java/org/sonarqube/tests/source/NoScmTest.java b/tests/src/test/java/org/sonarqube/tests/source/NoScmTest.java index de23ed8a23e..0add9b91c9a 100644 --- a/tests/src/test/java/org/sonarqube/tests/source/NoScmTest.java +++ b/tests/src/test/java/org/sonarqube/tests/source/NoScmTest.java @@ -26,6 +26,7 @@ import java.io.IOException; import java.text.ParseException; import java.util.Date; import java.util.Map; +import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -49,7 +50,7 @@ public class NoScmTest { @ClassRule public static Orchestrator orchestrator = ORCHESTRATOR; - private SourceScmWS ws = new SourceScmWS(orchestrator); + private SourceScmWS ws; @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); @@ -57,6 +58,11 @@ public class NoScmTest { @Rule public Tester tester = new Tester(orchestrator); + @Before + public void setUp() { + ws = new SourceScmWS(tester); + } + @Test public void two_analysis_without_scm_on_same_file() throws ParseException, IOException { diff --git a/tests/src/test/java/org/sonarqube/tests/source/NoScmThenScmTest.java b/tests/src/test/java/org/sonarqube/tests/source/NoScmThenScmTest.java index 8da6f589b67..f44fc83c68a 100644 --- a/tests/src/test/java/org/sonarqube/tests/source/NoScmThenScmTest.java +++ b/tests/src/test/java/org/sonarqube/tests/source/NoScmThenScmTest.java @@ -26,6 +26,7 @@ import java.io.IOException; import java.text.ParseException; import java.util.Date; import java.util.Map; +import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -48,7 +49,7 @@ public class NoScmThenScmTest { @ClassRule public static Orchestrator orchestrator = ORCHESTRATOR; - private SourceScmWS ws = new SourceScmWS(orchestrator); + private SourceScmWS ws; @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); @@ -56,6 +57,11 @@ public class NoScmThenScmTest { @Rule public Tester tester = new Tester(orchestrator); + @Before + public void setUp() { + ws = new SourceScmWS(tester); + } + @Test public void without_and_then_with_scm_on_same_file() throws ParseException, IOException { diff --git a/tests/src/test/java/org/sonarqube/tests/source/ScmThenNoScmTest.java b/tests/src/test/java/org/sonarqube/tests/source/ScmThenNoScmTest.java index b53fbcd6300..2a61e74dd37 100644 --- a/tests/src/test/java/org/sonarqube/tests/source/ScmThenNoScmTest.java +++ b/tests/src/test/java/org/sonarqube/tests/source/ScmThenNoScmTest.java @@ -28,6 +28,7 @@ import java.time.OffsetDateTime; import java.time.ZoneOffset; import java.util.Date; import java.util.Map; +import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -46,7 +47,7 @@ public class ScmThenNoScmTest { @ClassRule public static Orchestrator orchestrator = ORCHESTRATOR; - private SourceScmWS ws = new SourceScmWS(orchestrator); + private SourceScmWS ws; @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); @@ -54,6 +55,11 @@ public class ScmThenNoScmTest { @Rule public Tester tester = new Tester(orchestrator); + @Before + public void setUp() { + ws = new SourceScmWS(tester); + } + @Test public void with_and_then_without_scm_on_same_file() throws ParseException, IOException { diff --git a/tests/src/test/java/org/sonarqube/tests/source/SourceScmWS.java b/tests/src/test/java/org/sonarqube/tests/source/SourceScmWS.java index 77472ed4be0..c61aeca894c 100644 --- a/tests/src/test/java/org/sonarqube/tests/source/SourceScmWS.java +++ b/tests/src/test/java/org/sonarqube/tests/source/SourceScmWS.java @@ -19,25 +19,25 @@ */ package org.sonarqube.tests.source; -import com.sonar.orchestrator.Orchestrator; import java.text.ParseException; import java.util.HashMap; import java.util.Map; import org.sonar.wsclient.jsonsimple.JSONArray; import org.sonar.wsclient.jsonsimple.JSONObject; import org.sonar.wsclient.jsonsimple.JSONValue; +import org.sonarqube.qa.util.Tester; +import org.sonarqube.ws.client.GetRequest; public class SourceScmWS { + public final Tester tester; - private final Orchestrator orchestrator; - - public SourceScmWS(Orchestrator orchestrator) { - this.orchestrator = orchestrator; + public SourceScmWS(Tester tester) { + this.tester = tester; } public Map getScmData(String fileKey) throws ParseException { Map result = new HashMap<>(); - String json = orchestrator.getServer().adminWsClient().get("api/sources/scm", "key", fileKey); + String json = tester.asAnonymous().wsClient().wsConnector().call(new GetRequest("api/sources/scm").setParam("key", fileKey)).content(); JSONObject obj = (JSONObject) JSONValue.parse(json); JSONArray array = (JSONArray) obj.get("scm"); for (Object anArray : array) { @@ -48,5 +48,4 @@ public class SourceScmWS { return result; } - }