From 31b6a958fbee4cf483e464991c038f71d47a7525 Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Fri, 24 May 2019 07:40:06 -0500 Subject: [PATCH] SONAR_11996 Fallback to line comparison to detect new lines in Short Lived Branches and Pull Requests --- .../.attach_pid28812 | 0 .../projectanalysis/scm/ScmInfoDbLoader.java | 10 ++-- .../source/NewLinesRepository.java | 11 ++-- .../source/SourceLinesDiffImpl.java | 25 ++++++++- .../scm/ScmInfoDbLoaderTest.java | 38 ++++++++++++- .../source/NewLinesRepositoryTest.java | 18 ++++++- .../source/SourceLinesDiffImplTest.java | 53 ++++++++++++++++--- 7 files changed, 134 insertions(+), 21 deletions(-) create mode 100644 server/sonar-ce-task-projectanalysis/.attach_pid28812 diff --git a/server/sonar-ce-task-projectanalysis/.attach_pid28812 b/server/sonar-ce-task-projectanalysis/.attach_pid28812 new file mode 100644 index 00000000000..e69de29bb2d diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/scm/ScmInfoDbLoader.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/scm/ScmInfoDbLoader.java index ba8a81f75bd..9c8aefa912c 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/scm/ScmInfoDbLoader.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/scm/ScmInfoDbLoader.java @@ -60,14 +60,18 @@ public class ScmInfoDbLoader { } private Optional getFileUUid(Component file) { - if (!analysisMetadataHolder.isFirstAnalysis()) { + if (!analysisMetadataHolder.isFirstAnalysis() && !analysisMetadataHolder.isSLBorPR()) { return Optional.of(file.getUuid()); } - // at this point, it's the first analysis but had copyFromPrevious flag true + // at this point, it's the first analysis of a LLB with copyFromPrevious flag true or any analysis of a PR/SLB Branch branch = analysisMetadataHolder.getBranch(); if (!branch.isMain()) { - return Optional.ofNullable(mergeBranchComponentUuid.getMergeBranchComponentUuid(file.getDbKey())); + String uuid = mergeBranchComponentUuid.getTargetBranchComponentUuid(file.getDbKey()); + if (uuid == null) { + uuid = mergeBranchComponentUuid.getMergeBranchComponentUuid(file.getDbKey()); + } + return Optional.ofNullable(uuid); } return Optional.empty(); diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/source/NewLinesRepository.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/source/NewLinesRepository.java index 5a8854c8bc3..20386f3576c 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/source/NewLinesRepository.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/source/NewLinesRepository.java @@ -28,7 +28,6 @@ import java.util.Set; import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolder; import org.sonar.ce.task.projectanalysis.batch.BatchReportReader; import org.sonar.ce.task.projectanalysis.component.Component; -import org.sonar.ce.task.projectanalysis.period.Period; import org.sonar.ce.task.projectanalysis.period.PeriodHolder; import org.sonar.ce.task.projectanalysis.scm.Changeset; import org.sonar.ce.task.projectanalysis.scm.ScmInfo; @@ -67,7 +66,7 @@ public class NewLinesRepository { * if a line is new or not. */ private Optional> computeNewLinesFromScm(Component component) { - if (!periodHolder.hasPeriod()) { + if (!periodHolder.hasPeriod() && !analysisMetadataHolder.isSLBorPR()) { return Optional.empty(); } @@ -80,8 +79,10 @@ public class NewLinesRepository { Map allChangesets = scmInfo.getAllChangesets(); Set lines = new HashSet<>(); + // in SLB/PRs, we consider changes introduced in this analysis as new, hence subtracting 1. + long referenceDate = analysisMetadataHolder.isSLBorPR() ? analysisMetadataHolder.getAnalysisDate() - 1 : periodHolder.getPeriod().getSnapshotDate(); for (Map.Entry e : allChangesets.entrySet()) { - if (isLineInPeriod(e.getValue().getDate(), periodHolder.getPeriod())) { + if (isLineInPeriod(e.getValue().getDate(), referenceDate)) { lines.add(e.getKey()); } } @@ -91,8 +92,8 @@ public class NewLinesRepository { /** * A line belongs to a Period if its date is older than the SNAPSHOT's date of the period. */ - private static boolean isLineInPeriod(long lineDate, Period period) { - return lineDate > period.getSnapshotDate(); + private static boolean isLineInPeriod(long lineDate, long referenceDate) { + return lineDate > referenceDate; } private Optional> getChangedLinesFromReport(Component file) { diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/source/SourceLinesDiffImpl.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/source/SourceLinesDiffImpl.java index cbdbdbe6f35..de161a09518 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/source/SourceLinesDiffImpl.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/source/SourceLinesDiffImpl.java @@ -21,7 +21,9 @@ package org.sonar.ce.task.projectanalysis.source; import java.util.Collections; import java.util.List; +import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolder; import org.sonar.ce.task.projectanalysis.component.Component; +import org.sonar.ce.task.projectanalysis.component.MergeAndTargetBranchComponentUuids; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.source.FileSourceDao; @@ -31,11 +33,16 @@ public class SourceLinesDiffImpl implements SourceLinesDiff { private final DbClient dbClient; private final FileSourceDao fileSourceDao; private final SourceLinesHashRepository sourceLinesHash; + private final MergeAndTargetBranchComponentUuids mergeAndTargetBranchComponentUuids; + private final AnalysisMetadataHolder analysisMetadataHolder; - public SourceLinesDiffImpl(DbClient dbClient, FileSourceDao fileSourceDao, SourceLinesHashRepository sourceLinesHash) { + public SourceLinesDiffImpl(DbClient dbClient, FileSourceDao fileSourceDao, SourceLinesHashRepository sourceLinesHash, + MergeAndTargetBranchComponentUuids mergeAndTargetBranchComponentUuids, AnalysisMetadataHolder analysisMetadataHolder) { this.dbClient = dbClient; this.fileSourceDao = fileSourceDao; this.sourceLinesHash = sourceLinesHash; + this.mergeAndTargetBranchComponentUuids = mergeAndTargetBranchComponentUuids; + this.analysisMetadataHolder = analysisMetadataHolder; } @Override @@ -48,7 +55,21 @@ public class SourceLinesDiffImpl implements SourceLinesDiff { private List getDBLines(Component component) { try (DbSession dbSession = dbClient.openSession(false)) { - List database = fileSourceDao.selectLineHashes(dbSession, component.getUuid()); + String uuid; + if (analysisMetadataHolder.isSLBorPR()) { + uuid = mergeAndTargetBranchComponentUuids.getTargetBranchComponentUuid(component.getDbKey()); + if (uuid == null) { + uuid = mergeAndTargetBranchComponentUuids.getMergeBranchComponentUuid(component.getDbKey()); + } + } else { + uuid = component.getUuid(); + } + + if (uuid == null) { + return Collections.emptyList(); + } + + List database = fileSourceDao.selectLineHashes(dbSession, uuid); if (database == null) { return Collections.emptyList(); } diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/scm/ScmInfoDbLoaderTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/scm/ScmInfoDbLoaderTest.java index e560a364301..8a47495bbdb 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/scm/ScmInfoDbLoaderTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/scm/ScmInfoDbLoaderTest.java @@ -36,11 +36,13 @@ import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.ce.task.projectanalysis.component.MergeAndTargetBranchComponentUuids; import org.sonar.core.hash.SourceHashComputer; import org.sonar.db.DbTester; +import org.sonar.db.component.BranchType; import org.sonar.db.protobuf.DbFileSources; import org.sonar.db.source.FileSourceDto; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.sonar.api.utils.log.LoggerLevel.TRACE; import static org.sonar.ce.task.projectanalysis.component.ReportComponent.builder; @@ -93,7 +95,6 @@ public class ScmInfoDbLoaderTest { String mergeFileUuid = "mergeFileUuid"; String hash = computeSourceHash(1); - when(branch.getMergeBranchUuid()).thenReturn("mergeBranchUuid"); when(mergeAndTargetBranchComponentUuids.getMergeBranchComponentUuid(FILE.getDbKey())).thenReturn(mergeFileUuid); addFileSourceInDb("henry", DATE_1, "rev-1", hash, mergeFileUuid); @@ -104,6 +105,25 @@ public class ScmInfoDbLoaderTest { assertThat(logTester.logs(TRACE)).containsOnly("Reading SCM info from DB for file 'mergeFileUuid'"); } + @Test + public void read_from_target_if_pullrequest() { + Branch branch = mock(Branch.class); + when(branch.getType()).thenReturn(BranchType.PULL_REQUEST); + analysisMetadataHolder.setBaseAnalysis(null); + analysisMetadataHolder.setBranch(branch); + + String targetBranchFileUuid = "targetBranchFileUuid"; + String hash = computeSourceHash(1); + + when(mergeAndTargetBranchComponentUuids.getMergeBranchComponentUuid(FILE.getDbKey())).thenReturn(targetBranchFileUuid); + addFileSourceInDb("henry", DATE_1, "rev-1", hash, targetBranchFileUuid); + + DbScmInfo scmInfo = underTest.getScmInfo(FILE).get(); + assertThat(scmInfo.getAllChangesets()).hasSize(1); + assertThat(scmInfo.fileHash()).isEqualTo(hash); + assertThat(logTester.logs(TRACE)).containsOnly("Reading SCM info from DB for file 'targetBranchFileUuid'"); + } + @Test public void return_empty_if_no_dto_available() { analysisMetadataHolder.setBaseAnalysis(baseProjectAnalysis); @@ -116,15 +136,29 @@ public class ScmInfoDbLoaderTest { } @Test - public void do_not_read_from_db_on_first_analysis_and_no_merge_branch() { + public void do_not_read_from_db_on_first_analysis_if_there_is_no_merge_branch() { Branch branch = mock(Branch.class); + when(branch.getType()).thenReturn(BranchType.PULL_REQUEST); analysisMetadataHolder.setBaseAnalysis(null); analysisMetadataHolder.setBranch(branch); + assertThat(underTest.getScmInfo(FILE)).isEmpty(); assertThat(logTester.logs(TRACE)).isEmpty(); } + @Test + public void do_not_read_from_db_on_pr_is_there_is_no_target_and_merge_branch() { + analysisMetadataHolder.setBaseAnalysis(null); + analysisMetadataHolder.setBranch(mock(Branch.class)); + + assertThat(underTest.getScmInfo(FILE)).isEmpty(); + assertThat(logTester.logs(TRACE)).isEmpty(); + + verify(mergeAndTargetBranchComponentUuids).getMergeBranchComponentUuid(FILE.getDbKey()); + verify(mergeAndTargetBranchComponentUuids).getTargetBranchComponentUuid(FILE.getDbKey()); + } + private static List generateLines(int lineCount) { ImmutableList.Builder builder = ImmutableList.builder(); for (int i = 0; i < lineCount; i++) { diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/source/NewLinesRepositoryTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/source/NewLinesRepositoryTest.java index 2bb5eb37c60..8ca4af7f7da 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/source/NewLinesRepositoryTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/source/NewLinesRepositoryTest.java @@ -55,7 +55,7 @@ public class NewLinesRepositoryTest { private NewLinesRepository repository = new NewLinesRepository(reader, analysisMetadataHolder, periodHolder, scmInfoRepository); @Test - public void load_new_lines_from_report_if_available_and_pullrequest() { + public void load_new_lines_from_report_when_available_and_pullrequest() { setPullRequest(); createChangedLinesInReport(1, 2, 5); @@ -67,7 +67,7 @@ public class NewLinesRepositoryTest { } @Test - public void calculate_new_lines_from_period() { + public void compute_new_lines_using_scm_info_for_period() { periodHolder.setPeriod(new Period("", null, 1000L, "")); scmInfoRepository.setScmInfo(FILE.getReportAttributes().getRef(), createChangesets(1100L, 900L, 1000L, 800L)); @@ -78,6 +78,20 @@ public class NewLinesRepositoryTest { assertThat(repository.newLinesAvailable()).isTrue(); } + @Test + public void compute_new_lines_using_scm_info_for_pullrequest() { + periodHolder.setPeriod(null); + setPullRequest(); + analysisMetadataHolder.setAnalysisDate(1000L); + scmInfoRepository.setScmInfo(FILE.getReportAttributes().getRef(), createChangesets(1100L, 900L, 1000L, 800L)); + + Optional> newLines = repository.getNewLines(FILE); + + assertThat(newLines).isPresent(); + assertThat(newLines.get()).containsOnly(1, 3); + assertThat(repository.newLinesAvailable()).isTrue(); + } + @Test public void return_empty_if_no_period_and_not_pullrequest() { periodHolder.setPeriod(null); diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/source/SourceLinesDiffImplTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/source/SourceLinesDiffImplTest.java index 8d00aa5d47c..28e2a8f21f4 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/source/SourceLinesDiffImplTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/source/SourceLinesDiffImplTest.java @@ -23,13 +23,14 @@ import java.util.Arrays; import javax.annotation.Nullable; import org.junit.Before; import org.junit.Test; +import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolder; import org.sonar.ce.task.projectanalysis.component.Component; +import org.sonar.ce.task.projectanalysis.component.MergeAndTargetBranchComponentUuids; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.component.ComponentDao; import org.sonar.db.source.FileSourceDao; -import static java.lang.String.valueOf; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -43,11 +44,13 @@ public class SourceLinesDiffImplTest { private ComponentDao componentDao = mock(ComponentDao.class); private FileSourceDao fileSourceDao = mock(FileSourceDao.class); private SourceLinesHashRepository sourceLinesHash = mock(SourceLinesHashRepository.class); + private AnalysisMetadataHolder analysisMetadataHolder = mock(AnalysisMetadataHolder.class); + private MergeAndTargetBranchComponentUuids mergeAndTargetBranchComponentUuids = mock(MergeAndTargetBranchComponentUuids.class); - private SourceLinesDiffImpl underTest = new SourceLinesDiffImpl(dbClient, fileSourceDao, sourceLinesHash); + private SourceLinesDiffImpl underTest = new SourceLinesDiffImpl(dbClient, fileSourceDao, sourceLinesHash, + mergeAndTargetBranchComponentUuids, analysisMetadataHolder); private static final int FILE_REF = 1; - private static final String FILE_KEY = valueOf(FILE_REF); private static final String[] CONTENT = { "package org.sonar.ce.task.projectanalysis.source_diff;", @@ -67,18 +70,54 @@ public class SourceLinesDiffImplTest { } @Test - public void should_find_no_diff_when_report_and_db_content_are_identical() { + public void should_find_diff_with_target_branch_for_slbs() { Component component = fileComponent(FILE_REF); + Component componentInTarget = fileComponent(2); - mockLineHashesInDb("" + FILE_KEY, CONTENT); + mockLineHashesInDb(2, CONTENT); setLineHashesInReport(component, CONTENT); + when(analysisMetadataHolder.isSLBorPR()).thenReturn(true); + when(mergeAndTargetBranchComponentUuids.getTargetBranchComponentUuid(component.getKey())).thenReturn("uuid_2"); + assertThat(underTest.computeMatchingLines(component)).containsExactly(1, 2, 3, 4, 5, 6, 7); + } + + @Test + public void should_find_diff_with_merge_branch_for_slbs_if_not_found_in_target() { + Component component = fileComponent(FILE_REF); + Component componentInTarget = fileComponent(2); + + mockLineHashesInDb(2, CONTENT); + setLineHashesInReport(component, CONTENT); + + when(analysisMetadataHolder.isSLBorPR()).thenReturn(true); + when(mergeAndTargetBranchComponentUuids.getMergeBranchComponentUuid(component.getKey())).thenReturn("uuid_2"); + + assertThat(underTest.computeMatchingLines(component)).containsExactly(1, 2, 3, 4, 5, 6, 7); + } + + @Test + public void all_file_is_modified_if_no_source_in_db() { + Component component = fileComponent(FILE_REF); + + setLineHashesInReport(component, CONTENT); + assertThat(underTest.computeMatchingLines(component)).containsExactly(0, 0, 0, 0, 0, 0, 0); + } + + @Test + public void should_find_no_diff_when_report_and_db_content_are_identical() { + Component component = fileComponent(FILE_REF); + + mockLineHashesInDb(FILE_REF, CONTENT); + setLineHashesInReport(component, CONTENT); + + assertThat(underTest.computeMatchingLines(component)).containsExactly(1, 2, 3, 4, 5, 6, 7); } - private void mockLineHashesInDb(String key, @Nullable String[] lineHashes) { - when(fileSourceDao.selectLineHashes(dbSession, componentUuidOf(key))) + private void mockLineHashesInDb(int ref, @Nullable String[] lineHashes) { + when(fileSourceDao.selectLineHashes(dbSession, componentUuidOf(String.valueOf(ref)))) .thenReturn(Arrays.asList(lineHashes)); } -- 2.39.5