Fix ISE when code ownership is assigned to 2 emails of the same account
If code ownership is assigned to the primary and secondary email of one
account at the same time, there was an ISE in CodeOwnerResolver when
trying to create the CodeOwner to annotations map [1] (because it cannot
have 2 entries with the same CodeOwner as key).
Rewrite this logic to avoid this issue and add tests to cover this case.
[1]
Caused by: java.lang.IllegalArgumentException: Multiple entries with same key: CodeOwner{accountId=100019}=[] and CodeOwner{accountId=100019}=[]
at com.google.common.collect.ImmutableMap.conflictException(ImmutableMap.java:226)
at com.google.common.collect.ImmutableMap.checkNoConflict(ImmutableMap.java:220)
at com.google.common.collect.RegularImmutableMap.checkNoConflictInKeyBucket(RegularImmutableMap.java:146)
at com.google.common.collect.RegularImmutableMap.fromEntryArray(RegularImmutableMap.java:109)
at com.google.common.collect.ImmutableMap$Builder.build(ImmutableMap.java:405)
at java.base/java.util.stream.ReferencePipeline.collect(Unknown Source)
at com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver.resolve(CodeOwnerResolver.java:451)
...
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ib159ebbd08f92d11945cc74c9774028f93d6722a
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 6ed577e..254b3a6 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -47,6 +47,8 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
@@ -439,6 +441,8 @@
hasUnresolvedCodeOwners.set(true);
}
+ Map<CodeOwner, Set<CodeOwnerAnnotation>> codeOwnersWithAnnotations = new HashMap<>();
+
// Merge code owners that have been newly resolved with code owners which have been looked up
// from cache and return them with their annotations.
Stream<Pair<String, CodeOwner>> newlyResolvedCodeOwnersStream =
@@ -447,22 +451,25 @@
cachedCodeOwnersByEmail.entrySet().stream()
.filter(e -> e.getValue().isPresent())
.map(e -> Pair.of(e.getKey(), e.getValue().get()));
- return Streams.concat(newlyResolvedCodeOwnersStream, cachedCodeOwnersStream)
- .collect(
- toImmutableMap(
- Pair::value,
- p -> {
- ImmutableSet.Builder<CodeOwnerAnnotation> annotationBuilder =
- ImmutableSet.builder();
+ Streams.concat(newlyResolvedCodeOwnersStream, cachedCodeOwnersStream)
+ .forEach(
+ p -> {
+ ImmutableSet.Builder<CodeOwnerAnnotation> annotationBuilder = ImmutableSet.builder();
- annotationBuilder.addAll(annotations.get(CodeOwnerReference.create(p.key())));
+ annotationBuilder.addAll(annotations.get(CodeOwnerReference.create(p.key())));
- // annotations for the all users wildcard (aka '*') apply to all code owners
- annotationBuilder.addAll(
- annotations.get(CodeOwnerReference.create(ALL_USERS_WILDCARD)));
+ // annotations for the all users wildcard (aka '*') apply to all code owners
+ annotationBuilder.addAll(
+ annotations.get(CodeOwnerReference.create(ALL_USERS_WILDCARD)));
- return annotationBuilder.build();
- }));
+ if (!codeOwnersWithAnnotations.containsKey(p.value())) {
+ codeOwnersWithAnnotations.put(p.value(), new HashSet<>());
+ }
+ codeOwnersWithAnnotations.get(p.value()).addAll(annotationBuilder.build());
+ });
+
+ return codeOwnersWithAnnotations.entrySet().stream()
+ .collect(toImmutableMap(Map.Entry::getKey, e -> ImmutableSet.copyOf(e.getValue())));
}
/**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 6c6df4f..06e5ab1 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -485,6 +485,36 @@
}
@Test
+ public void resolvePathCodeOwnersWithAnnotations_annotationOnMultipleEmailsOfTheSameUser()
+ throws Exception {
+ // add secondary email to user account
+ String secondaryEmail = "user@foo.bar";
+ accountOperations.account(user.id()).forUpdate().addSecondaryEmail(secondaryEmail).update();
+
+ CodeOwnerConfig codeOwnerConfig =
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(user.email())
+ .addAnnotation(user.email(), CodeOwnerAnnotation.create("FOO"))
+ .addCodeOwnerEmail(secondaryEmail)
+ .addAnnotation(secondaryEmail, CodeOwnerAnnotation.create("BAR"))
+ .build())
+ .build();
+
+ // admin has the "Modify Account" global capability and hence can see the secondary email of the
+ // user account.
+ CodeOwnerResolverResult result =
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(user.id());
+ assertThat(result.annotations().keySet()).containsExactly(CodeOwner.create(user.id()));
+ assertThat(result.annotations().get(CodeOwner.create(user.id())))
+ .containsExactly(CodeOwnerAnnotation.create("FOO"), CodeOwnerAnnotation.create("BAR"));
+ }
+
+ @Test
public void cannotResolvePathCodeOwnersOfNullCodeOwnerConfig() throws Exception {
NullPointerException npe =
assertThrows(
@@ -738,4 +768,24 @@
assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_cache_reads"))
.isEqualTo(1);
}
+
+ @Test
+ public void resolveCodeOwnerReferencesThatPointToTheSameAccount() throws Exception {
+ // add secondary email to user account
+ String secondaryEmail = "user@foo.bar";
+ accountOperations.account(user.id()).forUpdate().addSecondaryEmail(secondaryEmail).update();
+
+ // admin has the "Modify Account" global capability and hence can see the secondary email of the
+ // user account.
+ CodeOwnerResolverResult result =
+ codeOwnerResolverProvider
+ .get()
+ .resolve(
+ ImmutableSet.of(
+ CodeOwnerReference.create(user.email()),
+ CodeOwnerReference.create(secondaryEmail)));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(user.id());
+ assertThat(result.ownedByAllUsers()).isFalse();
+ assertThat(result.hasUnresolvedCodeOwners()).isFalse();
+ }
}
