Use JSON arrays in REST API output format.
* Do not combine multiple owner/reviewer emails into one string.
* Do not encode multiple OwnerWeights counter into a string like "n1+n2+n3".
* Change the "reviewers" list to a simple list of emails,
without owner weight counters.
* Change the "owners" list to a list of OwnerInfo,
which contains "email" and "weights" attributes.
* Put OwnerWeights counters into an array.
* Change path2owners and owner2paths to
sorted maps of string to array of string.
* Change document, Java code, and JavaScript code to use the new JSON format.
Change-Id: Ia42c10364d19f7843409dee421d003ae1cb9ea72
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
index 834994e..13a1cc9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -92,14 +92,14 @@
return "?";
}
- private List<String> getOwners(OwnersDb db, Collection<String> files) {
+ private List<OwnerInfo> getOwners(OwnersDb db, Collection<String> files) {
Map<String, OwnerWeights> weights = new HashMap<>();
db.findOwners(files, weights);
- List<String> result = new ArrayList<>();
+ List<OwnerInfo> result = new ArrayList<>();
Set<String> emails = new HashSet<>();
for (String key : OwnerWeights.sortKeys(weights)) {
if (!emails.contains(key)) {
- result.add(key + " " + weights.get(key).encodeLevelCounts());
+ result.add(new OwnerInfo(key, weights.get(key).getLevelCounts()));
emails.add(key);
}
}
@@ -136,7 +136,7 @@
try {
for (Account.Id id : changeData.reviewers().all()) {
Account account = accountCache.get(id).getAccount();
- result.add(account.getPreferredEmail() + " []");
+ result.add(account.getPreferredEmail());
}
} catch (OrmException e) {
log.error("Exception", e);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnerInfo.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnerInfo.java
new file mode 100644
index 0000000..72894d0
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnerInfo.java
@@ -0,0 +1,28 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.findowners;
+
+import java.util.List;
+
+/** Keeps owner email and weights. */
+public class OwnerInfo {
+ OwnerInfo(String e, List<Integer> w) {
+ email = e;
+ weights = w;
+ }
+
+ String email;
+ List<Integer> weights;
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnerWeights.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnerWeights.java
index d57304b..233f1d2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnerWeights.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnerWeights.java
@@ -14,6 +14,7 @@
package com.googlesource.gerrit.plugins.findowners;
+import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
@@ -64,6 +65,11 @@
return "[" + countL1 + "+" + countL2 + "+" + countL3 + "]";
}
+ /** Return file counters as a list of integers. */
+ List<Integer> getLevelCounts() {
+ return ImmutableList.of(countL1, countL2, countL3);
+ }
+
OwnerWeights(String file, int level) {
addFile(file, level);
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
index b0b227c..f91ecbe 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
@@ -35,9 +35,9 @@
String ownerRevision;
DebugMessages dbgmsgs;
- SortedMap<String, String> file2owners = new TreeMap<>();
+ SortedMap<String, List<String>> file2owners = new TreeMap<>();
List<String> reviewers = new ArrayList<>();
- List<String> owners = new ArrayList<>();
+ List<OwnerInfo> owners = new ArrayList<>();
List<String> files = new ArrayList<>();
RestResult(int voteLevel, boolean addDebugMsg) {
@@ -54,7 +54,7 @@
String user;
String project;
String branch;
- SortedMap<String, String> path2owners;
- SortedMap<String, String> owner2paths;
+ SortedMap<String, List<String>> path2owners;
+ SortedMap<String, List<String>> owner2paths;
};
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Util.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Util.java
index 76a0e53..1888fa1 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Util.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Util.java
@@ -17,6 +17,7 @@
import com.google.common.collect.Ordering;
import java.io.File;
import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
@@ -46,10 +47,10 @@
return (s != null) && (s.equals("1") || s.equalsIgnoreCase("yes") || Boolean.parseBoolean(s));
}
- static SortedMap<String, String> makeSortedMap(Map<String, Set<String>> map) {
- SortedMap<String, String> result = new TreeMap<>();
+ static SortedMap<String, List<String>> makeSortedMap(Map<String, Set<String>> map) {
+ SortedMap<String, List<String>> result = new TreeMap<>();
for (String key : Ordering.natural().sortedCopy(map.keySet())) {
- result.put(key, String.join(" ", Ordering.natural().sortedCopy(map.get(key))));
+ result.put(key, Ordering.natural().sortedCopy(map.get(key)));
}
return result;
}
diff --git a/src/main/resources/Documentation/rest-api.md b/src/main/resources/Documentation/rest-api.md
index 34152d3..7676059 100644
--- a/src/main/resources/Documentation/rest-api.md
+++ b/src/main/resources/Documentation/rest-api.md
@@ -52,28 +52,29 @@
* **project**: the change's project name.
- * **branch**: the change's destination brach name.
+ * **branch**: the change's destination branch name.
* **path2owners**:
- a map from directory path or file glob to a string of owner emails
- separated by space. Note that `*` is a special owner email address.
+ a map from directory path or file glob to an array of owner emails.
+ Note that `*` is a special owner email address.
It means that there is no owner and anyone can be the owner.
Included directories are those affected by the change revision.
* **owner2paths**:
- a map from owner email to directory path or file glob.
+ a map from owner email to an array of directory path or file glob.
This is opposite to the path2owners map.
-* **file2owners**: a map from each file in the change patchset to
- the file owner emails, separated by space.
+* **file2owners**: a map from each file path in the change patchset to
+ an array of the file's owner emails.
-* **reviewers**: an array of current reviewer emails followed by
- optional extra information that should be ignored for now.
+* **reviewers**: an array of current reviewer emails.
-* **owners**: an array of owner emails followed by the owner weights,
- `[n1+n2+n3]`, which are the number of level 1, 2, 3+ controlled files.
- This list of owners are the keys in the owner2paths map.
- The array is sorted by owner weights.
+* **owners**: an array of owner info objects.
+ Each owner info object has "email" and "weights" attributes.
+ The weights attribute is an array of integers like [n1, n2, n3],
+ which are the number of level 1, 2, 3+ controlled files.
+ The email attributes are the keys in the owner2paths map.
+ This owners array is sorted by owner weights.
Users should try to pick owners with more weights to review a change.
* **files**: an alphabetically sorted files changed
diff --git a/src/main/resources/static/find-owners.js b/src/main/resources/static/find-owners.js
index 94d303f..8122474 100644
--- a/src/main/resources/static/find-owners.js
+++ b/src/main/resources/static/find-owners.js
@@ -176,7 +176,7 @@
}
function showFilesAndOwners(result, args) {
var sortedOwners = result.owners.map(
- function(line) { return line.split(' ')[0]; });
+ function(ownerInfo) { return ownerInfo.email; });
var groups = {};
// group name ==> {needReviewer, needApproval, owners}
var groupSize = {};
@@ -249,7 +249,7 @@
div.appendChild(nobr);
}
keys.forEach(function(key) {
- var owners = groups[key].owners;
+ var owners = groups[key].owners; // string of owner emails
var numFiles = groupSize[key];
var item = HTML_BULLET + ' <b>' + key + '</b>' +
((numFiles > 1) ? (' (' + numFiles + ' files):') : ':');
@@ -269,8 +269,11 @@
div.innerHTML = '';
div.style.display = 'inline';
div.appendChild(strElement(title));
- result.owners.sort().forEach(function(owner) {
- var email = owner.split(' ')[0];
+ function compareOwnerInfo(o1, o2) {
+ return o1.email.localeCompare(o2.email);
+ }
+ result.owners.sort(compareOwnerInfo).forEach(function(ownerInfo) {
+ var email = ownerInfo.email;
var vote = reviewerVote[email];
if ((email in reviewerVote) && vote != 0) {
email += ' <font color="' +
@@ -309,8 +312,8 @@
('minOwnerVoteLevel' in result ?
result['minOwnerVoteLevel'] : 1);
Object.keys(result.file2owners).sort().forEach(function(name) {
- var owners = result.file2owners[name];
- var splitOwners = owners.split(' ');
+ var splitOwners = result.file2owners[name];
+ var owners = splitOwners.join(' ');
if (owners in owners2group) {
groupSize[owners2group[owners]] += 1;
} else {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
index c8b3b3e..11274c6 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -79,15 +79,20 @@
assertThat(getOwnersResponse(c2)).contains("owners:[], files:[ t.c ]");
// Change of "t.c" file has owner after c1 is submitted.
approveSubmit(c1);
- assertThat(getOwnersResponse(c2)).contains("owners:[ a@a[1+0+0], x@x[1+0+0] ], files:[ t.c ]");
+ String ownerA = ownerJson("a@a");
+ String ownerB = ownerJson("b@b");
+ String ownerC = ownerJson("c@c");
+ String ownerX = ownerJson("x@x");
+ String ownersAX = "owners:[ " + ownerA + ", " + ownerX + " ]";
+ assertThat(getOwnersResponse(c2)).contains(ownersAX + ", files:[ t.c ]");
// A submitted change gets owners info from current repository.
- assertThat(getOwnersResponse(c1))
- .contains("owners:[ a@a[1+0+0], x@x[1+0+0] ], files:[ OWNERS ]");
+ assertThat(getOwnersResponse(c1)).contains(ownersAX + ", files:[ OWNERS ]");
// Check all fields in response.
String expectedTail =
- "path2owners:{ ./:a@ax@x }, owner2paths:{ a@a:./, x@x:./ } }"
- + ", file2owners:{ ./t.c:a@ax@x }, reviewers:[], owners:[ "
- + "a@a[1+0+0], x@x[1+0+0] ], files:[ t.c ] }";
+ "path2owners:{ ./:[ a@a, x@x ] }, owner2paths:{ a@a:[ ./ ], x@x:[ ./ ] } }"
+ + ", file2owners:{ ./t.c:[ a@a, x@x ] }, reviewers:[], "
+ + ownersAX
+ + ", files:[ t.c ] }";
assertThat(getOwnersDebugResponse(c2)).contains(expectedTail);
}
@@ -97,11 +102,15 @@
addFile("add OWNERS", "OWNERS", "per-file *.c=x@x\na@a\nc@c\nb@b\n");
// Add "t.c" file, which has per-file owner x@x, not a@a, b@b, c@c.
PushOneCommit.Result c2 = createChange("add t.c", "t.c", "Hello!");
- assertThat(getOwnersResponse(c2)).contains("owners:[ x@x[1+0+0] ], files:[ t.c ]");
+ String ownerA = ownerJson("a@a");
+ String ownerB = ownerJson("b@b");
+ String ownerC = ownerJson("c@c");
+ String ownerX = ownerJson("x@x");
+ assertThat(getOwnersResponse(c2)).contains("owners:[ " + ownerX + " ], files:[ t.c ]");
// Add "t.txt" file, which has new owners.
PushOneCommit.Result c3 = createChange("add t.txt", "t.txt", "Test!");
assertThat(getOwnersResponse(c3))
- .contains("owners:[ a@a[1+0+0], b@b[1+0+0], c@c[1+0+0] ], files:[ t.txt ]");
+ .contains("owners:[ " + ownerA + ", " + ownerB + ", " + ownerC + " ], files:[ t.txt ]");
}
@Test
@@ -113,25 +122,30 @@
addFile("add d3/OWNERS", "d3/OWNERS", "b@b\nset noparent\n");
// Add "t.c" file, which is not owned by subdirectory owners.
PushOneCommit.Result c2 = createChange("add t.c", "t.c", "Hello!");
- assertThat(getOwnersResponse(c2)).contains("owners:[ x@x[1+0+0] ], files:[ t.c ]");
+ String ownerA = ownerJson("a@a");
+ String ownerX = ownerJson("x@x");
+ assertThat(getOwnersResponse(c2)).contains("owners:[ " + ownerX + " ], files:[ t.c ]");
// Add "d1/t.c" file, which is owned by ./d1 and root owners.
PushOneCommit.Result c3 = createChange("add d1/t.c", "d1/t.c", "Hello!");
+ String ownerX010 = ownerJson("x@x", 0, 1, 0);
assertThat(getOwnersResponse(c3))
- .contains("owners:[ a@a[1+0+0], x@x[0+1+0] ], files:[ d1/t.c ]");
+ .contains("owners:[ " + ownerA + ", " + ownerX010 + " ], files:[ d1/t.c ]");
// Add "d2/t.c" file, which is owned by ./d2 and root owners.
PushOneCommit.Result c4 = createChange("add d2/t.c", "d2/t.c", "Hello!");
+ String ownerY = ownerJson("y@y");
assertThat(getOwnersResponse(c4))
- .contains("owners:[ y@y[1+0+0], x@x[0+1+0] ], files:[ d2/t.c ]");
+ .contains("owners:[ " + ownerY + ", " + ownerX010 + " ], files:[ d2/t.c ]");
// Add "d2/d1/t.c" file, which is owned by ./d2 and root owners.
PushOneCommit.Result c5 = createChange("add d2/d1/t.c", "d2/d1/t.c", "Hello!");
assertThat(getOwnersResponse(c5))
- .contains("owners:[ y@y[1+0+0], x@x[0+1+0] ], files:[ d2/d1/t.c ]");
+ .contains("owners:[ " + ownerY + ", " + ownerX010 + " ], files:[ d2/d1/t.c ]");
// Add "d3/t.c" file, which is owned only by ./d3 owners due to "set noparent".
PushOneCommit.Result c6 = createChange("add d3/t.c", "d3/t.c", "Hello!");
- assertThat(getOwnersResponse(c6)).contains("owners:[ b@b[1+0+0] ], files:[ d3/t.c ]");
+ String ownerB = ownerJson("b@b");
+ assertThat(getOwnersResponse(c6)).contains("owners:[ " + ownerB + " ], files:[ d3/t.c ]");
// Add "d3/d1/t.c" file, which is owned only by ./d3 owners due to "set noparent".
PushOneCommit.Result c7 = createChange("add d3/d1/t.c", "d3/d1/t.c", "Hello!");
- assertThat(getOwnersResponse(c7)).contains("owners:[ b@b[1+0+0] ], files:[ d3/d1/t.c ]");
+ assertThat(getOwnersResponse(c7)).contains("owners:[ " + ownerB + " ], files:[ d3/d1/t.c ]");
}
@Test
@@ -212,8 +226,10 @@
assertThat(Config.getOwnersFileName(pA)).isEqualTo("OWNERS");
assertThat(Config.getOwnersFileName(pB)).isEqualTo("OWNERS");
- assertThat(getOwnersResponse(cA)).contains("owners:[ x@x[1+0+0] ], files:[ tA.c ]");
- assertThat(getOwnersResponse(cB)).contains("owners:[ y@y[1+0+0] ], files:[ tB.c ]");
+ String ownerX = oneOwnerList("x@x");
+ String ownerY = oneOwnerList("y@y");
+ assertThat(getOwnersResponse(cA)).contains(ownerX + ", files:[ tA.c ]");
+ assertThat(getOwnersResponse(cB)).contains(ownerY + ", files:[ tB.c ]");
// Change owners file name to "OWNERS.alpha" and "OWNERS.beta"
switchProject(pA);
@@ -222,21 +238,23 @@
setProjectConfig("ownersFileName", "OWNERS.beta");
assertThat(Config.getOwnersFileName(pA)).isEqualTo("OWNERS.alpha");
assertThat(Config.getOwnersFileName(pB)).isEqualTo("OWNERS.beta");
- assertThat(getOwnersResponse(cA)).contains("owners:[ a@a[1+0+0] ], files:[ tA.c ]");
- assertThat(getOwnersResponse(cB)).contains("owners:[ b@b[1+0+0] ], files:[ tB.c ]");
+ String ownerA = oneOwnerList("a@a");
+ String ownerB = oneOwnerList("b@b");
+ assertThat(getOwnersResponse(cA)).contains(ownerA + ", files:[ tA.c ]");
+ assertThat(getOwnersResponse(cB)).contains(ownerB + ", files:[ tB.c ]");
// Change back to OWNERS in Project_A
switchProject(pA);
setProjectConfig("ownersFileName", "OWNERS");
assertThat(Config.getOwnersFileName(pA)).isEqualTo("OWNERS");
- assertThat(getOwnersResponse(cA)).contains("owners:[ x@x[1+0+0] ], files:[ tA.c ]");
- assertThat(getOwnersResponse(cB)).contains("owners:[ b@b[1+0+0] ], files:[ tB.c ]");
+ assertThat(getOwnersResponse(cA)).contains(ownerX + ", files:[ tA.c ]");
+ assertThat(getOwnersResponse(cB)).contains(ownerB + ", files:[ tB.c ]");
// Change back to OWNERS.alpha in Project_B, but there is no OWNERS.alpha
switchProject(pB);
setProjectConfig("ownersFileName", "OWNERS.alpha");
assertThat(Config.getOwnersFileName(pB)).isEqualTo("OWNERS.alpha");
- assertThat(getOwnersResponse(cA)).contains("owners:[ x@x[1+0+0] ], files:[ tA.c ]");
+ assertThat(getOwnersResponse(cA)).contains(ownerX + ", files:[ tA.c ]");
assertThat(getOwnersResponse(cB)).contains("owners:[], files:[ tB.c ]");
// Do not accept empty string or all-white-spaces for ownersFileName.
@@ -279,6 +297,18 @@
assertThat(cache.size()).isEqualTo(0);
}
+ private String oneOwnerList(String email) {
+ return "owners:[ " + ownerJson(email) + " ]";
+ }
+
+ private String ownerJson(String email) {
+ return "{ email:" + email + ", weights:[ 1, 0, 0 ] }";
+ }
+
+ private String ownerJson(String email, int w1, int w2, int w3) {
+ return "{ email:" + email + ", weights:[ " + w1 + ", " + w2 + ", " + w3 + " ] }";
+ }
+
private ChangeInfo newChangeInfo(String subject) throws Exception {
// should be called with different subject
ChangeInput in = new ChangeInput();
