AbstractGetCodeOwnersForPath: Make code owner scorings accessible
So far AbstractGetCodeOwnersForPath didn't know which code owner had
which score. The total scores were internals of CodeOwnerScorings and
not exposed to AbstractGetCodeOwnersForPath.
Refactor the code so that AbstractGetCodeOwnersForPath gets access to
the total code owner scores. This is done for 2 reasons:
1. we want to add a highest-score-only option that only returns the code
owners with the highest score, for this AbstractGetCodeOwnersForPath
needs to know what is the highest score and which code owners have it
2. at some point in time we may want to return the total scores to the
client, for this AbstractGetCodeOwnersForPath needs to know the total
scores so that they can be included into the JSON that is returned to
the client
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Iafb1d32f03b6b7a828f83b8843b4407b6075bc2e
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
index 7410e84..84bddff 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
@@ -14,10 +14,13 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+
import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import java.util.Comparator;
import java.util.Set;
+import java.util.function.Function;
import java.util.stream.Collectors;
/**
@@ -40,13 +43,13 @@
}
/**
- * Returns a comparator to sort code owners by the collected scorings.
+ * Returns the total scorings for the given code owners.
*
- * <p>Code owners with higher scoring come first. The order of code owners with the same scoring
- * is undefined.
+ * @param codeOwners the code owners for which the scorings should be returned
*/
- public Comparator<CodeOwner> comparingByScorings() {
- return Comparator.comparingDouble(this::sumWeightedScorings).reversed();
+ public ImmutableMap<CodeOwner, Double> getScorings(ImmutableSet<CodeOwner> codeOwners) {
+ return codeOwners.stream()
+ .collect(toImmutableMap(Function.identity(), this::sumWeightedScorings));
}
/** Returns the sum of all weighted scorings that available for the given code owner. */
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index a100097..acb327e 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -19,6 +19,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
@@ -53,6 +54,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashSet;
import java.util.List;
@@ -231,12 +233,12 @@
ImmutableSet<CodeOwner> immutableCodeOwners = ImmutableSet.copyOf(codeOwners);
CodeOwnerScorings codeOwnerScorings =
createScorings(rsrc, immutableCodeOwners, distanceScoring.build());
+ ImmutableMap<CodeOwner, Double> scoredCodeOwners =
+ codeOwnerScorings.getScorings(immutableCodeOwners);
CodeOwnersInfo codeOwnersInfo = new CodeOwnersInfo();
codeOwnersInfo.codeOwners =
- codeOwnerJsonFactory
- .create(getFillOptions())
- .format(sortAndLimit(rsrc, codeOwnerScorings, immutableCodeOwners));
+ codeOwnerJsonFactory.create(getFillOptions()).format(sortAndLimit(rsrc, scoredCodeOwners));
codeOwnersInfo.ownedByAllUsers = ownedByAllUsers.get() ? true : null;
return Response.ok(codeOwnersInfo);
}
@@ -378,26 +380,26 @@
}
private ImmutableList<CodeOwner> sortAndLimit(
- R rsrc, CodeOwnerScorings scorings, ImmutableSet<CodeOwner> codeOwners) {
- return sortCodeOwners(rsrc, seed, scorings, codeOwners).limit(limit).collect(toImmutableList());
+ R rsrc, ImmutableMap<CodeOwner, Double> scoredCodeOwners) {
+ return sortCodeOwners(rsrc, seed, scoredCodeOwners).limit(limit).collect(toImmutableList());
}
/**
* Sorts the code owners.
*
- * <p>Code owners with higher distance score are returned first.
+ * <p>Code owners with higher score are returned first.
*
- * <p>The order of code owners with the same distance score is random.
+ * <p>The order of code owners with the same score is random.
*
* @param rsrc resource on which this REST endpoint is invoked
* @param seed seed that should be used to randomize the order
- * @param scorings the scorings for the code owners
- * @param codeOwners the code owners that should be sorted
+ * @param scoredCodeOwners the code owners with their scores
* @return the sorted code owners
*/
private Stream<CodeOwner> sortCodeOwners(
- R rsrc, Optional<Long> seed, CodeOwnerScorings scorings, ImmutableSet<CodeOwner> codeOwners) {
- return randomizeOrder(seed, codeOwners).sorted(scorings.comparingByScorings());
+ R rsrc, Optional<Long> seed, ImmutableMap<CodeOwner, Double> scoredCodeOwners) {
+ return randomizeOrder(seed, scoredCodeOwners.keySet())
+ .sorted(Comparator.comparingDouble(scoredCodeOwners::get).reversed());
}
/**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
index 1bb7090..daa2f41 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_REVIEWER_SCORING_VALUE;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NO_REVIEWER_SCORING_VALUE;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import java.util.ArrayList;
import org.junit.Test;
@@ -25,7 +26,7 @@
/** Tests for {@link CodeOwnerScorings}. */
public class CodeOwnerScoringsTest extends AbstractCodeOwnersTest {
@Test
- public void sortCodeOwnersByLowerValueIsBetterScore() throws Exception {
+ public void getScorings_lowerValueIsBetterScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
@@ -43,12 +44,20 @@
.putValueForCodeOwner(codeOwner2, 100)
.putValueForCodeOwner(codeOwner3, 0)
.build();
- codeOwners.sort(CodeOwnerScorings.create(distanceScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner3, codeOwner1, codeOwner2).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(distanceScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2, codeOwner3)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.DISTANCE.weight() * 0.5,
+ codeOwner2,
+ 0.0,
+ codeOwner3,
+ CodeOwnerScore.DISTANCE.weight() * 1.0);
}
@Test
- public void sortCodeOwnersByGreaterValueIsBetterScore() throws Exception {
+ public void getScorings_greaterValueIsBetterScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
@@ -63,12 +72,18 @@
.putValueForCodeOwner(codeOwner1, 0)
.putValueForCodeOwner(codeOwner2, 1)
.build();
- codeOwners.sort(CodeOwnerScorings.create(isReviewerScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(isReviewerScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE,
+ codeOwner2,
+ CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.IS_REVIEWER_SCORING_VALUE);
}
@Test
- public void sortCodeOwnersByMultipleScore() throws Exception {
+ public void getScorings_multipleScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
@@ -95,9 +110,6 @@
.putValueForCodeOwner(codeOwner3, NO_REVIEWER_SCORING_VALUE)
.build();
- codeOwners.sort(
- CodeOwnerScorings.create(distanceScoring, isReviewerScoring).comparingByScorings());
-
// Expected scorings:
// codeOwner1: DISTANCE(weight=1)=0.5, IS_REVIEWER(weight=2)=0.0
// -> total scoring: 0.5 * 1 + 0.0 * 2 = 0.5
@@ -105,12 +117,23 @@
// -> total scoring: 0.25 * 1 + 1.0 * 2 = 2.25
// codeOwner3: DISTANCE(weight=1)=1.0, IS_REVIEWER(weight=2)=0.0
// -> total scoring: 1.0 * 1 + 0.0 * 2 = 1.0
- // => expected order (highest total score first): codeOwner2, codeOwner3, codeOwner1
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner3, codeOwner1).inOrder();
+ CodeOwnerScorings codeOwnerScorings =
+ CodeOwnerScorings.create(distanceScoring, isReviewerScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2, codeOwner3)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.DISTANCE.weight() * 0.5
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE,
+ codeOwner2,
+ CodeOwnerScore.DISTANCE.weight() * 0.25
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.IS_REVIEWER_SCORING_VALUE,
+ codeOwner3,
+ CodeOwnerScore.DISTANCE.weight() * 1.0
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE);
}
@Test
- public void sortCodeOwnersByScoringsIfAnyCodeOwnerHasNoScoring() throws Exception {
+ public void getScoringsIfAnyCodeOwnerHasNoScoring() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
@@ -122,7 +145,9 @@
CodeOwnerScoring.builder(CodeOwnerScore.DISTANCE, 100)
.putValueForCodeOwner(codeOwner2, 50)
.build();
- codeOwners.sort(CodeOwnerScorings.create(distanceScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(distanceScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2)))
+ .containsExactly(codeOwner1, 0.0, codeOwner2, 0.5);
}
}
