SONAR-11367 Write duplication for changed files in PR and SLB

server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolder.java

@@ -73,6 +73,14 @@ public interface TreeRootHolder {
   @Deprecated
   Optional<Component> getOptionalComponentByRef(int ref);
 
+  /**
+   * Return a component from the report tree (see {@link #getReportTreeRoot()}, by its batch reference.
+   *
+   * @throws IllegalStateException if the holder is empty (ie. there is no root yet)
+   * @throws IllegalArgumentException if there's no {@link Component} with the specified reference
+   */
+  Component getReportTreeComponentByRef(int ref);
+
   /**
    * Number of components, including root.
    *

server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolderImpl.java

@@ -35,6 +35,9 @@ public class TreeRootHolderImpl implements MutableTreeRootHolder {
   @CheckForNull
   private Map<Integer, Component> componentsByRef;
 
+  @CheckForNull
+  private Map<Integer, Component> extendedComponentsByRef;
+
   private Component root;
   private Component extendedTreeRoot;
 
@@ -76,6 +79,17 @@ public class TreeRootHolderImpl implements MutableTreeRootHolder {
     return Optional.ofNullable(componentsByRef.get(ref));
   }
 
+  @Override
+  public Component getReportTreeComponentByRef(int ref) {
+    checkInitialized();
+    ensureExtendedComponentByRefIsPopulated();
+    Component c = extendedComponentsByRef.get(ref);
+    if (c == null) {
+      throw new IllegalArgumentException(String.format("Component with ref '%s' can't be found", ref));
+    }
+    return c;
+  }
+
   @Override
   public int getSize() {
     checkInitialized();
@@ -83,6 +97,22 @@ public class TreeRootHolderImpl implements MutableTreeRootHolder {
     return componentsByRef.size();
   }
 
+  private void ensureExtendedComponentByRefIsPopulated() {
+    if (extendedComponentsByRef != null) {
+      return;
+    }
+
+    final ImmutableMap.Builder<Integer, Component> builder = ImmutableMap.builder();
+    new DepthTraversalTypeAwareCrawler(
+      new TypeAwareVisitorAdapter(CrawlerDepthLimit.FILE, POST_ORDER) {
+        @Override
+        public void visitAny(Component component) {
+          builder.put(component.getReportAttributes().getRef(), component);
+        }
+      }).visit(this.extendedTreeRoot);
+    this.extendedComponentsByRef = builder.build();
+  }
+
   private void ensureComponentByRefIsPopulated() {
     if (componentsByRef != null) {
       return;

server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/duplication/DuplicationMeasures.java

@@ -76,7 +76,7 @@ public class DuplicationMeasures {
   public void execute() {
     new PathAwareCrawler<>(
       FormulaExecutorComponentVisitor.newBuilder(metricRepository, measureRepository).buildFor(formulas))
-        .visit(treeRootHolder.getRoot());
+      .visit(treeRootHolder.getRoot());
   }
 
   protected DuplicationCounter createCounter() {

server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/step/LoadDuplicationsFromReportStep.java

@@ -63,7 +63,7 @@ public class LoadDuplicationsFromReportStep implements ComputationStep {
   @Override
   public void execute(ComputationStep.Context context) {
     DuplicationVisitor visitor = new DuplicationVisitor();
-    new DepthTraversalTypeAwareCrawler(visitor).visit(treeRootHolder.getRoot());
+    new DepthTraversalTypeAwareCrawler(visitor).visit(treeRootHolder.getReportTreeRoot());
     context.getStatistics().add("duplications", visitor.count);
   }
 
@@ -79,9 +79,8 @@ public class LoadDuplicationsFromReportStep implements ComputationStep {
     public Duplicate apply(@Nonnull ScannerReport.Duplicate input) {
       if (input.getOtherFileRef() != 0) {
         checkArgument(input.getOtherFileRef() != file.getReportAttributes().getRef(), "file and otherFile references can not be the same");
-        return new InProjectDuplicate(
-          treeRootHolder.getComponentByRef(input.getOtherFileRef()),
-          convert(input.getRange()));
+        Component otherComponent = treeRootHolder.getReportTreeComponentByRef(input.getOtherFileRef());
+        return new InProjectDuplicate(otherComponent, convert(input.getRange()));
       }
       return new InnerDuplicate(convert(input.getRange()));
     }

sonar-scanner-engine/src/main/java/org/sonar/scanner/cpd/CpdExecutor.java

@@ -20,9 +20,11 @@
 package org.sonar.scanner.cpd;
 
 import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Optional;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
@@ -34,6 +36,7 @@ import java.util.stream.Collectors;
 import org.sonar.api.batch.fs.InputComponent;
 import org.sonar.api.batch.fs.InputFile;
 import org.sonar.api.batch.fs.internal.DefaultInputComponent;
+import org.sonar.api.batch.fs.internal.DefaultInputFile;
 import org.sonar.api.utils.log.Logger;
 import org.sonar.api.utils.log.Loggers;
 import org.sonar.duplications.block.Block;
@@ -66,15 +69,21 @@ public class CpdExecutor {
   private final InputComponentStore componentStore;
   private final ProgressReport progressReport;
   private final CpdSettings settings;
-  private int count;
+  private final ExecutorService executorService;
+  private int count = 0;
   private int total;
 
   public CpdExecutor(CpdSettings settings, SonarCpdBlockIndex index, ReportPublisher publisher, InputComponentStore inputComponentCache) {
+    this(settings, index, publisher, inputComponentCache, Executors.newSingleThreadExecutor());
+  }
+
+  public CpdExecutor(CpdSettings settings, SonarCpdBlockIndex index, ReportPublisher publisher, InputComponentStore inputComponentCache, ExecutorService executorService) {
     this.settings = settings;
     this.index = index;
     this.publisher = publisher;
     this.componentStore = inputComponentCache;
     this.progressReport = new ProgressReport("CPD computation", TimeUnit.SECONDS.toMillis(10));
+    this.executorService = executorService;
   }
 
   public void execute() {
@@ -83,19 +92,28 @@ public class CpdExecutor {
 
   @VisibleForTesting
   void execute(long timeout) {
-    total = index.noResources();
-    int filesWithoutBlocks = index.noIndexedFiles() - total;
+    List<FileBlocks> components = new ArrayList<>(index.noResources());
+    Iterator<ResourceBlocks> it = index.iterator();
+
+    while (it.hasNext()) {
+      ResourceBlocks resourceBlocks = it.next();
+      Optional<FileBlocks> fileBlocks = toFileBlocks(resourceBlocks.resourceId(), resourceBlocks.blocks());
+      if (!fileBlocks.isPresent() || fileBlocks.get().getInputFile().status() == InputFile.Status.SAME) {
+        continue;
+      }
+      components.add(fileBlocks.get());
+    }
+
+    int filesWithoutBlocks = index.noIndexedFiles() - index.noResources();
     if (filesWithoutBlocks > 0) {
       LOG.info("{} {} had no CPD blocks", filesWithoutBlocks, pluralize(filesWithoutBlocks));
     }
+
+    total = components.size();
     progressReport.start(String.format("Calculating CPD for %d %s", total, pluralize(total)));
-    ExecutorService executorService = Executors.newSingleThreadExecutor();
     try {
-      Iterator<ResourceBlocks> it = index.iterator();
-
-      while (it.hasNext()) {
-        ResourceBlocks resourceBlocks = it.next();
-        runCpdAnalysis(executorService, resourceBlocks.resourceId(), resourceBlocks.blocks(), timeout);
+      for (FileBlocks fileBlocks : components) {
+        runCpdAnalysis(executorService, fileBlocks.getInputFile(), fileBlocks.getBlocks(), timeout);
         count++;
       }
       progressReport.stop("CPD calculation finished");
@@ -112,18 +130,7 @@ public class CpdExecutor {
   }
 
   @VisibleForTesting
-  void runCpdAnalysis(ExecutorService executorService, String componentKey, final Collection<Block> fileBlocks, long timeout) {
-    DefaultInputComponent component = (DefaultInputComponent) componentStore.getByKey(componentKey);
-    if (component == null) {
-      LOG.error("Resource not found in component store: {}. Skipping CPD computation for it", componentKey);
-      return;
-    }
-
-    InputFile inputFile = (InputFile) component;
-    if (inputFile.status() == InputFile.Status.SAME) {
-      return;
-    }
-
+  void runCpdAnalysis(ExecutorService executorService, DefaultInputFile inputFile, Collection<Block> fileBlocks, long timeout) {
     LOG.debug("Detection of duplications for {}", inputFile.absolutePath());
     progressReport.message(String.format("%d/%d - current file: %s", count, total, inputFile.absolutePath()));
 
@@ -150,7 +157,7 @@ public class CpdExecutor {
       filtered = duplications;
     }
 
-    saveDuplications(component, filtered);
+    saveDuplications(inputFile, filtered);
   }
 
   @VisibleForTesting final void saveDuplications(final DefaultInputComponent component, List<CloneGroup> duplications) {
@@ -173,6 +180,15 @@ public class CpdExecutor {
     publisher.getWriter().writeComponentDuplications(component.batchId(), reportDuplications);
   }
 
+  private Optional<FileBlocks> toFileBlocks(String componentKey, Collection<Block> fileBlocks) {
+    DefaultInputFile component = (DefaultInputFile) componentStore.getByKey(componentKey);
+    if (component == null) {
+      LOG.error("Resource not found in component store: {}. Skipping CPD computation for it", componentKey);
+      return Optional.empty();
+    }
+    return Optional.of(new FileBlocks(component, fileBlocks));
+  }
+
   private Duplication toReportDuplication(InputComponent component, Duplication.Builder dupBuilder, Duplicate.Builder blockBuilder, CloneGroup input) {
     dupBuilder.clear();
     ClonePart originBlock = input.getOriginPart();
@@ -207,4 +223,22 @@ public class CpdExecutor {
     }
     return dupBuilder.build();
   }
+
+  private static class FileBlocks {
+    public FileBlocks(DefaultInputFile inputFile, Collection<Block> blocks) {
+      this.inputFile = inputFile;
+      this.blocks = blocks;
+    }
+
+    private DefaultInputFile inputFile;
+    private Collection<Block> blocks;
+
+    public DefaultInputFile getInputFile() {
+      return inputFile;
+    }
+
+    public Collection<Block> getBlocks() {
+      return blocks;
+    }
+  }
 }

sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java

@@ -506,9 +506,6 @@ public class DefaultSensorStorage implements SensorStorage {
   @Override
   public void store(DefaultCpdTokens defaultCpdTokens) {
     DefaultInputFile inputFile = (DefaultInputFile) defaultCpdTokens.inputFile();
-    if (shouldSkipStorage(inputFile)) {
-      return;
-    }
     inputFile.setPublished(true);
     PmdBlockChunker blockChunker = new PmdBlockChunker(getCpdBlockSize(inputFile.language()));
     List<Block> blocks = blockChunker.chunk(inputFile.key(), defaultCpdTokens.getTokenLines());

sonar-scanner-engine/src/test/java/org/sonar/scanner/cpd/CpdExecutorTest.java

@@ -25,7 +25,10 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
 import org.junit.Before;
@@ -33,6 +36,7 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
+import org.mockito.ArgumentMatchers;
 import org.sonar.api.batch.fs.InputFile;
 import org.sonar.api.batch.fs.internal.DefaultInputFile;
 import org.sonar.api.batch.fs.internal.DefaultInputModule;
@@ -55,7 +59,8 @@ import org.sonar.scanner.scan.filesystem.InputComponentStore;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verifyZeroInteractions;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
 public class CpdExecutorTest {
@@ -69,6 +74,7 @@ public class CpdExecutorTest {
   public ExpectedException thrown = ExpectedException.none();
 
   private CpdExecutor executor;
+  private ExecutorService executorService = mock(ExecutorService.class);
   private CpdSettings settings = mock(CpdSettings.class);
   private ReportPublisher publisher = mock(ReportPublisher.class);
   private SonarCpdBlockIndex index = new SonarCpdBlockIndex(publisher, settings);
@@ -88,7 +94,7 @@ public class CpdExecutorTest {
 
     DefaultInputModule inputModule = TestInputFileBuilder.newDefaultInputModule("foo", baseDir);
     componentStore = new InputComponentStore(inputModule, mock(BranchConfiguration.class));
-    executor = new CpdExecutor(settings, index, publisher, componentStore);
+    executor = new CpdExecutor(settings, index, publisher, componentStore, executorService);
     reader = new ScannerReportReader(outputDir);
 
     batchComponent1 = createComponent("src/Foo.php", 5);
@@ -97,30 +103,16 @@ public class CpdExecutorTest {
     batchComponent4 = createComponent("src/Foo4.php", 5, f -> f.setStatus(InputFile.Status.SAME));
   }
 
-  private DefaultInputFile createComponent(String relativePath, int lines) {
-    return createComponent(relativePath, lines, f -> {
-    });
-  }
-
-  private DefaultInputFile createComponent(String relativePath, int lines, Consumer<TestInputFileBuilder> config) {
-    TestInputFileBuilder fileBuilder = new TestInputFileBuilder("foo", relativePath)
-      .setModuleBaseDir(baseDir.toPath())
-      .setLines(lines);
-    config.accept(fileBuilder);
-    DefaultInputFile file = fileBuilder.build();
-    componentStore.put(file);
-    return file;
-  }
-
   @Test
-  public void testNothingToSave() {
+  public void dont_fail_if_nothing_to_save() {
     executor.saveDuplications(batchComponent1, Collections.<CloneGroup>emptyList());
     assertThat(reader.readComponentDuplications(batchComponent1.batchId())).hasSize(0);
   }
 
   @Test
-  public void reportOneSimpleDuplicationBetweenTwoFiles() {
-    List<CloneGroup> groups = Collections.singletonList(newCloneGroup(new ClonePart(batchComponent1.key(), 0, 2, 4),
+  public void should_save_single_duplication() {
+    List<CloneGroup> groups = Collections.singletonList(newCloneGroup(
+      new ClonePart(batchComponent1.key(), 0, 2, 4),
       new ClonePart(batchComponent2.key(), 0, 15, 17)));
 
     executor.saveDuplications(batchComponent1, groups);
@@ -130,17 +122,10 @@ public class CpdExecutorTest {
   }
 
   @Test
-  public void dontReportDuplicationOnUnmodifiedFileInSLB() {
-    ExecutorService executorService = mock(ExecutorService.class);
-    executor.runCpdAnalysis(executorService, batchComponent4.key(), Collections.emptyList(), 1000L);
-
-    readDuplications(batchComponent4, 0);
-    verifyZeroInteractions(executorService);
-  }
-
-  @Test
-  public void reportDuplicationOnSameFile() {
-    List<CloneGroup> groups = Collections.singletonList(newCloneGroup(new ClonePart(batchComponent1.key(), 0, 5, 204), new ClonePart(batchComponent1.key(), 0, 215, 414)));
+  public void should_save_duplication_on_same_file() {
+    List<CloneGroup> groups = Collections.singletonList(newCloneGroup(
+      new ClonePart(batchComponent1.key(), 0, 5, 204),
+      new ClonePart(batchComponent1.key(), 0, 215, 414)));
     executor.saveDuplications(batchComponent1, groups);
 
     Duplication[] dups = readDuplications(1);
@@ -148,13 +133,13 @@ public class CpdExecutorTest {
   }
 
   @Test
-  public void reportTooManyDuplicates() {
+  public void should_limit_number_of_references() {
     // 1 origin part + 101 duplicates = 102
     List<ClonePart> parts = new ArrayList<>(CpdExecutor.MAX_CLONE_PART_PER_GROUP + 2);
     for (int i = 0; i < CpdExecutor.MAX_CLONE_PART_PER_GROUP + 2; i++) {
       parts.add(new ClonePart(batchComponent1.key(), i, i, i + 1));
     }
-    List<CloneGroup> groups = Arrays.asList(CloneGroup.builder().setLength(0).setOrigin(parts.get(0)).setParts(parts).build());
+    List<CloneGroup> groups = Collections.singletonList(CloneGroup.builder().setLength(0).setOrigin(parts.get(0)).setParts(parts).build());
     executor.saveDuplications(batchComponent1, groups);
 
     Duplication[] dups = readDuplications(1);
@@ -166,7 +151,7 @@ public class CpdExecutorTest {
   }
 
   @Test
-  public void reportTooManyDuplications() throws Exception {
+  public void should_limit_number_of_clones() {
     // 1 origin part + 101 duplicates = 102
     List<CloneGroup> dups = new ArrayList<>(CpdExecutor.MAX_CLONE_GROUP_PER_FILE + 1);
     for (int i = 0; i < CpdExecutor.MAX_CLONE_GROUP_PER_FILE + 1; i++) {
@@ -183,9 +168,11 @@ public class CpdExecutorTest {
   }
 
   @Test
-  public void reportOneDuplicatedGroupInvolvingMoreThanTwoFiles() throws Exception {
-    List<CloneGroup> groups = Collections.singletonList(newCloneGroup(new ClonePart(batchComponent1.key(), 0, 5, 204),
-      new ClonePart(batchComponent2.key(), 0, 15, 214), new ClonePart(batchComponent3.key(), 0, 25, 224)));
+  public void should_save_duplication_involving_three_files() {
+    List<CloneGroup> groups = Collections.singletonList(newCloneGroup(
+      new ClonePart(batchComponent1.key(), 0, 5, 204),
+      new ClonePart(batchComponent2.key(), 0, 15, 214),
+      new ClonePart(batchComponent3.key(), 0, 25, 224)));
     executor.saveDuplications(batchComponent1, groups);
 
     Duplication[] dups = readDuplications(1);
@@ -195,7 +182,7 @@ public class CpdExecutorTest {
   }
 
   @Test
-  public void reportTwoDuplicatedGroupsInvolvingThreeFiles() throws Exception {
+  public void should_save_two_duplicated_groups_involving_three_files() {
     List<CloneGroup> groups = Arrays.asList(
       newCloneGroup(new ClonePart(batchComponent1.key(), 0, 5, 204),
         new ClonePart(batchComponent2.key(), 0, 15, 214)),
@@ -209,36 +196,61 @@ public class CpdExecutorTest {
   }
 
   @Test
-  public void failOnMissingComponent() {
-    executor.runCpdAnalysis(null, "unknown", Collections.emptyList(), 1);
-    readDuplications(0);
+  public void should_ignore_unmodified_files_in_SLB() {
+    Block block = Block.builder().setBlockHash(new ByteArray("AAAABBBBCCCC")).build();
+    index.insert(batchComponent4, Collections.singletonList(block));
+    executor.execute();
+
+    verify(executorService).shutdown();
+    verifyNoMoreInteractions(executorService);
+    readDuplications(batchComponent4, 0);
+  }
+
+  @Test
+  public void should_ignore_missing_component() {
+    Block block = Block.builder()
+      .setBlockHash(new ByteArray("AAAABBBBCCCC"))
+      .setResourceId("unknown")
+      .build();
+    index.insert(batchComponent1, Collections.singletonList(block));
+    executor.execute();
+
+    verify(executorService).shutdown();
+    verifyNoMoreInteractions(executorService);
+    readDuplications(batchComponent1, 0);
     assertThat(logTester.logs(LoggerLevel.ERROR)).contains("Resource not found in component store: unknown. Skipping CPD computation for it");
   }
 
   @Test
-  public void timeout() {
-    for (int i = 1; i <= 2; i++) {
-      DefaultInputFile component = createComponent("src/Foo" + i + ".php", 100);
-      List<Block> blocks = new ArrayList<>();
-      for (int j = 1; j <= 10000; j++) {
-        blocks.add(Block.builder()
-          .setResourceId(component.key())
-          .setIndexInFile(j)
-          .setLines(j, j + 1)
-          .setUnit(j, j + 1)
-          .setBlockHash(new ByteArray("abcd1234".getBytes()))
-          .build());
-      }
-      index.insert(component, blocks);
-    }
+  public void should_timeout() {
+    Block block = Block.builder()
+      .setBlockHash(new ByteArray("AAAABBBBCCCC"))
+      .setResourceId(batchComponent1.key())
+      .build();
+    index.insert(batchComponent1, Collections.singletonList(block));
+    when(executorService.submit(ArgumentMatchers.any(Callable.class))).thenReturn(new CompletableFuture());
     executor.execute(1);
 
     readDuplications(0);
     assertThat(logTester.logs(LoggerLevel.WARN))
       .usingElementComparator((l, r) -> l.matches(r) ? 0 : 1)
       .containsOnly(
-        "Timeout during detection of duplications for .*Foo1.php",
-        "Timeout during detection of duplications for .*Foo2.php");
+        "Timeout during detection of duplications for .*Foo.php");
+  }
+
+  private DefaultInputFile createComponent(String relativePath, int lines) {
+    return createComponent(relativePath, lines, f -> {
+    });
+  }
+
+  private DefaultInputFile createComponent(String relativePath, int lines, Consumer<TestInputFileBuilder> config) {
+    TestInputFileBuilder fileBuilder = new TestInputFileBuilder("foo", relativePath)
+      .setModuleBaseDir(baseDir.toPath())
+      .setLines(lines);
+    config.accept(fileBuilder);
+    DefaultInputFile file = fileBuilder.build();
+    componentStore.put(file);
+    return file;
   }
 
   private Duplication[] readDuplications(int expected) {