SONAR-6887 Make it clear in SCM API that date and revision are mandatory on line blame

5 files changed, 78 insertions, 20 deletions

diff --git a/sonar-batch/src/main/java/org/sonar/batch/scm/DefaultBlameOutput.java b/sonar-batch/src/main/java/org/sonar/batch/scm/DefaultBlameOutput.java
index 7ed72faabad..cf7c526d5c3 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/scm/DefaultBlameOutput.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/scm/DefaultBlameOutput.java
@@ -21,7 +21,6 @@ package org.sonar.batch.scm;
 
 import com.google.common.base.Preconditions;
 import java.text.Normalizer;
-import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -84,17 +83,14 @@ class DefaultBlameOutput implements BlameOutput {
     Map<String, Integer> changesetsIdByRevision = new HashMap<>();
 
     for (BlameLine line : lines) {
-      if (StringUtils.isNotBlank(line.revision())) {
-        Integer changesetId = changesetsIdByRevision.get(line.revision());
-        if (changesetId == null) {
-          addChangeset(scmBuilder, line);
-          changesetId = scmBuilder.getChangesetCount() - 1;
-          changesetsIdByRevision.put(line.revision(), changesetId);
-        }
-        scmBuilder.addChangesetIndexByLine(changesetId);
-      } else {
+      validateLine(line);
+      Integer changesetId = changesetsIdByRevision.get(line.revision());
+      if (changesetId == null) {
         addChangeset(scmBuilder, line);
+        changesetId = scmBuilder.getChangesetCount() - 1;
+        changesetsIdByRevision.put(line.revision(), changesetId);
       }
+      scmBuilder.addChangesetIndexByLine(changesetId);
     }
     writer.writeComponentChangesets(scmBuilder.build());
     allFilesToBlame.remove(file);
@@ -102,18 +98,19 @@ class DefaultBlameOutput implements BlameOutput {
     progressReport.message(count + "/" + total + " files analyzed, last one was " + file.absolutePath());
   }
 
+  private static void validateLine(BlameLine line) {
+    Preconditions.checkNotNull(line.revision(), "Blame revision cannot be null");
+    Preconditions.checkNotNull(line.date(), "Blame date cannot be null");
+  }
+
   private static void addChangeset(Builder scmBuilder, BlameLine line) {
     BatchReport.Changesets.Changeset.Builder changesetBuilder = BatchReport.Changesets.Changeset.newBuilder();
-    if (StringUtils.isNotBlank(line.revision())) {
-      changesetBuilder.setRevision(line.revision());
-    }
+    changesetBuilder.setRevision(line.revision());
+    changesetBuilder.setDate(line.date().getTime());
     if (StringUtils.isNotBlank(line.author())) {
       changesetBuilder.setAuthor(normalizeString(line.author()));
     }
-    Date date = line.date();
-    if (date != null) {
-      changesetBuilder.setDate(date.getTime());
-    }
+
     scmBuilder.addChangeset(changesetBuilder.build());
   }
 
diff --git a/sonar-batch/src/test/java/org/sonar/batch/scm/DefaultBlameOutputTest.java b/sonar-batch/src/test/java/org/sonar/batch/scm/DefaultBlameOutputTest.java
index a92b20382dd..e7f00149111 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/scm/DefaultBlameOutputTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/scm/DefaultBlameOutputTest.java
@@ -19,20 +19,40 @@
  */
 package org.sonar.batch.scm;
 
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.sonar.api.batch.fs.InputComponent;
 import org.sonar.api.batch.fs.InputFile;
 import org.sonar.api.batch.fs.internal.DefaultInputFile;
 import org.sonar.api.batch.scm.BlameLine;
+import org.sonar.batch.index.BatchComponent;
+import org.sonar.batch.index.BatchComponentCache;
 
 import java.util.Arrays;
+import java.util.Date;
+
+import static org.mockito.Matchers.any;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class DefaultBlameOutputTest {
 
   @Rule
   public ExpectedException thrown = ExpectedException.none();
 
+  private BatchComponentCache componentCache;
+
+  @Before
+  public void prepare() {
+    componentCache = mock(BatchComponentCache.class);
+    BatchComponent component = mock(BatchComponent.class);
+    when(component.batchId()).thenReturn(1);
+    when(componentCache.get(any(InputComponent.class))).thenReturn(component);
+  }
+
   @Test
   public void shouldNotFailIfNotSameNumberOfLines() {
     InputFile file = new DefaultInputFile("foo", "src/main/java/Foo.java").setLines(10);
@@ -51,4 +71,26 @@ public class DefaultBlameOutputTest {
       .blameResult(file, Arrays.asList(new BlameLine().revision("1").author("guy")));
   }
 
+  @Test
+  public void shouldFailIfNullDate() {
+    InputFile file = new DefaultInputFile("foo", "src/main/java/Foo.java").setLines(1);
+
+    thrown.expect(NullPointerException.class);
+    thrown.expectMessage("Blame date cannot be null");
+
+    new DefaultBlameOutput(null, componentCache, Arrays.<InputFile>asList(file))
+      .blameResult(file, Arrays.asList(new BlameLine().revision("1").author("guy")));
+  }
+
+  @Test
+  public void shouldFailIfNullRevision() {
+    InputFile file = new DefaultInputFile("foo", "src/main/java/Foo.java").setLines(1);
+
+    thrown.expect(NullPointerException.class);
+    thrown.expectMessage("Blame revision cannot be null");
+
+    new DefaultBlameOutput(null, componentCache, Arrays.<InputFile>asList(file))
+      .blameResult(file, Arrays.asList(new BlameLine().date(new Date()).author("guy")));
+  }
+
 }
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/BlameCommand.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/BlameCommand.java
index c23bebb1160..ccecfd0af12 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/BlameCommand.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/BlameCommand.java
@@ -39,6 +39,7 @@ public abstract class BlameCommand {
    * Computation can be done in parallel if this is more efficient.
    * If there is an error that prevent to blame a file then an exception should be raised. If 
    * one file is new or contains local modifications then an exception should be raised.
+   * @see BlameOutput#blameResult(InputFile, List)
    */
   public abstract void blame(BlameInput input, BlameOutput output);
 
@@ -67,7 +68,7 @@ public abstract class BlameCommand {
     /**
      * Add result of the blame command for a single file. Number of lines should
      * be consistent with {@link InputFile#lines()}. This method is thread safe.
-     * @param lines One entry per line in the file.
+     * @param lines One entry per line in the file. <b>Every line must have a <code>non-null</code> date and revision </b>.
      */
     void blameResult(InputFile file, List<BlameLine> lines);
 
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/BlameLine.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/BlameLine.java
index 9aed38d9f32..5650f7e2eec 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/BlameLine.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/BlameLine.java
@@ -37,11 +37,27 @@ public class BlameLine {
   private Date date;
   private String revision;
   private String author;
+  
+  public BlameLine() {
+    // for backward compatibility
+  }
+  
+  /**
+   * Preferred constructor. Date and revision must be set.
+   * @since 5.2
+   */
+  public BlameLine(Date date, String revision) {
+    this.date = date;
+    this.revision = revision;
+  }
 
   public String revision() {
     return revision;
   }
 
+  /**
+   * Mandatory field
+   */
   public BlameLine revision(String revision) {
     this.revision = revision;
     return this;
@@ -60,11 +76,13 @@ public class BlameLine {
   /**
    * @return the commit date
    */
-  @CheckForNull
   public Date date() {
     return date;
   }
 
+  /**
+   * Mandatory field
+   */
   public BlameLine date(@Nullable Date date) {
     this.date = date;
     return this;
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmProvider.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmProvider.java
index 9c41ad4426c..0a2b9131fe3 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmProvider.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmProvider.java
@@ -42,7 +42,7 @@ public abstract class ScmProvider {
   public abstract String key();
 
   /**
-   * Does this provider able to manage files located in this directory.
+   * Whether this provider is able to manage files located in this directory.
    * Used by autodetection. Not considered if user has forced the provider key.
    * @return false by default
    */