From 5abe7f209af3621d1bd94ef15c9d43e93a056200 Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Wed, 20 Sep 2017 17:11:21 +0200 Subject: [PATCH] SONAR-9837 Detect files changed in the current branch with SCM --- .../api/batch/scm/ScmBranchProvider.java | 6 ++-- .../filesystem/DefaultModuleFileSystem.java | 13 ++++--- .../filesystem/SameInputFilePredicate.java | 30 +++++++--------- .../scan/filesystem/StatusDetection.java | 34 ++++++++++++++++--- .../scanner/scm/ScmChangedFilesProvider.java | 26 +++++++++++--- .../scan/filesystem/StatusDetectionTest.java | 24 +++++++++++++ .../scm/ScmChangedFilesProviderTest.java | 21 ++++++++++-- 7 files changed, 115 insertions(+), 39 deletions(-) diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmBranchProvider.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmBranchProvider.java index 5984182c010..1cb4c476702 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmBranchProvider.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmBranchProvider.java @@ -21,7 +21,7 @@ package org.sonar.api.batch.scm; import java.nio.file.Path; -import java.util.Collection; +import java.util.Set; import javax.annotation.Nullable; import org.sonar.api.ExtensionPoint; import org.sonar.api.batch.InstantiationStrategy; @@ -37,11 +37,11 @@ import org.sonar.api.batch.ScannerSide; public abstract class ScmBranchProvider extends ScmProvider { /** - * Return absolute path of files changed in the current branch, compared to the provided target branch. + * Return absolute path of the files changed in the current branch, compared to the provided target branch. * @return null if SCM provider was not able to compute the list of files. */ @Nullable - public Collection branchChangedFiles(String targetBranchName, Path rootBaseDir) { + public Set branchChangedFiles(String targetBranchName, Path rootBaseDir) { return null; } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/DefaultModuleFileSystem.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/DefaultModuleFileSystem.java index 46c019850a5..0748430e4cb 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/DefaultModuleFileSystem.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/DefaultModuleFileSystem.java @@ -23,29 +23,28 @@ import com.google.common.annotations.VisibleForTesting; import org.sonar.api.batch.fs.internal.DefaultFileSystem; import org.sonar.api.batch.fs.internal.DefaultInputModule; import org.sonar.scanner.analysis.DefaultAnalysisMode; -import org.sonar.scanner.repository.ProjectRepositories; public class DefaultModuleFileSystem extends DefaultFileSystem { public DefaultModuleFileSystem(ModuleInputComponentStore moduleInputFileCache, DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, - ProjectRepositories projectRepositories) { + StatusDetection statusDetection) { super(module.getBaseDir(), moduleInputFileCache); - setFields(module, initializer, mode, projectRepositories); + setFields(module, initializer, mode, statusDetection); } @VisibleForTesting - public DefaultModuleFileSystem(DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, ProjectRepositories projectRepositories) { + public DefaultModuleFileSystem(DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, StatusDetection statusDetection) { super(module.getBaseDir()); - setFields(module, initializer, mode, projectRepositories); + setFields(module, initializer, mode, statusDetection); } - private void setFields(DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, ProjectRepositories projectRepositories) { + private void setFields(DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, StatusDetection statusDetection) { setWorkDir(module.getWorkDir()); setEncoding(initializer.defaultEncoding()); // filter the files sensors have access to if (!mode.scanAllFiles()) { - setDefaultPredicate(p -> new SameInputFilePredicate(p, projectRepositories, module.definition().getKeyWithBranch())); + setDefaultPredicate(p -> new SameInputFilePredicate(p, statusDetection, module.definition().getKeyWithBranch())); } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/SameInputFilePredicate.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/SameInputFilePredicate.java index 5e75d5213e0..32eaa709204 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/SameInputFilePredicate.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/SameInputFilePredicate.java @@ -20,45 +20,39 @@ package org.sonar.scanner.scan.filesystem; import java.util.function.Predicate; -import org.apache.commons.lang.StringUtils; import org.sonar.api.batch.fs.FilePredicate; import org.sonar.api.batch.fs.InputFile; +import org.sonar.api.batch.fs.InputFile.Status; +import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.batch.fs.internal.OperatorPredicate; import org.sonar.api.batch.fs.internal.StatusPredicate; -import org.sonar.scanner.repository.FileData; -import org.sonar.scanner.repository.ProjectRepositories; public class SameInputFilePredicate implements Predicate { - private final ProjectRepositories projectRepositories; + private final StatusDetection statusDetection; private final String moduleKeyWithBranch; private final FilePredicate currentPredicate; - public SameInputFilePredicate(FilePredicate currentPredicate, ProjectRepositories projectRepositories, String moduleKeyWithBranch) { + public SameInputFilePredicate(FilePredicate currentPredicate, StatusDetection statusDetection, String moduleKeyWithBranch) { this.currentPredicate = currentPredicate; - this.projectRepositories = projectRepositories; + this.statusDetection = statusDetection; this.moduleKeyWithBranch = moduleKeyWithBranch; } @Override public boolean test(InputFile inputFile) { if (hasExplicitFilterOnStatus(currentPredicate)) { - // If user explicitely requested a given status, don't change the result + // If user explicitly requested a given status, don't change the result return true; } - // Try to avoid initializing metadata - FileData fileDataPerPath = projectRepositories.fileData(moduleKeyWithBranch, inputFile.relativePath()); - if (fileDataPerPath == null) { - // ADDED - return true; - } - String previousHash = fileDataPerPath.hash(); - if (StringUtils.isEmpty(previousHash)) { - // ADDED - return true; + + // TODO: the inputFile could try to calculate the status itself without generating metadata + Status status = statusDetection.getStatusWithoutMetadata(moduleKeyWithBranch, (DefaultInputFile) inputFile); + if (status != null) { + return status != Status.SAME; } // this will trigger computation of metadata - return inputFile.status() != InputFile.Status.SAME; + return inputFile.status() != Status.SAME; } static boolean hasExplicitFilterOnStatus(FilePredicate predicate) { diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/StatusDetection.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/StatusDetection.java index 8ba900e3f65..bf06399d676 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/StatusDetection.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/StatusDetection.java @@ -19,6 +19,7 @@ */ package org.sonar.scanner.scan.filesystem; +import javax.annotation.CheckForNull; import javax.annotation.concurrent.Immutable; import org.apache.commons.lang.StringUtils; import org.sonar.api.batch.fs.InputFile; @@ -27,6 +28,8 @@ import org.sonar.scanner.repository.FileData; import org.sonar.scanner.repository.ProjectRepositories; import org.sonar.scanner.scm.ScmChangedFiles; +import static org.sonar.api.batch.fs.InputFile.Status.*; + @Immutable public class StatusDetection { @@ -41,18 +44,39 @@ public class StatusDetection { InputFile.Status status(String projectKeyWithBranch, DefaultInputFile inputFile, String hash) { FileData fileDataPerPath = projectRepositories.fileData(projectKeyWithBranch, inputFile.relativePath()); if (fileDataPerPath == null) { - return InputFile.Status.ADDED; + return checkChanged(ADDED, inputFile); } String previousHash = fileDataPerPath.hash(); if (StringUtils.equals(hash, previousHash)) { - return InputFile.Status.SAME; + return SAME; } if (StringUtils.isEmpty(previousHash)) { - return InputFile.Status.ADDED; + return checkChanged(ADDED, inputFile); + } + return checkChanged(CHANGED, inputFile); + } + + /** + * If possible, get the status of the provided file without initializing metadata of the file. + * @return null if it was not possible to get the status without calculating metadata + */ + @CheckForNull + public InputFile.Status getStatusWithoutMetadata(String projectKeyWithBranch, DefaultInputFile inputFile) { + FileData fileDataPerPath = projectRepositories.fileData(projectKeyWithBranch, inputFile.relativePath()); + if (fileDataPerPath == null) { + return checkChanged(ADDED, inputFile); } + String previousHash = fileDataPerPath.hash(); + if (StringUtils.isEmpty(previousHash)) { + return checkChanged(ADDED, inputFile); + } + return null; + } + + private InputFile.Status checkChanged(InputFile.Status status, DefaultInputFile inputFile) { if (!scmChangedFiles.verifyChanged(inputFile.path())) { - return InputFile.Status.SAME; + return SAME; } - return InputFile.Status.CHANGED; + return status; } } 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 60b87a1fc0a..aed26e03144 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 @@ -24,15 +24,17 @@ import java.util.Collection; import javax.annotation.CheckForNull; import org.picocontainer.annotations.Nullable; import org.picocontainer.injectors.ProviderAdapter; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.sonar.api.batch.fs.internal.InputModuleHierarchy; import org.sonar.api.batch.scm.ScmBranchProvider; import org.sonar.api.batch.scm.ScmProvider; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; +import org.sonar.api.utils.log.Profiler; import org.sonar.scanner.scan.branch.BranchConfiguration; public class ScmChangedFilesProvider extends ProviderAdapter { - private static final Logger LOG = LoggerFactory.getLogger(ScmChangedFilesProvider.class); + private static final Logger LOG = Loggers.get(ScmChangedFilesProvider.class); + private static final String LOG_MSG = "SCM collecting changed files in the branch"; private ScmChangedFiles scmBranchChangedFiles; @@ -46,21 +48,30 @@ public class ScmChangedFilesProvider extends ProviderAdapter { } else { Path rootBaseDir = inputModuleHierarchy.root().getBaseDir(); Collection changedFiles = loadChangedFilesIfNeeded(scmConfiguration, branchConfiguration, rootBaseDir); + validatePaths(changedFiles); scmBranchChangedFiles = new ScmChangedFiles(changedFiles); } } return scmBranchChangedFiles; } + private static void validatePaths(@javax.annotation.Nullable Collection paths) { + if (paths != null && paths.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."); + } + } + @CheckForNull private static Collection loadChangedFilesIfNeeded(ScmConfiguration scmConfiguration, BranchConfiguration branchConfiguration, Path rootBaseDir) { if (branchConfiguration.isShortLivingBranch()) { ScmProvider scmProvider = scmConfiguration.provider(); if (scmProvider != null && (scmProvider instanceof ScmBranchProvider)) { + Profiler profiler = Profiler.create(LOG).startInfo(LOG_MSG); ScmBranchProvider scmBranchProvider = (ScmBranchProvider) scmProvider; Collection changedFiles = scmBranchProvider.branchChangedFiles(branchConfiguration.branchTarget(), rootBaseDir); + profiler.stopInfo(); if (changedFiles != null) { - LOG.debug("SCM reported {} files changed in the branch", changedFiles.size()); + LOG.debug("SCM reported {} {} changed in the branch", changedFiles.size(), pluralize("file", changedFiles.size())); return changedFiles; } } @@ -70,4 +81,11 @@ public class ScmChangedFilesProvider extends ProviderAdapter { return null; } + private static String pluralize(String str, int i) { + if (i == 1) { + return str; + } + return str + "s"; + } + } 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 bcf70eb65d7..8a8470f135e 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 @@ -33,6 +33,10 @@ import org.sonar.scanner.repository.ProjectRepositories; import org.sonar.scanner.scm.ScmChangedFiles; 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.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; public class StatusDetectionTest { @Test @@ -54,6 +58,26 @@ public class StatusDetectionTest { // normally changed assertThat(statusDetection.status("foo", createFile("src/Foo.java"), "XXXXX")).isEqualTo(InputFile.Status.SAME); + + // normally added + assertThat(statusDetection.status("foo", createFile("src/Other.java"), "QWERT")).isEqualTo(InputFile.Status.SAME); + } + + @Test + public void detect_status_without_metadata() { + DefaultInputFile mockedFile = mock(DefaultInputFile.class); + when(mockedFile.relativePath()).thenReturn("module/src/Foo.java"); + when(mockedFile.path()).thenReturn(Paths.get("module", "src", "Foo.java")); + + ProjectRepositories ref = new ProjectRepositories(ImmutableTable.of(), createTable(), null); + ScmChangedFiles changedFiles = new ScmChangedFiles(Collections.singletonList(Paths.get("module", "src", "Foo.java"))); + StatusDetection statusDetection = new StatusDetection(ref, changedFiles); + + assertThat(statusDetection.getStatusWithoutMetadata("foo", mockedFile)).isEqualTo(InputFile.Status.ADDED); + + verify(mockedFile).path(); + verify(mockedFile).relativePath(); + verifyNoMoreInteractions(mockedFile); } @Test 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 6777aaa7333..bf278fee6ce 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 @@ -23,7 +23,9 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.Collections; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.sonar.api.batch.fs.internal.DefaultInputModule; @@ -48,6 +50,9 @@ public class ScmChangedFilesProviderTest { @Mock private ScmBranchProvider scmProvider; + @Rule + public ExpectedException exception = ExpectedException.none(); + private Path rootBaseDir = Paths.get("root"); private ScmChangedFilesProvider provider; @@ -69,6 +74,18 @@ public class ScmChangedFilesProviderTest { verify(scmConfiguration).provider(); } + @Test + public void testFailIfRelativePath() { + when(branchConfiguration.branchTarget()).thenReturn("target"); + when(branchConfiguration.isShortLivingBranch()).thenReturn(true); + when(scmConfiguration.provider()).thenReturn(scmProvider); + when(scmProvider.branchChangedFiles("target", rootBaseDir)).thenReturn(Collections.singleton(Paths.get("changedFile"))); + + exception.expect(IllegalStateException.class); + exception.expectMessage("changed file with a relative path"); + provider.provide(scmConfiguration, branchConfiguration, inputModuleHierarchy); + } + @Test public void testProviderDoesntSupport() { when(branchConfiguration.branchTarget()).thenReturn("target"); @@ -108,10 +125,10 @@ public class ScmChangedFilesProviderTest { when(branchConfiguration.branchTarget()).thenReturn("target"); when(branchConfiguration.isShortLivingBranch()).thenReturn(true); when(scmConfiguration.provider()).thenReturn(scmProvider); - when(scmProvider.branchChangedFiles("target", rootBaseDir)).thenReturn(Collections.singletonList(Paths.get("changedFile"))); + when(scmProvider.branchChangedFiles("target", rootBaseDir)).thenReturn(Collections.singleton(Paths.get("changedFile").toAbsolutePath())); ScmChangedFiles scmChangedFiles = provider.provide(scmConfiguration, branchConfiguration, inputModuleHierarchy); - assertThat(scmChangedFiles.get()).containsOnly(Paths.get("changedFile")); + assertThat(scmChangedFiles.get()).containsOnly(Paths.get("changedFile").toAbsolutePath()); verify(scmProvider).branchChangedFiles("target", rootBaseDir); } -- 2.39.5