Merge "CodeOwnerApprovalCheck: Remove wrong shortcut for fallback code owners"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 0e34b9f..174b401 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -90,7 +90,7 @@
private final ChangedFiles changedFiles;
private final PureRevertCache pureRevertCache;
private final Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider;
- private final Provider<CodeOwnerResolver> codeOwnerResolver;
+ private final Provider<CodeOwnerResolver> codeOwnerResolverProvider;
private final ApprovalsUtil approvalsUtil;
private final CodeOwnerMetrics codeOwnerMetrics;
@@ -102,7 +102,7 @@
ChangedFiles changedFiles,
PureRevertCache pureRevertCache,
Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider,
- Provider<CodeOwnerResolver> codeOwnerResolver,
+ Provider<CodeOwnerResolver> codeOwnerResolverProvider,
ApprovalsUtil approvalsUtil,
CodeOwnerMetrics codeOwnerMetrics) {
this.permissionBackend = permissionBackend;
@@ -111,7 +111,7 @@
this.changedFiles = changedFiles;
this.pureRevertCache = pureRevertCache;
this.codeOwnerConfigHierarchyProvider = codeOwnerConfigHierarchyProvider;
- this.codeOwnerResolver = codeOwnerResolver;
+ this.codeOwnerResolverProvider = codeOwnerResolverProvider;
this.approvalsUtil = approvalsUtil;
this.codeOwnerMetrics = codeOwnerMetrics;
}
@@ -182,9 +182,10 @@
"checking if change %d in project %s is submittable",
changeNotes.getChangeId().get(), changeNotes.getProjectName());
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
+ CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get().enforceVisibility(false);
try {
boolean isSubmittable =
- !getFileStatuses(codeOwnerConfigHierarchy, changeNotes)
+ !getFileStatuses(codeOwnerConfigHierarchy, codeOwnerResolver, changeNotes)
.anyMatch(
fileStatus ->
(fileStatus.newPathStatus().isPresent()
@@ -204,6 +205,10 @@
codeOwnerConfigHierarchy.getCodeOwnerConfigCounters().getBackendReadCount());
codeOwnerMetrics.codeOwnerConfigCacheReadsPerChange.record(
codeOwnerConfigHierarchy.getCodeOwnerConfigCounters().getCacheReadCount());
+ codeOwnerMetrics.codeOwnerResolutionsPerChange.record(
+ codeOwnerResolver.getCodeOwnerCounters().getResolutionCount());
+ codeOwnerMetrics.codeOwnerConfigCacheReadsPerChange.record(
+ codeOwnerResolver.getCodeOwnerCounters().getCacheReadCount());
}
}
@@ -213,7 +218,7 @@
*
* @param start number of file statuses to skip
* @param limit the max number of file statuses that should be returned (0 = unlimited)
- * @see #getFileStatuses(CodeOwnerConfigHierarchy, ChangeNotes)
+ * @see #getFileStatuses(CodeOwnerConfigHierarchy, CodeOwnerResolver, ChangeNotes)
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
ChangeNotes changeNotes, int start, int limit)
@@ -225,7 +230,10 @@
"compute file statuses (project = %s, change = %d, start = %d, limit = %d)",
changeNotes.getProjectName(), changeNotes.getChangeId().get(), start, limit);
Stream<FileCodeOwnerStatus> fileStatuses =
- getFileStatuses(codeOwnerConfigHierarchyProvider.get(), changeNotes);
+ getFileStatuses(
+ codeOwnerConfigHierarchyProvider.get(),
+ codeOwnerResolverProvider.get().enforceVisibility(false),
+ changeNotes);
if (start > 0) {
fileStatuses = fileStatuses.skip(start);
}
@@ -265,7 +273,9 @@
* returned
*/
private Stream<FileCodeOwnerStatus> getFileStatuses(
- CodeOwnerConfigHierarchy codeOwnerConfigHierarchy, ChangeNotes changeNotes)
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
+ CodeOwnerResolver codeOwnerResolver,
+ ChangeNotes changeNotes)
throws ResourceConflictException, IOException, PatchListNotAvailableException,
DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
@@ -331,10 +341,7 @@
logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
CodeOwnerResolverResult globalCodeOwners =
- codeOwnerResolver
- .get()
- .enforceVisibility(false)
- .resolveGlobalCodeOwners(changeNotes.getProjectName());
+ codeOwnerResolver.resolveGlobalCodeOwners(changeNotes.getProjectName());
logger.atFine().log("global code owners = %s", globalCodeOwners);
ImmutableSet<Account.Id> reviewerAccountIds =
@@ -352,6 +359,7 @@
changedFile ->
getFileStatus(
codeOwnerConfigHierarchy,
+ codeOwnerResolver,
branch,
revision,
globalCodeOwners,
@@ -410,11 +418,14 @@
"fallbackCodeOwner = %s, isProjectOwner = %s", fallbackCodeOwners, isProjectOwner);
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
+ CodeOwnerResolver codeOwnerResolver =
+ codeOwnerResolverProvider.get().enforceVisibility(false);
return changedFiles.getOrCompute(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
changedFile ->
getFileStatus(
codeOwnerConfigHierarchy,
+ codeOwnerResolver,
branch,
revision,
/* globalCodeOwners= */ CodeOwnerResolverResult.createEmpty(),
@@ -467,6 +478,7 @@
private FileCodeOwnerStatus getFileStatus(
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
+ CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@@ -487,6 +499,7 @@
newPath ->
getPathCodeOwnerStatus(
codeOwnerConfigHierarchy,
+ codeOwnerResolver,
branch,
revision,
globalCodeOwners,
@@ -509,6 +522,7 @@
Optional.of(
getPathCodeOwnerStatus(
codeOwnerConfigHierarchy,
+ codeOwnerResolver,
branch,
revision,
globalCodeOwners,
@@ -529,6 +543,7 @@
private PathCodeOwnerStatus getPathCodeOwnerStatus(
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
+ CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@@ -569,7 +584,8 @@
absolutePath,
(PathCodeOwnersVisitor)
pathCodeOwners -> {
- CodeOwnerResolverResult codeOwners = resolveCodeOwners(pathCodeOwners);
+ CodeOwnerResolverResult codeOwners =
+ resolveCodeOwners(codeOwnerResolver, pathCodeOwners);
logger.atFine().log(
"code owners = %s (code owner config folder path = %s, file name = %s)",
codeOwners,
@@ -867,10 +883,13 @@
/**
* Resolves the given path code owners.
*
+ * @param codeOwnerResolver the {@code CodeOwnerResolver} that should be used to resolve code
+ * owners
* @param pathCodeOwners the path code owners that should be resolved
*/
- private CodeOwnerResolverResult resolveCodeOwners(PathCodeOwners pathCodeOwners) {
- return codeOwnerResolver.get().enforceVisibility(false).resolvePathCodeOwners(pathCodeOwners);
+ private CodeOwnerResolverResult resolveCodeOwners(
+ CodeOwnerResolver codeOwnerResolver, PathCodeOwners pathCodeOwners) {
+ return codeOwnerResolver.resolvePathCodeOwners(pathCodeOwners);
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 10b33c8..b363dd2 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -15,11 +15,13 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.Objects.requireNonNull;
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.collect.Iterables;
import com.google.common.flogger.FluentLogger;
@@ -42,11 +44,16 @@
import com.google.inject.Provider;
import java.io.IOException;
import java.nio.file.Path;
+import java.util.Collection;
import java.util.List;
+import java.util.Map;
+import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Function;
import java.util.function.Predicate;
+import java.util.stream.Stream;
/**
* Class to resolve {@link CodeOwnerReference}s to {@link CodeOwner}s.
@@ -81,6 +88,10 @@
* <li>users with the {@code Modify Account} global capability can see the secondary emails of all
* accounts
* </ul>
+ *
+ * <p>Resolved code owners are cached within this class so that each email needs to be resolved only
+ * once. To take advantage of this caching callers should reuse {@link CodeOwnerResolver} instances
+ * where possible.
*/
public class CodeOwnerResolver {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -96,6 +107,7 @@
private final PathCodeOwners.Factory pathCodeOwnersFactory;
private final CodeOwnerMetrics codeOwnerMetrics;
private final UnresolvedImportFormatter unresolvedImportFormatter;
+ private final TransientCodeOwnerCache transientCodeOwnerCache;
// Enforce visibility by default.
private boolean enforceVisibility = true;
@@ -115,7 +127,8 @@
AccountControl.Factory accountControlFactory,
PathCodeOwners.Factory pathCodeOwnersFactory,
CodeOwnerMetrics codeOwnerMetrics,
- UnresolvedImportFormatter unresolvedImportFormatter) {
+ UnresolvedImportFormatter unresolvedImportFormatter,
+ TransientCodeOwnerCache transientCodeOwnerCache) {
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.permissionBackend = permissionBackend;
this.currentUser = currentUser;
@@ -125,6 +138,7 @@
this.pathCodeOwnersFactory = pathCodeOwnersFactory;
this.codeOwnerMetrics = codeOwnerMetrics;
this.unresolvedImportFormatter = unresolvedImportFormatter;
+ this.transientCodeOwnerCache = transientCodeOwnerCache;
}
/**
@@ -137,6 +151,7 @@
public CodeOwnerResolver enforceVisibility(boolean enforceVisibility) {
logger.atFine().log("enforceVisibility = %s", enforceVisibility);
this.enforceVisibility = enforceVisibility;
+ transientCodeOwnerCache.clear();
return this;
}
@@ -158,6 +173,7 @@
public CodeOwnerResolver forUser(IdentifiedUser user) {
logger.atFine().log("user = %s", user.getLoggableName());
this.user = user;
+ transientCodeOwnerCache.clear();
return this;
}
@@ -244,10 +260,13 @@
/**
* Resolves the given {@link CodeOwnerReference}s to {@link CodeOwner}s.
*
+ * <p>The accounts for the given {@link CodeOwnerReference}s are loaded from the account cache in
+ * parallel (via {@link AccountCache#get(Set)}.
+ *
* @param codeOwnerReferences the code owner references that should be resolved
* @param unresolvedImports list of unresolved imports
* @param pathCodeOwnersMessages messages that were collected when resolving path code owners
- * @return the {@link CodeOwner} for the given code owner references
+ * @return the resolved code owner references as a {@link CodeOwnerResolverResult}
*/
private CodeOwnerResolverResult resolve(
Set<CodeOwnerReference> codeOwnerReferences,
@@ -267,19 +286,8 @@
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
AtomicBoolean hasUnresolvedCodeOwners = new AtomicBoolean(false);
ImmutableSet<CodeOwner> codeOwners =
- codeOwnerReferences.stream()
- .filter(filterOutAllUsersWildCard(ownedByAllUsers))
- .map(codeOwnerReference -> resolve(messageBuilder, codeOwnerReference))
- .filter(
- codeOwner -> {
- if (!codeOwner.isPresent()) {
- hasUnresolvedCodeOwners.set(true);
- return false;
- }
- return true;
- })
- .map(Optional::get)
- .collect(toImmutableSet());
+ resolve(messageBuilder, ownedByAllUsers, hasUnresolvedCodeOwners, codeOwnerReferences);
+
CodeOwnerResolverResult codeOwnerResolverResult =
CodeOwnerResolverResult.create(
codeOwners,
@@ -321,75 +329,103 @@
*/
public OptionalResultWithMessages<CodeOwner> resolveWithMessages(
CodeOwnerReference codeOwnerReference) {
+ requireNonNull(codeOwnerReference, "codeOwnerReference");
+
+ if (CodeOwnerResolver.ALL_USERS_WILDCARD.equals(codeOwnerReference.email())) {
+ return OptionalResultWithMessages.createEmpty(
+ String.format(
+ "cannot resolve code owner email %s: no account with this email exists",
+ CodeOwnerResolver.ALL_USERS_WILDCARD));
+ }
+
ImmutableList.Builder<String> messageBuilder = ImmutableList.builder();
- Optional<CodeOwner> codeOwner = resolve(messageBuilder, codeOwnerReference);
- return OptionalResultWithMessages.create(codeOwner, messageBuilder.build());
+ AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
+ AtomicBoolean hasUnresolvedCodeOwners = new AtomicBoolean(false);
+ ImmutableSet<CodeOwner> codeOwners =
+ resolve(
+ messageBuilder,
+ ownedByAllUsers,
+ hasUnresolvedCodeOwners,
+ ImmutableSet.of(codeOwnerReference));
+ ImmutableList<String> messages = messageBuilder.build();
+ if (codeOwners.isEmpty()) {
+ return OptionalResultWithMessages.createEmpty(messages);
+ }
+ return OptionalResultWithMessages.create(Iterables.getOnlyElement(codeOwners), messages);
}
/**
- * Resolves a {@link CodeOwnerReference} to a {@link CodeOwner}.
+ * Resolves the given {@link CodeOwnerReference}s to {@link CodeOwner}s.
*
- * <p>This method does not resolve {@link CodeOwnerReference}s that assign the code ownership to
- * all user by using {@link #ALL_USERS_WILDCARD} as email.
- *
- * <p>Debug messages are returned with the result.
+ * <p>The accounts for the given {@link CodeOwnerReference}s are loaded from the account cache in
+ * parallel (via {@link AccountCache#get(Set)}.
*
* @param messages a builder to which debug messages are added
- * @param codeOwnerReference the code owner reference that should be resolved
- * @return the code owner to which the given code owner reference was resolved, {@link
- * Optional#empty()} if the code owner reference couldn't be resolved
+ * @param ownedByAllUsers a flag that is set if any of the given {@link CodeOwnerReference}s
+ * assigns code ownership to all users
+ * @param hasUnresolvedCodeOwners a flag that is set any of the given {@link CodeOwnerReference}s
+ * cannot be resolved
+ * @param codeOwnerReferences the code owner references that should be resolved
+ * @return the resolved code owner references as a {@link CodeOwner}s
*/
- private Optional<CodeOwner> resolve(
- ImmutableList.Builder<String> messages, CodeOwnerReference codeOwnerReference) {
- String email = requireNonNull(codeOwnerReference, "codeOwnerReference").email();
+ private ImmutableSet<CodeOwner> resolve(
+ ImmutableList.Builder<String> messages,
+ AtomicBoolean ownedByAllUsers,
+ AtomicBoolean hasUnresolvedCodeOwners,
+ Set<CodeOwnerReference> codeOwnerReferences) {
+ requireNonNull(codeOwnerReferences, "codeOwnerReferences");
- if (!isEmailDomainAllowed(messages, email)) {
- return Optional.empty();
- }
+ ImmutableSet<String> emailsToResolve =
+ codeOwnerReferences.stream()
+ .map(CodeOwnerReference::email)
+ .filter(filterOutAllUsersWildCard(ownedByAllUsers))
+ .collect(toImmutableSet());
- Optional<ImmutableSet<ExternalId>> extIds = lookupExternalIdsForEmail(messages, email);
- if (!extIds.isPresent()) {
- return Optional.empty();
- }
+ ImmutableMap<String, Optional<CodeOwner>> cachedCodeOwnersByEmail =
+ transientCodeOwnerCache.get(emailsToResolve);
- ImmutableSet<AccountState> accountStates = lookupAccounts(messages, extIds.get());
- ImmutableSet<AccountState> activeAccountStates =
- removeInactiveAccounts(messages, email, accountStates);
- if (activeAccountStates.isEmpty()) {
- messages.add(
- String.format(
- "cannot resolve code owner email %s: no active account with this email found",
- email));
- return Optional.empty();
- }
+ ImmutableSet<String> emailsToLookup =
+ emailsToResolve.stream()
+ .filter(email -> !cachedCodeOwnersByEmail.containsKey(email))
+ .filter(filterOutEmailsWithNonAllowedDomains(messages))
+ .collect(toImmutableSet());
- if (activeAccountStates.size() > 1) {
- messages.add(String.format("cannot resolve code owner email %s: email is ambiguous", email));
- return Optional.empty();
- }
+ ImmutableMap<String, Collection<ExternalId>> externalIdsByEmail =
+ lookupExternalIds(messages, emailsToLookup);
- AccountState activeAccountState = Iterables.getOnlyElement(activeAccountStates);
+ Stream<Pair<String, AccountState>> accountsByEmail =
+ lookupAccounts(messages, externalIdsByEmail)
+ .map(removeInactiveAccounts(messages))
+ .filter(filterOutEmailsWithoutAccounts(messages))
+ .filter(filterOutAmbiguousEmails(messages))
+ .map(mapToOnlyAccount(messages));
+
if (enforceVisibility) {
- if (!canSee(activeAccountState)) {
- messages.add(
- String.format(
- "cannot resolve code owner email %s: account %s is not visible to user %s",
- email,
- activeAccountState.account().id(),
- user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
- return Optional.empty();
- }
-
- if (!isEmailVisible(messages, activeAccountState, email)) {
- return Optional.empty();
- }
+ accountsByEmail =
+ accountsByEmail
+ .filter(filterOutEmailsOfNonVisibleAccounts(messages))
+ .filter(filterOutNonVisibleSecondaryEmails(messages));
} else {
messages.add("code owner visibility is not checked");
}
- CodeOwner codeOwner = CodeOwner.create(activeAccountState.account().id());
- messages.add(String.format("resolved email %s to account %s", email, codeOwner.accountId()));
- return Optional.of(codeOwner);
+ ImmutableMap<String, CodeOwner> codeOwnersByEmail =
+ accountsByEmail.map(mapToCodeOwner()).collect(toImmutableMap(Pair::key, Pair::value));
+
+ if (codeOwnersByEmail.keySet().size() < emailsToResolve.size()) {
+ hasUnresolvedCodeOwners.set(true);
+ }
+
+ ImmutableSet<CodeOwner> cachedCodeOwners =
+ cachedCodeOwnersByEmail.values().stream()
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .collect(toImmutableSet());
+
+ ImmutableSet.Builder<CodeOwner> codeOwners = ImmutableSet.builder();
+ codeOwners.addAll(cachedCodeOwners);
+ codeOwners.addAll(codeOwnersByEmail.values());
+ return codeOwners.build();
}
/**
@@ -398,9 +434,9 @@
* @param ownedByAllUsers flag that is set if any of the emails is the all users wild card (aka
* {@code *})
*/
- private Predicate<CodeOwnerReference> filterOutAllUsersWildCard(AtomicBoolean ownedByAllUsers) {
- return codeOwnerReference -> {
- if (ALL_USERS_WILDCARD.equals(codeOwnerReference.email())) {
+ private Predicate<String> filterOutAllUsersWildCard(AtomicBoolean ownedByAllUsers) {
+ return email -> {
+ if (ALL_USERS_WILDCARD.equals(email)) {
ownedByAllUsers.set(true);
return false;
}
@@ -409,6 +445,25 @@
}
/**
+ * Creates a predicate to filter out emails that have a non-allowed email domain.
+ *
+ * <p>Which emails domains are allowed is controlled via the plugin configuration (see {@link
+ * com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginGlobalConfigSnapshot#getAllowedEmailDomains()}
+ *
+ * @param messages builder to which debug messages are added
+ */
+ private Predicate<String> filterOutEmailsWithNonAllowedDomains(
+ ImmutableList.Builder<String> messages) {
+ return email -> {
+ boolean isEmailDomainAllowed = isEmailDomainAllowed(messages, email);
+ if (!isEmailDomainAllowed) {
+ transientCodeOwnerCache.cacheNonResolvable(email);
+ }
+ return isEmailDomainAllowed;
+ };
+ }
+
+ /**
* Whether the domain of the given email is allowed for code owners.
*
* <p>Which emails domains are allowed is controlled via the plugin configuration (see {@link
@@ -470,64 +525,91 @@
}
/**
- * Looks up the external IDs for the given email.
+ * Looks up the external IDs for the given emails.
*
- * @param messages a builder to which debug messages are added
- * @param email the email for which the external IDs should be looked up
+ * <p>Looks up all emails from the external ID cache at once, which is more efficient than looking
+ * up external IDs for emails one by one (see {@link ExternalIds#byEmails(String...)}).
+ *
+ * @param messages builder to which debug messages are added
+ * @param emails the emails for which the external IDs should be looked up
+ * @return external IDs per email
*/
- private Optional<ImmutableSet<ExternalId>> lookupExternalIdsForEmail(
- ImmutableList.Builder<String> messages, String email) {
- ImmutableSet<ExternalId> extIds;
+ private ImmutableMap<String, Collection<ExternalId>> lookupExternalIds(
+ ImmutableList.Builder<String> messages, ImmutableSet<String> emails) {
try {
- extIds = externalIds.byEmail(email);
+ ImmutableMap<String, Collection<ExternalId>> extIdsByEmail =
+ externalIds.byEmails(emails.toArray(new String[0])).asMap();
+ emails.stream()
+ .filter(email -> !extIdsByEmail.containsKey(email))
+ .forEach(
+ email -> {
+ transientCodeOwnerCache.cacheNonResolvable(email);
+ messages.add(
+ String.format(
+ "cannot resolve code owner email %s: no account with this email exists",
+ email));
+ });
+ return extIdsByEmail;
} catch (IOException e) {
throw new CodeOwnersInternalServerErrorException(
- String.format("cannot resolve code owner email %s", email), e);
+ String.format("cannot resolve code owner emails: %s", emails), e);
}
-
- if (extIds.isEmpty()) {
- messages.add(
- String.format(
- "cannot resolve code owner email %s: no account with this email exists", email));
- return Optional.empty();
- }
-
- return Optional.of(extIds);
}
/**
* Looks up the accounts for the given external IDs.
*
- * @param messages a builder to which debug messages are added
- * @param extIds the external IDs for which the accounts should be looked up
+ * <p>Looks up all accounts from the account cache at once, which is more efficient than looking
+ * up accounts one by one (see {@link AccountCache#get(Set)}).
+ *
+ * @param messages builder to which debug messages are added
+ * @param externalIdsByEmail external IDs for which the accounts should be looked up
+ * @return account states per email
*/
- private ImmutableSet<AccountState> lookupAccounts(
- ImmutableList.Builder<String> messages, ImmutableSet<ExternalId> extIds) {
- return extIds.stream()
- .map(externalId -> lookupAccount(messages, externalId.accountId(), externalId.email()))
- .filter(Optional::isPresent)
- .map(Optional::get)
- .collect(toImmutableSet());
+ private Stream<Pair<String, Collection<AccountState>>> lookupAccounts(
+ ImmutableList.Builder<String> messages,
+ ImmutableMap<String, Collection<ExternalId>> externalIdsByEmail) {
+ ImmutableSet<Account.Id> accountIds =
+ externalIdsByEmail.values().stream()
+ .flatMap(Collection::stream)
+ .map(ExternalId::accountId)
+ .collect(toImmutableSet());
+ Map<Account.Id, AccountState> accounts = accountCache.get(accountIds);
+ return externalIdsByEmail.entrySet().stream()
+ .map(
+ e ->
+ Pair.of(
+ e.getKey(),
+ e.getValue().stream()
+ .map(
+ extId -> {
+ Account.Id accountId = extId.accountId();
+ AccountState accountState = accounts.get(accountId);
+ if (accountState == null) {
+ messages.add(
+ String.format(
+ "cannot resolve account %s for email %s: account does not"
+ + " exists",
+ accountId, e.getKey()));
+ }
+ return accountState;
+ })
+ .filter(Objects::nonNull)
+ .collect(toImmutableSet())));
}
/**
- * Looks up an account by account ID and returns the corresponding {@link AccountState} if it is
- * found.
+ * Creates a map function that removes inactive accounts from a {@code Pair<String,
+ * Collection<AccountState>>}.
*
- * @param messages a builder to which debug messages are added
- * @param accountId the ID of the account that should be looked up
- * @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
+ * <p>The pair which is provided as input to the function maps an email to a collection of account
+ * states.
+ *
+ * @param messages builder to which debug messages are added
*/
- private Optional<AccountState> lookupAccount(
- ImmutableList.Builder<String> messages, Account.Id accountId, String email) {
- Optional<AccountState> accountState = accountCache.get(accountId);
- if (!accountState.isPresent()) {
- messages.add(
- String.format(
- "cannot resolve account %s for email %s: account does not exists", accountId, email));
- }
- return accountState;
+ private Function<Pair<String, Collection<AccountState>>, Pair<String, Collection<AccountState>>>
+ removeInactiveAccounts(ImmutableList.Builder<String> messages) {
+ return e -> Pair.of(e.key(), removeInactiveAccounts(messages, e.key(), e.value()));
}
/**
@@ -541,7 +623,7 @@
private ImmutableSet<AccountState> removeInactiveAccounts(
ImmutableList.Builder<String> messages,
String email,
- ImmutableSet<AccountState> accountStates) {
+ Collection<AccountState> accountStates) {
return accountStates.stream()
.filter(
accountState -> {
@@ -557,6 +639,99 @@
.collect(toImmutableSet());
}
+ /**
+ * Creates a predicate to filter out emails without accounts.
+ *
+ * <p>The pair which is provided as input to the predicate maps an email to a collection of
+ * account states. If the collection of account states is empty, the email is filtered out.
+ *
+ * @param messages builder to which debug messages are added
+ */
+ private Predicate<Pair<String, Collection<AccountState>>> filterOutEmailsWithoutAccounts(
+ ImmutableList.Builder<String> messages) {
+ return e -> {
+ if (e.value().isEmpty()) {
+ String email = e.key();
+ transientCodeOwnerCache.cacheNonResolvable(email);
+ messages.add(
+ String.format(
+ "cannot resolve code owner email %s: no active account with this email found",
+ email));
+ return false;
+ }
+ return true;
+ };
+ }
+
+ /**
+ * Creates a predicate to filter out ambiguous emails (emails that belong to multiple accounts).
+ *
+ * <p>The pair which is provided as input to the predicate maps an email to a collection of
+ * account states. If the collection of account states contains more than 1 entry, the email is
+ * filtered out.
+ *
+ * @param messages builder to which debug messages are added
+ */
+ private Predicate<Pair<String, Collection<AccountState>>> filterOutAmbiguousEmails(
+ ImmutableList.Builder<String> messages) {
+ return e -> {
+ if (e.value().size() > 1) {
+ String email = e.key();
+ transientCodeOwnerCache.cacheNonResolvable(email);
+ messages.add(
+ String.format("cannot resolve code owner email %s: email is ambiguous", email));
+ return false;
+ }
+ return true;
+ };
+ }
+
+ /**
+ * Creates a map function that maps a {@code Pair<String, Collection<AccountState>>} to a {@code
+ * Pair<String, AccountState>}.
+ *
+ * <p>The pair which is provided as input to the function maps an email to a collection of account
+ * states, which must contain exactly one entry. As output the function returns a pair that maps
+ * the email to the only account state.
+ *
+ * @param messages builder to which debug messages are added
+ */
+ private Function<Pair<String, Collection<AccountState>>, Pair<String, AccountState>>
+ mapToOnlyAccount(ImmutableList.Builder<String> messages) {
+ return e -> {
+ String email = e.key();
+ AccountState accountState = Iterables.getOnlyElement(e.value());
+ messages.add(
+ String.format("resolved email %s to account %s", email, accountState.account().id()));
+ return Pair.of(email, accountState);
+ };
+ }
+
+ /**
+ * Creates a predicate to filter out emails that belong to non-visible accounts.
+ *
+ * @param messages builder to which debug messages are added
+ */
+ private Predicate<Pair<String, AccountState>> filterOutEmailsOfNonVisibleAccounts(
+ ImmutableList.Builder<String> messages) {
+ return e -> {
+ String email = e.key();
+ AccountState accountState = e.value();
+ if (!canSee(accountState)) {
+ transientCodeOwnerCache.cacheNonResolvable(email);
+ messages.add(
+ 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()));
+ return false;
+ }
+
+ return true;
+ };
+ }
+
/** Whether the given account can be seen. */
private boolean canSee(AccountState accountState) {
AccountControl accountControl =
@@ -565,12 +740,9 @@
}
/**
- * Checks whether the email is visible to the {@link #user} or the calling user (if {@link #user}
- * is unset).
+ * Creates a predicate to filter out non-visible secondary emails.
*
- * <p>Primary emails are always visible if the account is visible.
- *
- * <p>If the email is a secondary email it is only visible if
+ * <p>A secondary email is only visible if
*
* <ul>
* <li>it is owned by the {@link #user} or the calling user (if {@link #user} is unset)
@@ -578,16 +750,22 @@
* Modify Account} global capability
* </ul>
*
- * @param messages a builder to which debug messages are added
- * @param visibleAccountState account to which the given email belongs and that is visible to the
- * user
- * @param email email for which it should be checked if it is visible to the user
- * @return {@code true} if the given email is visible to the user, otherwise {@code false}
+ * @param messages builder to which debug messages are added
*/
- private boolean isEmailVisible(
- ImmutableList.Builder<String> messages, AccountState visibleAccountState, String email) {
- if (!email.equals(visibleAccountState.account().preferredEmail())) {
- // the email is a secondary email of the account
+ private Predicate<Pair<String, AccountState>> filterOutNonVisibleSecondaryEmails(
+ ImmutableList.Builder<String> messages) {
+ return e -> {
+ String email = e.key();
+ AccountState accountState = e.value();
+ if (email.equals(accountState.account().preferredEmail())) {
+ // the email is a primary email of the account
+ messages.add(
+ String.format(
+ "account %s is visible to user %s",
+ accountState.account().id(),
+ user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
+ return true;
+ }
if (user != null) {
if (user.hasEmailAddress(email)) {
@@ -615,46 +793,61 @@
try {
if (user != null) {
if (!permissionBackend.user(user).test(GlobalPermission.MODIFY_ACCOUNT)) {
+ transientCodeOwnerCache.cacheNonResolvable(email);
messages.add(
String.format(
"cannot resolve code owner email %s: account %s is referenced by secondary email"
+ " but user %s cannot see secondary emails",
- email, visibleAccountState.account().id(), user.getLoggableName()));
+ email, accountState.account().id(), user.getLoggableName()));
return false;
}
messages.add(
String.format(
"resolved code owner email %s: account %s is referenced by secondary email"
+ " and user %s can see secondary emails",
- email, visibleAccountState.account().id(), user.getLoggableName()));
+ email, accountState.account().id(), user.getLoggableName()));
return true;
} else if (!permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
+ transientCodeOwnerCache.cacheNonResolvable(email);
messages.add(
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, visibleAccountState.account().id(), currentUser.get().getLoggableName()));
+ email, accountState.account().id(), currentUser.get().getLoggableName()));
return false;
} else {
messages.add(
String.format(
"resolved code owner email %s: account %s is referenced by secondary email"
+ " and the calling user %s can see secondary emails",
- email, visibleAccountState.account().id(), currentUser.get().getLoggableName()));
+ email, accountState.account().id(), currentUser.get().getLoggableName()));
return true;
}
- } catch (PermissionBackendException e) {
+ } catch (PermissionBackendException ex) {
throw new CodeOwnersInternalServerErrorException(
String.format(
"failed to test the %s global capability", GlobalPermission.MODIFY_ACCOUNT),
- e);
+ ex);
}
- }
- messages.add(
- String.format(
- "account %s is visible to user %s",
- visibleAccountState.account().id(),
- user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
- return true;
+ };
+ }
+
+ /**
+ * Creates a map function that maps a {@code Pair<String, AccountState>} to a code owner.
+ *
+ * <p>The pair which is provided as input to the function maps an email to an account states.
+ */
+ private Function<Pair<String, AccountState>, Pair<String, CodeOwner>> mapToCodeOwner() {
+ return e -> {
+ String email = e.key();
+ CodeOwner codeOwner = CodeOwner.create(e.value().account().id());
+ transientCodeOwnerCache.cache(email, codeOwner);
+ return Pair.of(email, codeOwner);
+ };
+ }
+
+ /** Returns the counters for resolutions and cache reads of code owners. */
+ public TransientCodeOwnerCache.Counters getCodeOwnerCounters() {
+ return transientCodeOwnerCache.getCounters();
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/Pair.java b/java/com/google/gerrit/plugins/codeowners/backend/Pair.java
new file mode 100644
index 0000000..ecd0a2e
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/Pair.java
@@ -0,0 +1,35 @@
+// Copyright (C) 2021 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 com.google.auto.value.AutoValue;
+
+/**
+ * Key-value pair.
+ *
+ * @param <K> the type of the key
+ * @param <V> the type of the value
+ */
+@AutoValue
+public abstract class Pair<K, V> {
+
+ public abstract K key();
+
+ public abstract V value();
+
+ public static <K, V> Pair<K, V> of(K key, V value) {
+ return new AutoValue_Pair<>(key, value);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerCache.java b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerCache.java
new file mode 100644
index 0000000..0229860
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerCache.java
@@ -0,0 +1,117 @@
+// Copyright (C) 2021 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 com.google.common.collect.ImmutableMap.toImmutableMap;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
+import com.google.inject.Inject;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Class to cache resolved {@link CodeOwner}s within a request.
+ *
+ * <p>This cache is transient, which means the code owners stay cached only for the lifetime of the
+ * {@code TransientCodeOwnerCache} instance.
+ *
+ * <p><strong>Note</strong>: This class is not thread-safe.
+ */
+public class TransientCodeOwnerCache {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final Optional<Integer> maxCacheSize;
+ private final Counters counters;
+ private final HashMap<String, Optional<CodeOwner>> cache = new HashMap<>();
+
+ @Inject
+ TransientCodeOwnerCache(
+ CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
+ CodeOwnerMetrics codeOwnerMetrics) {
+ this.maxCacheSize =
+ codeOwnersPluginConfiguration.getGlobalConfig().getMaxCodeOwnerConfigCacheSize();
+ this.counters = new Counters(codeOwnerMetrics);
+ }
+
+ public ImmutableMap<String, Optional<CodeOwner>> get(Set<String> emails) {
+ ImmutableMap<String, Optional<CodeOwner>> cachedCodeOwnersByEmail =
+ cache.entrySet().stream()
+ .filter(e -> emails.contains(e.getKey()))
+ .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
+ counters.incrementCacheReads(cachedCodeOwnersByEmail.size());
+ return cachedCodeOwnersByEmail;
+ }
+
+ public void clear() {
+ cache.clear();
+ }
+
+ public void cacheNonResolvable(String email) {
+ cache(email, Optional.empty());
+ }
+
+ public void cache(String email, CodeOwner codeOwner) {
+ cache(email, Optional.of(codeOwner));
+ }
+
+ private void cache(String email, Optional<CodeOwner> codeOwner) {
+ counters.incrementResolutions();
+ if (!maxCacheSize.isPresent() || cache.size() < maxCacheSize.get()) {
+ cache.put(email, codeOwner);
+ } else if (maxCacheSize.isPresent()) {
+ logger.atWarning().atMostEvery(1, TimeUnit.DAYS).log(
+ "exceeded limit of %s", getClass().getSimpleName());
+ }
+ }
+
+ public Counters getCounters() {
+ return counters;
+ }
+
+ public static class Counters {
+ private final CodeOwnerMetrics codeOwnerMetrics;
+
+ private int resolutionCount;
+ private int cacheReadCount;
+
+ private Counters(CodeOwnerMetrics codeOwnerMetrics) {
+ this.codeOwnerMetrics = codeOwnerMetrics;
+ }
+
+ private void incrementCacheReads(long value) {
+ codeOwnerMetrics.countCodeOwnerCacheReads.incrementBy(value);
+ cacheReadCount++;
+ }
+
+ private void incrementResolutions() {
+ codeOwnerMetrics.countCodeOwnerResolutions.increment();
+ resolutionCount++;
+ }
+
+ public int getResolutionCount() {
+ return resolutionCount;
+ }
+
+ public int getCacheReadCount() {
+ return cacheReadCount;
+ }
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginGlobalConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginGlobalConfigSnapshot.java
index c466b8a..472df1a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginGlobalConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginGlobalConfigSnapshot.java
@@ -32,8 +32,10 @@
static final String KEY_ENABLE_EXPERIMENTAL_REST_ENDPOINTS = "enableExperimentalRestEndpoints";
@VisibleForTesting static final int DEFAULT_MAX_CODE_OWNER_CONFIG_CACHE_SIZE = 10000;
+ @VisibleForTesting static final int DEFAULT_MAX_CODE_OWNER_CACHE_SIZE = 10000;
private static final String KEY_MAX_CODE_OWNER_CONFIG_CACHE_SIZE = "maxCodeOwnerConfigCacheSize";
+ private static final String KEY_MAX_CODE_OWNER_CACHE_SIZE = "maxCodeOwnerCacheSize";
public interface Factory {
CodeOwnersPluginGlobalConfigSnapshot create();
@@ -115,30 +117,39 @@
*/
public Optional<Integer> getMaxCodeOwnerConfigCacheSize() {
if (maxCodeOwnerConfigCacheSize == null) {
- maxCodeOwnerConfigCacheSize = readMaxCodeOwnerConfigCacheSize();
+ maxCodeOwnerConfigCacheSize =
+ readMaxCacheSize(
+ KEY_MAX_CODE_OWNER_CONFIG_CACHE_SIZE, DEFAULT_MAX_CODE_OWNER_CONFIG_CACHE_SIZE);
}
return maxCodeOwnerConfigCacheSize;
}
- private Optional<Integer> readMaxCodeOwnerConfigCacheSize() {
+ /**
+ * Gets the maximum size for the {@link
+ * com.google.gerrit.plugins.codeowners.backend.TransientCodeOwnerConfigCache}.
+ *
+ * @return the maximum cache size, {@link Optional#empty()} if the cache size is not limited
+ */
+ public Optional<Integer> getMaxCodeOwnerCacheSize() {
+ if (maxCodeOwnerConfigCacheSize == null) {
+ maxCodeOwnerConfigCacheSize =
+ readMaxCacheSize(KEY_MAX_CODE_OWNER_CACHE_SIZE, DEFAULT_MAX_CODE_OWNER_CACHE_SIZE);
+ }
+ return maxCodeOwnerConfigCacheSize;
+ }
+
+ private Optional<Integer> readMaxCacheSize(String key, int defaultCacheSize) {
try {
int maxCodeOwnerConfigCacheSize =
- pluginConfigFactory
- .getFromGerritConfig(pluginName)
- .getInt(
- KEY_MAX_CODE_OWNER_CONFIG_CACHE_SIZE, DEFAULT_MAX_CODE_OWNER_CONFIG_CACHE_SIZE);
+ pluginConfigFactory.getFromGerritConfig(pluginName).getInt(key, defaultCacheSize);
return maxCodeOwnerConfigCacheSize > 0
? Optional.of(maxCodeOwnerConfigCacheSize)
: Optional.empty();
} catch (IllegalArgumentException e) {
logger.atWarning().withCause(e).log(
"Value '%s' in gerrit.config (parameter plugin.%s.%s) is invalid.",
- pluginConfigFactory
- .getFromGerritConfig(pluginName)
- .getString(KEY_MAX_CODE_OWNER_CONFIG_CACHE_SIZE),
- pluginName,
- KEY_MAX_CODE_OWNER_CONFIG_CACHE_SIZE);
- return Optional.empty();
+ pluginConfigFactory.getFromGerritConfig(pluginName).getString(key), pluginName, key);
+ return Optional.of(defaultCacheSize);
}
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index 89fac59..f3dd7b5 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -51,17 +51,21 @@
public final Timer0 runCodeOwnerSubmitRule;
// code owner config metrics
+ public final Histogram0 codeOwnerCacheReadsPerChange;
public final Histogram0 codeOwnerConfigBackendReadsPerChange;
public final Histogram0 codeOwnerConfigCacheReadsPerChange;
+ public final Histogram0 codeOwnerResolutionsPerChange;
public final Timer1<String> loadCodeOwnerConfig;
public final Timer0 readCodeOwnerConfig;
public final Timer1<String> parseCodeOwnerConfig;
// counter metrics
+ public final Counter0 countCodeOwnerCacheReads;
public final Counter0 countCodeOwnerConfigReads;
public final Counter0 countCodeOwnerConfigCacheReads;
public final Counter3<ValidationTrigger, ValidationResult, Boolean>
countCodeOwnerConfigValidations;
+ public final Counter0 countCodeOwnerResolutions;
public final Counter1<String> countCodeOwnerSubmitRuleErrors;
public final Counter0 countCodeOwnerSubmitRuleRuns;
public final Counter1<Boolean> countCodeOwnerSuggestions;
@@ -136,6 +140,9 @@
"run_code_owner_submit_rule", "Latency for running the code owner submit rule");
// code owner config metrics
+ this.codeOwnerCacheReadsPerChange =
+ createHistogram(
+ "code_owner_cache_reads_per_change", "Number of code owner cache reads per change");
this.codeOwnerConfigBackendReadsPerChange =
createHistogram(
"code_owner_config_backend_reads_per_change",
@@ -144,6 +151,9 @@
createHistogram(
"code_owner_config_cache_reads_per_change",
"Number of code owner config cache reads per change");
+ this.codeOwnerResolutionsPerChange =
+ createHistogram(
+ "code_owner_resolutions_per_change", "Number of code owner resolutions per change");
this.loadCodeOwnerConfig =
createTimerWithClassField(
"load_code_owner_config",
@@ -157,6 +167,9 @@
"read_code_owner_config", "Latency for reading a code owner config file");
// counter metrics
+ this.countCodeOwnerCacheReads =
+ createCounter(
+ "count_code_owner_config_reads", "Total number of code owner reads from cache");
this.countCodeOwnerConfigReads =
createCounter(
"count_code_owner_config_reads",
@@ -179,6 +192,8 @@
Field.ofBoolean("dry_run", (metadataBuilder, resolveAllUsers) -> {})
.description("Whether the validation was a dry run.")
.build());
+ this.countCodeOwnerResolutions =
+ createCounter("count_code_owner_resolutions", "Total number of code owner resolutions");
this.countCodeOwnerSubmitRuleErrors =
createCounter1(
"count_code_owner_submit_rule_errors",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index c2cd1dc..629f42c 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -21,6 +21,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestMetricMaker;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
@@ -51,12 +52,13 @@
@Inject @ServerInitiated private Provider<AccountsUpdate> accountsUpdate;
@Inject private AccountOperations accountOperations;
@Inject private ExternalIdNotes.Factory externalIdNotesFactory;
+ @Inject private TestMetricMaker testMetricMaker;
- private Provider<CodeOwnerResolver> codeOwnerResolver;
+ private Provider<CodeOwnerResolver> codeOwnerResolverProvider;
@Before
public void setUpCodeOwnersPlugin() throws Exception {
- codeOwnerResolver =
+ codeOwnerResolverProvider =
plugin.getSysInjector().getInstance(new Key<Provider<CodeOwnerResolver>>() {});
}
@@ -66,7 +68,7 @@
assertThrows(
NullPointerException.class,
() ->
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.resolve(/* codeOwnerReference= */ (CodeOwnerReference) null));
assertThat(npe).hasMessageThat().isEqualTo("codeOwnerReference");
@@ -75,7 +77,7 @@
assertThrows(
NullPointerException.class,
() ->
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.resolve(/* codeOwnerReferences= */ (Set<CodeOwnerReference>) null));
assertThat(npe).hasMessageThat().isEqualTo("codeOwnerReferences");
@@ -85,7 +87,9 @@
public void resolveCodeOwnerReferenceForNonExistingEmail() throws Exception {
String nonExistingEmail = "non-existing@example.com";
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(nonExistingEmail));
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(nonExistingEmail));
assertThat(result).isEmpty();
assertThat(result)
.hasMessagesThat()
@@ -98,7 +102,9 @@
@Test
public void resolveCodeOwnerReferenceForEmail() throws Exception {
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(admin.email()));
assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
assertThat(result)
.hasMessagesThat()
@@ -108,7 +114,7 @@
@Test
public void cannotResolveCodeOwnerReferenceForStarAsEmail() throws Exception {
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.resolveWithMessages(CodeOwnerReference.create(CodeOwnerResolver.ALL_USERS_WILDCARD));
assertThat(result).isEmpty();
@@ -138,7 +144,9 @@
accountOperations.account(user.id()).forUpdate().inactive().update();
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(admin.email()));
assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
}
@@ -156,7 +164,9 @@
"foo", "bar", user.id(), admin.email(), /* hashedPassword= */ null)));
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(admin.email()));
assertThat(result).isEmpty();
assertThat(result)
.hasMessagesThat()
@@ -177,7 +187,7 @@
}
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(email));
+ codeOwnerResolverProvider.get().resolveWithMessages(CodeOwnerReference.create(email));
assertThat(result).isEmpty();
assertThat(result)
.hasMessagesThat()
@@ -194,7 +204,9 @@
public void resolveCodeOwnerReferenceForInactiveUser() throws Exception {
accountOperations.account(user.id()).forUpdate().inactive().update();
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(user.email()));
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(user.email()));
assertThat(result).isEmpty();
assertThat(result)
.hasMessagesThat()
@@ -213,7 +225,9 @@
// user2 cannot see the admin account since they do not share any group and
// "accounts.visibility" is set to "SAME_GROUP".
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(admin.email()));
assertThat(result).isEmpty();
assertThat(result)
.hasMessagesThat()
@@ -234,7 +248,9 @@
// admin has the "Modify Account" global capability and hence can see the secondary email of the
// user account.
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
assertThat(result.get()).hasAccountIdThat().isEqualTo(user.id());
assertThat(result)
.hasMessagesThat()
@@ -247,7 +263,7 @@
// user account if another user is the calling user
requestScopeOperations.setApiUser(user2.id());
result =
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.forUser(identifiedUserFactory.create(admin.id()))
.resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
@@ -261,7 +277,10 @@
// user can see its own secondary email.
requestScopeOperations.setApiUser(user.id());
- result = codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ result =
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
assertThat(result.get()).hasAccountIdThat().isEqualTo(user.id());
assertThat(result)
.hasMessagesThat()
@@ -273,7 +292,7 @@
// user can see its own secondary email if another user is the calling user.
requestScopeOperations.setApiUser(user2.id());
result =
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.forUser(identifiedUserFactory.create(user.id()))
.resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
@@ -296,7 +315,9 @@
// email of the admin account.
requestScopeOperations.setApiUser(user.id());
OptionalResultWithMessages<CodeOwner> result =
- codeOwnerResolver.get().resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
assertThat(result).isEmpty();
assertThat(result)
.hasMessagesThat()
@@ -309,7 +330,7 @@
// email of the admin account if another user is the calling user
requestScopeOperations.setApiUser(admin.id());
result =
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.forUser(identifiedUserFactory.create(user.id()))
.resolveWithMessages(CodeOwnerReference.create(secondaryEmail));
@@ -328,7 +349,9 @@
CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
.build();
CodeOwnerResolverResult result =
- codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
assertThat(result.codeOwners()).isEmpty();
assertThat(result.ownedByAllUsers()).isFalse();
assertThat(result.hasUnresolvedCodeOwners()).isFalse();
@@ -342,7 +365,9 @@
.build();
CodeOwnerResolverResult result =
- codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id(), user.id());
assertThat(result.ownedByAllUsers()).isFalse();
assertThat(result.hasUnresolvedCodeOwners()).isFalse();
@@ -357,7 +382,9 @@
.build();
CodeOwnerResolverResult result =
- codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
assertThat(result.codeOwnersAccountIds()).isEmpty();
assertThat(result.ownedByAllUsers()).isTrue();
assertThat(result.hasUnresolvedCodeOwners()).isFalse();
@@ -372,7 +399,9 @@
admin.email(), "non-existing@example.com"))
.build();
CodeOwnerResolverResult result =
- codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id());
assertThat(result.ownedByAllUsers()).isFalse();
assertThat(result.hasUnresolvedCodeOwners()).isTrue();
@@ -388,7 +417,9 @@
"*", admin.email(), "non-existing@example.com"))
.build();
CodeOwnerResolverResult result =
- codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id());
assertThat(result.ownedByAllUsers()).isTrue();
assertThat(result.hasUnresolvedCodeOwners()).isTrue();
@@ -400,7 +431,7 @@
assertThrows(
NullPointerException.class,
() ->
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.resolvePathCodeOwners(/* codeOwnerConfig= */ null, Paths.get("/README.md")));
assertThat(npe).hasMessageThat().isEqualTo("codeOwnerConfig");
@@ -416,7 +447,7 @@
assertThrows(
NullPointerException.class,
() ->
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.resolvePathCodeOwners(codeOwnerConfig, /* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
@@ -427,7 +458,8 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> codeOwnerResolver.get().resolvePathCodeOwners(/* pathCodeOwners= */ null));
+ () ->
+ codeOwnerResolverProvider.get().resolvePathCodeOwners(/* pathCodeOwners= */ null));
assertThat(npe).hasMessageThat().isEqualTo("pathCodeOwners");
}
@@ -442,7 +474,7 @@
assertThrows(
IllegalStateException.class,
() ->
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.resolvePathCodeOwners(codeOwnerConfig, Paths.get(relativePath)));
assertThat(npe)
@@ -462,11 +494,11 @@
// 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(adminCodeOwnerReference)).isEmpty();
+ assertThat(codeOwnerResolverProvider.get().resolve(adminCodeOwnerReference)).isEmpty();
// if visibility is not enforced the code owner reference can be resolved regardless
Optional<CodeOwner> codeOwner =
- codeOwnerResolver.get().enforceVisibility(false).resolve(adminCodeOwnerReference);
+ codeOwnerResolverProvider.get().enforceVisibility(false).resolve(adminCodeOwnerReference);
assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(admin.id());
}
@@ -478,10 +510,10 @@
TestAccount user2 = accountCreator.user2();
// admin is the current user and can see the account
- assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create(user.email())))
+ assertThat(codeOwnerResolverProvider.get().resolve(CodeOwnerReference.create(user.email())))
.isPresent();
assertThat(
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.forUser(identifiedUserFactory.create(admin.id()))
.resolve(CodeOwnerReference.create(user.email())))
@@ -489,7 +521,7 @@
// user2 cannot see the account
assertThat(
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.forUser(identifiedUserFactory.create(user2.id()))
.resolve(CodeOwnerReference.create(user.email())))
@@ -499,7 +531,8 @@
@Test
@GerritConfig(name = "plugin.code-owners.allowedEmailDomain", value = "example.net")
public void resolveCodeOwnerReferenceForEmailWithNonAllowedEmailDomain() throws Exception {
- assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create("foo@example.com")))
+ assertThat(
+ codeOwnerResolverProvider.get().resolve(CodeOwnerReference.create("foo@example.com")))
.isEmpty();
}
@@ -508,7 +541,7 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> codeOwnerResolver.get().isEmailDomainAllowed(/* email= */ null));
+ () -> codeOwnerResolverProvider.get().isEmailDomainAllowed(/* email= */ null));
assertThat(npe).hasMessageThat().isEqualTo("email");
}
@@ -551,7 +584,7 @@
private void assertIsEmailDomainAllowed(
String email, boolean expectedResult, String expectedMessage) {
OptionalResultWithMessages<Boolean> isEmailDomainAllowedResult =
- codeOwnerResolver.get().isEmailDomainAllowed(email);
+ codeOwnerResolverProvider.get().isEmailDomainAllowed(email);
assertThat(isEmailDomainAllowedResult.get()).isEqualTo(expectedResult);
assertThat(isEmailDomainAllowedResult.messages()).containsExactly(expectedMessage);
}
@@ -559,7 +592,7 @@
@Test
public void resolveCodeOwnerReferences() throws Exception {
CodeOwnerResolverResult result =
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.resolve(
ImmutableSet.of(
@@ -573,7 +606,7 @@
@Test
public void resolveCodeOwnerReferencesNonResolveableCodeOwnersAreFilteredOut() throws Exception {
CodeOwnerResolverResult result =
- codeOwnerResolver
+ codeOwnerResolverProvider
.get()
.resolve(
ImmutableSet.of(
@@ -586,14 +619,64 @@
@Test
public void isResolvable() throws Exception {
- assertThat(codeOwnerResolver.get().isResolvable(CodeOwnerReference.create(admin.email())))
+ assertThat(
+ codeOwnerResolverProvider.get().isResolvable(CodeOwnerReference.create(admin.email())))
.isTrue();
}
@Test
public void isNotResolvable() throws Exception {
assertThat(
- codeOwnerResolver.get().isResolvable(CodeOwnerReference.create("unknown@example.com")))
+ codeOwnerResolverProvider
+ .get()
+ .isResolvable(CodeOwnerReference.create("unknown@example.com")))
.isFalse();
}
+
+ @Test
+ public void emailIsResolvedOnlyOnce() throws Exception {
+ testMetricMaker.reset();
+ CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get();
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_resolutions"))
+ .isEqualTo(1);
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_config_reads"))
+ .isEqualTo(0);
+
+ // Doing the same lookup again doesn't resolve the code owner again.
+ testMetricMaker.reset();
+ result = codeOwnerResolver.resolveWithMessages(CodeOwnerReference.create(admin.email()));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_resolutions"))
+ .isEqualTo(0);
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_config_reads"))
+ .isEqualTo(1);
+ }
+
+ @Test
+ public void nonExistingEmailIsResolvedOnlyOnce() throws Exception {
+ testMetricMaker.reset();
+ CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get();
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolver.resolveWithMessages(
+ CodeOwnerReference.create("non-existing@example.com"));
+ assertThat(result).isEmpty();
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_resolutions"))
+ .isEqualTo(1);
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_config_reads"))
+ .isEqualTo(0);
+
+ // Doing the same lookup again doesn't resolve the code owner again.
+ testMetricMaker.reset();
+ result =
+ codeOwnerResolver.resolveWithMessages(
+ CodeOwnerReference.create("non-existing@example.com"));
+ assertThat(result).isEmpty();
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_resolutions"))
+ .isEqualTo(0);
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_config_reads"))
+ .isEqualTo(1);
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginGlobalConfigSnapshotTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginGlobalConfigSnapshotTest.java
index e382566..6a5c03d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginGlobalConfigSnapshotTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginGlobalConfigSnapshotTest.java
@@ -85,7 +85,36 @@
@Test
@GerritConfig(name = "plugin.code-owners.maxCodeOwnerConfigCacheSize", value = "invalid")
public void maxCodeOwnerConfigCacheSize_invalidConfig() throws Exception {
- assertThat(cfgSnapshot().getMaxCodeOwnerConfigCacheSize()).isEmpty();
+ assertThat(cfgSnapshot().getMaxCodeOwnerConfigCacheSize())
+ .value()
+ .isEqualTo(CodeOwnersPluginGlobalConfigSnapshot.DEFAULT_MAX_CODE_OWNER_CONFIG_CACHE_SIZE);
+ }
+
+ @Test
+ public void codeOwnerCacheSizeIsLimitedByDefault() throws Exception {
+ assertThat(cfgSnapshot().getMaxCodeOwnerCacheSize())
+ .value()
+ .isEqualTo(CodeOwnersPluginGlobalConfigSnapshot.DEFAULT_MAX_CODE_OWNER_CACHE_SIZE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.maxCodeOwnerCacheSize", value = "0")
+ public void codeOwnerCacheSizeIsUnlimited() throws Exception {
+ assertThat(cfgSnapshot().getMaxCodeOwnerCacheSize()).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.maxCodeOwnerCacheSize", value = "10")
+ public void codeOwnerCacheSizeIsLimited() throws Exception {
+ assertThat(cfgSnapshot().getMaxCodeOwnerCacheSize()).value().isEqualTo(10);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.maxCodeOwnerCacheSize", value = "invalid")
+ public void maxCodeOwnerCacheSize_invalidConfig() throws Exception {
+ assertThat(cfgSnapshot().getMaxCodeOwnerCacheSize())
+ .value()
+ .isEqualTo(CodeOwnersPluginGlobalConfigSnapshot.DEFAULT_MAX_CODE_OWNER_CACHE_SIZE);
}
private CodeOwnersPluginGlobalConfigSnapshot cfgSnapshot() {
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index b005dc7..b39e732 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -546,6 +546,16 @@
code owner config files that are cached per request.\
By default `10000`.
+<a id="pluginCodeOwnersMaxCodeOwnerCacheSize">plugin.@PLUGIN@.maxCodeOwnerCacheSize</a>
+: When computing code owner file statuses for a change (e.g. to compute
+ the results for the code owners submit rule) emails that are mentioned
+ in relevant code owner config files need to be resolved to Gerrit
+ accounts. The resolved code owners are cached in memory for the time of
+ the request so that this resolution has to be done only once per email.\
+ This configuration parameter allows to set a limit for the number of
+ resolved code owners that are cached per request.\
+ By default `10000`.
+
# <a id="projectConfiguration">Project configuration in @PLUGIN@.config</a>
<a id="codeOwnersDisabled">codeOwners.disabled</a>
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 0c4ccf5..b4fc3af 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -48,10 +48,14 @@
## <a id="codeOwnerConfigMetrics"> Code Owner Config Metrics
+* `code_owner_cache_reads_per_change`:
+ Number of code owner cache reads per change.
* `code_owner_config_cache_reads_per_change`:
Number of code owner config cache reads per change.
* `code_owner_config_backend_reads_per_change`:
Number of code owner config backend reads per change.
+* `code_owner_resolutions_per_change`:
+ Number of code owner resolutions per change.
* `load_code_owner_config`:
Latency for loading a code owner config file (read + parse).
* `parse_code_owner_config`:
@@ -62,6 +66,8 @@
## <a id="counterMetrics"> Counter Metrics
* `count_code_owner_config_reads`:
+ Total number of code owner reads from cache.
+* `count_code_owner_config_reads`:
Total number of code owner config reads from backend.
* `count_code_owner_config_cache_reads`:
Total number of code owner config reads from cache.
@@ -73,6 +79,8 @@
The result of the validation.
* `dry_run`:
Whether the validation was a dry run.
+* `count_code_owner_resolutions
+ Total number of code owner resolutions.
* `count_code_owner_submit_rule_errors`:
Total number of code owner submit rule errors.
* `cause`:
diff --git a/ui/owner-status-column.js b/ui/owner-status-column.js
index ae56d36..b9f5631 100644
--- a/ui/owner-status-column.js
+++ b/ui/owner-status-column.js
@@ -48,6 +48,19 @@
};
class BaseEl extends CodeOwnersModelMixin(Polymer.Element) {
+ static get properties() {
+ return {
+ patchRange: Object,
+
+ hidden: {
+ type: Boolean,
+ reflectToAttribute: true,
+ computed: 'computeHidden(change, patchRange, ' +
+ 'model.status.newerPatchsetUploaded)',
+ },
+ };
+ }
+
computeHidden(change, patchRange, newerPatchsetUploaded) {
if ([change, patchRange, newerPatchsetUploaded].includes(undefined)) {
return true;
@@ -93,19 +106,6 @@
<div></div>
`;
}
-
- static get properties() {
- return {
- patchRange: Object,
-
- hidden: {
- type: Boolean,
- reflectToAttribute: true,
- computed: 'computeHidden(change, patchRange, ' +
- 'model.status.newerPatchsetUploaded)',
- },
- };
- }
}
customElements.define(OwnerStatusColumnHeader.is, OwnerStatusColumnHeader);
@@ -122,12 +122,6 @@
return {
path: String,
oldPath: String,
- patchRange: Object,
- hidden: {
- type: Boolean,
- reflectToAttribute: true,
- computed: 'computeHidden(change, patchRange)',
- },
ownerService: Object,
statusIcon: {
type: String,
