Fix `OWNERS` file over-caching problem
Issue:
When 'OWENRS' file of the parent's refs/meta/config is modified after
child's own file the updates are not reflected in the child's PathOwners
content.
Solution:
Cache only the parsed 'OWNERS' file content so that it doesn't have to
be read over and over again from the disk however parent's hierarchy is
still traversed. In addition make sure that static EMPTY entry is only
used for reading.
Bug: Issue 16830
Change-Id: I6eed6cee12ae99ed95387fb1518129dc3aa1101b
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
index ab3a7f1..17e4347 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -213,11 +213,11 @@
OwnersMap ownersMap = new OwnersMap();
try {
// Using a `map` would have needed a try/catch inside the lamba, resulting in more code
- List<PathOwnersEntry> parentsPathOwnersEntries =
+ List<ReadOnlyPathOwnersEntry> parentsPathOwnersEntries =
getPathOwnersEntries(parentProjectsNames, RefNames.REFS_CONFIG, cache);
- PathOwnersEntry projectEntry =
- getPathOwnersEntry(project, repository, RefNames.REFS_CONFIG, cache);
- PathOwnersEntry rootEntry = getPathOwnersEntry(project, repository, branch, cache);
+ ReadOnlyPathOwnersEntry projectEntry =
+ getPathOwnersEntryOrEmpty(project, repository, RefNames.REFS_CONFIG, cache);
+ PathOwnersEntry rootEntry = getPathOwnersEntryOrNew(project, repository, branch, cache);
Map<String, PathOwnersEntry> entries = new HashMap<>();
PathOwnersEntry currentEntry = null;
@@ -267,41 +267,51 @@
}
}
- private List<PathOwnersEntry> getPathOwnersEntries(
+ private List<ReadOnlyPathOwnersEntry> getPathOwnersEntries(
List<Project.NameKey> projectNames, String branch, PathOwnersEntriesCache cache)
throws IOException, ExecutionException {
- ImmutableList.Builder<PathOwnersEntry> pathOwnersEntries = ImmutableList.builder();
+ ImmutableList.Builder<ReadOnlyPathOwnersEntry> pathOwnersEntries = ImmutableList.builder();
for (Project.NameKey projectName : projectNames) {
try (Repository repo = repositoryManager.openRepository(projectName)) {
pathOwnersEntries =
- pathOwnersEntries.add(getPathOwnersEntry(projectName.get(), repo, branch, cache));
+ pathOwnersEntries.add(
+ getPathOwnersEntryOrEmpty(projectName.get(), repo, branch, cache));
}
}
return pathOwnersEntries.build();
}
- private PathOwnersEntry getPathOwnersEntry(
+ private ReadOnlyPathOwnersEntry getPathOwnersEntryOrEmpty(
+ String project, Repository repo, String branch, PathOwnersEntriesCache cache)
+ throws ExecutionException {
+ return getPathOwnersEntry(project, repo, branch, cache)
+ .map(v -> (ReadOnlyPathOwnersEntry) v)
+ .orElse(PathOwnersEntry.EMPTY);
+ }
+
+ private PathOwnersEntry getPathOwnersEntryOrNew(
+ String project, Repository repo, String branch, PathOwnersEntriesCache cache)
+ throws ExecutionException {
+ return getPathOwnersEntry(project, repo, branch, cache).orElseGet(PathOwnersEntry::new);
+ }
+
+ private Optional<PathOwnersEntry> getPathOwnersEntry(
String project, Repository repo, String branch, PathOwnersEntriesCache cache)
throws ExecutionException {
String rootPath = "OWNERS";
- return cache.get(
- project,
- branch,
- rootPath,
- () ->
- getOwnersConfig(repo, rootPath, branch)
- .map(
- conf ->
- new PathOwnersEntry(
- rootPath,
- conf,
- accounts,
- Optional.empty(),
- Collections.emptySet(),
- Collections.emptySet(),
- Collections.emptySet(),
- Collections.emptySet()))
- .orElse(PathOwnersEntry.EMPTY));
+ return cache
+ .get(project, branch, rootPath, () -> getOwnersConfig(repo, rootPath, branch))
+ .map(
+ conf ->
+ new PathOwnersEntry(
+ rootPath,
+ conf,
+ accounts,
+ Optional.empty(),
+ Collections.emptySet(),
+ Collections.emptySet(),
+ Collections.emptySet(),
+ Collections.emptySet()));
}
private void processMatcherPerPath(
@@ -352,8 +362,8 @@
String project,
String path,
String branch,
- PathOwnersEntry projectEntry,
- List<PathOwnersEntry> parentsPathOwnersEntries,
+ ReadOnlyPathOwnersEntry projectEntry,
+ List<ReadOnlyPathOwnersEntry> parentsPathOwnersEntries,
PathOwnersEntry rootEntry,
Map<String, PathOwnersEntry> entries,
PathOwnersEntriesCache cache)
@@ -366,7 +376,7 @@
calculateCurrentEntry(rootEntry, projectEntry, currentEntry);
// Inherit from Parent Project if OWNER in Project enables inheritance
- for (PathOwnersEntry parentPathOwnersEntry : parentsPathOwnersEntries) {
+ for (ReadOnlyPathOwnersEntry parentPathOwnersEntry : parentsPathOwnersEntries) {
calculateCurrentEntry(projectEntry, parentPathOwnersEntry, currentEntry);
}
@@ -384,31 +394,31 @@
String ownersPath = partial + "OWNERS";
PathOwnersEntry pathFallbackEntry = currentEntry;
currentEntry =
- cache.get(
- project,
- branch,
- ownersPath,
- () ->
- getOwnersConfig(repository, ownersPath, branch)
- .map(
- c -> {
- Optional<LabelDefinition> label = pathFallbackEntry.getLabel();
- final Set<Id> owners = pathFallbackEntry.getOwners();
- final Set<Id> reviewers = pathFallbackEntry.getReviewers();
- Collection<Matcher> inheritedMatchers =
- pathFallbackEntry.getMatchers().values();
- Set<String> groupOwners = pathFallbackEntry.getGroupOwners();
- return new PathOwnersEntry(
- ownersPath,
- c,
- accounts,
- label,
- owners,
- reviewers,
- inheritedMatchers,
- groupOwners);
- })
- .orElse(pathFallbackEntry));
+ cache
+ .get(
+ project,
+ branch,
+ ownersPath,
+ () -> getOwnersConfig(repository, ownersPath, branch))
+ .map(
+ c -> {
+ Optional<LabelDefinition> label = pathFallbackEntry.getLabel();
+ final Set<Id> owners = pathFallbackEntry.getOwners();
+ final Set<Id> reviewers = pathFallbackEntry.getReviewers();
+ Collection<Matcher> inheritedMatchers =
+ pathFallbackEntry.getMatchers().values();
+ Set<String> groupOwners = pathFallbackEntry.getGroupOwners();
+ return new PathOwnersEntry(
+ ownersPath,
+ c,
+ accounts,
+ label,
+ owners,
+ reviewers,
+ inheritedMatchers,
+ groupOwners);
+ })
+ .orElse(pathFallbackEntry);
entries.put(partial, currentEntry);
}
}
@@ -416,7 +426,9 @@
}
private void calculateCurrentEntry(
- PathOwnersEntry rootEntry, PathOwnersEntry projectEntry, PathOwnersEntry currentEntry) {
+ ReadOnlyPathOwnersEntry rootEntry,
+ ReadOnlyPathOwnersEntry projectEntry,
+ PathOwnersEntry currentEntry) {
if (rootEntry.isInherited()) {
for (Matcher matcher : projectEntry.getMatchers().values()) {
if (!currentEntry.hasMatcher(matcher.getPath())) {
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
index c2ec085..04afebd 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
@@ -25,7 +25,9 @@
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.Singleton;
+import com.google.inject.TypeLiteral;
import java.util.Objects;
+import java.util.Optional;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
@@ -36,7 +38,7 @@
return new CacheModule() {
@Override
protected void configure() {
- cache(CACHE_NAME, Key.class, PathOwnersEntry.class);
+ cache(CACHE_NAME, Key.class, new TypeLiteral<Optional<OwnersConfig>>() {});
bind(PathOwnersEntriesCache.class).to(PathOwnersEntriesCacheImpl.class);
DynamicSet.bind(binder(), GitReferenceUpdatedListener.class)
.to(OwnersRefUpdateListener.class);
@@ -45,7 +47,8 @@
};
}
- PathOwnersEntry get(String project, String branch, String path, Callable<PathOwnersEntry> loader)
+ Optional<OwnersConfig> get(
+ String project, String branch, String path, Callable<Optional<OwnersConfig>> loader)
throws ExecutionException;
void invalidate(String project, String branch);
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java
index 74f9585..3af7a58 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java
@@ -25,18 +25,19 @@
import com.google.inject.name.Named;
import java.time.Duration;
import java.util.Collection;
+import java.util.Optional;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
@Singleton
class PathOwnersEntriesCacheImpl implements PathOwnersEntriesCache {
- private final Cache<Key, PathOwnersEntry> cache;
+ private final Cache<Key, Optional<OwnersConfig>> cache;
private final Multimap<String, Key> keysIndex;
private final LoadingCache<String, Object> keyLocks;
@Inject
- PathOwnersEntriesCacheImpl(@Named(CACHE_NAME) Cache<Key, PathOwnersEntry> cache) {
+ PathOwnersEntriesCacheImpl(@Named(CACHE_NAME) Cache<Key, Optional<OwnersConfig>> cache) {
this.cache = cache;
this.keysIndex = HashMultimap.create();
this.keyLocks =
@@ -46,14 +47,14 @@
}
@Override
- public PathOwnersEntry get(
- String project, String branch, String path, Callable<PathOwnersEntry> loader)
+ public Optional<OwnersConfig> get(
+ String project, String branch, String path, Callable<Optional<OwnersConfig>> loader)
throws ExecutionException {
Key key = new Key(project, branch, path);
return cache.get(
key,
() -> {
- PathOwnersEntry entry = loader.call();
+ Optional<OwnersConfig> entry = loader.call();
String indexKey = indexKey(project, branch);
synchronized (keyLocks.getUnchecked(indexKey)) {
keysIndex.put(indexKey, key);
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
index 1ede8a6..8e4c53c 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
@@ -31,20 +31,11 @@
*
* <p>Used internally by PathOwners to represent and compute the owners at a specific path.
*/
-class PathOwnersEntry {
- static final PathOwnersEntry EMPTY = new PathOwnersEntry();
-
- private final boolean inherited;
- private Optional<LabelDefinition> label;
- private Set<Account.Id> owners = Sets.newHashSet();
- private Set<Account.Id> reviewers = Sets.newHashSet();
- private String ownersPath;
- private Map<String, Matcher> matchers = Maps.newHashMap();
- private Set<String> groupOwners = Sets.newHashSet();
+class PathOwnersEntry extends ReadOnlyPathOwnersEntry {
+ static final ReadOnlyPathOwnersEntry EMPTY = new ReadOnlyPathOwnersEntry(true) {};
public PathOwnersEntry() {
- inherited = true;
- label = Optional.empty();
+ super(true);
}
public PathOwnersEntry(
@@ -56,6 +47,7 @@
Set<Account.Id> inheritedReviewers,
Collection<Matcher> inheritedMatchers,
Set<String> inheritedGroupOwners) {
+ super(config.isInherited());
this.ownersPath = path;
this.owners =
config.getOwners().stream()
@@ -82,20 +74,6 @@
} else {
this.label = config.getLabel();
}
- this.inherited = config.isInherited();
- }
-
- @Override
- public String toString() {
- return "PathOwnersEntry [ownersPath="
- + ownersPath
- + ", owners="
- + owners
- + ", matchers="
- + matchers
- + ", label="
- + label
- + "]";
}
public void addMatcher(Matcher matcher) {
@@ -103,6 +81,52 @@
this.matchers.put(matcher.getPath(), matcher.merge(currMatchers));
}
+ public void setOwners(Set<Account.Id> owners) {
+ this.owners = owners;
+ }
+
+ public void setReviewers(Set<Account.Id> reviewers) {
+ this.reviewers = reviewers;
+ }
+
+ public void setOwnersPath(String ownersPath) {
+ this.ownersPath = ownersPath;
+ }
+
+ public void setMatchers(Map<String, Matcher> matchers) {
+ this.matchers = matchers;
+ }
+
+ public void setLabel(Optional<LabelDefinition> label) {
+ this.label = label;
+ }
+
+ public void addMatchers(Collection<Matcher> values) {
+ for (Matcher matcher : values) {
+ addMatcher(matcher);
+ }
+ }
+
+ @Override
+ protected String className() {
+ return getClass().getSimpleName();
+ }
+}
+
+abstract class ReadOnlyPathOwnersEntry {
+ protected final boolean inherited;
+ protected Optional<LabelDefinition> label;
+ protected Set<Account.Id> owners = Sets.newHashSet();
+ protected Set<Account.Id> reviewers = Sets.newHashSet();
+ protected String ownersPath;
+ protected Map<String, Matcher> matchers = Maps.newHashMap();
+ protected Set<String> groupOwners = Sets.newHashSet();
+
+ protected ReadOnlyPathOwnersEntry(boolean inherited) {
+ this.inherited = inherited;
+ label = Optional.empty();
+ }
+
public Map<String, Matcher> getMatchers() {
return matchers;
}
@@ -115,30 +139,14 @@
return groupOwners;
}
- public void setOwners(Set<Account.Id> owners) {
- this.owners = owners;
- }
-
public Set<Account.Id> getReviewers() {
return reviewers;
}
- public void setReviewers(Set<Account.Id> reviewers) {
- this.reviewers = reviewers;
- }
-
public String getOwnersPath() {
return ownersPath;
}
- public void setOwnersPath(String ownersPath) {
- this.ownersPath = ownersPath;
- }
-
- public void setMatchers(Map<String, Matcher> matchers) {
- this.matchers = matchers;
- }
-
public boolean isInherited() {
return inherited;
}
@@ -147,16 +155,6 @@
return label;
}
- public void setLabel(Optional<LabelDefinition> label) {
- this.label = label;
- }
-
- public void addMatchers(Collection<Matcher> values) {
- for (Matcher matcher : values) {
- addMatcher(matcher);
- }
- }
-
public boolean hasMatcher(String path) {
return this.matchers.containsKey(path);
}
@@ -164,4 +162,22 @@
public static String stripOwnerDomain(String owner) {
return Splitter.on('@').split(owner).iterator().next();
}
+
+ @Override
+ public String toString() {
+ return className()
+ + " [ownersPath="
+ + ownersPath
+ + ", owners="
+ + owners
+ + ", matchers="
+ + matchers
+ + ", label="
+ + label
+ + "]";
+ }
+
+ protected String className() {
+ return "ReadOnlyPathOwnersEntry";
+ }
}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java
index 1eac62c..752bf7f 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java
@@ -14,6 +14,7 @@
package com.googlesource.gerrit.owners.common;
+import java.util.Optional;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import org.junit.Ignore;
@@ -30,8 +31,8 @@
public void invalidateIndexKey(Key key) {}
@Override
- public PathOwnersEntry get(
- String project, String branch, String path, Callable<PathOwnersEntry> loader)
+ public Optional<OwnersConfig> get(
+ String project, String branch, String path, Callable<Optional<OwnersConfig>> loader)
throws ExecutionException {
try {
hit++;
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
index ec3e816..5cd1599 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
@@ -116,6 +117,21 @@
@Test
@UseLocalDisk
+ public void shouldReflectChangesInParentProject() throws Exception {
+ addOwnerFileToProjectConfig(allProjects, true, admin);
+
+ String changeId = createChange().getChangeId();
+ Response<FilesOwnersResponse> resp =
+ assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+ assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(rootOwner));
+
+ addOwnerFileToProjectConfig(allProjects, true, user);
+ resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+ assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(projectOwner));
+ }
+
+ @Test
+ @UseLocalDisk
public void shouldNotReturnInheritedOwnersFromProjectsOwners() throws Exception {
assertNotInheritFromProject(project);
}
@@ -157,6 +173,11 @@
private void addOwnerFileToProjectConfig(Project.NameKey projectNameKey, boolean inherit)
throws Exception {
+ addOwnerFileToProjectConfig(projectNameKey, inherit, user);
+ }
+
+ private void addOwnerFileToProjectConfig(
+ Project.NameKey projectNameKey, boolean inherit, TestAccount account) throws Exception {
TestRepository<InMemoryRepository> project = cloneProject(projectNameKey);
GitUtil.fetch(project, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
project.reset(RefNames.REFS_CONFIG);
@@ -168,7 +189,7 @@
"OWNERS",
String.format(
"inherited: %s\nmatchers:\n" + "- suffix: .txt\n owners:\n - %s\n",
- inherit, user.email()))
+ inherit, account.email()))
.to(RefNames.REFS_CONFIG);
}
