SONAR-8368 fix misleading Function.Context.setLine(Integer)

by replacing it by method unsetLine() as it was only used with null parameter

7 files changed, 51 insertions, 21 deletions

diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java
index 3e5b82b2772..6d1577420f9 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java
@@ -140,9 +140,10 @@ public class IssueFieldsSetter {
     return true;
   }
 
-  public boolean setLine(DefaultIssue issue, @Nullable Integer line) {
-    if (!Objects.equals(line, issue.line())) {
-      issue.setLine(line);
+  public boolean unsetLine(DefaultIssue issue) {
+    Integer currentValue = issue.line();
+    if (currentValue != null) {
+      issue.setLine(null);
       issue.setChanged(true);
       return true;
     }
@@ -152,7 +153,12 @@ public class IssueFieldsSetter {
   public boolean setPastLine(DefaultIssue issue, @Nullable Integer previousLine) {
     Integer currentLine = issue.line();
     issue.setLine(previousLine);
-    return setLine(issue, currentLine);
+    if (!Objects.equals(currentLine, previousLine)) {
+      issue.setLine(currentLine);
+      issue.setChanged(true);
+      return true;
+    }
+    return false;
   }
 
   public boolean setLocations(DefaultIssue issue, @Nullable Object locations) {
diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/Function.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/Function.java
index 0a161a335f5..9cc425cc881 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/Function.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/Function.java
@@ -36,7 +36,7 @@ interface Function {
 
     Context unsetCloseDate();
 
-    Context setLine(@Nullable Integer line);
+    Context unsetLine();
 
     Context setType(@Nullable RuleType type);
   }
diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/FunctionExecutor.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/FunctionExecutor.java
index 8d3f65e7c7f..14b408b4904 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/FunctionExecutor.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/FunctionExecutor.java
@@ -89,8 +89,8 @@ public class FunctionExecutor {
     }
 
     @Override
-    public Function.Context setLine(@Nullable Integer line) {
-      updater.setLine(issue, line);
+    public Function.Context unsetLine() {
+      updater.unsetLine(issue);
       return this;
     }
 
diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/SetClosed.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/SetClosed.java
index 3c0a386d72c..f5d0105c964 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/SetClosed.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/workflow/SetClosed.java
@@ -37,6 +37,6 @@ public enum SetClosed implements Function {
     // closed issues are not "tracked" -> the line number does not evolve anymore
     // when code changes. That's misleading for end-users, so line number
     // is unset.
-    context.setLine(null);
+    context.unsetLine();
   }
 }
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java
index 0be2e028f14..5d82ac516a9 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java
@@ -22,6 +22,7 @@ package org.sonar.server.issue;
 import java.util.Calendar;
 import java.util.Date;
 import java.util.Map;
+import java.util.Random;
 import org.apache.commons.lang.time.DateUtils;
 import org.junit.Rule;
 import org.junit.Test;
@@ -211,35 +212,58 @@ public class IssueFieldsSetterTest {
   }
 
   @Test
-  public void set_line() {
-    boolean updated = underTest.setLine(issue, 123);
+  public void unset_line() {
+    issue.setLine(new Random().nextInt());
+
+    boolean updated = underTest.unsetLine(issue);
+
     assertThat(updated).isTrue();
-    assertThat(issue.line()).isEqualTo(123);
+    assertThat(issue.isChanged()).isTrue();
+    assertThat(issue.line()).isNull();
     assertThat(issue.mustSendNotifications()).isFalse();
     // do not save change
     assertThat(issue.currentChange()).isNull();
   }
 
   @Test
+  public void unset_line_has_no_effect_if_line_is_already_null() {
+    issue.setLine(null);
+
+    boolean updated = underTest.unsetLine(issue);
+
+    assertThat(updated).isFalse();
+    assertThat(issue.line()).isNull();
+    assertThat(issue.isChanged()).isFalse();
+    assertThat(issue.currentChange()).isNull();
+    assertThat(issue.mustSendNotifications()).isFalse();
+  }
+
+  @Test
   public void set_past_line() {
     issue.setLine(42);
+
     boolean updated = underTest.setPastLine(issue, 123);
+
     assertThat(updated).isTrue();
+    assertThat(issue.isChanged()).isTrue();
     assertThat(issue.line()).isEqualTo(42);
     assertThat(issue.mustSendNotifications()).isFalse();
-
     // do not save change
     assertThat(issue.currentChange()).isNull();
   }
 
   @Test
-  public void line_is_not_changed() {
-    issue.setLine(123);
-    boolean updated = underTest.setLine(issue, 123);
+  public void set_past_line_has_no_effect_if_line_already_had_value() {
+    issue.setLine(42);
+
+    boolean updated = underTest.setPastLine(issue, 42);
+
     assertThat(updated).isFalse();
-    assertThat(issue.line()).isEqualTo(123);
-    assertThat(issue.currentChange()).isNull();
+    assertThat(issue.isChanged()).isFalse();
+    assertThat(issue.line()).isEqualTo(42);
     assertThat(issue.mustSendNotifications()).isFalse();
+    // do not save change
+    assertThat(issue.currentChange()).isNull();
   }
 
   @Test
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/SetCloseDateTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/SetCloseDateTest.java
index ee139df8786..ccd001e7bce 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/SetCloseDateTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/SetCloseDateTest.java
@@ -28,9 +28,9 @@ import static org.mockito.Mockito.verify;
 public class SetCloseDateTest {
   @Test
   public void should_set_close_date() {
-    SetCloseDate function = new SetCloseDate(true);
+    SetCloseDate function = SetCloseDate.INSTANCE;
     Function.Context context = mock(Function.Context.class);
     function.execute(context);
-    verify(context, times(1)).setCloseDate(true);
+    verify(context, times(1)).setCloseDate();
   }
 }
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/SetClosedTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/SetClosedTest.java
index 8c526c4b16b..54a76cb74e1 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/SetClosedTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/issue/workflow/SetClosedTest.java
@@ -31,7 +31,7 @@ import static org.sonar.server.issue.workflow.SetClosed.INSTANCE;
 
 public class SetClosedTest {
 
-  Function.Context context = mock(Function.Context.class);
+  private Function.Context context = mock(Function.Context.class);
 
   @Test
   public void should_resolve_as_fixed() {
@@ -54,6 +54,6 @@ public class SetClosedTest {
     Issue issue = new DefaultIssue().setBeingClosed(true).setLine(10);
     when(context.issue()).thenReturn(issue);
     INSTANCE.execute(context);
-    verify(context).setLine(null);
+    verify(context).unsetLine();
   }
 }