From cd092a41d1eb70bf487a6345ae450d3e1cc6fa57 Mon Sep 17 00:00:00 2001
From: Duarte Meneses <duarte.meneses@sonarsource.com>
Date: Thu, 19 Apr 2018 17:28:53 +0200
Subject: [PATCH] Fix quality issues

---
 .../issue/internal/DefaultExternalIssue.java  |  1 +
 .../internal/DefaultExternalIssueTest.java    | 17 +++++++
 .../externalissue/ExternalIssueImporter.java  | 51 +++++++++++--------
 .../scanner/externalissue/package-info.java   | 24 +++++++++
 .../org/sonar/scanner/issue/ModuleIssues.java |  5 +-
 5 files changed, 74 insertions(+), 24 deletions(-)
 create mode 100644 sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/package-info.java

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/issue/internal/DefaultExternalIssue.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/issue/internal/DefaultExternalIssue.java
index ee68be1d242..9118a4a373c 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/issue/internal/DefaultExternalIssue.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/issue/internal/DefaultExternalIssue.java
@@ -72,6 +72,7 @@ public class DefaultExternalIssue extends AbstractDefaultIssue<DefaultExternalIs
     requireNonNull(this.ruleKey, "Rule key is mandatory on external issue");
     checkState(primaryLocation != null, "Primary location is mandatory on every external issue");
     checkState(primaryLocation.inputComponent().isFile(), "External issues must be located in files");
+    checkState(primaryLocation.message() != null, "External issues must have a message");
     checkState(severity != null, "Severity is mandatory on every external issue");
     checkState(type != null, "Type is mandatory on every external issue");
     storage.store(this);
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/batch/sensor/issue/internal/DefaultExternalIssueTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/batch/sensor/issue/internal/DefaultExternalIssueTest.java
index 079d4995f2b..dc77fbc63fb 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/batch/sensor/issue/internal/DefaultExternalIssueTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/batch/sensor/issue/internal/DefaultExternalIssueTest.java
@@ -100,6 +100,23 @@ public class DefaultExternalIssueTest {
     exception.expectMessage("External issues must be located in files");
     issue.save();
   }
+  
+  @Test
+  public void fail_to_store_if_primary_location_has_no_message() {
+    SensorStorage storage = mock(SensorStorage.class);
+    DefaultExternalIssue issue = new DefaultExternalIssue(storage)
+      .at(new DefaultIssueLocation()
+        .on(inputFile)
+        .at(inputFile.selectLine(1)))
+      .forRule(RuleKey.of("repo", "rule"))
+      .remediationEffortMinutes(10l)
+      .type(RuleType.BUG)
+      .severity(Severity.BLOCKER);
+
+    exception.expect(IllegalStateException.class);
+    exception.expectMessage("External issues must have a message");
+    issue.save();
+  }
 
   @Test
   public void fail_to_store_if_no_severity() {
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueImporter.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueImporter.java
index b68fa4c910f..7f0d15a0420 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueImporter.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueImporter.java
@@ -55,28 +55,8 @@ public class ExternalIssueImporter {
     int issueCount = 0;
 
     for (Issue issue : report.issues) {
-      NewExternalIssue externalIssue = context.newExternalIssue()
-        .forRule(RuleKey.of(issue.engineId, issue.ruleId))
-        .severity(Severity.valueOf(issue.severity))
-        .remediationEffortMinutes(20L)
-        .type(RuleType.valueOf(issue.type));
-
-      NewIssueLocation primary = fillLocation(context, externalIssue.newLocation(), issue.primaryLocation);
-      if (primary != null) {
-        knownFiles.add(issue.primaryLocation.filePath);
-        externalIssue.at(primary);
-        if (issue.secondaryLocations != null) {
-          for (Location l : issue.secondaryLocations) {
-            NewIssueLocation secondary = fillLocation(context, externalIssue.newLocation(), l);
-            if (secondary != null) {
-              externalIssue.addLocation(secondary);
-            }
-          }
-        }
+      if (importIssue(issue)) {
         issueCount++;
-        externalIssue.save();
-      } else {
-        unknownFiles.add(issue.primaryLocation.filePath);
       }
     }
 
@@ -88,6 +68,33 @@ public class ExternalIssueImporter {
     }
   }
 
+  private boolean importIssue(Issue issue) {
+    NewExternalIssue externalIssue = context.newExternalIssue()
+      .forRule(RuleKey.of(issue.engineId, issue.ruleId))
+      .severity(Severity.valueOf(issue.severity))
+      .remediationEffortMinutes(20L)
+      .type(RuleType.valueOf(issue.type));
+
+    NewIssueLocation primary = fillLocation(context, externalIssue.newLocation(), issue.primaryLocation);
+    if (primary != null) {
+      knownFiles.add(issue.primaryLocation.filePath);
+      externalIssue.at(primary);
+      if (issue.secondaryLocations != null) {
+        for (Location l : issue.secondaryLocations) {
+          NewIssueLocation secondary = fillLocation(context, externalIssue.newLocation(), l);
+          if (secondary != null) {
+            externalIssue.addLocation(secondary);
+          }
+        }
+      }
+      externalIssue.save();
+      return true;
+    } else {
+      unknownFiles.add(issue.primaryLocation.filePath);
+      return false;
+    }
+  }
+
   private static String pluralize(String msg, int count) {
     if (count == 1) {
       return msg;
@@ -118,7 +125,7 @@ public class ExternalIssueImporter {
   }
 
   @CheckForNull
-  private InputFile findFile(SensorContext context, String filePath) {
+  private static InputFile findFile(SensorContext context, String filePath) {
     return context.fileSystem().inputFile(context.fileSystem().predicates().hasPath(filePath));
   }
 
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/package-info.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/package-info.java
new file mode 100644
index 00000000000..e58ec069e25
--- /dev/null
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/package-info.java
@@ -0,0 +1,24 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 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.
+ */
+@ParametersAreNonnullByDefault
+package org.sonar.scanner.externalissue;
+
+import javax.annotation.ParametersAreNonnullByDefault;
+
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/issue/ModuleIssues.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/issue/ModuleIssues.java
index 1aeefadb7a6..d064b302f48 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/issue/ModuleIssues.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/issue/ModuleIssues.java
@@ -112,10 +112,11 @@ public class ModuleIssues {
   }
 
   private static ScannerReport.ExternalIssue createReportExternalIssue(ExternalIssue issue, int componentRef) {
-    String primaryMessage = Strings.isNullOrEmpty(issue.primaryLocation().message()) ? issue.ruleKey().toString() : issue.primaryLocation().message();
+    // primary location of an external issue must have a message
+    String primaryMessage = issue.primaryLocation().message();
     Severity severity = Severity.valueOf(issue.severity().name());
     IssueType issueType = IssueType.valueOf(issue.type().name());
-    
+
     ScannerReport.ExternalIssue.Builder builder = ScannerReport.ExternalIssue.newBuilder();
     ScannerReport.IssueLocation.Builder locationBuilder = IssueLocation.newBuilder();
     ScannerReport.TextRange.Builder textRangeBuilder = ScannerReport.TextRange.newBuilder();
-- 
2.39.5