SONAR-5531 Fix bug when adding a comment on an issue linked on a removed rule

diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
index 8a8ebbe593468a30b066cff7d89fa45058134d81..3344994ce30ff63c3b1ac0e377e3c57edcd9d6ee 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
@@ -247,7 +247,10 @@ public class IssueService implements ServerComponent {
       if (!ruleKey.isManual()) {
         throw new IllegalArgumentException("Issues can be created only on rules marked as 'manual': " + ruleKey);
       }
-      Rule rule = getRuleByKey(ruleKey);
+      Rule rule = getNullableRuleByKey(ruleKey);
+      if (rule == null) {
+        throw new IllegalArgumentException("Unknown rule: " + ruleKey);
+      }
 
       DefaultIssue issue = new DefaultIssueBuilder()
         .componentKey(component.getKey())
@@ -319,7 +322,7 @@ public class IssueService implements ServerComponent {
     }
     issueStorage.save(session, issue);
     issueNotifications.sendChanges(issue, context,
-      getRuleByKey(issue.ruleKey()),
+      getNullableRuleByKey(issue.ruleKey()),
       dbClient.componentDao().getByKey(session, projectKey),
       dbClient.componentDao().getNullableByKey(session, issue.componentKey()),
       comment);
@@ -328,13 +331,10 @@ public class IssueService implements ServerComponent {
 
   /**
    * Should use {@link org.sonar.server.rule.RuleService#getByKey(org.sonar.api.rule.RuleKey)}, but it's not possible as IssueNotifications is still used by the batch.
+   * Can be null for removed rules
    */
-  private Rule getRuleByKey(RuleKey ruleKey) {
-    Rule rule = ruleFinder.findByKey(ruleKey);
-    if (rule == null) {
-      throw new IllegalArgumentException("Unknown rule: " + ruleKey);
-    }
-    return rule;
+  private Rule getNullableRuleByKey(RuleKey ruleKey) {
+    return ruleFinder.findByKey(ruleKey);
   }
 
   public org.sonar.server.search.Result<Issue> search(IssueQuery query, QueryContext options) {

diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceMediumTest.java
index cb731587dc9353b1687092f0d0f8d7dca1340f9c..bfdf88108bf23bb8a0cf61cf57fa37f721bd7f04 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceMediumTest.java
@@ -24,7 +24,10 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Test;
+import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.internal.DefaultIssueComment;
+import org.sonar.api.rule.RuleKey;
+import org.sonar.api.rule.RuleStatus;
 import org.sonar.api.security.DefaultGroups;
 import org.sonar.api.web.UserRole;
 import org.sonar.core.component.ComponentDto;
@@ -109,4 +112,20 @@ public class IssueCommentServiceMediumTest {
     assertThat(comments.get(0).markdownText()).isEqualTo("my comment");
   }
 
+  @Test
+  public void add_comment_on_removed_issue() throws Exception {
+    RuleDto removedRule = RuleTesting.newDto(RuleKey.of("removed", "rule")).setStatus(RuleStatus.REMOVED);
+    tester.get(RuleDao.class).insert(session, removedRule);
+
+    IssueDto issue = IssueTesting.newDto(removedRule, file, project).setStatus(Issue.STATUS_CLOSED).setResolution(Issue.RESOLUTION_REMOVED);
+    tester.get(IssueDao.class).insert(session, issue);
+    session.commit();
+
+    service.addComment(issue.getKey(), "my comment", MockUserSession.get());
+
+    List<DefaultIssueComment> comments = service.findComments(issue.getKey());
+    assertThat(comments).hasSize(1);
+    assertThat(comments.get(0).markdownText()).isEqualTo("my comment");
+  }
+
 }

diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java
index b935d1fe797c3fa9533201487c62dac5387228df..c7141e421e4bc6cb02774ceb576325d2a0050d55 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java
@@ -28,6 +28,7 @@ import org.sonar.api.issue.DefaultTransitions;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.resources.Qualifiers;
 import org.sonar.api.resources.Scopes;
+import org.sonar.api.rule.RuleKey;
 import org.sonar.api.rule.Severity;
 import org.sonar.api.security.DefaultGroups;
 import org.sonar.api.utils.DateUtils;
@@ -342,6 +343,11 @@ public class IssueServiceMediumTest {
     }
   }
 
+  @Test(expected = IllegalArgumentException.class)
+  public void fail_create_manual_issue_if_rule_does_not_exists() {
+    service.createManualIssue(file.key(), RuleKey.of("rule", "unknown"), 10, "Fix it", null, 2d);
+  }
+
   @Test(expected = ForbiddenException.class)
   public void fail_create_manual_issue_if_not_having_required_role() {
     // User has not the 'user' role on the project

diff --git a/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java b/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java
index d084741ac9027cc2d1ec29e417fec6d7f819aca7..96e012b8e493eab076125d56e604d0cefc599567 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java
@@ -68,12 +68,13 @@ public class IssueNotifications implements BatchComponent, ServerComponent {
   }
 
   @CheckForNull
-  public List<Notification> sendChanges(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project, @Nullable Component component) {
+  public List<Notification> sendChanges(DefaultIssue issue, IssueChangeContext context, @Nullable Rule rule, Component project, @Nullable Component component) {
     return sendChanges(issue, context, rule, project, component, null);
   }
 
   @CheckForNull
-  public List<Notification> sendChanges(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project, @Nullable Component component, @Nullable String comment) {
+  public List<Notification> sendChanges(DefaultIssue issue, IssueChangeContext context, @Nullable Rule rule, Component project, @Nullable Component component,
+                                        @Nullable String comment) {
     Map<DefaultIssue, Rule> issues = Maps.newHashMap();
     issues.put(issue, rule);
     return sendChanges(issues, context, project, component, comment);
@@ -95,7 +96,7 @@ public class IssueNotifications implements BatchComponent, ServerComponent {
   }
 
   @CheckForNull
-  private Notification createChangeNotification(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project,
+  private Notification createChangeNotification(DefaultIssue issue, IssueChangeContext context, @Nullable Rule rule, Component project,
     @Nullable Component component, @Nullable String comment) {
     Notification notification = null;
     if (comment != null || issue.mustSendNotifications()) {