Improve error message when issue is out of line range during issue tracking

and add test for binary file line computation

5 files changed, 46 insertions, 15 deletions

diff --git a/sonar-batch/src/main/java/org/sonar/batch/issue/tracking/IssueTracking.java b/sonar-batch/src/main/java/org/sonar/batch/issue/tracking/IssueTracking.java
index 54c7970bc34..adbdbc97525 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/issue/tracking/IssueTracking.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/issue/tracking/IssueTracking.java
@@ -22,6 +22,7 @@ package org.sonar.batch.issue.tracking;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
@@ -32,11 +33,7 @@ import org.sonar.api.issue.internal.DefaultIssue;
 
 import javax.annotation.Nullable;
 
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 @InstantiationStrategy(InstantiationStrategy.PER_BATCH)
 public class IssueTracking implements BatchComponent {
@@ -60,10 +57,13 @@ public class IssueTracking implements BatchComponent {
     if (issues.isEmpty()) {
       return;
     }
+    FileHashes hashedSource = sourceHashHolder.getHashedSource();
     for (DefaultIssue issue : issues) {
       Integer line = issue.line();
       if (line != null) {
-        issue.setChecksum(sourceHashHolder.getHashedSource().getHash(line));
+        // Extra verification if some plugin managed to create issue on a wrong line
+        Preconditions.checkState(line <= hashedSource.length(), "Invalid line number for issue %s. File has only %s line(s)", issue, hashedSource.length());
+        issue.setChecksum(hashedSource.getHash(line));
       }
     }
   }
diff --git a/sonar-batch/src/main/java/org/sonar/batch/scan/filesystem/FileMetadata.java b/sonar-batch/src/main/java/org/sonar/batch/scan/filesystem/FileMetadata.java
index 122ca40a5eb..05161741c13 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/scan/filesystem/FileMetadata.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/scan/filesystem/FileMetadata.java
@@ -25,22 +25,17 @@ import org.apache.commons.codec.binary.Hex;
 import org.apache.commons.codec.digest.DigestUtils;
 import org.apache.commons.io.ByteOrderMark;
 import org.apache.commons.io.input.BOMInputStream;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import org.sonar.api.BatchComponent;
 import org.sonar.api.CoreProperties;
 import org.sonar.api.batch.AnalysisMode;
 import org.sonar.api.batch.fs.internal.DefaultInputFile;
+import org.sonar.api.utils.log.Logger;
+import org.sonar.api.utils.log.Loggers;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 
-import java.io.BufferedReader;
-import java.io.File;
-import java.io.FileInputStream;
-import java.io.IOException;
-import java.io.InputStreamReader;
-import java.io.Reader;
+import java.io.*;
 import java.nio.charset.Charset;
 import java.security.MessageDigest;
 import java.util.ArrayList;
@@ -52,7 +47,7 @@ import java.util.List;
  */
 public class FileMetadata implements BatchComponent {
 
-  private static final Logger LOG = LoggerFactory.getLogger(FileMetadata.class);
+  private static final Logger LOG = Loggers.get(FileMetadata.class);
 
   private static final char LINE_FEED = '\n';
   private static final char CARRIAGE_RETURN = '\r';
diff --git a/sonar-batch/src/test/java/org/sonar/batch/issue/tracking/IssueTrackingTest.java b/sonar-batch/src/test/java/org/sonar/batch/issue/tracking/IssueTrackingTest.java
index 332356c9279..d07d08a978c 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/issue/tracking/IssueTrackingTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/issue/tracking/IssueTrackingTest.java
@@ -29,6 +29,7 @@ import org.apache.commons.lang.StringUtils;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.sonar.api.batch.fs.internal.DefaultInputFile;
 import org.sonar.api.issue.Issue;
@@ -54,6 +55,9 @@ public class IssueTrackingTest {
   @Rule
   public TemporaryFolder temp = new TemporaryFolder();
 
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
   IssueTracking tracking;
   Resource project;
   SourceHashHolder sourceHashHolder;
@@ -235,6 +239,18 @@ public class IssueTrackingTest {
     assertThat(result.matching(newIssue)).isEqualTo(referenceIssue);
   }
 
+  @Test
+  public void check_valid_line() throws Exception {
+    initLastHashes("example2-v1", "example2-v2");
+
+    DefaultIssue newIssue = newDefaultIssue("1 branch need to be covered", 200, RuleKey.of("squid", "AvoidCycle"), null);
+
+    thrown
+      .expectMessage("Invalid line number for issue DefaultIssue[key=<null>,componentUuid=<null>,componentKey=<null>,moduleUuid=<null>,moduleUuidPath=<null>,projectUuid=<null>,projectKey=<null>,ruleKey=squid:AvoidCycle,language=<null>,severity=<null>,manualSeverity=false,message=1 branch need to be covered,line=200,effortToFix=<null>,debt=<null>,status=OPEN,resolution=<null>,reporter=<null>,assignee=<null>,checksum=<null>,attributes=<null>,authorLogin=<null>,actionPlanKey=<null>,comments=<null>,tags=<null>,creationDate=<null>,updateDate=<null>,closeDate=<null>,currentChange=<null>,changes=<null>,isNew=true,endOfLife=false,onDisabledRule=false,isChanged=false,sendNotifications=false,selectedAt=<null>]. File has only 17 line(s)");
+
+    tracking.track(sourceHashHolder, Collections.<ServerIssue>emptyList(), newArrayList(newIssue));
+  }
+
   /**
    * SONAR-3072
    */
diff --git a/sonar-batch/src/test/java/org/sonar/batch/scan/filesystem/FileMetadataTest.java b/sonar-batch/src/test/java/org/sonar/batch/scan/filesystem/FileMetadataTest.java
index f1d45c62440..a470378a99e 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/scan/filesystem/FileMetadataTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/scan/filesystem/FileMetadataTest.java
@@ -28,6 +28,8 @@ import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.sonar.api.batch.AnalysisMode;
 import org.sonar.api.batch.fs.internal.DefaultInputFile;
+import org.sonar.api.utils.log.LogTester;
+import org.sonar.api.utils.log.LoggerLevel;
 import org.sonar.batch.scan.filesystem.FileMetadata.LineHashConsumer;
 
 import javax.annotation.Nullable;
@@ -49,6 +51,9 @@ public class FileMetadataTest {
 
   private AnalysisMode mode = mock(AnalysisMode.class);
 
+  @Rule
+  public LogTester logTester = new LogTester();
+
   @Test
   public void empty_file() throws Exception {
     File tempFile = temp.newFile();
@@ -255,4 +260,19 @@ public class FileMetadataTest {
     assertThat(hash1).isNotEqualTo(hash2);
   }
 
+  @Test
+  public void binary_file_with_unmappable_character() throws Exception {
+    File woff = new File(this.getClass().getResource("glyphicons-halflings-regular.woff").toURI());
+
+    FileMetadata.Metadata metadata = new FileMetadata(mode).read(woff, Charsets.UTF_8);
+    assertThat(metadata.lines).isEqualTo(135);
+    assertThat(metadata.nonBlankLines).isEqualTo(134);
+    assertThat(metadata.hash).isNotEmpty();
+    assertThat(metadata.empty).isFalse();
+
+    assertThat(logTester.logs(LoggerLevel.WARN).get(0)).contains("Invalid character encountered in file");
+    assertThat(logTester.logs(LoggerLevel.WARN).get(0)).contains(
+      "glyphicons-halflings-regular.woff at line 1 for encoding UTF-8. Please fix file content or configure the encoding to be used using property 'sonar.sourceEncoding'.");
+  }
+
 }
diff --git a/sonar-batch/src/test/resources/org/sonar/batch/scan/filesystem/glyphicons-halflings-regular.woff b/sonar-batch/src/test/resources/org/sonar/batch/scan/filesystem/glyphicons-halflings-regular.woff
new file mode 100644
index 00000000000..2cc3e4852a5
--- /dev/null
+++ b/sonar-batch/src/test/resources/org/sonar/batch/scan/filesystem/glyphicons-halflings-regular.woff