diff options
33 files changed, 1028 insertions, 384 deletions
diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/Component.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/Component.java index 69302fcbfcd..4889401417a 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/Component.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/Component.java @@ -88,11 +88,6 @@ public interface Component { String getName(); /** - * Get component old relative path. - */ - String getOldName(); - - /** * The component short name. For files and directories this is the parent relative path (ie filename for files). For projects and view this is the same as {@link #getName()} */ String getShortName(); diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/ComponentImpl.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/ComponentImpl.java index b06e66bb465..a9ac6fbbd26 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/ComponentImpl.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/ComponentImpl.java @@ -93,11 +93,6 @@ public class ComponentImpl implements Component { } @Override - public String getOldName() { - return this.getFileAttributes().getOldName(); - } - - @Override public String getShortName() { return this.shortName; } @@ -164,7 +159,6 @@ public class ComponentImpl implements Component { private String uuid; private String key; private String name; - private String oldName; private String shortName; private String description; private FileAttributes fileAttributes; diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/FileAttributes.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/FileAttributes.java index fc62e592e14..b9f1959be39 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/FileAttributes.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/FileAttributes.java @@ -38,19 +38,19 @@ public class FileAttributes { private final String languageKey; private final boolean markedAsUnchanged; private final int lines; - private String oldName; + private String oldRelativePath; public FileAttributes(boolean unitTest, @Nullable String languageKey, int lines) { this(unitTest, languageKey, lines, false, null); } - public FileAttributes(boolean unitTest, @Nullable String languageKey, int lines, boolean markedAsUnchanged, @Nullable String oldName) { + public FileAttributes(boolean unitTest, @Nullable String languageKey, int lines, boolean markedAsUnchanged, @Nullable String oldRelativePath) { this.unitTest = unitTest; this.languageKey = languageKey; this.markedAsUnchanged = markedAsUnchanged; checkArgument(lines > 0, "Number of lines must be greater than zero"); this.lines = lines; - this.oldName = formatOldName(oldName); + this.oldRelativePath = formatOldRelativePath(oldRelativePath); } public boolean isMarkedAsUnchanged() { @@ -66,9 +66,12 @@ public class FileAttributes { return languageKey; } + /** + * The old relative path of a file when a move is detected by the SCM in the scope of a Pull Request. + */ @CheckForNull - public String getOldName() { - return oldName; + public String getOldRelativePath() { + return oldRelativePath; } /** @@ -85,11 +88,11 @@ public class FileAttributes { ", unitTest=" + unitTest + ", lines=" + lines + ", markedAsUnchanged=" + markedAsUnchanged + - ", oldName=" + oldName + + ", oldRelativePath='" + oldRelativePath + '\'' + '}'; } - private String formatOldName(@Nullable String name) { - return abbreviate(trimToNull(name), MAX_COMPONENT_NAME_LENGTH); + private static String formatOldRelativePath(@Nullable String path) { + return abbreviate(trimToNull(path), MAX_COMPONENT_NAME_LENGTH); } } diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/AddedFileRepositoryImpl.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/AddedFileRepositoryImpl.java index 3b55cf06934..c4eadfb2bda 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/AddedFileRepositoryImpl.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/AddedFileRepositoryImpl.java @@ -49,7 +49,7 @@ public class AddedFileRepositoryImpl implements MutableAddedFileRepository { public void register(Component component) { checkComponent(component); checkArgument(component.getType() == Component.Type.FILE, "component must be a file"); - checkState(!analysisMetadataHolder.isFirstAnalysis(), "No file can be registered on first analysis"); + checkState(analysisMetadataHolder.isPullRequest() || !analysisMetadataHolder.isFirstAnalysis(), "No file can be registered on first branch analysis"); addedComponents.add(component); } diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStep.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStep.java index 7bddb40e717..81cd19b3473 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStep.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStep.java @@ -356,7 +356,7 @@ public class FileMoveDetectionStep implements ComputationStep { matchesPerFileForScore.put(match.getDbUuid(), match); matchesPerFileForScore.put(match.getReportUuid(), match); } - // validate non ambiguous matches (ie. the match is the only match of either the db file and the report file) + // validate non-ambiguous matches (i.e. the match is the only match of either the db file and the report file) for (Match match : matchesToValidate) { int dbFileMatchesCount = matchesPerFileForScore.get(match.getDbUuid()).size(); int reportFileMatchesCount = matchesPerFileForScore.get(match.getReportUuid()).size(); @@ -372,13 +372,13 @@ public class FileMoveDetectionStep implements ComputationStep { } @Immutable - private static final class DbComponent { + public static final class DbComponent { private final String key; private final String uuid; private final String path; private final int lineCount; - private DbComponent(String key, String uuid, String path, int lineCount) { + public DbComponent(String key, String uuid, String path, int lineCount) { this.key = key; this.uuid = uuid; this.path = path; diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/MutableMovedFilesRepositoryImpl.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/MutableMovedFilesRepositoryImpl.java index b93e7e26e19..4c68a64c767 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/MutableMovedFilesRepositoryImpl.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/MutableMovedFilesRepositoryImpl.java @@ -52,7 +52,7 @@ public class MutableMovedFilesRepositoryImpl implements MutableMovedFilesReposit return retrieveOriginalFileFromCache(originalPullRequestFiles, file); } - private void storeOriginalFileInCache(Map<String, OriginalFile> originalFiles, Component file, OriginalFile originalFile) { + private static void storeOriginalFileInCache(Map<String, OriginalFile> originalFiles, Component file, OriginalFile originalFile) { requireNonNull(file, "file can't be null"); requireNonNull(originalFile, "originalFile can't be null"); checkArgument(file.getType() == Component.Type.FILE, "file must be of type FILE"); @@ -67,7 +67,7 @@ public class MutableMovedFilesRepositoryImpl implements MutableMovedFilesReposit } } - private Optional<OriginalFile> retrieveOriginalFileFromCache(Map<String, OriginalFile> originalFiles, Component file) { + private static Optional<OriginalFile> retrieveOriginalFileFromCache(Map<String, OriginalFile> originalFiles, Component file) { requireNonNull(file, "file can't be null"); if (file.getType() != Component.Type.FILE) { diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/PullRequestFileMoveDetectionStep.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/PullRequestFileMoveDetectionStep.java index 3a3197fdf3f..d0615bdfbd1 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/PullRequestFileMoveDetectionStep.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/filemove/PullRequestFileMoveDetectionStep.java @@ -20,19 +20,12 @@ package org.sonar.ce.task.projectanalysis.filemove; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Sets; import java.util.Collection; -import java.util.LinkedList; -import java.util.List; +import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.Set; -import java.util.function.Consumer; -import java.util.function.Function; -import javax.annotation.concurrent.Immutable; import org.apache.ibatis.session.ResultHandler; -import org.jetbrains.annotations.NotNull; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolder; @@ -41,6 +34,7 @@ import org.sonar.ce.task.projectanalysis.component.CrawlerDepthLimit; import org.sonar.ce.task.projectanalysis.component.DepthTraversalTypeAwareCrawler; import org.sonar.ce.task.projectanalysis.component.TreeRootHolder; import org.sonar.ce.task.projectanalysis.component.TypeAwareVisitorAdapter; +import org.sonar.ce.task.projectanalysis.filemove.FileMoveDetectionStep.DbComponent; import org.sonar.ce.task.projectanalysis.filemove.MovedFilesRepository.OriginalFile; import org.sonar.ce.task.step.ComputationStep; import org.sonar.db.DbClient; @@ -48,6 +42,8 @@ import org.sonar.db.DbSession; import org.sonar.db.component.BranchDto; import org.sonar.db.component.FileMoveRowDto; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toMap; import static org.sonar.ce.task.projectanalysis.component.ComponentVisitor.Order.POST_ORDER; @@ -95,28 +91,30 @@ public class PullRequestFileMoveDetectionStep implements ComputationStep { if (targetBranchDbFilesByUuid.isEmpty()) { registerNewlyAddedFiles(reportFilesByUuid); context.getStatistics().add("addedFiles", reportFilesByUuid.size()); - LOG.debug("Previous snapshot has no file. No file move detection."); + LOG.debug("Target branch has no files. No file move detection."); return; } - Map<String, Component> movedFilesByUuid = getMovedFilesByUuid(reportFilesByUuid); - context.getStatistics().add("movedFiles", movedFilesByUuid.size()); + Collection<Component> movedFiles = getMovedFilesByUuid(reportFilesByUuid); + context.getStatistics().add("movedFiles", movedFiles.size()); Map<String, Component> newlyAddedFilesByUuid = getNewlyAddedFilesByUuid(reportFilesByUuid, targetBranchDbFilesByUuid); context.getStatistics().add("addedFiles", newlyAddedFilesByUuid.size()); - registerMovedFiles(movedFilesByUuid.values(), targetBranchDbFilesByUuid.values()); + Map<String, DbComponent> dbFilesByPathReference = toDbFilesByPathReferenceMap(targetBranchDbFilesByUuid.values()); + + registerMovedFiles(movedFiles, dbFilesByPathReference); registerNewlyAddedFiles(newlyAddedFilesByUuid); } - private void registerMovedFiles(Collection<Component> movedFiles, Collection<DbComponent> dbFiles) { + private void registerMovedFiles(Collection<Component> movedFiles, Map<String, DbComponent> dbFilesByPathReference) { movedFiles - .forEach(registerMovedFile(dbFiles)); + .forEach(movedFile -> registerMovedFile(dbFilesByPathReference, movedFile)); } - private Consumer<Component> registerMovedFile(Collection<DbComponent> dbFiles) { - return movedFile -> retrieveDbFile(dbFiles, movedFile) - .ifPresent(dbFile -> movedFilesRepository.setOriginalPullRequestFile(movedFile, toOriginalFile(dbFile))); + private void registerMovedFile(Map<String, DbComponent> dbFiles, Component movedFile) { + retrieveDbFile(dbFiles, movedFile) + .ifPresent(dbFile -> movedFilesRepository.setOriginalPullRequestFile(movedFile, toOriginalFile(dbFile))); } private void registerNewlyAddedFiles(Map<String, Component> newAddedFilesByUuid) { @@ -125,66 +123,61 @@ public class PullRequestFileMoveDetectionStep implements ComputationStep { .forEach(addedFileRepository::register); } - @NotNull - private Map<String, Component> getNewlyAddedFilesByUuid(Map<String, Component> reportFilesByUuid, Map<String, DbComponent> dbFilesByUuid) { + private static Map<String, Component> getNewlyAddedFilesByUuid(Map<String, Component> reportFilesByUuid, Map<String, DbComponent> dbFilesByUuid) { return reportFilesByUuid .values() .stream() - .filter(file -> Objects.isNull(file.getOldName())) + .filter(file -> Objects.isNull(file.getFileAttributes().getOldRelativePath())) .filter(file -> !dbFilesByUuid.containsKey(file.getUuid())) - .collect(toMap(Component::getUuid, Function.identity())); + .collect(toMap(Component::getUuid, identity())); } - private Map<String, Component> getMovedFilesByUuid(Map<String, Component> reportFilesByUuid) { + private static Collection<Component> getMovedFilesByUuid(Map<String, Component> reportFilesByUuid) { return reportFilesByUuid .values() .stream() - .filter(file -> Objects.nonNull(file.getOldName())) - .collect(toMap(Component::getUuid, Function.identity())); + .filter(file -> Objects.nonNull(file.getFileAttributes().getOldRelativePath())) + .collect(toList()); } - private Optional<DbComponent> retrieveDbFile(Collection<DbComponent> dbFiles, Component file) { - return dbFiles - .stream() - .filter(dbFile -> dbFile.getPath().equals(file.getOldName())) - .findFirst(); - } - - public Set<String> difference(Set<String> set1, Set<String> set2) { - if (set1.isEmpty() || set2.isEmpty()) { - return set1; - } - - return Sets.difference(set1, set2).immutableCopy(); + private static Optional<DbComponent> retrieveDbFile(Map<String, DbComponent> dbFilesByPathReference, Component file) { + return Optional.ofNullable(dbFilesByPathReference.get(file.getFileAttributes().getOldRelativePath())); } private Map<String, DbComponent> getTargetBranchDbFilesByUuid(AnalysisMetadataHolder analysisMetadataHolder) { try (DbSession dbSession = dbClient.openSession(false)) { - String targetBranchUuid = getTargetBranchUuid(dbSession, analysisMetadataHolder.getProject().getUuid(), analysisMetadataHolder.getBranch().getTargetBranchName()); - - return getTargetBranchDbFiles(dbSession, targetBranchUuid) - .stream() - .collect(toMap(DbComponent::getUuid, Function.identity())); + return getTargetBranchUuid(dbSession, analysisMetadataHolder.getProject().getUuid(), analysisMetadataHolder.getBranch().getTargetBranchName()) + .map(targetBranchUUid -> getTargetBranchDbFilesByUuid(dbSession, targetBranchUUid)) + .orElse(Map.of()); } } - private List<DbComponent> getTargetBranchDbFiles(DbSession dbSession, String targetBranchUuid) { - List<DbComponent> files = new LinkedList<>(); + private Map<String, DbComponent> getTargetBranchDbFilesByUuid(DbSession dbSession, String targetBranchUuid) { + Map<String, DbComponent> files = new HashMap<>(); + dbClient.componentDao().scrollAllFilesForFileMove(dbSession, targetBranchUuid, accumulateFilesForFileMove(files)); + return files; + } - ResultHandler<FileMoveRowDto> storeFileMove = resultContext -> { - FileMoveRowDto row = resultContext.getResultObject(); - files.add(new DbComponent(row.getKey(), row.getUuid(), row.getPath(), row.getLineCount())); + private static ResultHandler<FileMoveRowDto> accumulateFilesForFileMove(Map<String, DbComponent> accumulator) { + return resultContext -> { + DbComponent component = rowToDbComponent(resultContext.getResultObject()); + accumulator.put(component.getUuid(), component); }; + } - dbClient.componentDao().scrollAllFilesForFileMove(dbSession, targetBranchUuid, storeFileMove); + private static DbComponent rowToDbComponent(FileMoveRowDto row) { + return new DbComponent(row.getKey(), row.getUuid(), row.getPath(), row.getLineCount()); + } - return files; + private Optional<String> getTargetBranchUuid(DbSession dbSession, String projectUuid, String targetBranchName) { + return dbClient.branchDao().selectByBranchKey(dbSession, projectUuid, targetBranchName) + .map(BranchDto::getUuid); } - private String getTargetBranchUuid(DbSession dbSession, String projectUuid, String targetBranchName) { - return dbClient.branchDao().selectByBranchKey(dbSession, projectUuid, targetBranchName) - .map(BranchDto::getUuid) - .orElseThrow(() -> new IllegalStateException("Pull Request has no target branch")); + private static Map<String, DbComponent> toDbFilesByPathReferenceMap(Collection<DbComponent> dbFiles) { + return dbFiles + .stream() + .collect(toMap(DbComponent::getPath, identity())); } private static Map<String, Component> getReportFilesByUuid(Component root) { @@ -204,35 +197,4 @@ public class PullRequestFileMoveDetectionStep implements ComputationStep { private static OriginalFile toOriginalFile(DbComponent dbComponent) { return new OriginalFile(dbComponent.getUuid(), dbComponent.getKey()); } - - @Immutable - private static final class DbComponent { - private final String key; - private final String uuid; - private final String path; - private final int lineCount; - - private DbComponent(String key, String uuid, String path, int lineCount) { - this.key = key; - this.uuid = uuid; - this.path = path; - this.lineCount = lineCount; - } - - public String getKey() { - return key; - } - - public String getUuid() { - return uuid; - } - - public String getPath() { - return path; - } - - public int getLineCount() { - return lineCount; - } - } } diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/TrackerTargetBranchInputFactory.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/TrackerTargetBranchInputFactory.java index a1f513c8371..7554c1ea04e 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/TrackerTargetBranchInputFactory.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/TrackerTargetBranchInputFactory.java @@ -47,7 +47,6 @@ public class TrackerTargetBranchInputFactory { this.targetBranchComponentUuids = targetBranchComponentUuids; this.dbClient = dbClient; this.movedFilesRepository = movedFilesRepository; - // TODO detect file moves? } public boolean hasTargetBranchAnalysis() { @@ -60,16 +59,16 @@ public class TrackerTargetBranchInputFactory { } private String getTargetBranchComponentUuid(Component component) { - Optional<String> targetBranchOriginalComponentUuid = getOriginalComponentUuid(component); + Optional<String> targetBranchOriginalComponentKey = getOriginalComponentKey(component); - if (targetBranchOriginalComponentUuid.isPresent()) { - return targetBranchComponentUuids.getTargetBranchComponentUuid(targetBranchOriginalComponentUuid.get()); + if (targetBranchOriginalComponentKey.isPresent()) { + return targetBranchComponentUuids.getTargetBranchComponentUuid(targetBranchOriginalComponentKey.get()); } return targetBranchComponentUuids.getTargetBranchComponentUuid(component.getKey()); } - private Optional<String> getOriginalComponentUuid(Component component) { + private Optional<String> getOriginalComponentKey(Component component) { return movedFilesRepository .getOriginalPullRequestFile(component) .map(OriginalFile::getKey); diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/FileAttributesTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/FileAttributesTest.java index 1e70547c45e..2a1318c37ac 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/FileAttributesTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/FileAttributesTest.java @@ -27,27 +27,39 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; public class FileAttributesTest { @Test public void create_production_file() { - FileAttributes underTest = new FileAttributes(true, "java", 10, true); + FileAttributes underTest = new FileAttributes(false, "java", 10, true,"C"); - assertThat(underTest.isUnitTest()).isTrue(); + assertThat(underTest.isUnitTest()).isFalse(); assertThat(underTest.getLanguageKey()).isEqualTo("java"); assertThat(underTest.getLines()).isEqualTo(10); assertThat(underTest.isMarkedAsUnchanged()).isTrue(); + assertThat(underTest.getOldRelativePath()).isEqualTo("C"); } @Test public void create_unit_test() { - FileAttributes underTest = new FileAttributes(true, "java", 10, false); + FileAttributes underTest = new FileAttributes(true, "java", 10, false,"oldName"); assertThat(underTest.isUnitTest()).isTrue(); assertThat(underTest.getLanguageKey()).isEqualTo("java"); assertThat(underTest.getLines()).isEqualTo(10); assertThat(underTest.isMarkedAsUnchanged()).isFalse(); + assertThat(underTest.getOldRelativePath()).isEqualTo("oldName"); + } + + @Test + public void create_without_oldName() { + FileAttributes underTest = new FileAttributes(true, "TypeScript", 10, false, null); + + assertThat(underTest.isUnitTest()).isTrue(); + assertThat(underTest.getLanguageKey()).isEqualTo("TypeScript"); + assertThat(underTest.getLines()).isEqualTo(10); + assertThat(underTest.getOldRelativePath()).isNull(); } @Test public void create_without_language() { - FileAttributes underTest = new FileAttributes(true, null, 10, false); + FileAttributes underTest = new FileAttributes(true, null, 10, false, null); assertThat(underTest.isUnitTest()).isTrue(); assertThat(underTest.getLanguageKey()).isNull(); @@ -56,23 +68,23 @@ public class FileAttributesTest { @Test public void fail_with_IAE_when_lines_is_0() { - assertThatThrownBy(() -> new FileAttributes(true, "java", 0, false)) + assertThatThrownBy(() -> new FileAttributes(true, "java", 0, false, null)) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Number of lines must be greater than zero"); } @Test public void fail_with_IAE_when_lines_is_less_than_0() { - assertThatThrownBy(() -> new FileAttributes(true, "java", -10, false)) + assertThatThrownBy(() -> new FileAttributes(true, "java", -10, false, null)) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Number of lines must be greater than zero"); } @Test public void test_toString() { - assertThat(new FileAttributes(true, "java", 10, true)) - .hasToString("FileAttributes{languageKey='java', unitTest=true, lines=10, markedAsUnchanged=true}"); - assertThat(new FileAttributes(false, null, 1, false)) - .hasToString("FileAttributes{languageKey='null', unitTest=false, lines=1, markedAsUnchanged=false}"); + assertThat(new FileAttributes(true, "java", 10, true, "bobo")) + .hasToString("FileAttributes{languageKey='java', unitTest=true, lines=10, markedAsUnchanged=true, oldRelativePath='bobo'}"); + assertThat(new FileAttributes(false, null, 1, false, null)) + .hasToString("FileAttributes{languageKey='null', unitTest=false, lines=1, markedAsUnchanged=false, oldRelativePath='null'}"); } } diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/FileStatusesImplTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/FileStatusesImplTest.java index 3b181db1893..ebe6e47bd20 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/FileStatusesImplTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/FileStatusesImplTest.java @@ -79,7 +79,7 @@ public class FileStatusesImplTest { @Test public void isDataUnchanged_returns_false_if_any_SAME_status_is_incorrect() { Component file1 = ReportComponent.builder(Component.Type.FILE, 2, "FILE1_KEY").setStatus(Component.Status.SAME) - .setFileAttributes(new FileAttributes(false, null, 10, true)).build(); + .setFileAttributes(new FileAttributes(false, null, 10, true, null)).build(); Component file2 = ReportComponent.builder(Component.Type.FILE, 3, "FILE2_KEY").setStatus(Component.Status.SAME).build(); addDbFileHash(file1, "hash1"); @@ -104,7 +104,7 @@ public class FileStatusesImplTest { analysisMetadataHolder.setBaseAnalysis(null); Component file1 = ReportComponent.builder(Component.Type.FILE, 2, "FILE1_KEY").setStatus(Component.Status.SAME) - .setFileAttributes(new FileAttributes(false, null, 10, true)).build(); + .setFileAttributes(new FileAttributes(false, null, 10, true, null)).build(); Component file2 = ReportComponent.builder(Component.Type.FILE, 3, "FILE2_KEY").setStatus(Component.Status.SAME).build(); addReportFileHash(file1, "hash1"); @@ -125,7 +125,7 @@ public class FileStatusesImplTest { @Test public void isDataUnchanged_returns_false_if_not_set_by_analyzer() { Component file1 = ReportComponent.builder(Component.Type.FILE, 2, "FILE1_KEY").setStatus(Component.Status.SAME) - .setFileAttributes(new FileAttributes(false, null, 10, false)).build(); + .setFileAttributes(new FileAttributes(false, null, 10, false,null)).build(); Component file2 = ReportComponent.builder(Component.Type.FILE, 3, "FILE2_KEY").setStatus(Component.Status.SAME).build(); addDbFileHash(file1, "hash1"); @@ -148,7 +148,7 @@ public class FileStatusesImplTest { @Test public void isDataUnchanged_returns_true_if_set_by_analyzer_and_all_SAME_status_are_correct() { Component file1 = ReportComponent.builder(Component.Type.FILE, 2, "FILE1_KEY").setStatus(Component.Status.SAME) - .setFileAttributes(new FileAttributes(false, null, 10, true)).build(); + .setFileAttributes(new FileAttributes(false, null, 10, true,null)).build(); Component file2 = ReportComponent.builder(Component.Type.FILE, 3, "FILE2_KEY").setStatus(Component.Status.SAME).build(); Component file3 = ReportComponent.builder(Component.Type.FILE, 4, "FILE3_KEY").setStatus(Component.Status.CHANGED).build(); diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/AddedFileRepositoryImplTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/AddedFileRepositoryImplTest.java index acd308b1493..5fe204f0be4 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/AddedFileRepositoryImplTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/AddedFileRepositoryImplTest.java @@ -111,7 +111,7 @@ public class AddedFileRepositoryImplTest { assertThatThrownBy(() -> underTest.register(component)) .isInstanceOf(IllegalStateException.class) - .hasMessage("No file can be registered on first analysis"); + .hasMessage("No file can be registered on first branch analysis"); } private static Component newComponent(Component.Type type) { diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStepTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStepTest.java index fec6d2ee4d9..65b9cd1bc19 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStepTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStepTest.java @@ -49,6 +49,7 @@ import org.sonar.core.hash.SourceLineHashesComputer; import org.sonar.core.util.Uuids; import org.sonar.db.DbClient; import org.sonar.db.DbTester; +import org.sonar.db.component.BranchType; import org.sonar.db.component.ComponentDto; import org.sonar.db.component.ComponentTesting; import org.sonar.db.source.FileSourceDto; @@ -59,6 +60,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.sonar.ce.task.projectanalysis.component.ReportComponent.builder; import static org.sonar.ce.task.projectanalysis.filemove.FileMoveDetectionStep.MIN_REQUIRED_SCORE; +import static org.sonar.db.component.BranchType.*; public class FileMoveDetectionStepTest { @@ -212,8 +214,6 @@ public class FileMoveDetectionStepTest { }; @Rule - public AnalysisMetadataHolderRule analysisMetadataHolder = new AnalysisMetadataHolderRule(); - @Rule public TreeRootHolderRule treeRootHolder = new TreeRootHolderRule(); @Rule public MutableMovedFilesRepositoryRule movedFilesRepository = new MutableMovedFilesRepositoryRule(); @@ -222,15 +222,16 @@ public class FileMoveDetectionStepTest { @Rule public LogTester logTester = new LogTester(); - private DbClient dbClient = dbTester.getDbClient(); + private final DbClient dbClient = dbTester.getDbClient(); private ComponentDto project; - private SourceLinesHashRepository sourceLinesHash = mock(SourceLinesHashRepository.class); - private FileSimilarity fileSimilarity = new FileSimilarityImpl(new SourceSimilarityImpl()); - private CapturingScoreMatrixDumper scoreMatrixDumper = new CapturingScoreMatrixDumper(); - private RecordingMutableAddedFileRepository addedFileRepository = new RecordingMutableAddedFileRepository(); + private final AnalysisMetadataHolderRule analysisMetadataHolder = mock(AnalysisMetadataHolderRule.class); + private final SourceLinesHashRepository sourceLinesHash = mock(SourceLinesHashRepository.class); + private final FileSimilarity fileSimilarity = new FileSimilarityImpl(new SourceSimilarityImpl()); + private final CapturingScoreMatrixDumper scoreMatrixDumper = new CapturingScoreMatrixDumper(); + private final RecordingMutableAddedFileRepository addedFileRepository = new RecordingMutableAddedFileRepository(); - private FileMoveDetectionStep underTest = new FileMoveDetectionStep(analysisMetadataHolder, treeRootHolder, dbClient, + private final FileMoveDetectionStep underTest = new FileMoveDetectionStep(analysisMetadataHolder, treeRootHolder, dbClient, fileSimilarity, movedFilesRepository, sourceLinesHash, scoreMatrixDumper, addedFileRepository); @Before @@ -245,8 +246,8 @@ public class FileMoveDetectionStepTest { } @Test - public void execute_detects_no_move_on_first_analysis() { - analysisMetadataHolder.setBaseAnalysis(null); + public void execute_detects_no_move_if_in_pull_request_scope() { + prepareAnalysis(PULL_REQUEST, ANALYSIS); TestComputationStepContext context = new TestComputationStepContext(); underTest.execute(context); @@ -256,8 +257,19 @@ public class FileMoveDetectionStepTest { } @Test + public void execute_detects_no_move_on_first_analysis() { + prepareAnalysis(BRANCH, null); + + TestComputationStepContext context = new TestComputationStepContext(); + underTest.execute(context); + + assertThat(movedFilesRepository.getComponentsWithOriginal()).isEmpty(); + verifyStatistics(context, 0, null, null, null); + } + + @Test public void execute_detects_no_move_if_baseSnapshot_has_no_file_and_report_has_no_file() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); TestComputationStepContext context = new TestComputationStepContext(); underTest.execute(context); @@ -269,7 +281,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_baseSnapshot_has_no_file() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, null); setFilesInReport(file1, file2); @@ -284,7 +296,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_there_is_no_file_in_report() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); insertFiles( /* no components */); setFilesInReport(); @@ -298,7 +310,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_file_key_exists_in_both_DB_and_report() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, null); insertFiles(file1.getUuid(), file2.getUuid()); @@ -316,7 +328,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_move_if_content_of_file_is_same_in_DB_and_report() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, CONTENT1); ComponentDto[] dtos = insertFiles(file1.getUuid()); @@ -336,7 +348,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_content_of_file_is_not_similar_enough() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, LESS_CONTENT1); insertFiles(file1.getKey()); @@ -356,7 +368,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_content_of_file_is_empty_in_DB() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, CONTENT1); insertFiles(file1.getKey()); @@ -374,7 +386,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_content_of_file_has_no_path_in_DB() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, CONTENT1); insertFiles(key -> newComponentDto(key).setPath(null), file1.getKey()); @@ -392,7 +404,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_content_of_file_is_empty_in_report() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, CONTENT_EMPTY); insertFiles(file1.getKey()); @@ -411,7 +423,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_two_added_files_have_same_content_as_the_one_in_db() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, CONTENT1); Component file3 = fileComponent(FILE_3_REF, CONTENT1); @@ -430,7 +442,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_two_deleted_files_have_same_content_as_the_one_added() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, null); Component file3 = fileComponent(FILE_3_REF, CONTENT1); @@ -450,7 +462,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_detects_no_move_if_two_files_are_empty_in_DB() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, null); insertFiles(file1.getUuid(), file2.getUuid()); @@ -474,7 +486,7 @@ public class FileMoveDetectionStepTest { // - file2 deleted // - file4 untouched // - file5 renamed to file6 with a small change - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, null); Component file3 = fileComponent(FILE_3_REF, CONTENT1); @@ -505,7 +517,7 @@ public class FileMoveDetectionStepTest { @Test public void execute_does_not_compute_any_distance_if_all_files_sizes_are_all_too_different() { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); Component file1 = fileComponent(FILE_1_REF, null); Component file2 = fileComponent(FILE_2_REF, null); Component file3 = fileComponent(FILE_3_REF, arrayOf(118)); @@ -535,7 +547,7 @@ public class FileMoveDetectionStepTest { */ @Test public void real_life_use_case() throws Exception { - analysisMetadataHolder.setBaseAnalysis(ANALYSIS); + prepareBranchAnalysis(ANALYSIS); for (File f : FileUtils.listFiles(new File("src/test/resources/org/sonar/ce/task/projectanalysis/filemove/FileMoveDetectionStepTest/v1"), null, false)) { insertFiles("uuid_" + f.getName().hashCode()); insertContentOfFileInDb("uuid_" + f.getName().hashCode(), readLines(f)); @@ -656,7 +668,20 @@ public class FileMoveDetectionStepTest { } } - private static void verifyStatistics(TestComputationStepContext context, + private void prepareBranchAnalysis(Analysis analysis) { + prepareAnalysis(BRANCH, analysis); + } + + private void prepareAnalysis(BranchType branch, Analysis analysis) { + mockBranchType(branch); + analysisMetadataHolder.setBaseAnalysis(analysis); + } + + private void mockBranchType(BranchType branchType) { + when(analysisMetadataHolder.isPullRequest()).thenReturn(branchType == PULL_REQUEST); + } + + public static void verifyStatistics(TestComputationStepContext context, @Nullable Integer expectedReportFiles, @Nullable Integer expectedDbFiles, @Nullable Integer expectedAddedFiles, @Nullable Integer expectedMovedFiles) { context.getStatistics().assertValue("reportFiles", expectedReportFiles); @@ -665,7 +690,7 @@ public class FileMoveDetectionStepTest { context.getStatistics().assertValue("movedFiles", expectedMovedFiles); } - private static class RecordingMutableAddedFileRepository implements MutableAddedFileRepository { + public static class RecordingMutableAddedFileRepository implements MutableAddedFileRepository { private final List<Component> components = new ArrayList<>(); @Override diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/PullRequestFileMoveDetectionStepTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/PullRequestFileMoveDetectionStepTest.java new file mode 100644 index 00000000000..29f44343861 --- /dev/null +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/filemove/PullRequestFileMoveDetectionStepTest.java @@ -0,0 +1,385 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.ce.task.projectanalysis.filemove; + +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import javax.annotation.CheckForNull; +import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.sonar.api.utils.System2; +import org.sonar.api.utils.log.LogTester; +import org.sonar.ce.task.projectanalysis.analysis.Analysis; +import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolderRule; +import org.sonar.ce.task.projectanalysis.analysis.Branch; +import org.sonar.ce.task.projectanalysis.component.Component; +import org.sonar.ce.task.projectanalysis.component.FileAttributes; +import org.sonar.ce.task.projectanalysis.component.TreeRootHolderRule; +import org.sonar.ce.task.projectanalysis.filemove.MovedFilesRepository.OriginalFile; +import org.sonar.ce.task.step.TestComputationStepContext; +import org.sonar.core.util.Uuids; +import org.sonar.db.DbClient; +import org.sonar.db.DbTester; +import org.sonar.db.component.BranchType; +import org.sonar.db.component.ComponentDto; +import org.sonar.db.component.ComponentTesting; +import org.sonar.db.source.FileSourceDto; +import org.sonar.server.project.Project; + +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; +import static java.util.stream.Collectors.toSet; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.sonar.ce.task.projectanalysis.component.Component.Type.FILE; +import static org.sonar.ce.task.projectanalysis.component.Component.Type.PROJECT; +import static org.sonar.ce.task.projectanalysis.component.ReportComponent.builder; +import static org.sonar.ce.task.projectanalysis.filemove.FileMoveDetectionStepTest.RecordingMutableAddedFileRepository; +import static org.sonar.ce.task.projectanalysis.filemove.FileMoveDetectionStepTest.verifyStatistics; +import static org.sonar.db.component.BranchType.BRANCH; +import static org.sonar.db.component.BranchType.PULL_REQUEST; + +public class PullRequestFileMoveDetectionStepTest { + private static final String ROOT_REF = "0"; + private static final String FILE_1_REF = "1"; + private static final String FILE_2_REF = "2"; + private static final String FILE_3_REF = "3"; + private static final String FILE_4_REF = "4"; + private static final String FILE_5_REF = "5"; + private static final String FILE_6_REF = "6"; + private static final String FILE_7_REF = "7"; + private static final String TARGET_BRANCH = "target_branch"; + private static final String BRANCH_UUID = "branch_uuid"; + private static final String SNAPSHOT_UUID = "uuid_1"; + + private static final Analysis ANALYSIS = new Analysis.Builder() + .setUuid(SNAPSHOT_UUID) + .setCreatedAt(86521) + .build(); + + @Rule + public TreeRootHolderRule treeRootHolder = new TreeRootHolderRule(); + @Rule + public MutableMovedFilesRepositoryRule movedFilesRepository = new MutableMovedFilesRepositoryRule(); + @Rule + public DbTester dbTester = DbTester.create(System2.INSTANCE); + @Rule + public LogTester logTester = new LogTester(); + + private ComponentDto branch; + private ComponentDto project; + + private final DbClient dbClient = dbTester.getDbClient(); + private final AnalysisMetadataHolderRule analysisMetadataHolder = mock(AnalysisMetadataHolderRule.class); + private final RecordingMutableAddedFileRepository addedFileRepository = new RecordingMutableAddedFileRepository(); + private final PullRequestFileMoveDetectionStep underTest = new PullRequestFileMoveDetectionStep(analysisMetadataHolder, treeRootHolder, dbClient, movedFilesRepository, addedFileRepository); + + @Before + public void setUp() throws Exception { + project = dbTester.components().insertPrivateProject(); + branch = dbTester.components().insertProjectBranch(project, branchDto -> branchDto.setUuid(BRANCH_UUID).setKey(TARGET_BRANCH)); + treeRootHolder.setRoot(builder(Component.Type.PROJECT, Integer.parseInt(ROOT_REF)).setUuid(project.uuid()).build()); + } + + @Test + public void getDescription_returns_description() { + assertThat(underTest.getDescription()).isEqualTo("Detect file moves in Pull Request scope"); + } + + @Test + public void execute_does_not_detect_any_files_if_not_in_pull_request_scope() { + prepareAnalysis(BRANCH, ANALYSIS); + + TestComputationStepContext context = new TestComputationStepContext(); + underTest.execute(context); + + assertThat(movedFilesRepository.getComponentsWithOriginal()).isEmpty(); + verifyStatistics(context, null, null, null, null); + } + + @Test + public void execute_detects_no_move_if_report_has_no_file() { + preparePullRequestAnalysis(ANALYSIS); + + TestComputationStepContext context = new TestComputationStepContext(); + underTest.execute(context); + + assertThat(movedFilesRepository.getComponentsWithOriginal()).isEmpty(); + assertThat(addedFileRepository.getComponents()).isEmpty(); + verifyStatistics(context, 0, null, null, null); + } + + @Test + public void execute_detects_no_move_if_target_branch_has_no_files() { + preparePullRequestAnalysis(ANALYSIS); + Set<FileReference> fileReferences = Set.of(FileReference.of(FILE_1_REF), FileReference.of(FILE_2_REF)); + Map<String, Component> reportFilesByUuid = initializeAnalysisReportComponents(fileReferences); + + TestComputationStepContext context = new TestComputationStepContext(); + underTest.execute(context); + + assertThat(movedFilesRepository.getComponentsWithOriginal()).isEmpty(); + assertThat(addedFileRepository.getComponents()).containsOnlyOnceElementsOf(reportFilesByUuid.values()); + verifyStatistics(context, 2, 0, 2, null); + } + + @Test + public void execute_detects_no_move_if_there_are_no_files_in_report() { + preparePullRequestAnalysis(ANALYSIS); + initializeAnalysisReportComponents(Set.of()); + + TestComputationStepContext context = new TestComputationStepContext(); + underTest.execute(context); + + assertThat(movedFilesRepository.getComponentsWithOriginal()).isEmpty(); + assertThat(addedFileRepository.getComponents()).isEmpty(); + verifyStatistics(context, 0, null, null, null); + } + + @Test + public void execute_detects_no_move_if_file_key_exists_in_both_database_and_report() { + preparePullRequestAnalysis(ANALYSIS); + + Set<FileReference> fileReferences = Set.of(FileReference.of(FILE_1_REF), FileReference.of(FILE_2_REF)); + initializeAnalysisReportComponents(fileReferences); + initializeTargetBranchDatabaseComponents(fileReferences); + + TestComputationStepContext context = new TestComputationStepContext(); + underTest.execute(context); + + assertThat(movedFilesRepository.getComponentsWithOriginal()).isEmpty(); + assertThat(addedFileRepository.getComponents()).isEmpty(); + verifyStatistics(context, 2, 2, 0, 0); + } + + @Test + public void execute_detects_renamed_file() { + // - FILE_1_REF on target branch is renamed to FILE_2_REF on Pull Request + preparePullRequestAnalysis(ANALYSIS); + + Set<FileReference> reportFileReferences = Set.of(FileReference.of(FILE_2_REF, FILE_1_REF)); + Set<FileReference> databaseFileReferences = Set.of(FileReference.of(FILE_1_REF)); + + Map<String, Component> reportFilesByUuid = initializeAnalysisReportComponents(reportFileReferences); + Map<String, Component> databaseFilesByUuid = initializeTargetBranchDatabaseComponents(databaseFileReferences); + + TestComputationStepContext context = new TestComputationStepContext(); + underTest.execute(context); + + assertThat(addedFileRepository.getComponents()).isEmpty(); + assertThat(movedFilesRepository.getComponentsWithOriginal()).hasSize(1); + assertThatFileRenameHasBeenDetected(reportFilesByUuid, databaseFilesByUuid, FILE_2_REF, FILE_1_REF); + verifyStatistics(context, 1, 1, 0, 1); + } + + @Test + public void execute_detects_several_renamed_file() { + // - FILE_1_REF has been renamed to FILE_3_REF on Pull Request + // - FILE_2_REF has been deleted on Pull Request + // - FILE_4_REF has been left untouched + // - FILE_5_REF has been renamed to FILE_6_REF on Pull Request + // - FILE_7_REF has been added on Pull Request + preparePullRequestAnalysis(ANALYSIS); + + Set<FileReference> reportFileReferences = Set.of( + FileReference.of(FILE_3_REF, FILE_1_REF), + FileReference.of(FILE_4_REF), + FileReference.of(FILE_6_REF, FILE_5_REF), + FileReference.of(FILE_7_REF)); + + Set<FileReference> databaseFileReferences = Set.of( + FileReference.of(FILE_1_REF), + FileReference.of(FILE_2_REF), + FileReference.of(FILE_4_REF), + FileReference.of(FILE_5_REF)); + + Map<String, Component> reportFilesByUuid = initializeAnalysisReportComponents(reportFileReferences); + Map<String, Component> databaseFilesByUuid = initializeTargetBranchDatabaseComponents(databaseFileReferences); + + TestComputationStepContext context = new TestComputationStepContext(); + underTest.execute(context); + + assertThat(addedFileRepository.getComponents()).hasSize(1); + assertThat(movedFilesRepository.getComponentsWithOriginal()).hasSize(2); + assertThatFileAdditionHasBeenDetected(reportFilesByUuid, FILE_7_REF); + assertThatFileRenameHasBeenDetected(reportFilesByUuid, databaseFilesByUuid, FILE_3_REF, FILE_1_REF); + assertThatFileRenameHasBeenDetected(reportFilesByUuid, databaseFilesByUuid, FILE_6_REF, FILE_5_REF); + verifyStatistics(context, 4, 4, 1, 2); + } + + private void assertThatFileAdditionHasBeenDetected(Map<String, Component> reportFilesByUuid, String fileInReportReference) { + Component fileInReport = reportFilesByUuid.get(fileInReportReference); + + assertThat(addedFileRepository.getComponents()).contains(fileInReport); + assertThat(movedFilesRepository.getOriginalPullRequestFile(fileInReport)).isEmpty(); + } + + + private void assertThatFileRenameHasBeenDetected(Map<String, Component> reportFilesByUuid, Map<String, Component> databaseFilesByUuid, String fileInReportReference, String originalFileInDatabaseReference) { + Component fileInReport = reportFilesByUuid.get(fileInReportReference); + Component originalFileInDatabase = databaseFilesByUuid.get(originalFileInDatabaseReference); + + assertThat(movedFilesRepository.getComponentsWithOriginal()).contains(fileInReport); + assertThat(movedFilesRepository.getOriginalPullRequestFile(fileInReport)).isPresent(); + + OriginalFile detectedOriginalFile = movedFilesRepository.getOriginalPullRequestFile(fileInReport).get(); + assertThat(detectedOriginalFile.getKey()).isEqualTo(originalFileInDatabase.getKey()); + assertThat(detectedOriginalFile.getUuid()).isEqualTo(originalFileInDatabase.getUuid()); + } + + private Map<String, Component> initializeTargetBranchDatabaseComponents(Set<FileReference> references) { + Set<Component> fileComponents = createFileComponents(references); + insertFileComponentsInDatabase(fileComponents); + return toFileComponentsByUuidMap(fileComponents); + } + + private Map<String, Component> initializeAnalysisReportComponents(Set<FileReference> refs) { + Set<Component> fileComponents = createFileComponents(refs); + insertFileComponentsInReport(fileComponents); + return toFileComponentsByUuidMap(fileComponents); + } + + private Map<String, Component> toFileComponentsByUuidMap(Set<Component> fileComponents) { + return fileComponents + .stream() + .collect(toMap(Component::getUuid, identity())); + } + + private static Set<Component> createFileComponents(Set<FileReference> references) { + return references + .stream() + .map(PullRequestFileMoveDetectionStepTest::createReportFileComponent) + .collect(toSet()); + } + + private static Component createReportFileComponent(FileReference fileReference) { + return builder(FILE, Integer.parseInt(fileReference.getReference())) + .setUuid(fileReference.getReference()) + .setName("report_path" + fileReference.getReference()) + .setFileAttributes(new FileAttributes(false, null, 1, false, composeComponentPath(fileReference.getPastReference()))) + .build(); + } + + private void insertFileComponentsInReport(Set<Component> files) { + treeRootHolder + .setRoot(builder(PROJECT, Integer.parseInt(ROOT_REF)) + .setUuid(project.uuid()) + .addChildren(files.toArray(Component[]::new)) + .build()); + } + + private Set<ComponentDto> insertFileComponentsInDatabase(Set<Component> files) { + return files + .stream() + .map(Component::getUuid) + .map(this::composeComponentDto) + .peek(this::insertComponentDto) + .peek(this::insertContentOfFileInDatabase) + .collect(toSet()); + } + + private void insertComponentDto(ComponentDto component) { + dbTester.components().insertComponent(component); + } + + private ComponentDto composeComponentDto(String uuid) { + return ComponentTesting + .newFileDto(project) + .setBranchUuid(branch.uuid()) + .setKey("key_" + uuid) + .setUuid(uuid) + .setPath(composeComponentPath(uuid)); + } + + @CheckForNull + private static String composeComponentPath(@Nullable String reference) { + return Optional.ofNullable(reference) + .map(r -> String.join("_", "path", r)) + .orElse(null); + } + + private FileSourceDto insertContentOfFileInDatabase(ComponentDto file) { + FileSourceDto fileSourceDto = composeFileSourceDto(file); + persistFileSourceDto(fileSourceDto); + return fileSourceDto; + } + + private static FileSourceDto composeFileSourceDto(ComponentDto file) { + return new FileSourceDto() + .setUuid(Uuids.createFast()) + .setFileUuid(file.uuid()) + .setProjectUuid(file.branchUuid()); + } + + private void persistFileSourceDto(FileSourceDto fileSourceDto) { + dbTester.getDbClient().fileSourceDao().insert(dbTester.getSession(), fileSourceDto); + dbTester.commit(); + } + + private void preparePullRequestAnalysis(Analysis analysis) { + prepareAnalysis(PULL_REQUEST, analysis); + } + + private void prepareAnalysis(BranchType branch, Analysis analysis) { + mockBranchType(branch); + analysisMetadataHolder.setBaseAnalysis(analysis); + } + + private void mockBranchType(BranchType branchType) { + Branch branch = mock(Branch.class); + when(analysisMetadataHolder.getBranch()).thenReturn(branch); + when(analysisMetadataHolder.getBranch().getTargetBranchName()).thenReturn(TARGET_BRANCH); + when(analysisMetadataHolder.isPullRequest()).thenReturn(branchType == PULL_REQUEST); + when(analysisMetadataHolder.getProject()).thenReturn(Project.from(project)); + } + + @Immutable + private static class FileReference { + private final String reference; + private final String pastReference; + + private FileReference(String reference, @Nullable String pastReference) { + this.reference = reference; + this.pastReference = pastReference; + } + + public String getReference() { + return reference; + } + + @CheckForNull + public String getPastReference() { + return pastReference; + } + + public static FileReference of(String reference, String pastReference) { + return new FileReference(reference, pastReference); + } + + public static FileReference of(String reference) { + return new FileReference(reference, null); + } + } +} diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerTargetBranchInputFactoryTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerTargetBranchInputFactoryTest.java index 4e5a5f05dd0..75cced7d741 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerTargetBranchInputFactoryTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerTargetBranchInputFactoryTest.java @@ -20,6 +20,7 @@ package org.sonar.ce.task.projectanalysis.issue; import java.util.Collections; +import java.util.Optional; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -33,11 +34,14 @@ import org.sonar.db.component.ComponentTesting; 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; public class TrackerTargetBranchInputFactoryTest { private final static String COMPONENT_KEY = "file1"; private final static String COMPONENT_UUID = "uuid1"; + private final static String ORIGINAL_COMPONENT_KEY = "file2"; + private final static String ORIGINAL_COMPONENT_UUID = "uuid2"; @Rule public DbTester db = DbTester.create(); @@ -60,9 +64,7 @@ public class TrackerTargetBranchInputFactoryTest { ComponentDto fileDto = ComponentTesting.newFileDto(ComponentTesting.newPublicProjectDto()).setUuid(COMPONENT_UUID); db.fileSources().insertFileSource(fileDto, 3); - Component component = mock(Component.class); - when(component.getKey()).thenReturn(COMPONENT_KEY); - when(component.getType()).thenReturn(Component.Type.FILE); + Component component = getComponent(); Input<DefaultIssue> input = underTest.createForTargetBranch(component); assertThat(input.getIssues()).containsOnly(issue1); @@ -77,9 +79,7 @@ public class TrackerTargetBranchInputFactoryTest { ComponentDto fileDto = ComponentTesting.newFileDto(ComponentTesting.newPublicProjectDto()).setUuid(COMPONENT_UUID); db.fileSources().insertFileSource(fileDto, 0); - Component component = mock(Component.class); - when(component.getKey()).thenReturn(COMPONENT_KEY); - when(component.getType()).thenReturn(Component.Type.FILE); + Component component = getComponent(); Input<DefaultIssue> input = underTest.createForTargetBranch(component); assertThat(input.getIssues()).containsOnly(issue1); @@ -88,9 +88,7 @@ public class TrackerTargetBranchInputFactoryTest { @Test public void gets_nothing_when_there_is_no_matching_component() { - Component component = mock(Component.class); - when(component.getKey()).thenReturn(COMPONENT_KEY); - when(component.getType()).thenReturn(Component.Type.FILE); + Component component = getComponent(); Input<DefaultIssue> input = underTest.createForTargetBranch(component); assertThat(input.getIssues()).isEmpty(); @@ -98,6 +96,30 @@ public class TrackerTargetBranchInputFactoryTest { } @Test + public void uses_original_component_uuid_when_component_is_moved_file() { + Component component = getComponent(); + MovedFilesRepository.OriginalFile originalFile = new MovedFilesRepository.OriginalFile(ORIGINAL_COMPONENT_UUID, ORIGINAL_COMPONENT_KEY); + when(movedFilesRepository.getOriginalPullRequestFile(component)).thenReturn(Optional.of(originalFile)); + when(targetBranchComponentUuids.getTargetBranchComponentUuid(ORIGINAL_COMPONENT_KEY)) + .thenReturn(ORIGINAL_COMPONENT_UUID); + DefaultIssue issue1 = new DefaultIssue(); + when(componentIssuesLoader.loadOpenIssuesWithChanges(ORIGINAL_COMPONENT_UUID)).thenReturn(Collections.singletonList(issue1)); + + + Input<DefaultIssue> targetBranchIssue = underTest.createForTargetBranch(component); + + verify(targetBranchComponentUuids).getTargetBranchComponentUuid(ORIGINAL_COMPONENT_KEY); + assertThat(targetBranchIssue.getIssues()).containsOnly(issue1); + } + + private Component getComponent() { + Component component = mock(Component.class); + when(component.getKey()).thenReturn(COMPONENT_KEY); + when(component.getType()).thenReturn(Component.Type.FILE); + return component; + } + + @Test public void hasTargetBranchAnalysis_returns_true_if_source_branch_of_pr_was_analysed() { when(targetBranchComponentUuids.hasTargetBranchAnalysis()).thenReturn(true); diff --git a/server/sonar-ce-task-projectanalysis/src/testFixtures/java/org/sonar/ce/task/projectanalysis/component/ReportComponent.java b/server/sonar-ce-task-projectanalysis/src/testFixtures/java/org/sonar/ce/task/projectanalysis/component/ReportComponent.java index c99dc67efb3..2a4818cd975 100644 --- a/server/sonar-ce-task-projectanalysis/src/testFixtures/java/org/sonar/ce/task/projectanalysis/component/ReportComponent.java +++ b/server/sonar-ce-task-projectanalysis/src/testFixtures/java/org/sonar/ce/task/projectanalysis/component/ReportComponent.java @@ -36,7 +36,7 @@ import static java.util.Objects.requireNonNull; */ public class ReportComponent implements Component { - private static final FileAttributes DEFAULT_FILE_ATTRIBUTES = new FileAttributes(false, null, 1, false); + private static final FileAttributes DEFAULT_FILE_ATTRIBUTES = new FileAttributes(false, null, 1, false, null); public static final Component DUMB_PROJECT = builder(Type.PROJECT, 1) .setKey("PROJECT_KEY") diff --git a/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/DefaultIndexedFile.java b/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/DefaultIndexedFile.java index be062dec668..8bc0255a286 100644 --- a/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/DefaultIndexedFile.java +++ b/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/DefaultIndexedFile.java @@ -25,7 +25,6 @@ import java.io.InputStream; import java.net.URI; import java.nio.file.Files; import java.nio.file.Path; -import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; import javax.annotation.CheckForNull; import javax.annotation.Nullable; @@ -48,7 +47,7 @@ public class DefaultIndexedFile extends DefaultInputComponent implements Indexed private final Type type; private final Path absolutePath; private final SensorStrategy sensorStrategy; - private final String oldFilePath; + private final String oldRelativeFilePath; /** * Testing purposes only! @@ -64,7 +63,7 @@ public class DefaultIndexedFile extends DefaultInputComponent implements Indexed } public DefaultIndexedFile(Path absolutePath, String projectKey, String projectRelativePath, String moduleRelativePath, Type type, @Nullable String language, int batchId, - SensorStrategy sensorStrategy, @Nullable String oldFilePath) { + SensorStrategy sensorStrategy, @Nullable String oldRelativeFilePath) { super(batchId); this.projectKey = projectKey; this.projectRelativePath = PathUtils.sanitize(projectRelativePath); @@ -73,7 +72,7 @@ public class DefaultIndexedFile extends DefaultInputComponent implements Indexed this.language = language; this.sensorStrategy = sensorStrategy; this.absolutePath = absolutePath; - this.oldFilePath = oldFilePath; + this.oldRelativeFilePath = oldRelativeFilePath; } @Override @@ -105,12 +104,8 @@ public class DefaultIndexedFile extends DefaultInputComponent implements Indexed } @CheckForNull - public String oldPath() { - return oldFilePath; - } - - public boolean isMovedFile() { - return Objects.nonNull(this.oldPath()); + public String oldRelativePath() { + return oldRelativeFilePath; } @Override diff --git a/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/DefaultInputFile.java b/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/DefaultInputFile.java index fe4c116bebd..d6bc5786e48 100644 --- a/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/DefaultInputFile.java +++ b/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/DefaultInputFile.java @@ -178,12 +178,8 @@ public class DefaultInputFile extends DefaultInputComponent implements InputFile } @CheckForNull - public String oldPath() { - return indexedFile.oldPath(); - } - - public boolean isMovedFile() { - return indexedFile.isMovedFile(); + public String oldRelativePath() { + return indexedFile.oldRelativePath(); } @Override diff --git a/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/TestInputFileBuilder.java b/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/TestInputFileBuilder.java index ec909d055c8..f7204de7670 100644 --- a/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/TestInputFileBuilder.java +++ b/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/fs/internal/TestInputFileBuilder.java @@ -57,6 +57,7 @@ public class TestInputFileBuilder { private final int id; private final String relativePath; + private String oldRelativePath; private final String projectKey; @CheckForNull private Path projectBaseDir; @@ -101,6 +102,14 @@ public class TestInputFileBuilder { this.id = id; } + public TestInputFileBuilder(String projectKey, String relativePath, String oldRelativePath, int id) { + this.projectKey = projectKey; + setModuleBaseDir(Paths.get(projectKey)); + this.relativePath = PathUtils.sanitize(relativePath); + this.oldRelativePath = oldRelativePath; + this.id = id; + } + public static TestInputFileBuilder create(String moduleKey, File moduleBaseDir, File filePath) { return new TestInputFileBuilder(moduleKey, moduleBaseDir, filePath); } @@ -218,7 +227,7 @@ public class TestInputFileBuilder { projectBaseDir = moduleBaseDir; } String projectRelativePath = projectBaseDir.relativize(absolutePath).toString(); - DefaultIndexedFile indexedFile = new DefaultIndexedFile(absolutePath, projectKey, projectRelativePath, relativePath, type, language, id, new SensorStrategy()); + DefaultIndexedFile indexedFile = new DefaultIndexedFile(absolutePath, projectKey, projectRelativePath, relativePath, type, language, id, new SensorStrategy(), oldRelativePath); DefaultInputFile inputFile = new DefaultInputFile(indexedFile, f -> f.setMetadata(new Metadata(lines, nonBlankLines, hash, originalLineStartOffsets, originalLineEndOffsets, lastValidOffset)), contents); diff --git a/sonar-plugin-api-impl/src/test/java/org/sonar/api/batch/fs/internal/fs/DefaultInputFileTest.java b/sonar-plugin-api-impl/src/test/java/org/sonar/api/batch/fs/internal/fs/DefaultInputFileTest.java index 62333075385..4117f881c7e 100644 --- a/sonar-plugin-api-impl/src/test/java/org/sonar/api/batch/fs/internal/fs/DefaultInputFileTest.java +++ b/sonar-plugin-api-impl/src/test/java/org/sonar/api/batch/fs/internal/fs/DefaultInputFileTest.java @@ -55,6 +55,7 @@ public class DefaultInputFileTest { private static final String PROJECT_RELATIVE_PATH = "module1/src/Foo.php"; private static final String MODULE_RELATIVE_PATH = "src/Foo.php"; + private static final String OLD_RELATIVE_PATH = "src/previous/Foo.php"; @Rule public TemporaryFolder temp = new TemporaryFolder(); @@ -102,11 +103,23 @@ public class DefaultInputFileTest { } @Test + public void test_moved_file() { + DefaultIndexedFile indexedFileForMovedFile = new DefaultIndexedFile(baseDir.resolve(PROJECT_RELATIVE_PATH), "ABCDE", PROJECT_RELATIVE_PATH, MODULE_RELATIVE_PATH, InputFile.Type.TEST, "php", 0, + sensorStrategy, OLD_RELATIVE_PATH); + Metadata metadata = new Metadata(42, 42, "", new int[0], new int[0], 10); + DefaultInputFile inputFile = new DefaultInputFile(indexedFileForMovedFile, (f) -> f.setMetadata(metadata)) + .setStatus(InputFile.Status.ADDED) + .setCharset(StandardCharsets.ISO_8859_1); + + assertThat(inputFile.oldRelativePath()).isEqualTo(OLD_RELATIVE_PATH); + } + + @Test public void test_content() throws IOException { Path testFile = baseDir.resolve(PROJECT_RELATIVE_PATH); Files.createDirectories(testFile.getParent()); String content = "test é string"; - Files.write(testFile, content.getBytes(StandardCharsets.ISO_8859_1)); + Files.writeString(testFile, content, StandardCharsets.ISO_8859_1); assertThat(Files.readAllLines(testFile, StandardCharsets.ISO_8859_1).get(0)).hasSize(content.length()); @@ -171,12 +184,12 @@ public class DefaultInputFileTest { @Test public void test_toString() { DefaultInputFile file = new DefaultInputFile(new DefaultIndexedFile("ABCDE", Paths.get("module"), MODULE_RELATIVE_PATH, null), (f) -> mock(Metadata.class)); - assertThat(file.toString()).isEqualTo(MODULE_RELATIVE_PATH); + assertThat(file).hasToString(MODULE_RELATIVE_PATH); } @Test public void checkValidPointer() { - Metadata metadata = new Metadata(2, 2, "", new int[] {0, 10}, new int[] {9, 15}, 16); + Metadata metadata = new Metadata(2, 2, "", new int[]{0, 10}, new int[]{9, 15}, 16); DefaultInputFile file = new DefaultInputFile(new DefaultIndexedFile("ABCDE", Paths.get("module"), MODULE_RELATIVE_PATH, null), f -> f.setMetadata(metadata)); assertThat(file.newPointer(1, 0).line()).isOne(); assertThat(file.newPointer(1, 0).lineOffset()).isZero(); @@ -213,7 +226,7 @@ public class DefaultInputFileTest { @Test public void checkValidPointerUsingGlobalOffset() { - Metadata metadata = new Metadata(2, 2, "", new int[] {0, 10}, new int[] {8, 15}, 16); + Metadata metadata = new Metadata(2, 2, "", new int[]{0, 10}, new int[]{8, 15}, 16); DefaultInputFile file = new DefaultInputFile(new DefaultIndexedFile("ABCDE", Paths.get("module"), MODULE_RELATIVE_PATH, null), f -> f.setMetadata(metadata)); assertThat(file.newPointer(0).line()).isOne(); assertThat(file.newPointer(0).lineOffset()).isZero(); @@ -299,7 +312,7 @@ public class DefaultInputFileTest { @Test public void checkValidRangeUsingGlobalOffset() { - Metadata metadata = new Metadata(2, 2, "", new int[] {0, 10}, new int[] {9, 15}, 16); + Metadata metadata = new Metadata(2, 2, "", new int[]{0, 10}, new int[]{9, 15}, 16); DefaultInputFile file = new DefaultInputFile(new DefaultIndexedFile("ABCDE", Paths.get("module"), MODULE_RELATIVE_PATH, null), f -> f.setMetadata(metadata)); TextRange newRange = file.newRange(10, 13); assertThat(newRange.start().line()).isEqualTo(2); @@ -310,7 +323,7 @@ public class DefaultInputFileTest { @Test public void testRangeOverlap() { - Metadata metadata = new Metadata(2, 2, "", new int[] {0, 10}, new int[] {9, 15}, 16); + Metadata metadata = new Metadata(2, 2, "", new int[]{0, 10}, new int[]{9, 15}, 16); DefaultInputFile file = new DefaultInputFile(new DefaultIndexedFile("ABCDE", Paths.get("module"), MODULE_RELATIVE_PATH, null), f -> f.setMetadata(metadata)); // Don't fail assertThat(file.newRange(file.newPointer(1, 0), file.newPointer(1, 1)).overlap(file.newRange(file.newPointer(1, 0), file.newPointer(1, 1)))).isTrue(); diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ChangedLinesPublisher.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ChangedLinesPublisher.java index 56737d1e068..1d434618862 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ChangedLinesPublisher.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ChangedLinesPublisher.java @@ -20,12 +20,13 @@ package org.sonar.scanner.report; import java.nio.file.Path; +import java.util.Collection; import java.util.Collections; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; import java.util.stream.StreamSupport; +import javax.annotation.CheckForNull; import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.batch.fs.internal.DefaultInputProject; import org.sonar.api.batch.scm.ScmProvider; @@ -39,9 +40,12 @@ import org.sonar.scanner.repository.ReferenceBranchSupplier; import org.sonar.scanner.scan.branch.BranchConfiguration; import org.sonar.scanner.scan.filesystem.InputComponentStore; import org.sonar.scanner.scm.ScmConfiguration; +import org.sonar.scm.git.ChangedFile; import org.sonar.scm.git.GitScmProvider; import static java.util.Optional.empty; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; public class ChangedLinesPublisher implements ReportPublisherStep { private static final Logger LOG = Loggers.get(ChangedLinesPublisher.class); @@ -88,9 +92,9 @@ public class ChangedLinesPublisher implements ReportPublisherStep { private int writeChangedLines(ScmProvider provider, ScannerReportWriter writer, String targetScmBranch) { Path rootBaseDir = project.getBaseDir(); Map<Path, DefaultInputFile> changedFiles = StreamSupport.stream(inputComponentStore.allChangedFilesToPublish().spliterator(), false) - .collect(Collectors.toMap(DefaultInputFile::path, f -> f)); + .collect(toMap(DefaultInputFile::path, identity())); - Map<Path, Set<Integer>> pathSetMap = ((GitScmProvider) provider).branchChangedLines(targetScmBranch, rootBaseDir, changedFiles); // TODO: Extend ScmProvider abstract + Map<Path, Set<Integer>> pathSetMap = getBranchChangedLinesByScm(provider, targetScmBranch, rootBaseDir, toChangedFilesByPathMap(changedFiles.values())); int count = 0; if (pathSetMap == null) { @@ -126,4 +130,20 @@ public class ChangedLinesPublisher implements ReportPublisherStep { builder.addAllLine(changedLines); writer.writeComponentChangedLines(fileRef, builder.build()); } + + @CheckForNull + private static Map<Path, Set<Integer>> getBranchChangedLinesByScm(ScmProvider scmProvider, String targetScmBranch, Path rootBaseDir, Map<Path, ChangedFile> changedFiles) { + if (scmProvider instanceof GitScmProvider) { + return ((GitScmProvider) scmProvider).branchChangedLinesWithFileMovementDetection(targetScmBranch, rootBaseDir, changedFiles); + } + + return scmProvider.branchChangedLines(targetScmBranch, rootBaseDir, changedFiles.keySet()); + } + + private static Map<Path, ChangedFile> toChangedFilesByPathMap(Collection<DefaultInputFile> files) { + return files + .stream() + .map(ChangedFile::of) + .collect(toMap(ChangedFile::getAbsolutFilePath, identity())); + } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ComponentsPublisher.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ComponentsPublisher.java index b21da5f2f94..404d646e191 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ComponentsPublisher.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ComponentsPublisher.java @@ -68,8 +68,9 @@ public class ComponentsPublisher implements ReportPublisherStep { fileBuilder.setStatus(convert(file.status())); fileBuilder.setMarkedAsUnchanged(file.isMarkedAsUnchanged()); - if (file.isMovedFile()) { - fileBuilder.setOldRelativeFilePath(file.oldPath()); + String oldRelativePath = file.oldRelativePath(); + if (oldRelativePath != null) { + fileBuilder.setOldRelativeFilePath(oldRelativePath); } String lang = getLanguageKey(file); diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/FileIndexer.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/FileIndexer.java index 4653c24d171..f62617d166a 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/FileIndexer.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/FileIndexer.java @@ -136,7 +136,7 @@ public class FileIndexer { language != null ? language.key() : null, scannerComponentIdGenerator.getAsInt(), sensorStrategy, - scmChangedFiles.getFileOldPath(realAbsoluteFile) + scmChangedFiles.getOldRelativeFilePath(realAbsoluteFile) ); DefaultInputFile inputFile = new DefaultInputFile(indexedFile, f -> metadataGenerator.setMetadata(module.key(), f, module.getEncoding())); diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFiles.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFiles.java index 010f2de4cbf..a26f57ce2dd 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFiles.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFiles.java @@ -21,21 +21,26 @@ package org.sonar.scanner.scm; import java.nio.file.Path; import java.util.Collection; -import java.util.List; +import java.util.Map; import java.util.Optional; -import java.util.function.Predicate; import javax.annotation.CheckForNull; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; import org.sonar.scm.git.ChangedFile; +import static java.util.Collections.emptyMap; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; + @Immutable public class ScmChangedFiles { @Nullable private final Collection<ChangedFile> changedFiles; + private final Map<Path, ChangedFile> changedFilesByPath; public ScmChangedFiles(@Nullable Collection<ChangedFile> changedFiles) { this.changedFiles = changedFiles; + this.changedFilesByPath = toChangedFilesByPathMap(changedFiles); } public boolean isChanged(Path file) { @@ -43,7 +48,7 @@ public class ScmChangedFiles { throw new IllegalStateException("Scm didn't provide valid data"); } - return this.findFile(file).isPresent(); + return this.getChangedFile(file).isPresent(); } public boolean isValid() { @@ -56,20 +61,20 @@ public class ScmChangedFiles { } @CheckForNull - public String getFileOldPath(Path absoluteFilePath) { - return this.findFile(absoluteFilePath) - .filter(ChangedFile::isMoved) - .map(ChangedFile::getOldFilePath) + public String getOldRelativeFilePath(Path absoluteFilePath) { + return this.getChangedFile(absoluteFilePath) + .filter(ChangedFile::isMovedFile) + .map(ChangedFile::getOldRelativeFilePathReference) .orElse(null); } - private Optional<ChangedFile> findFile(Path absoluteFilePath) { - Predicate<ChangedFile> isTargetFile = file -> file.getAbsolutFilePath().equals(absoluteFilePath); + private Optional<ChangedFile> getChangedFile(Path absoluteFilePath) { + return Optional.ofNullable(changedFilesByPath.get(absoluteFilePath)); + } - return Optional.ofNullable(this.get()) - .orElseGet(List::of) - .stream() - .filter(isTargetFile) - .findFirst(); + private static Map<Path, ChangedFile> toChangedFilesByPathMap(@Nullable Collection<ChangedFile> changedFiles) { + return Optional.ofNullable(changedFiles) + .map(files -> files.stream().collect(toMap(ChangedFile::getAbsolutFilePath, identity()))) + .orElse(emptyMap()); } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java index 392996d7e74..4ec314a4566 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java @@ -22,9 +22,10 @@ package org.sonar.scanner.scm; import java.nio.file.Path; import java.util.Collection; import java.util.Set; -import java.util.stream.Collectors; import javax.annotation.CheckForNull; +import javax.annotation.Nullable; import org.sonar.api.batch.fs.internal.DefaultInputProject; +import org.sonar.api.batch.scm.ScmProvider; import org.sonar.api.impl.utils.ScannerUtils; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; @@ -34,6 +35,8 @@ import org.sonar.scm.git.ChangedFile; import org.sonar.scm.git.GitScmProvider; import org.springframework.context.annotation.Bean; +import static java.util.stream.Collectors.toSet; + public class ScmChangedFilesProvider { private static final Logger LOG = Loggers.get(ScmChangedFilesProvider.class); private static final String LOG_MSG = "SCM collecting changed files in the branch"; @@ -44,33 +47,33 @@ public class ScmChangedFilesProvider { Collection<ChangedFile> changedFiles = loadChangedFilesIfNeeded(scmConfiguration, branchConfiguration, rootBaseDir); if (changedFiles != null) { - validatePaths(getFilePaths(changedFiles)); + validatePaths(getAbsoluteFilePaths(changedFiles)); } return new ScmChangedFiles(changedFiles); } private static void validatePaths(Set<Path> changedFilePaths) { - if (changedFilePaths != null && changedFilePaths.stream().anyMatch(p -> !p.isAbsolute())) { + if (changedFilePaths.stream().anyMatch(p -> !p.isAbsolute())) { throw new IllegalStateException("SCM provider returned a changed file with a relative path but paths must be absolute. Please fix the provider."); } } - private static Set<Path> getFilePaths(Collection<ChangedFile> changedFiles) { + private static Set<Path> getAbsoluteFilePaths(Collection<ChangedFile> changedFiles) { return changedFiles .stream() .map(ChangedFile::getAbsolutFilePath) - .collect(Collectors.toSet()); + .collect(toSet()); } @CheckForNull private static Collection<ChangedFile> loadChangedFilesIfNeeded(ScmConfiguration scmConfiguration, BranchConfiguration branchConfiguration, Path rootBaseDir) { final String targetBranchName = branchConfiguration.targetBranchName(); if (branchConfiguration.isPullRequest() && targetBranchName != null) { - GitScmProvider scmProvider = (GitScmProvider) scmConfiguration.provider(); + ScmProvider scmProvider = scmConfiguration.provider(); if (scmProvider != null) { Profiler profiler = Profiler.create(LOG).startInfo(LOG_MSG); - Collection<ChangedFile> changedFiles = scmProvider.branchModifiedFiles(targetBranchName, rootBaseDir); + Collection<ChangedFile> changedFiles = getChangedFilesByScm(scmProvider, targetBranchName, rootBaseDir); profiler.stopInfo(); if (changedFiles != null) { LOG.debug("SCM reported {} {} changed in the branch", changedFiles.size(), ScannerUtils.pluralize("file", changedFiles.size())); @@ -83,4 +86,23 @@ public class ScmChangedFilesProvider { return null; } + private static Collection<ChangedFile> getChangedFilesByScm(ScmProvider scmProvider, String targetBranchName, Path rootBaseDir) { + if (scmProvider instanceof GitScmProvider) { + return ((GitScmProvider) scmProvider).branchChangedFilesWithFileMovementDetection(targetBranchName, rootBaseDir); + } + + return toChangedFiles(scmProvider.branchChangedFiles(targetBranchName, rootBaseDir)); + } + + @CheckForNull + private static Collection<ChangedFile> toChangedFiles(@Nullable Set<Path> changedPaths) { + if (changedPaths == null) { + return null; + } + + return changedPaths + .stream() + .map(ChangedFile::of) + .collect(toSet()); + } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/ChangedFile.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/ChangedFile.java index 6f30d24e744..15b711d1fcb 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/ChangedFile.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/ChangedFile.java @@ -23,37 +23,62 @@ import java.nio.file.Path; import java.util.Objects; import javax.annotation.CheckForNull; import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; +import org.sonar.api.batch.fs.internal.DefaultInputFile; +@Immutable public class ChangedFile { - @Nullable - private final String oldFilePath; - private final String filePath; private final Path absoluteFilePath; + private final String oldRelativeFilePathReference; - public ChangedFile(String filePath, Path absoluteFilePath) { - this(filePath, absoluteFilePath, null); + private ChangedFile(Path absoluteFilePath, @Nullable String oldRelativeFilePathReference) { + this.absoluteFilePath = absoluteFilePath; + this.oldRelativeFilePathReference = oldRelativeFilePathReference; } - public ChangedFile(String filePath, Path absoluteFilePath, @Nullable String oldFilePath) { - this.filePath = filePath; - this.oldFilePath = oldFilePath; - this.absoluteFilePath = absoluteFilePath; + public Path getAbsolutFilePath() { + return absoluteFilePath; } @CheckForNull - public String getOldFilePath() { - return oldFilePath; + public String getOldRelativeFilePathReference() { + return oldRelativeFilePathReference; } - public boolean isMoved() { - return Objects.nonNull(this.getOldFilePath()); + public boolean isMovedFile() { + return this.getOldRelativeFilePathReference() != null; } - public String getFilePath() { - return filePath; + public static ChangedFile of(Path path) { + return new ChangedFile(path, null); } - public Path getAbsolutFilePath() { - return absoluteFilePath; + public static ChangedFile of(Path path, @Nullable String oldRelativeFilePathReference) { + return new ChangedFile(path, oldRelativeFilePathReference); + } + + public static ChangedFile of(DefaultInputFile file) { + return new ChangedFile(file.path(), file.oldRelativePath()); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + + if (o == null || getClass() != o.getClass()) { + return false; + } + + ChangedFile that = (ChangedFile) o; + + return Objects.equals(oldRelativeFilePathReference, that.oldRelativeFilePathReference) + && Objects.equals(absoluteFilePath, that.absoluteFilePath); + } + + @Override + public int hashCode() { + return Objects.hash(oldRelativeFilePathReference, absoluteFilePath); } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitScmProvider.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitScmProvider.java index c51035ee8f1..bfcbe5f3726 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitScmProvider.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitScmProvider.java @@ -27,16 +27,15 @@ import java.time.Instant; import java.util.Arrays; import java.util.Collection; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; import java.util.regex.Pattern; -import java.util.stream.Collectors; import javax.annotation.CheckForNull; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.GitAPIException; @@ -62,7 +61,6 @@ import org.eclipse.jgit.treewalk.FileTreeIterator; import org.eclipse.jgit.treewalk.filter.PathFilter; import org.eclipse.jgit.treewalk.filter.PathFilterGroup; import org.eclipse.jgit.treewalk.filter.TreeFilter; -import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.batch.scm.BlameCommand; import org.sonar.api.batch.scm.ScmProvider; import org.sonar.api.notifications.AnalysisWarnings; @@ -71,6 +69,11 @@ import org.sonar.api.utils.System2; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; +import static java.lang.String.format; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; +import static java.util.stream.Collectors.toSet; +import static java.util.stream.Collectors.toUnmodifiableMap; import static org.eclipse.jgit.diff.DiffEntry.ChangeType.ADD; import static org.eclipse.jgit.diff.DiffEntry.ChangeType.MODIFY; import static org.eclipse.jgit.diff.DiffEntry.ChangeType.RENAME; @@ -79,6 +82,7 @@ public class GitScmProvider extends ScmProvider { private static final Logger LOG = Loggers.get(GitScmProvider.class); private static final String COULD_NOT_FIND_REF = "Could not find ref '%s' in refs/heads, refs/remotes, refs/remotes/upstream or refs/remotes/origin"; + private static final String NO_MERGE_BASE_FOUND_MESSAGE = "No merge base found between HEAD and %s"; private final BlameCommand blameCommand; private final AnalysisWarnings analysisWarnings; private final GitIgnoreCommand gitIgnoreCommand; @@ -90,7 +94,7 @@ public class GitScmProvider extends ScmProvider { this.analysisWarnings = analysisWarnings; this.gitIgnoreCommand = gitIgnoreCommand; this.system2 = system2; - this.documentationLink = "/documentation/analysis/scm-integration/"; + this.documentationLink = "/documentation/analysis/scm-integration/"; } @Override @@ -117,48 +121,13 @@ public class GitScmProvider extends ScmProvider { @CheckForNull @Override public Set<Path> branchChangedFiles(String targetBranchName, Path rootBaseDir) { - try (Repository repo = buildRepo(rootBaseDir)) { - Ref targetRef = resolveTargetRef(targetBranchName, repo); - if (targetRef == null) { - addWarningTargetNotFound(targetBranchName); - return null; - } - - if (isDiffAlgoInvalid(repo.getConfig())) { - LOG.warn("The diff algorithm configured in git is not supported. " - + "No information regarding changes in the branch will be collected, which can lead to unexpected results."); - return null; - } - - Optional<RevCommit> mergeBaseCommit = findMergeBase(repo, targetRef); - if (!mergeBaseCommit.isPresent()) { - LOG.warn("No merge base found between HEAD and " + targetRef.getName()); - return null; - } - AbstractTreeIterator mergeBaseTree = prepareTreeParser(repo, mergeBaseCommit.get()); - - // we compare a commit with HEAD, so no point ignoring line endings (it will be whatever is committed) - try (Git git = newGit(repo)) { - List<DiffEntry> diffEntries = git.diff() - .setShowNameAndStatusOnly(true) - .setOldTree(mergeBaseTree) - .setNewTree(prepareNewTree(repo)) - .call(); - - return diffEntries.stream() - .filter(diffEntry -> diffEntry.getChangeType() == DiffEntry.ChangeType.ADD || diffEntry.getChangeType() == DiffEntry.ChangeType.MODIFY) - .map(diffEntry -> repo.getWorkTree().toPath().resolve(diffEntry.getNewPath())) - .collect(Collectors.toSet()); - } - } catch (IOException | GitAPIException e) { - LOG.warn(e.getMessage(), e); - } - return null; + return Optional.ofNullable((branchChangedFilesWithFileMovementDetection(targetBranchName, rootBaseDir))) + .map(GitScmProvider::extractAbsoluteFilePaths) + .orElse(null); } - // TODO: Adjust ScmProvider abstract @CheckForNull - public Collection<ChangedFile> branchModifiedFiles(String targetBranchName, Path rootBaseDir) { + public Collection<ChangedFile> branchChangedFilesWithFileMovementDetection(String targetBranchName, Path rootBaseDir) { try (Repository repo = buildRepo(rootBaseDir)) { Ref targetRef = resolveTargetRef(targetBranchName, repo); if (targetRef == null) { @@ -174,7 +143,7 @@ public class GitScmProvider extends ScmProvider { Optional<RevCommit> mergeBaseCommit = findMergeBase(repo, targetRef); if (mergeBaseCommit.isEmpty()) { - LOG.warn("No merge base found between HEAD and " + targetRef.getName()); + LOG.warn(composeNoMergeBaseFoundWarning(targetRef.getName())); return null; } AbstractTreeIterator mergeBaseTree = prepareTreeParser(repo, mergeBaseCommit.get()); @@ -201,11 +170,12 @@ public class GitScmProvider extends ScmProvider { Map<String, String> renamedFilePaths = computeRenamedFilePaths(repository, diffEntries); Set<String> changedFilePaths = computeChangedFilePaths(diffEntries); - List<ChangedFile> changedFiles = new LinkedList<>(); - - Consumer<String> collectChangedFiles = filePath -> changedFiles.add(new ChangedFile(filePath, workingDirectory.resolve(filePath), renamedFilePaths.getOrDefault(filePath, null))); - changedFilePaths.forEach(collectChangedFiles); + return collectChangedFiles(workingDirectory, renamedFilePaths, changedFilePaths); + } + private static List<ChangedFile> collectChangedFiles(Path workingDirectory, Map<String, String> renamedFilePaths, Set<String> changedFilePaths) { + List<ChangedFile> changedFiles = new LinkedList<>(); + changedFilePaths.forEach(filePath -> changedFiles.add(ChangedFile.of(workingDirectory.resolve(filePath), renamedFilePaths.get(filePath)))); return changedFiles; } @@ -217,7 +187,7 @@ public class GitScmProvider extends ScmProvider { .compute() .stream() .filter(entry -> RENAME.equals(entry.getChangeType())) - .collect(Collectors.toUnmodifiableMap(DiffEntry::getNewPath, DiffEntry::getOldPath)); + .collect(toUnmodifiableMap(DiffEntry::getNewPath, DiffEntry::getOldPath)); } private static Set<String> computeChangedFilePaths(List<DiffEntry> diffEntries) { @@ -225,10 +195,10 @@ public class GitScmProvider extends ScmProvider { .stream() .filter(isAllowedChangeType(ADD, MODIFY)) .map(DiffEntry::getNewPath) - .collect(Collectors.toSet()); + .collect(toSet()); } - private static Predicate<DiffEntry> isAllowedChangeType(ChangeType ...changeTypes) { + private static Predicate<DiffEntry> isAllowedChangeType(ChangeType... changeTypes) { Function<ChangeType, Predicate<DiffEntry>> isChangeType = type -> entry -> type.equals(entry.getChangeType()); return Arrays @@ -240,41 +210,11 @@ public class GitScmProvider extends ScmProvider { @CheckForNull @Override public Map<Path, Set<Integer>> branchChangedLines(String targetBranchName, Path projectBaseDir, Set<Path> changedFiles) { - try (Repository repo = buildRepo(projectBaseDir)) { - Ref targetRef = resolveTargetRef(targetBranchName, repo); - if (targetRef == null) { - addWarningTargetNotFound(targetBranchName); - return null; - } - - if (isDiffAlgoInvalid(repo.getConfig())) { - // we already print a warning when branchChangedFiles is called - return null; - } - - // force ignore different line endings when comparing a commit with the workspace - repo.getConfig().setBoolean("core", null, "autocrlf", true); - - Optional<RevCommit> mergeBaseCommit = findMergeBase(repo, targetRef); - if (!mergeBaseCommit.isPresent()) { - LOG.warn("No merge base found between HEAD and " + targetRef.getName()); - return null; - } - - Map<Path, Set<Integer>> changedLines = new HashMap<>(); - - for (Path path : changedFiles) { - collectChangedLines(repo, mergeBaseCommit.get(), changedLines, path); - } - return changedLines; - } catch (Exception e) { - LOG.warn("Failed to get changed lines from git", e); - } - return null; + return branchChangedLinesWithFileMovementDetection(targetBranchName, projectBaseDir, toChangedFileByPathsMap(changedFiles)); } - // TODO: Adjust ScmProvider abstract - public Map<Path, Set<Integer>> branchChangedLines(String targetBranchName, Path projectBaseDir, Map<Path, DefaultInputFile> changedFiles) { + @CheckForNull + public Map<Path, Set<Integer>> branchChangedLinesWithFileMovementDetection(String targetBranchName, Path projectBaseDir, Map<Path, ChangedFile> changedFiles) { try (Repository repo = buildRepo(projectBaseDir)) { Ref targetRef = resolveTargetRef(targetBranchName, repo); if (targetRef == null) { @@ -293,13 +233,13 @@ public class GitScmProvider extends ScmProvider { Optional<RevCommit> mergeBaseCommit = findMergeBase(repo, targetRef); if (mergeBaseCommit.isEmpty()) { - LOG.warn("No merge base found between HEAD and " + targetRef.getName()); + LOG.warn(composeNoMergeBaseFoundWarning(targetRef.getName())); return null; } Map<Path, Set<Integer>> changedLines = new HashMap<>(); - for (Map.Entry<Path, DefaultInputFile> entry : changedFiles.entrySet()) { + for (Map.Entry<Path, ChangedFile> entry : changedFiles.entrySet()) { collectChangedLines(repo, mergeBaseCommit.get(), changedLines, entry.getKey(), entry.getValue()); } @@ -307,56 +247,33 @@ public class GitScmProvider extends ScmProvider { } catch (Exception e) { LOG.warn("Failed to get changed lines from git", e); } + return null; } + private static String composeNoMergeBaseFoundWarning(String targetRef) { + return format(NO_MERGE_BASE_FOUND_MESSAGE, targetRef); + } + private void addWarningTargetNotFound(String targetBranchName) { - analysisWarnings.addUnique(String.format(COULD_NOT_FIND_REF + analysisWarnings.addUnique(format(COULD_NOT_FIND_REF + ". You may see unexpected issues and changes. " + "Please make sure to fetch this ref before pull request analysis and refer to" + " <a href=\"%s\" target=\"_blank\">the documentation</a>.", targetBranchName, documentationLink)); } - private void collectChangedLines(Repository repo, RevCommit mergeBaseCommit, Map<Path, Set<Integer>> changedLines, Path changedFile) { + private void collectChangedLines(Repository repo, RevCommit mergeBaseCommit, Map<Path, Set<Integer>> changedLines, Path changedFilePath, ChangedFile changedFile) { ChangedLinesComputer computer = new ChangedLinesComputer(); try (DiffFormatter diffFmt = new DiffFormatter(new BufferedOutputStream(computer.receiver()))) { - // copied from DiffCommand so that we can use a custom DiffFormatter which ignores white spaces. diffFmt.setRepository(repo); diffFmt.setProgressMonitor(NullProgressMonitor.INSTANCE); diffFmt.setDiffComparator(RawTextComparator.WS_IGNORE_ALL); - Path workTree = repo.getWorkTree().toPath(); - String relativePath = workTree.relativize(changedFile).toString(); - PathFilter pathFilter = PathFilter.create(toGitPath(relativePath)); - diffFmt.setPathFilter(pathFilter); - - AbstractTreeIterator mergeBaseTree = prepareTreeParser(repo, mergeBaseCommit); - List<DiffEntry> diffEntries = diffFmt.scan(mergeBaseTree, new FileTreeIterator(repo)); - diffFmt.format(diffEntries); - diffFmt.flush(); - diffEntries.stream() - .filter(diffEntry -> diffEntry.getChangeType() == DiffEntry.ChangeType.ADD || diffEntry.getChangeType() == DiffEntry.ChangeType.MODIFY) - .findAny() - .ifPresent(diffEntry -> changedLines.put(changedFile, computer.changedLines())); - } catch (Exception e) { - LOG.warn("Failed to get changed lines from git for file " + changedFile, e); - } - } - - // TODO: Adjust ScmProvider abstract - private void collectChangedLines(Repository repo, RevCommit mergeBaseCommit, Map<Path, Set<Integer>> changedLines, Path changedFilePath, DefaultInputFile changedFileData) { - ChangedLinesComputer computer = new ChangedLinesComputer(); - - try (DiffFormatter diffFmt = new DiffFormatter(new BufferedOutputStream(computer.receiver()))) { - diffFmt.setRepository(repo); - diffFmt.setProgressMonitor(NullProgressMonitor.INSTANCE); - diffFmt.setDiffComparator(RawTextComparator.WS_IGNORE_ALL); - - diffFmt.setDetectRenames(changedFileData.isMovedFile()); + diffFmt.setDetectRenames(changedFile.isMovedFile()); Path workTree = repo.getWorkTree().toPath(); - TreeFilter treeFilter = getTreeFilter(changedFileData, workTree); + TreeFilter treeFilter = getTreeFilter(changedFile, workTree); diffFmt.setPathFilter(treeFilter); AbstractTreeIterator mergeBaseTree = prepareTreeParser(repo, mergeBaseCommit); @@ -384,17 +301,24 @@ public class GitScmProvider extends ScmProvider { return path.replaceAll(Pattern.quote(File.separator), "/"); } - private TreeFilter getTreeFilter(DefaultInputFile changedFile, Path baseDir) { - String oldPath = toGitPath(changedFile.oldPath()); - String path = toGitPath(relativizeFilePath(baseDir, changedFile.path())); + private static TreeFilter getTreeFilter(ChangedFile changedFile, Path baseDir) { + String path = toGitPath(relativizeFilePath(baseDir, changedFile.getAbsolutFilePath())); + String oldRelativePath = changedFile.getOldRelativeFilePathReference(); - if (changedFile.isMovedFile()) { - return PathFilterGroup.createFromStrings(path, oldPath); + if (oldRelativePath != null) { + return PathFilterGroup.createFromStrings(path, toGitPath(oldRelativePath)); } return PathFilter.create(path); } + private static Set<Path> extractAbsoluteFilePaths(Collection<ChangedFile> changedFiles) { + return changedFiles + .stream() + .map(ChangedFile::getAbsolutFilePath) + .collect(toSet()); + } + @CheckForNull private Ref resolveTargetRef(String targetBranchName, Repository repo) throws IOException { String localRef = "refs/heads/" + targetBranchName; @@ -506,6 +430,12 @@ public class GitScmProvider extends ScmProvider { } } + private static Map<Path, ChangedFile> toChangedFileByPathsMap(Set<Path> changedFiles) { + return changedFiles + .stream() + .collect(toMap(identity(), ChangedFile::of, (x, y) -> y, LinkedHashMap::new)); + } + private static String relativizeFilePath(Path baseDirectory, Path filePath) { return baseDirectory.relativize(filePath).toString(); } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ChangedLinesPublisherTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ChangedLinesPublisherTest.java index 0c6e5d2c2cc..52cb1d1bd71 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ChangedLinesPublisherTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ChangedLinesPublisherTest.java @@ -43,10 +43,15 @@ import org.sonar.scanner.repository.ReferenceBranchSupplier; import org.sonar.scanner.scan.branch.BranchConfiguration; import org.sonar.scanner.scan.filesystem.InputComponentStore; import org.sonar.scanner.scm.ScmConfiguration; +import org.sonar.scm.git.GitScmProvider; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assumptions.assumeThat; +import static org.mockito.ArgumentMatchers.anyMap; +import static org.mockito.ArgumentMatchers.anySet; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -54,14 +59,14 @@ public class ChangedLinesPublisherTest { private static final String TARGET_BRANCH = "target"; private static final Path BASE_DIR = Paths.get("/root"); - private ScmConfiguration scmConfiguration = mock(ScmConfiguration.class); - private InputModuleHierarchy inputModuleHierarchy = mock(InputModuleHierarchy.class); - private InputComponentStore inputComponentStore = mock(InputComponentStore.class); - private BranchConfiguration branchConfiguration = mock(BranchConfiguration.class); - private ReferenceBranchSupplier referenceBranchSupplier = mock(ReferenceBranchSupplier.class); + private final ScmConfiguration scmConfiguration = mock(ScmConfiguration.class); + private final InputModuleHierarchy inputModuleHierarchy = mock(InputModuleHierarchy.class); + private final InputComponentStore inputComponentStore = mock(InputComponentStore.class); + private final BranchConfiguration branchConfiguration = mock(BranchConfiguration.class); + private final ReferenceBranchSupplier referenceBranchSupplier = mock(ReferenceBranchSupplier.class); private ScannerReportWriter writer; - private ScmProvider provider = mock(ScmProvider.class); - private DefaultInputProject project = mock(DefaultInputProject.class); + private final ScmProvider provider = mock(ScmProvider.class); + private final DefaultInputProject project = mock(DefaultInputProject.class); @Rule public TemporaryFolder temp = new TemporaryFolder(); @@ -69,7 +74,7 @@ public class ChangedLinesPublisherTest { @Rule public LogTester logTester = new LogTester(); - private ChangedLinesPublisher publisher = new ChangedLinesPublisher(scmConfiguration, project, inputComponentStore, branchConfiguration, referenceBranchSupplier); + private final ChangedLinesPublisher publisher = new ChangedLinesPublisher(scmConfiguration, project, inputComponentStore, branchConfiguration, referenceBranchSupplier); @Before public void setUp() { @@ -141,6 +146,19 @@ public class ChangedLinesPublisherTest { } @Test + public void write_changed_file_with_GitScmProvider() { + GitScmProvider provider = mock(GitScmProvider.class); + when(scmConfiguration.provider()).thenReturn(provider); + Set<Integer> lines = new HashSet<>(Arrays.asList(1, 10)); + when(provider.branchChangedLines(eq(TARGET_BRANCH), eq(BASE_DIR), anySet())) + .thenReturn(ImmutableMap.of(BASE_DIR.resolve("path1"), lines, BASE_DIR.resolve("path3"), Collections.emptySet())); + + publisher.publish(writer); + + verify(provider).branchChangedLinesWithFileMovementDetection(eq(TARGET_BRANCH), eq(BASE_DIR), anyMap()); + } + + @Test public void write_last_line_as_changed_if_all_other_lines_are_changed_and_last_line_is_empty() { DefaultInputFile fileWithChangedLines = createInputFile("path1", "l1\nl2\nl3\n"); DefaultInputFile fileWithoutChangedLines = createInputFile("path2", "l1\nl2\nl3\n"); @@ -156,7 +174,7 @@ public class ChangedLinesPublisherTest { } @Test - public void dont_write_last_line_as_changed_if_its_not_empty() { + public void do_not_write_last_line_as_changed_if_its_not_empty() { DefaultInputFile fileWithChangedLines = createInputFile("path1", "l1\nl2\nl3\nl4"); DefaultInputFile fileWithoutChangedLines = createInputFile("path2", "l1\nl2\nl3\nl4"); Set<Path> paths = new HashSet<>(Arrays.asList(BASE_DIR.resolve("path1"), BASE_DIR.resolve("path2"))); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ComponentsPublisherTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ComponentsPublisherTest.java index 97d98920dfa..c6ae8dc010c 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ComponentsPublisherTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ComponentsPublisherTest.java @@ -31,11 +31,11 @@ import org.sonar.api.SonarRuntime; import org.sonar.api.batch.bootstrap.ProjectDefinition; import org.sonar.api.batch.fs.InputFile; import org.sonar.api.batch.fs.InputFile.Type; -import org.sonar.api.utils.DateUtils; -import org.sonar.scanner.ProjectInfo; import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.batch.fs.internal.DefaultInputProject; import org.sonar.api.batch.fs.internal.TestInputFileBuilder; +import org.sonar.api.utils.DateUtils; +import org.sonar.scanner.ProjectInfo; import org.sonar.scanner.protocol.output.FileStructure; import org.sonar.scanner.protocol.output.ScannerReport.Component; import org.sonar.scanner.protocol.output.ScannerReport.Component.FileStatus; @@ -107,6 +107,9 @@ public class ComponentsPublisherTest { DefaultInputFile testFile = new TestInputFileBuilder("foo", "module1/test/FooTest.java", 7).setType(Type.TEST).setStatus(InputFile.Status.ADDED).setLines(4).build(); store.put("module1", testFile); + DefaultInputFile movedFile = new TestInputFileBuilder("foo", "module1/src/MovedFile.java", "module0/src/MovedFile.java", 9).setStatus(InputFile.Status.CHANGED).setLines(4).build(); + store.put("module1", movedFile); + ComponentsPublisher publisher = new ComponentsPublisher(project, store); publisher.publish(writer); @@ -114,6 +117,7 @@ public class ComponentsPublisherTest { assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 4)).isTrue(); assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 6)).isTrue(); assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 7)).isTrue(); + assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 9)).isTrue(); // not marked for publishing assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 5)).isFalse(); @@ -126,6 +130,9 @@ public class ComponentsPublisherTest { assertThat(rootProtobuf.getDescription()).isEqualTo("Root description"); assertThat(rootProtobuf.getLinkCount()).isZero(); + Component movedFileProtobuf = reader.readComponent(9); + assertThat(movedFileProtobuf.getOldRelativeFilePath()).isEqualTo("module0/src/MovedFile.java"); + assertThat(reader.readComponent(4).getStatus()).isEqualTo(FileStatus.SAME); assertThat(reader.readComponent(6).getStatus()).isEqualTo(FileStatus.CHANGED); assertThat(reader.readComponent(7).getStatus()).isEqualTo(FileStatus.ADDED); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java index 94629f59b60..cc3c7ff31e9 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java @@ -65,7 +65,7 @@ public class StatusDetectionTest { @Test public void detect_status_branches_confirm() { Path filePath = Paths.get("module", "src", "Foo.java"); - ScmChangedFiles changedFiles = new ScmChangedFiles(List.of(new ChangedFile(filePath.toString(), filePath))); + ScmChangedFiles changedFiles = new ScmChangedFiles(List.of(ChangedFile.of(filePath))); StatusDetection statusDetection = new StatusDetection(projectRepositories, changedFiles); assertThat(statusDetection.status("foo", createFile("src/Foo.java"), "XXXXX")).isEqualTo(InputFile.Status.CHANGED); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java index abbc78a3819..bf3092a55e7 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java @@ -28,9 +28,9 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.sonar.api.batch.fs.internal.DefaultInputProject; import org.sonar.api.batch.scm.ScmProvider; -import org.sonar.scanner.fs.InputModuleHierarchy; import org.sonar.scanner.scan.branch.BranchConfiguration; import org.sonar.scm.git.ChangedFile; +import org.sonar.scm.git.GitScmProvider; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -45,13 +45,12 @@ public class ScmChangedFilesProviderTest { @Mock private BranchConfiguration branchConfiguration; @Mock - private InputModuleHierarchy inputModuleHierarchy; - @Mock private ScmProvider scmProvider; - private Path rootBaseDir = Paths.get("root"); + private final Path rootBaseDir = Paths.get("root"); + private final DefaultInputProject project = mock(DefaultInputProject.class); + private ScmChangedFilesProvider provider; - private DefaultInputProject project = mock(DefaultInputProject.class); @Before public void setUp() { @@ -124,6 +123,21 @@ public class ScmChangedFilesProviderTest { } @Test + public void testGitScmProvider(){ + GitScmProvider gitScmProvider = mock(GitScmProvider.class); + + when(scmConfiguration.provider()).thenReturn(gitScmProvider); + when(branchConfiguration.isPullRequest()).thenReturn(true); + when(branchConfiguration.targetBranchName()).thenReturn("target"); + + ScmChangedFiles scmChangedFiles = provider.provide(scmConfiguration, branchConfiguration, project); + + assertThat(scmChangedFiles.get()).isEmpty(); + verify(scmConfiguration).provider(); + + } + + @Test public void testReturnChangedFiles() { when(branchConfiguration.targetBranchName()).thenReturn("target"); when(branchConfiguration.isPullRequest()).thenReturn(true); @@ -132,7 +146,7 @@ public class ScmChangedFilesProviderTest { ScmChangedFiles scmChangedFiles = provider.provide(scmConfiguration, branchConfiguration, project); Path filePath = Paths.get("changedFile").toAbsolutePath(); - ChangedFile changedFile = new ChangedFile(filePath.toString(), filePath); + ChangedFile changedFile = ChangedFile.of(filePath); assertThat(scmChangedFiles.get()).containsOnly(changedFile); verify(scmProvider).branchChangedFiles("target", rootBaseDir); } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesTest.java index 856d0573124..766c8ec8457 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesTest.java @@ -35,7 +35,7 @@ public class ScmChangedFilesTest { @Test public void testGetter() { Path filePath = Paths.get("files"); - ChangedFile file = new ChangedFile(filePath.toString(), filePath); + ChangedFile file = ChangedFile.of(filePath); Collection<ChangedFile> files = Collections.singletonList(file); scmChangedFiles = new ScmChangedFiles(files); assertThat(scmChangedFiles.get()).containsOnly(file); @@ -54,7 +54,7 @@ public class ScmChangedFilesTest { @Test public void testConfirm() { Path filePath = Paths.get("files"); - Collection<ChangedFile> files = Collections.singletonList(new ChangedFile(filePath.toString(), filePath)); + Collection<ChangedFile> files = Collections.singletonList(ChangedFile.of(filePath)); scmChangedFiles = new ScmChangedFiles(files); assertThat(scmChangedFiles.isValid()).isTrue(); assertThat(scmChangedFiles.isChanged(Paths.get("files"))).isTrue(); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/ChangedFileTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/ChangedFileTest.java new file mode 100644 index 00000000000..99a73d8f0ba --- /dev/null +++ b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/ChangedFileTest.java @@ -0,0 +1,95 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.scm.git; + +import java.nio.file.Path; +import org.junit.Test; +import org.sonar.api.batch.fs.InputFile; +import org.sonar.api.batch.fs.internal.DefaultIndexedFile; +import org.sonar.api.batch.fs.internal.DefaultInputFile; +import org.sonar.api.batch.fs.internal.SensorStrategy; + +import static org.apache.commons.lang.RandomStringUtils.random; +import static org.apache.commons.lang.RandomStringUtils.randomNumeric; +import static org.assertj.core.api.Assertions.assertThat; + + +public class ChangedFileTest { + + @Test + public void test_unMovedFile(){ + Path absolutePath = Path.of("/absolutePath"); + ChangedFile changedFile = ChangedFile.of(absolutePath); + + assertThat(changedFile.getAbsolutFilePath()).isSameAs(absolutePath); + assertThat(changedFile.getOldRelativeFilePathReference()).isNull(); + assertThat(changedFile.isMovedFile()).isFalse(); + } + + @Test + public void test_movedFile(){ + Path absolutePath = Path.of("/absolutePath"); + ChangedFile changedFile = ChangedFile.of(absolutePath, "/oldRelativePath"); + + assertThat(changedFile.getAbsolutFilePath()).isSameAs(absolutePath); + assertThat(changedFile.getOldRelativeFilePathReference()).isEqualTo("/oldRelativePath"); + assertThat(changedFile.isMovedFile()).isTrue(); + } + + @Test + public void test_equalsAndHashCode(){ + Path absolutePath = Path.of("/absolutePath"); + ChangedFile changedFile1 = ChangedFile.of(absolutePath, "/oldRelativePath"); + ChangedFile changedFile2 = ChangedFile.of(absolutePath, "/oldRelativePath"); + + assertThat(changedFile1).isEqualTo(changedFile2).hasSameHashCodeAs(changedFile2); + } + + @Test + public void test_changed_file_is_created_properly_from_default_input_file() { + String oldRelativeReference = "old/relative/path"; + Path path = Path.of("absolute/path"); + DefaultInputFile file = composeDefaultInputFile(path, oldRelativeReference); + ChangedFile changedFile = ChangedFile.of(file); + + assertThat(changedFile.getAbsolutFilePath()).isEqualTo(path); + assertThat(changedFile.isMovedFile()).isTrue(); + assertThat(changedFile.getOldRelativeFilePathReference()).isEqualTo(oldRelativeReference); + } + + private DefaultInputFile composeDefaultInputFile(Path path, String oldRelativeReference) { + DefaultIndexedFile indexedFile = composeDefaultIndexFile(path, oldRelativeReference); + return new DefaultInputFile(indexedFile, f -> f.setPublished(true)); + } + + private DefaultIndexedFile composeDefaultIndexFile(Path path, String oldRelativePath) { + return new DefaultIndexedFile( + path, + random(5), + random(5), + random(5), + InputFile.Type.MAIN, + random(5), + Integer.parseInt(randomNumeric(5)), + new SensorStrategy(), + oldRelativePath); + } + +} diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitScmProviderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitScmProviderTest.java index a41f2a508b9..e75152cdb74 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitScmProviderTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitScmProviderTest.java @@ -61,9 +61,11 @@ import org.sonar.api.utils.System2; import org.sonar.api.utils.log.LogAndArguments; import org.sonar.api.utils.log.LogTester; +import static java.lang.String.format; import static java.util.Collections.emptySet; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.AssertionsForClassTypes.tuple; import static org.assertj.core.data.MapEntry.entry; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -206,6 +208,35 @@ public class GitScmProviderTest { } @Test + public void branchChangedFilesWithFileMovementDetection_correctly_detects_several_file_moves_in_pull_request_base_branch() throws IOException, GitAPIException { + String fileM1 = "file-m1.xoo"; + String newFileM1 = "new-file-m1.xoo"; + String fileM2 = "file-m2.xoo"; + String newFileM2 = "new-file-m2.xoo"; + + Path newFileM1AbsolutPath = worktree.resolve(newFileM1); + Path newFileM2AbsolutPath = worktree.resolve(newFileM2); + + createAndCommitFile(fileM1); + createAndCommitFile(fileM2); + + createBranch(); + + editLineOfFile(fileM1, 1); + commit(fileM1); + + moveAndCommitFile(fileM1, newFileM1); + moveAndCommitFile(fileM2, newFileM2); + + assertThat(newScmProvider().branchChangedFilesWithFileMovementDetection("master", worktree)) + .extracting(ChangedFile::getAbsolutFilePath, ChangedFile::getOldRelativeFilePathReference) + .containsExactlyInAnyOrder( + tuple(newFileM1AbsolutPath, fileM1), + tuple(newFileM2AbsolutPath, fileM2) + ); + } + + @Test public void branchChangedFiles_should_not_fail_with_patience_diff_algo() throws IOException { Path gitConfig = worktree.resolve(".git").resolve("config"); Files.write(gitConfig, "[diff]\nalgorithm = patience\n".getBytes(StandardCharsets.UTF_8)); @@ -260,7 +291,7 @@ public class GitScmProviderTest { assertThat(newScmProvider().branchChangedLines("master", worktree, changedFiles)) .containsOnly( entry(worktree.resolve("lao.txt"), new HashSet<>(Arrays.asList(2, 3, 11, 12, 13))), - entry(worktree.resolve("file-m1.xoo"), new HashSet<>(Arrays.asList(4))), + entry(worktree.resolve("file-m1.xoo"), new HashSet<>(List.of(4))), entry(worktree.resolve("file-b2.xoo"), new HashSet<>(Arrays.asList(1, 2, 3)))); assertThat(newScmProvider().branchChangedLines("master", worktree, Collections.singleton(worktree.resolve("nonexistent")))) @@ -388,6 +419,45 @@ public class GitScmProviderTest { } @Test + public void branchChangedLinesWithFileMovementDetection_detects_modified_lines_on_renamed_pr_files_compared_to_original_files_on_target_branch() throws GitAPIException, IOException { + String fileM1 = "file-m1.xoo"; + String newFileM1 = "new-file-m1.xoo"; + String fileM2 = "file-m2.xoo"; + String newFileM2 = "new-file-m2.xoo"; + + Path newFileM1AbsolutPath = worktree.resolve(newFileM1); + Path newFileM2AbsolutPath = worktree.resolve(newFileM2); + + createAndCommitFile(fileM1); + createAndCommitFile(fileM2); + + createBranch(); + + editLineOfFile(fileM1, 2); + commit(fileM1); + + editLineOfFile(fileM2, 1); + commit(fileM2); + + moveAndCommitFile(fileM1, newFileM1); + moveAndCommitFile(fileM2, newFileM2); + + Map<Path, ChangedFile> changedFiles = Map.of( + newFileM1AbsolutPath, ChangedFile.of(newFileM1AbsolutPath, fileM1), + newFileM2AbsolutPath, ChangedFile.of(newFileM2AbsolutPath, fileM2) + ); + + Map<Path, Set<Integer>> changedLines = newScmProvider().branchChangedLinesWithFileMovementDetection("master", + worktree.resolve("project1"), changedFiles); + + assertThat(changedLines) + .containsOnly( + entry(newFileM1AbsolutPath, Set.of(2)), + entry(newFileM2AbsolutPath, Set.of(1)) + ); + } + + @Test public void branchChangedFiles_when_git_work_tree_is_above_project_basedir() throws IOException, GitAPIException { git.branchCreate().setName("b1").call(); git.checkout().setName("b1").call(); @@ -422,7 +492,7 @@ public class GitScmProviderTest { git.branchCreate().setName("b1").setStartPoint(forkPoint.getName()).call(); git.checkout().setName("b1").call(); - String newFileContent = new String(Files.readAllBytes(filePath), StandardCharsets.UTF_8).replaceAll("\n", "\r\n"); + String newFileContent = Files.readString(filePath).replaceAll("\n", "\r\n"); Files.write(filePath, newFileContent.getBytes(StandardCharsets.UTF_8), StandardOpenOption.TRUNCATE_EXISTING); commit("file-m1.xoo"); @@ -531,7 +601,7 @@ public class GitScmProviderTest { } @Test - public void branchChangedFiles_should_throw_when_repo_nonexistent() throws IOException { + public void branchChangedFiles_should_throw_when_repo_nonexistent() { assertThatThrownBy(() -> newScmProvider().branchChangedFiles("master", temp.newFolder().toPath())) .isInstanceOf(MessageException.class) .hasMessageContaining("Not inside a Git work tree: "); @@ -612,8 +682,10 @@ public class GitScmProviderTest { public void branchChangedLines_omits_files_with_git_api_errors() throws IOException, GitAPIException { String f1 = "file-in-first-commit.xoo"; String f2 = "file2-in-first-commit.xoo"; + String f3 = "file3-in-first-commit.xoo"; createAndCommitFile(f2); + createAndCommitFile(f3); git.branchCreate().setName("b1").call(); git.checkout().setName("b1").call(); @@ -621,26 +693,30 @@ public class GitScmProviderTest { // both files modified addLineToFile(f1, 1); addLineToFile(f2, 2); + addLineToFile(f3, 3); commit(f1); commit(f2); + commit(f3); AtomicInteger callCount = new AtomicInteger(0); GitScmProvider provider = new GitScmProvider(mockCommand(), analysisWarnings, gitIgnoreCommand, system2) { @Override AbstractTreeIterator prepareTreeParser(Repository repo, RevCommit commit) throws IOException { - if (callCount.getAndIncrement() == 1) { + if (callCount.getAndIncrement() == 2) { throw new RuntimeException("error"); } return super.prepareTreeParser(repo, commit); } }; - Set<Path> changedFiles = new LinkedHashSet<>(); - changedFiles.add(worktree.resolve(f1)); - changedFiles.add(worktree.resolve(f2)); + + Set<Path> changedFiles = new LinkedHashSet<>(List.of(worktree.resolve(f1), worktree.resolve(f2), worktree.resolve(f3))); assertThat(provider.branchChangedLines("master", worktree, changedFiles)) - .isEqualTo(Collections.singletonMap(worktree.resolve(f1), Collections.singleton(1))); + .containsOnly( + entry(worktree.resolve(f1), Collections.singleton(1)), + entry(worktree.resolve(f2), Collections.singleton(2)) + ); } @Test @@ -798,6 +874,27 @@ public class GitScmProviderTest { git.commit().setAuthor(person).setCommitter(person).setMessage(relativePath).call(); } + private void createBranch() throws IOException, GitAPIException { + ObjectId forkPoint = git.getRepository().exactRef("HEAD").getObjectId(); + git.branchCreate().setName(BRANCH_NAME).setStartPoint(forkPoint.getName()).call(); + git.checkout().setName(BRANCH_NAME).call(); + } + + private void editLineOfFile(String relativePath, int lineNumber) throws IOException { + Path filePath = worktree.resolve(relativePath); + List<String> lines = Files.readAllLines(filePath); + lines.set(lineNumber - 1, randomizedLine(relativePath)); + Files.write(filePath, lines, StandardOpenOption.TRUNCATE_EXISTING); + } + + private void moveAndCommitFile(String filename, String newFilename) throws GitAPIException, IOException { + Path file = worktree.resolve(filename); + Files.move(file, file.resolveSibling(newFilename)); + git.add().addFilepattern(filename).setUpdate(true).call(); + git.add().addFilepattern(newFilename).call(); + git.commit().setAuthor("joe", "joe@example.com").setMessage(format("Move file from [%s] to [%s]", filename, newFilename)).call(); + } + private GitScmProvider newScmProvider() { return new GitScmProvider(mockCommand(), analysisWarnings, gitIgnoreCommand, system2); } |