Merge "Reject (robot) comments with ranges referring to line 0"

Documentation/rest-api-changes.txt

@@ -5386,10 +5386,10 @@ The `CommentRange` entity describes the range of an inline comment.
 [options="header",cols="1,^1,5"]
 |===========================
 |Field Name    ||Description
-|`start_line`        ||The start line number of the range.
+|`start_line`        ||The start line number of the range. (1-based, inclusive)
-|`start_character`   ||The character position in the start line.
+|`start_character`   ||The character position in the start line. (0-based, inclusive)
-|`end_line`          ||The end line number of the range.
+|`end_line`          ||The end line number of the range. (1-based, exclusive)
-|`end_character`     ||The character position in the end line.
+|`end_character`     ||The character position in the end line. (0-based, exclusive)
 |===========================
 
 [[commit-info]]

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java

@@ -29,7 +29,7 @@ public abstract class Comment {
   public String path;
   public Side side;
   public Integer parent;
-  public Integer line;
+  public Integer line; // value 0 or null indicates a file comment, normal lines start at 1
   public Range range;
   public String inReplyTo;
   public Timestamp updated;
@@ -37,15 +37,15 @@ public abstract class Comment {
   public Boolean unresolved;
 
   public static class Range {
-    public int startLine;
+    public int startLine; // 1-based, inclusive
-    public int startCharacter;
+    public int startCharacter; // 0-based, inclusive
-    public int endLine;
+    public int endLine; // 1-based, exclusive
-    public int endCharacter;
+    public int endCharacter; // 0-based, exclusive
 
     public boolean isValid() {
-      return startLine >= 0
+      return startLine > 0
           && startCharacter >= 0
-          && endLine >= 0
+          && endLine > 0
           && endCharacter >= 0
           && startLine <= endLine
           && (startLine != endLine || startCharacter <= endCharacter);

gerrit-extension-api/src/test/java/com/google/gerrit/extensions/client/RangeTest.java

@@ -62,6 +62,30 @@ public class RangeTest {
     assertThat(range).isInvalid();
   }
 
+  @Test
+  public void zeroStartLineResultsInInvalidRange() {
+    Comment.Range range = createRange(0, 2, 19, 10);
+    assertThat(range).isInvalid();
+  }
+
+  @Test
+  public void zeroEndLineResultsInInvalidRange() {
+    Comment.Range range = createRange(13, 2, 0, 10);
+    assertThat(range).isInvalid();
+  }
+
+  @Test
+  public void zeroStartCharacterResultsInValidRange() {
+    Comment.Range range = createRange(13, 0, 19, 10);
+    assertThat(range).isValid();
+  }
+
+  @Test
+  public void zeroEndCharacterResultsInValidRange() {
+    Comment.Range range = createRange(13, 31, 19, 0);
+    assertThat(range).isValid();
+  }
+
   @Test
   public void startLineGreaterThanEndLineResultsInInvalidRange() {
     Comment.Range range = createRange(20, 2, 19, 10);

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java

@@ -131,10 +131,10 @@ public class Comment {
   }
 
   public static class Range {
-    public int startLine;
+    public int startLine; // 1-based, inclusive
-    public int startChar;
+    public int startChar; // 0-based, inclusive
-    public int endLine;
+    public int endLine; // 1-based, exclusive
-    public int endChar;
+    public int endChar; // 0-based, exclusive
 
     public Range(Range r) {
       this(r.startLine, r.startChar, r.endLine, r.endChar);