CodeOwnerResolverResult: Include debug messages
Collect debug messages while resolving code owners so that we can make
them available to host admins if they need to investigate issues with
code owner config files that do not work as expected. Requires further
changes to expose this information to the host admins.
The debug messages are still written into the server logs on fine level
so that they are available in the logs when requests are traced.
Within CodeOwnerResolver the generation of debug messages is spread over
many methods. In order to be able to return them to the caller, we add
two new AutoValue classes (ResultWithMessages and
OptionalResultWithMessages) that wrap a result and also can contain
debug messages.
Adapt the CodeOwnerResolver tests to also verify the messages that
explain the result.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I12bb9d131815f461873e62d7a2b59b3e109b3055
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index da32379..8e1d906 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -19,6 +19,7 @@
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
@@ -42,6 +43,8 @@
import com.google.inject.Provider;
import java.io.IOException;
import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
@@ -156,7 +159,7 @@
PathCodeOwnersResult pathCodeOwnersResult =
pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).resolveCodeOwnerConfig();
return resolve(
- pathCodeOwnersResult.getPathCodeOwners(), pathCodeOwnersResult.hasUnresolvedImports());
+ pathCodeOwnersResult.getPathCodeOwners(), pathCodeOwnersResult.unresolvedImports());
}
}
@@ -178,22 +181,26 @@
* @see #resolve(CodeOwnerReference)
*/
public CodeOwnerResolverResult resolve(Set<CodeOwnerReference> codeOwnerReferences) {
- return resolve(codeOwnerReferences, /* hasUnresolvedImports= */ false);
+ return resolve(codeOwnerReferences, /* unresolvedImports= */ ImmutableList.of());
}
/**
* Resolves the given {@link CodeOwnerReference}s to {@link CodeOwner}s.
*
* @param codeOwnerReferences the code owner references that should be resolved
- * @param hasUnresolvedImports whether there are unresolved imports
+ * @param unresolvedImports list of unresolved imports
* @return the {@link CodeOwner} for the given code owner references
* @see #resolve(CodeOwnerReference)
*/
private CodeOwnerResolverResult resolve(
- Set<CodeOwnerReference> codeOwnerReferences, boolean hasUnresolvedImports) {
+ Set<CodeOwnerReference> codeOwnerReferences, List<UnresolvedImport> unresolvedImports) {
requireNonNull(codeOwnerReferences, "codeOwnerReferences");
+ requireNonNull(unresolvedImports, "unresolvedImports");
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
AtomicBoolean hasUnresolvedCodeOwners = new AtomicBoolean(false);
+ List<String> messages = new ArrayList<>();
+ unresolvedImports.forEach(
+ unresolvedImport -> messages.add(unresolvedImport.format(codeOwnersPluginConfiguration)));
ImmutableSet<CodeOwner> codeOwners =
codeOwnerReferences.stream()
.filter(
@@ -204,19 +211,27 @@
}
return true;
})
- .map(this::resolve)
+ .map(this::resolveWithMessages)
.filter(
- codeOwner -> {
- if (!codeOwner.isPresent()) {
+ resolveResult -> {
+ messages.addAll(resolveResult.messages());
+ if (!resolveResult.isPresent()) {
hasUnresolvedCodeOwners.set(true);
return false;
}
return true;
})
- .map(Optional::get)
+ .map(OptionalResultWithMessages::get)
.collect(toImmutableSet());
- return CodeOwnerResolverResult.create(
- codeOwners, ownedByAllUsers.get(), hasUnresolvedCodeOwners.get(), hasUnresolvedImports);
+ CodeOwnerResolverResult codeOwnerResolverResult =
+ CodeOwnerResolverResult.create(
+ codeOwners,
+ ownedByAllUsers.get(),
+ hasUnresolvedCodeOwners.get(),
+ !unresolvedImports.isEmpty(),
+ messages);
+ logger.atFine().log("resolve result = %s", codeOwnerResolverResult);
+ return codeOwnerResolverResult;
}
/**
@@ -261,33 +276,55 @@
* Optional#empty()}
*/
public Optional<CodeOwner> resolve(CodeOwnerReference codeOwnerReference) {
+ OptionalResultWithMessages<CodeOwner> resolveResult = resolveWithMessages(codeOwnerReference);
+ logger.atFine().log("resolve result = %s", resolveResult);
+ return resolveResult.result();
+ }
+
+ public OptionalResultWithMessages<CodeOwner> resolveWithMessages(
+ CodeOwnerReference codeOwnerReference) {
String email = requireNonNull(codeOwnerReference, "codeOwnerReference").email();
- logger.atFine().log("resolving code owner reference %s", codeOwnerReference);
- if (!isEmailDomainAllowed(email)) {
- logger.atFine().log("domain of email %s is not allowed", email);
- return Optional.empty();
+ List<String> messages = new ArrayList<>();
+ messages.add(String.format("resolving code owner reference %s", codeOwnerReference));
+
+ OptionalResultWithMessages<Boolean> emailDomainAllowedResult = isEmailDomainAllowed(email);
+ messages.addAll(emailDomainAllowedResult.messages());
+ if (!emailDomainAllowedResult.get()) {
+ return OptionalResultWithMessages.createEmpty(messages);
}
- Optional<AccountState> accountState =
- lookupEmail(email).flatMap(accountId -> lookupAccount(accountId, email));
- if (!accountState.isPresent()) {
- logger.atFine().log("no account for email %s", email);
- return Optional.empty();
- }
- if (!accountState.get().account().isActive()) {
- logger.atFine().log("account for email %s is inactive", email);
- return Optional.empty();
- }
- if (enforceVisibility && !isVisible(accountState.get(), email)) {
- logger.atFine().log(
- "account %d of email %s is not visible", accountState.get().account().id().get(), email);
- return Optional.empty();
+ OptionalResultWithMessages<Account.Id> lookupEmailResult = lookupEmail(email);
+ messages.addAll(lookupEmailResult.messages());
+ if (lookupEmailResult.isEmpty()) {
+ return OptionalResultWithMessages.createEmpty(messages);
}
- CodeOwner codeOwner = CodeOwner.create(accountState.get().account().id());
- logger.atFine().log("resolved to code owner %s", codeOwner);
- return Optional.of(codeOwner);
+ Account.Id accountId = lookupEmailResult.get();
+ OptionalResultWithMessages<AccountState> lookupAccountResult = lookupAccount(accountId, email);
+ messages.addAll(lookupAccountResult.messages());
+ if (lookupAccountResult.isEmpty()) {
+ return OptionalResultWithMessages.createEmpty(messages);
+ }
+
+ AccountState accountState = lookupAccountResult.get();
+ if (!accountState.account().isActive()) {
+ messages.add(String.format("account %s for email %s is inactive", accountId, email));
+ return OptionalResultWithMessages.createEmpty(messages);
+ }
+ if (enforceVisibility) {
+ OptionalResultWithMessages<Boolean> isVisibleResult = isVisible(accountState, email);
+ messages.addAll(isVisibleResult.messages());
+ if (!isVisibleResult.get()) {
+ return OptionalResultWithMessages.createEmpty(messages);
+ }
+ } else {
+ messages.add("code owner visibility is not checked");
+ }
+
+ CodeOwner codeOwner = CodeOwner.create(accountState.account().id());
+ messages.add(String.format("resolved to account %s", codeOwner.accountId()));
+ return OptionalResultWithMessages.create(codeOwner, messages);
}
/** Whether the given account can be seen. */
@@ -306,7 +343,7 @@
* @param email the email that should be looked up
* @return the ID of the account to which the email belongs if was found
*/
- private Optional<Account.Id> lookupEmail(String email) {
+ private OptionalResultWithMessages<Account.Id> lookupEmail(String email) {
ImmutableSet<ExternalId> extIds;
try {
extIds = externalIds.byEmail(email);
@@ -315,17 +352,17 @@
}
if (extIds.isEmpty()) {
- logger.atFine().log(
- "cannot resolve code owner email %s: no account with this email exists", email);
- return Optional.empty();
+ return OptionalResultWithMessages.createEmpty(
+ String.format(
+ "cannot resolve code owner email %s: no account with this email exists", email));
}
if (extIds.stream().map(ExternalId::accountId).distinct().count() > 1) {
- logger.atFine().log("cannot resolve code owner email %s: email is ambiguous", email);
- return Optional.empty();
+ return OptionalResultWithMessages.createEmpty(
+ String.format("cannot resolve code owner email %s: email is ambiguous", email));
}
- return Optional.of(extIds.stream().findFirst().get().accountId());
+ return OptionalResultWithMessages.create(extIds.stream().findFirst().get().accountId());
}
/**
@@ -336,16 +373,17 @@
* @param email the email that was resolved to the account ID
* @return the {@link AccountState} of the account with the given account ID, if it exists
*/
- private Optional<AccountState> lookupAccount(Account.Id accountId, String email) {
+ private OptionalResultWithMessages<AccountState> lookupAccount(
+ Account.Id accountId, String email) {
Optional<AccountState> accountState = accountCache.get(accountId);
if (!accountState.isPresent()) {
- logger.atFine().log(
- "cannot resolve code owner email %s: email belongs to account %s,"
- + " but no account with this ID exists",
- email, accountId);
- return Optional.empty();
+ return OptionalResultWithMessages.createEmpty(
+ String.format(
+ "cannot resolve code owner email %s: email belongs to account %s,"
+ + " but no account with this ID exists",
+ email, accountId));
}
- return accountState;
+ return OptionalResultWithMessages.create(accountState.get());
}
/**
@@ -365,14 +403,15 @@
* @return {@code true} if the given account and email are visible to the user, otherwise {@code
* false}
*/
- private boolean isVisible(AccountState accountState, String email) {
+ private OptionalResultWithMessages<Boolean> isVisible(AccountState accountState, String email) {
if (!canSee(accountState)) {
- logger.atFine().log(
- "cannot resolve code owner email %s: account %s is not visible to user %s",
- email,
- accountState.account().id(),
- user != null ? user.getLoggableName() : currentUser.get().getLoggableName());
- return false;
+ return OptionalResultWithMessages.create(
+ false,
+ String.format(
+ "cannot resolve code owner email %s: account %s is not visible to user %s",
+ email,
+ accountState.account().id(),
+ user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
}
if (!email.equals(accountState.account().preferredEmail())) {
@@ -380,14 +419,23 @@
if (user != null) {
if (user.hasEmailAddress(email)) {
- // it's a secondary email of the user, users can always see their own secondary emails
- return true;
+ return OptionalResultWithMessages.create(
+ true,
+ String.format(
+ "email %s is visible to user %s: email is a secondary email that is owned by this"
+ + " user",
+ email, user.getLoggableName()));
}
} else if (currentUser.get().isIdentifiedUser()
&& currentUser.get().asIdentifiedUser().hasEmailAddress(email)) {
// it's a secondary email of the calling user, users can always see their own secondary
// emails
- return true;
+ return OptionalResultWithMessages.create(
+ true,
+ String.format(
+ "email %s is visible to the calling user %s: email is a secondary email that is"
+ + " owned by this user",
+ email, currentUser.get().getLoggableName()));
}
// the email is a secondary email of another account, check if the user can see secondary
@@ -395,18 +443,33 @@
try {
if (user != null) {
if (!permissionBackend.user(user).test(GlobalPermission.MODIFY_ACCOUNT)) {
- logger.atFine().log(
- "cannot resolve code owner email %s: account %s is referenced by secondary email,"
- + " but user %s cannot see secondary emails",
- email, accountState.account().id(), user.getLoggableName());
- return false;
+ return OptionalResultWithMessages.create(
+ false,
+ String.format(
+ "cannot resolve code owner email %s: account %s is referenced by secondary email"
+ + " but user %s cannot see secondary emails",
+ email, accountState.account().id(), user.getLoggableName()));
}
+ return OptionalResultWithMessages.create(
+ true,
+ String.format(
+ "resolved code owner email %s: account %s is referenced by secondary email"
+ + " and user %s can see secondary emails",
+ email, accountState.account().id(), user.getLoggableName()));
} else if (!permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
- logger.atFine().log(
- "cannot resolve code owner email %s: account %s is referenced by secondary email,"
- + " but the calling user %s cannot see secondary emails",
- email, accountState.account().id(), currentUser.get().getLoggableName());
- return false;
+ return OptionalResultWithMessages.create(
+ false,
+ String.format(
+ "cannot resolve code owner email %s: account %s is referenced by secondary email"
+ + " but the calling user %s cannot see secondary emails",
+ email, accountState.account().id(), currentUser.get().getLoggableName()));
+ } else {
+ return OptionalResultWithMessages.create(
+ true,
+ String.format(
+ "resolved code owner email %s: account %s is referenced by secondary email"
+ + " and the calling user %s can see secondary emails",
+ email, accountState.account().id(), currentUser.get().getLoggableName()));
}
} catch (PermissionBackendException e) {
throw new StorageException(
@@ -415,8 +478,12 @@
e);
}
}
-
- return true;
+ return OptionalResultWithMessages.create(
+ true,
+ String.format(
+ "account %s is visible to user %s",
+ accountState.account().id(),
+ user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
}
/**
@@ -426,29 +493,30 @@
* @return {@code true} if the domain of the given email is allowed for code owners, otherwise
* {@code false}
*/
- public boolean isEmailDomainAllowed(String email) {
+ public OptionalResultWithMessages<Boolean> isEmailDomainAllowed(String email) {
requireNonNull(email, "email");
ImmutableSet<String> allowedEmailDomains =
codeOwnersPluginConfiguration.getAllowedEmailDomains();
if (allowedEmailDomains.isEmpty()) {
- // all domains are allowed
- return true;
+ return OptionalResultWithMessages.create(true, "all domains are allowed");
}
if (email.equals(ALL_USERS_WILDCARD)) {
- return true;
+ return OptionalResultWithMessages.create(true, "all users wildcard is allowed");
}
int emailAtIndex = email.lastIndexOf('@');
if (emailAtIndex >= 0 && emailAtIndex < email.length() - 1) {
String emailDomain = email.substring(emailAtIndex + 1);
- logger.atFine().log("email domain = %s", emailDomain);
- return allowedEmailDomains.contains(emailDomain);
+ boolean isEmailDomainAllowed = allowedEmailDomains.contains(emailDomain);
+ return OptionalResultWithMessages.create(
+ isEmailDomainAllowed,
+ String.format(
+ "domain %s of email %s is %s",
+ emailDomain, email, isEmailDomainAllowed ? "allowed" : "not allowed"));
}
- // email has no domain
- logger.atFine().log("email %s has no domain", email);
- return false;
+ return OptionalResultWithMessages.create(false, String.format("email %s has no domain", email));
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
index b5b71c0..4a9e372 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
@@ -18,8 +18,10 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Account;
+import java.util.List;
/** The result of resolving code owner references via {@link CodeOwnerResolver}. */
@AutoValue
@@ -49,6 +51,9 @@
/** Whether there are imports which couldn't be resolved. */
public abstract boolean hasUnresolvedImports();
+ /** Gets messages that were collected while resolving the code owners. */
+ public abstract ImmutableList<String> messages();
+
/**
* Whether there are any code owners defined for the path, regardless of whether they can be
* resolved or not.
@@ -67,6 +72,7 @@
.add("ownedByAllUsers", ownedByAllUsers())
.add("hasUnresolvedCodeOwners", hasUnresolvedCodeOwners())
.add("hasUnresolvedImports", hasUnresolvedImports())
+ .add("messages", messages())
.toString();
}
@@ -75,9 +81,14 @@
ImmutableSet<CodeOwner> codeOwners,
boolean ownedByAllUsers,
boolean hasUnresolvedCodeOwners,
- boolean hasUnresolvedImports) {
+ boolean hasUnresolvedImports,
+ List<String> messages) {
return new AutoValue_CodeOwnerResolverResult(
- codeOwners, ownedByAllUsers, hasUnresolvedCodeOwners, hasUnresolvedImports);
+ codeOwners,
+ ownedByAllUsers,
+ hasUnresolvedCodeOwners,
+ hasUnresolvedImports,
+ ImmutableList.copyOf(messages));
}
/** Creates a empty {@link CodeOwnerResolverResult} instance. */
@@ -86,6 +97,7 @@
/* codeOwners= */ ImmutableSet.of(),
/* ownedByAllUsers= */ false,
/* hasUnresolvedCodeOwners= */ false,
- /* hasUnresolvedImports= */ false);
+ /* hasUnresolvedImports= */ false,
+ /* messages= */ ImmutableList.of());
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OptionalResultWithMessages.java b/java/com/google/gerrit/plugins/codeowners/backend/OptionalResultWithMessages.java
new file mode 100644
index 0000000..66fce67
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OptionalResultWithMessages.java
@@ -0,0 +1,83 @@
+// Copyright (C) 2020 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.google.gerrit.plugins.codeowners.backend;
+
+import static java.util.Objects.requireNonNull;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * An optional result of an operation with optional messages.
+ *
+ * @param <T> type of the optional result
+ */
+@AutoValue
+public abstract class OptionalResultWithMessages<T> {
+ /** Gets the result. */
+ public abstract Optional<T> result();
+
+ /** Whether the result is present. */
+ public boolean isPresent() {
+ return result().isPresent();
+ }
+
+ /** Whether the result is empty. */
+ public boolean isEmpty() {
+ return !result().isPresent();
+ }
+
+ /** Returns the result value, if present. Fails if the result is not present. */
+ public T get() {
+ return result().get();
+ }
+
+ /** Gets the messages. */
+ public abstract ImmutableList<String> messages();
+
+ /** Creates a {@link OptionalResultWithMessages} instance without messages. */
+ public static <T> OptionalResultWithMessages<T> create(T result) {
+ return create(result, ImmutableList.of());
+ }
+
+ /** Creates an empty {@link OptionalResultWithMessages} instance with a single message. */
+ public static <T> OptionalResultWithMessages<T> createEmpty(String message) {
+ requireNonNull(message, "message");
+ return createEmpty(ImmutableList.of(message));
+ }
+
+ /** Creates an empty {@link OptionalResultWithMessages} instance with messages. */
+ public static <T> OptionalResultWithMessages<T> createEmpty(List<String> messages) {
+ requireNonNull(messages, "messages");
+ return new AutoValue_OptionalResultWithMessages<>(
+ Optional.empty(), ImmutableList.copyOf(messages));
+ }
+
+ /** Creates a {@link OptionalResultWithMessages} instance with messages. */
+ public static <T> OptionalResultWithMessages<T> create(T result, String message) {
+ requireNonNull(message, "message");
+ return create(result, ImmutableList.of(message));
+ }
+
+ /** Creates a {@link OptionalResultWithMessages} instance with messages. */
+ public static <T> OptionalResultWithMessages<T> create(T result, List<String> messages) {
+ requireNonNull(result, "result");
+ requireNonNull(messages, "messages");
+ return new AutoValue_OptionalResultWithMessages<>(
+ Optional.of(result), ImmutableList.copyOf(messages));
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/OptionalResultWithMessagesSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/OptionalResultWithMessagesSubject.java
new file mode 100644
index 0000000..a0a9082
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/testing/OptionalResultWithMessagesSubject.java
@@ -0,0 +1,72 @@
+// Copyright (C) 2020 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.google.gerrit.plugins.codeowners.testing;
+
+import static com.google.common.truth.Truth.assertAbout;
+import static com.google.gerrit.truth.OptionalSubject.optionals;
+
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.IterableSubject;
+import com.google.common.truth.Subject;
+import com.google.gerrit.plugins.codeowners.backend.OptionalResultWithMessages;
+
+/** {@link Subject} for doing assertions on {@link OptionalResultWithMessages}s. */
+public class OptionalResultWithMessagesSubject extends Subject {
+ /**
+ * Starts fluent chain to do assertions on a {@link OptionalResultWithMessages}.
+ *
+ * @param optionalResultWithMessages the optionalResultWithMessages instance on which assertions
+ * should be done
+ * @return the created {@link OptionalResultWithMessagesSubject}
+ */
+ public static OptionalResultWithMessagesSubject assertThat(
+ OptionalResultWithMessages<?> optionalResultWithMessages) {
+ return assertAbout(optionalResultsWithMessages()).that(optionalResultWithMessages);
+ }
+
+ /**
+ * Creates subject factory for mapping {@link OptionalResultWithMessages}s to {@link
+ * OptionalResultWithMessagesSubject}s.
+ */
+ private static Subject.Factory<OptionalResultWithMessagesSubject, OptionalResultWithMessages<?>>
+ optionalResultsWithMessages() {
+ return OptionalResultWithMessagesSubject::new;
+ }
+
+ private final OptionalResultWithMessages<?> optionalResultWithMessages;
+
+ private OptionalResultWithMessagesSubject(
+ FailureMetadata metadata, OptionalResultWithMessages<?> optionalResultWithMessages) {
+ super(metadata, optionalResultWithMessages);
+ this.optionalResultWithMessages = optionalResultWithMessages;
+ }
+
+ public void isPresent() {
+ check("result()").about(optionals()).that(optionalResultWithMessages().result()).isPresent();
+ }
+
+ public void isEmpty() {
+ check("result()").about(optionals()).that(optionalResultWithMessages().result()).isEmpty();
+ }
+
+ public IterableSubject hasMessagesThat() {
+ return check("messages()").that(optionalResultWithMessages().messages());
+ }
+
+ private OptionalResultWithMessages<?> optionalResultWithMessages() {
+ isNotNull();
+ return optionalResultWithMessages;
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index e659580..b2dd664 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -734,7 +734,7 @@
private Optional<CommitValidationMessage> validateCodeOwnerReference(
IdentifiedUser user, Path codeOwnerConfigFilePath, CodeOwnerReference codeOwnerReference) {
CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get().forUser(user);
- if (!codeOwnerResolver.isEmailDomainAllowed(codeOwnerReference.email())) {
+ if (!codeOwnerResolver.isEmailDomainAllowed(codeOwnerReference.email()).get()) {
return error(
String.format(
"the domain of the code owner email '%s' in '%s' is not allowed for code owners",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 1210ae5..0297426 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerSubject.assertThat;
+import static com.google.gerrit.plugins.codeowners.testing.OptionalResultWithMessagesSubject.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableSet;
@@ -82,25 +83,41 @@
@Test
public void resolveCodeOwnerReferenceForNonExistingEmail() throws Exception {
- assertThat(
- codeOwnerResolver.get().resolve(CodeOwnerReference.create("non-existing@example.com")))
- .isEmpty();
+ String nonExistingEmail = "non-existing@example.com";
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(nonExistingEmail));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "cannot resolve code owner email %s: no account with this email exists",
+ nonExistingEmail));
}
@Test
public void resolveCodeOwnerReferenceForEmail() throws Exception {
- Optional<CodeOwner> codeOwner =
- codeOwnerResolver.get().resolve(CodeOwnerReference.create(admin.email()));
- assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(admin.id());
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(String.format("account %s is visible to user %s", admin.id(), admin.username()));
}
@Test
public void cannotResolveCodeOwnerReferenceForStarAsEmail() throws Exception {
- Optional<CodeOwner> codeOwner =
+ OptionalResultWithMessages<CodeOwner> result =
codeOwnerResolver
.get()
- .resolve(CodeOwnerReference.create(CodeOwnerResolver.ALL_USERS_WILDCARD));
- assertThat(codeOwner).isEmpty();
+ .resolveWithMessages(CodeOwnerReference.create(CodeOwnerResolver.ALL_USERS_WILDCARD));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "cannot resolve code owner email %s: no account with this email exists",
+ CodeOwnerResolver.ALL_USERS_WILDCARD));
}
@Test
@@ -116,27 +133,47 @@
ExternalId.create(
"foo", "bar", user.id(), admin.email(), /* hashedPassword= */ null)));
- assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create(admin.email()))).isEmpty();
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format("cannot resolve code owner email %s: email is ambiguous", admin.email()));
}
@Test
public void resolveCodeOwnerReferenceForOrphanedEmail() throws Exception {
// Create an external ID with an email for a non-existing account.
String email = "foo.bar@example.com";
+ Account.Id accountId = Account.id(999999);
try (Repository allUsersRepo = repoManager.openRepository(allUsers);
MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
ExternalIdNotes extIdNotes = externalIdNotesFactory.load(allUsersRepo);
- extIdNotes.upsert(ExternalId.createEmail(Account.id(999999), email));
+ extIdNotes.upsert(ExternalId.createEmail(accountId, email));
extIdNotes.commit(md);
}
- assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create(email))).isEmpty();
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(email));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "cannot resolve code owner email %s: email belongs to account %s, but no account with this ID exists",
+ email, accountId));
}
@Test
public void resolveCodeOwnerReferenceForInactiveUser() throws Exception {
accountOperations.account(user.id()).forUpdate().inactive().update();
- assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create(user.email()))).isEmpty();
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(user.email()));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(String.format("account %s for email %s is inactive", user.id(), user.email()));
}
@Test
@@ -149,7 +186,15 @@
// user2 cannot see the admin account since they do not share any group and
// "accounts.visibility" is set to "SAME_GROUP".
- assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create(admin.email()))).isEmpty();
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "cannot resolve code owner email %s: account %s is not visible to user %s",
+ admin.email(), admin.id(), user2.username()));
}
@Test
@@ -162,33 +207,57 @@
// admin has the "Modify Account" global capability and hence can see the secondary email of the
// user account.
- Optional<CodeOwner> codeOwner =
- codeOwnerResolver.get().resolve(CodeOwnerReference.create(secondaryEmail));
- assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id());
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(user.id());
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "resolved code owner email %s: account %s is referenced by secondary email and the calling user %s can see secondary emails",
+ secondaryEmail, user.id(), admin.username()));
// admin has the "Modify Account" global capability and hence can see the secondary email of the
// user account if another user is the calling user
requestScopeOperations.setApiUser(user2.id());
- codeOwner =
+ result =
codeOwnerResolver
.get()
.forUser(identifiedUserFactory.create(admin.id()))
- .resolve(CodeOwnerReference.create(secondaryEmail));
- assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id());
+ .resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(user.id());
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "resolved code owner email %s: account %s is referenced by secondary email and user %s can see secondary emails",
+ secondaryEmail, user.id(), admin.username()));
// user can see its own secondary email.
requestScopeOperations.setApiUser(user.id());
- codeOwner = codeOwnerResolver.get().resolve(CodeOwnerReference.create(secondaryEmail));
- assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id());
+ result = codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(user.id());
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "email %s is visible to the calling user %s: email is a secondary email that is owned by this user",
+ secondaryEmail, user.username()));
// user can see its own secondary email if another user is the calling user.
requestScopeOperations.setApiUser(user2.id());
- codeOwner =
+ result =
codeOwnerResolver
.get()
.forUser(identifiedUserFactory.create(user.id()))
- .resolve(CodeOwnerReference.create(secondaryEmail));
- assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id());
+ .resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(user.id());
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "email %s is visible to user %s: email is a secondary email that is owned by this user",
+ secondaryEmail, user.username()));
}
@Test
@@ -200,18 +269,31 @@
// user doesn't have the "Modify Account" global capability and hence cannot see the secondary
// email of the admin account.
requestScopeOperations.setApiUser(user.id());
- assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create(secondaryEmail)))
- .isEmpty();
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "cannot resolve code owner email %s: account %s is referenced by secondary email but the calling user %s cannot see secondary emails",
+ secondaryEmail, admin.id(), user.username()));
// user doesn't have the "Modify Account" global capability and hence cannot see the secondary
// email of the admin account if another user is the calling user
requestScopeOperations.setApiUser(admin.id());
- assertThat(
- codeOwnerResolver
- .get()
- .forUser(identifiedUserFactory.create(user.id()))
- .resolve(CodeOwnerReference.create(secondaryEmail)))
- .isEmpty();
+ result =
+ codeOwnerResolver
+ .get()
+ .forUser(identifiedUserFactory.create(user.id()))
+ .resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(
+ String.format(
+ "cannot resolve code owner email %s: account %s is referenced by secondary email but user %s cannot see secondary emails",
+ secondaryEmail, admin.id(), user.username()));
}
@Test
@@ -400,30 +482,43 @@
name = "plugin.code-owners.allowedEmailDomain",
values = {"example.com", "example.net"})
public void configuredEmailDomainsAreAllowed() throws Exception {
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.com")).isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.net")).isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.org@example.com"))
- .isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.org")).isFalse();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo")).isFalse();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.com@example.org"))
- .isFalse();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed(CodeOwnerResolver.ALL_USERS_WILDCARD))
- .isTrue();
+ assertIsEmailDomainAllowed(
+ "foo@example.com", true, "domain example.com of email foo@example.com is allowed");
+ assertIsEmailDomainAllowed(
+ "foo@example.net", true, "domain example.net of email foo@example.net is allowed");
+ assertIsEmailDomainAllowed(
+ "foo@example.org@example.com",
+ true,
+ "domain example.com of email foo@example.org@example.com is allowed");
+ assertIsEmailDomainAllowed(
+ "foo@example.org", false, "domain example.org of email foo@example.org is not allowed");
+ assertIsEmailDomainAllowed("foo", false, "email foo has no domain");
+ assertIsEmailDomainAllowed(
+ "foo@example.com@example.org",
+ false,
+ "domain example.org of email foo@example.com@example.org is not allowed");
+ assertIsEmailDomainAllowed(
+ CodeOwnerResolver.ALL_USERS_WILDCARD, true, "all users wildcard is allowed");
}
@Test
public void allEmailDomainsAreAllowed() throws Exception {
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.com")).isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.net")).isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.org@example.com"))
- .isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.org")).isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo")).isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed("foo@example.com@example.org"))
- .isTrue();
- assertThat(codeOwnerResolver.get().isEmailDomainAllowed(CodeOwnerResolver.ALL_USERS_WILDCARD))
- .isTrue();
+ String expectedMessage = "all domains are allowed";
+ assertIsEmailDomainAllowed("foo@example.com", true, expectedMessage);
+ assertIsEmailDomainAllowed("foo@example.net", true, expectedMessage);
+ assertIsEmailDomainAllowed("foo@example.org@example.com", true, expectedMessage);
+ assertIsEmailDomainAllowed("foo@example.org", true, expectedMessage);
+ assertIsEmailDomainAllowed("foo", true, expectedMessage);
+ assertIsEmailDomainAllowed("foo@example.com@example.org", true, expectedMessage);
+ assertIsEmailDomainAllowed(CodeOwnerResolver.ALL_USERS_WILDCARD, true, expectedMessage);
+ }
+
+ private void assertIsEmailDomainAllowed(
+ String email, boolean expectedResult, String expectedMessage) {
+ OptionalResultWithMessages<Boolean> isEmailDomainAllowedResult =
+ codeOwnerResolver.get().isEmailDomainAllowed(email);
+ assertThat(isEmailDomainAllowedResult.get()).isEqualTo(expectedResult);
+ assertThat(isEmailDomainAllowedResult.messages()).containsExactly(expectedMessage);
}
@Test
