Merge branch 'stable-3.0' into stable-3.1
* stable-3.0:
Clarify OWNERS use with both directory and matchers
Upgrade bazlets to latest stable-3.0
Upgrade bazlets to latest stable-2.16
Add reviewers from OWNERS all at once
Implement visibility using permission backend
Do not assign reviewers that cannot see the change
Fix NPE when target branch doesn't exist
Make transitive starlark loads explicit
Format java files with google-java-format
Create a user-context if not propagated by Gerrit
Change-Id: If2157c1b867c24f683fa90f4a7bd2b7e5d778794
diff --git a/config.md b/config.md
index b294ae8..6cba6eb 100644
--- a/config.md
+++ b/config.md
@@ -200,7 +200,7 @@
## Example 4 - Owners based on matchers
Often the ownership comes from the developer's skills and competencies and
-cannot be purely defined by the project's directory structure.
+cannot be defined solely by the project's directory structure.
For instance, all the files ending with .sql should be owned and signed-off by
the DBA while all the ones ending with .css by approved by the UX Team.
@@ -235,6 +235,19 @@
(Mister Dba) and the .css files (either John Creative or Matt Designer) have
provided their Code-Review +2 feedback.
+The `add_match_owner_approval` predicate would also honour the OWNERS file
+without matchers, giving, therefore, the possibility of having different ownership
+criteria for different subdirectories. Example: /foo-dir/OWNERS can define a
+directory-based ownership while /bar-dir/OWNERS can rely on matching rules.
+
+__PERFORMANCE NOTE: The predicate `add_match_owner_approval` looks,
+at first sight, more powerful and versatile. However, it may generate a significant
+number of reductions and therefore, impact the Gerrit server performance.
+When used with changes with a high number of files involved, it may even crash
+the Gerrit default `rules.reductionLimit`.
+When not using any matcher in the OWNERS file, prefer the `add_owner_approval`,
+which generates a minimal number of reductions.__
+
## Example 5 - Owners details on a per-file basis
When using the owners with a series of matchers associated to different set of
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
index 9e8f482..ac101df 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
@@ -23,19 +23,25 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.annotations.Listen;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.Changes;
+import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import java.io.IOException;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
@@ -54,22 +60,58 @@
private final Accounts accounts;
private final ReviewerManager reviewerManager;
+ private final OneOffRequestContext oneOffReqCtx;
+
+ private Provider<CurrentUser> currentUserProvider;
+
@Inject
public GitRefListener(
GerritApi api,
PatchListCache patchListCache,
GitRepositoryManager repositoryManager,
Accounts accounts,
- ReviewerManager reviewerManager) {
+ ReviewerManager reviewerManager,
+ OneOffRequestContext oneOffReqCtx,
+ Provider<CurrentUser> currentUserProvider) {
this.api = api;
this.patchListCache = patchListCache;
this.repositoryManager = repositoryManager;
this.accounts = accounts;
this.reviewerManager = reviewerManager;
+ this.oneOffReqCtx = oneOffReqCtx;
+ this.currentUserProvider = currentUserProvider;
}
@Override
public void onGitReferenceUpdated(Event event) {
+ AccountInfo updaterAccountInfo = event.getUpdater();
+ CurrentUser currentUser = currentUserProvider.get();
+ if (currentUser.isIdentifiedUser()) {
+ handleGitReferenceUpdated(event);
+ } else if (updaterAccountInfo != null) {
+ handleGitReferenceUpdatedAsUser(event, Account.id(updaterAccountInfo._accountId));
+ } else {
+ handleGitReferenceUpdatedAsServer(event);
+ }
+ }
+
+ private void handleGitReferenceUpdatedAsUser(Event event, Account.Id updaterAccountId) {
+ try (ManualRequestContext ctx = oneOffReqCtx.openAs(updaterAccountId)) {
+ handleGitReferenceUpdated(event);
+ } catch (StorageException e) {
+ logger.warn("Unable to process event {} on project {}", event, event.getProjectName(), e);
+ }
+ }
+
+ private void handleGitReferenceUpdatedAsServer(Event event) {
+ try (ManualRequestContext ctx = oneOffReqCtx.open()) {
+ handleGitReferenceUpdated(event);
+ } catch (StorageException e) {
+ logger.warn("Unable to process event {} on project {}", event, event.getProjectName(), e);
+ }
+ }
+
+ private void handleGitReferenceUpdated(Event event) {
String projectName = event.getProjectName();
Repository repository;
try {
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
index 564b681..c3708b5 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
@@ -17,12 +17,18 @@
package com.googlesource.gerrit.owners.common;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
@@ -38,11 +44,22 @@
private final OneOffRequestContext requestContext;
private final GerritApi gApi;
+ private final IdentifiedUser.GenericFactory userFactory;
+ private final ChangeData.Factory changeDataFactory;
+ private final PermissionBackend permissionBackend;
@Inject
- public ReviewerManager(OneOffRequestContext requestContext, GerritApi gApi) {
+ public ReviewerManager(
+ OneOffRequestContext requestContext,
+ GerritApi gApi,
+ IdentifiedUser.GenericFactory userFactory,
+ ChangeData.Factory changeDataFactory,
+ PermissionBackend permissionBackend) {
this.requestContext = requestContext;
this.gApi = gApi;
+ this.userFactory = userFactory;
+ this.changeDataFactory = changeDataFactory;
+ this.permissionBackend = permissionBackend;
}
public void addReviewers(ChangeApi cApi, Collection<Account.Id> reviewers)
@@ -56,9 +73,16 @@
ReviewInput in = new ReviewInput();
in.reviewers = new ArrayList<>(reviewers.size());
for (Account.Id account : reviewers) {
- AddReviewerInput addReviewerInput = new AddReviewerInput();
- addReviewerInput.reviewer = account.toString();
- in.reviewers.add(addReviewerInput);
+ if (isVisibleTo(changeInfo, account)) {
+ AddReviewerInput addReviewerInput = new AddReviewerInput();
+ addReviewerInput.reviewer = account.toString();
+ in.reviewers.add(addReviewerInput);
+ } else {
+ log.warn(
+ "Not adding account {} as reviewer to change {} because the associated ref is not visible",
+ account,
+ changeInfo._number);
+ }
}
gApi.changes().id(changeInfo.id).current().review(in);
}
@@ -67,4 +91,14 @@
throw new ReviewerManagerException(e);
}
}
+
+ private boolean isVisibleTo(ChangeInfo changeInfo, Account.Id account) {
+ ChangeData changeData =
+ changeDataFactory.create(
+ Project.nameKey(changeInfo.project), Change.id(changeInfo._number));
+ return permissionBackend
+ .user(userFactory.create(account))
+ .change(changeData)
+ .testOrFalse(ChangePermission.READ);
+ }
}
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
index 6730024..5c9f1a7 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
@@ -21,7 +21,11 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
import org.eclipse.jgit.transport.ReceiveCommand.Type;
import org.junit.Test;
@@ -30,6 +34,13 @@
sysModule = "com.vmware.gerrit.owners.common.GitRefListenerIT$TestModule")
public class GitRefListenerIT extends LightweightPluginDaemonTest {
+ @Inject GitRefListenerTest gitRefListener;
+ @Inject ThreadLocalRequestContext requestContext;
+
+ String aRefChange = RefNames.REFS_CHANGES + "01/01/01";
+ String anOldObjectId = "anOldRef";
+ String aNewObjectId = "aNewRef";
+
public static class TestModule extends AbstractModule {
@Override
protected void configure() {
@@ -39,15 +50,14 @@
@Test
public void shouldNotProcessNoteDbOnlyRefs() {
- GitRefListenerTest gitRefListener = getPluginInstance(GitRefListenerTest.class);
-
- String aRefChange = RefNames.REFS_CHANGES + "01/01" + RefNames.META_SUFFIX;
- String anOldObjectId = "anOldRef";
- String aNewObjectId = "aNewRef";
-
ReferenceUpdatedEventTest refUpdatedEvent =
new ReferenceUpdatedEventTest(
- project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE);
+ project,
+ RefNames.REFS_CHANGES + "01/01" + RefNames.META_SUFFIX,
+ anOldObjectId,
+ aNewObjectId,
+ Type.CREATE,
+ admin.id());
gitRefListener.onGitReferenceUpdated(refUpdatedEvent);
assertEquals(0, gitRefListener.getProcessedEvents());
@@ -55,21 +65,31 @@
@Test
public void shouldProcessRefChanges() {
- GitRefListenerTest gitRefListener = getPluginInstance(GitRefListenerTest.class);
-
- String aRefChange = RefNames.REFS_CHANGES + "01/01/01";
- String anOldObjectId = "anOldRef";
- String aNewObjectId = "aNewRef";
-
- ReferenceUpdatedEventTest refUpdatedEvent =
- new ReferenceUpdatedEventTest(
- project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE);
-
- gitRefListener.onGitReferenceUpdated(refUpdatedEvent);
+ gitRefListener.onGitReferenceUpdated(newRefUpdateEvent());
assertEquals(1, gitRefListener.getProcessedEvents());
}
- private <T> T getPluginInstance(Class<T> clazz) {
- return plugin.getSysInjector().getInstance(clazz);
+ @Test
+ public void shouldRetrieveChangeFromAnonymousContext() throws Exception {
+ try (ManualRequestContext ctx = new ManualRequestContext(new AnonymousUser(), requestContext)) {
+ gitRefListener.onGitReferenceUpdated(newRefUpdateEvent());
+ assertEquals(1, gitRefListener.getProcessedEvents());
+ }
+ }
+
+ @Test
+ public void shouldRetrieveChangeFromAnonymousContextWithoutAccountId() throws Exception {
+ ReferenceUpdatedEventTest refUpdateWithoutAccountId =
+ new ReferenceUpdatedEventTest(
+ project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE, null);
+ try (ManualRequestContext ctx = new ManualRequestContext(new AnonymousUser(), requestContext)) {
+ gitRefListener.onGitReferenceUpdated(refUpdateWithoutAccountId);
+ assertEquals(1, gitRefListener.getProcessedEvents());
+ }
+ }
+
+ private ReferenceUpdatedEventTest newRefUpdateEvent() {
+ return new ReferenceUpdatedEventTest(
+ project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE, admin.id());
}
}
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
index a409ede..e3d4be4 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
@@ -16,9 +16,12 @@
package com.vmware.gerrit.owners.common;
import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.GitRefListener;
import com.googlesource.gerrit.owners.common.ReviewerManager;
@@ -35,8 +38,17 @@
PatchListCache patchListCache,
GitRepositoryManager repositoryManager,
Accounts accounts,
- ReviewerManager reviewerManager) {
- super(api, patchListCache, repositoryManager, accounts, reviewerManager);
+ ReviewerManager reviewerManager,
+ OneOffRequestContext oneOffReqCtx,
+ Provider<CurrentUser> currentUserProvider) {
+ super(
+ api,
+ patchListCache,
+ repositoryManager,
+ accounts,
+ reviewerManager,
+ oneOffReqCtx,
+ currentUserProvider);
}
@Override
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java
index 5f7b11b..7024543 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java
@@ -15,6 +15,7 @@
package com.vmware.gerrit.owners.common;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.common.AccountInfo;
@@ -30,18 +31,21 @@
private final String oldObjectId;
private final String newObjectId;
private final ReceiveCommand.Type type;
+ private final Account.Id eventAccountId;
public ReferenceUpdatedEventTest(
Project.NameKey project,
String ref,
String oldObjectId,
String newObjectId,
- ReceiveCommand.Type type) {
+ ReceiveCommand.Type type,
+ Account.Id eventAccountId) {
this.projectName = project.get();
this.ref = ref;
this.oldObjectId = oldObjectId;
this.newObjectId = newObjectId;
this.type = type;
+ this.eventAccountId = eventAccountId;
}
@Override
@@ -81,7 +85,11 @@
@Override
public AccountInfo getUpdater() {
- return null;
+ if (eventAccountId == null) {
+ return null;
+ }
+
+ return new AccountInfo(eventAccountId.get());
}
@Override
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/JgitWrapper.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/JgitWrapper.java
index 9668af8..c1a7368 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/JgitWrapper.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/JgitWrapper.java
@@ -34,9 +34,14 @@
public static Optional<byte[]> getBlobAsBytes(Repository repository, String revision, String path)
throws IOException {
+ ObjectId objectId = repository.resolve(revision);
+ if (objectId == null) {
+ return Optional.empty();
+ }
+
try (final TreeWalk w =
TreeWalk.forPath(
- repository, path, parseCommit(repository, repository.resolve(revision)).getTree())) {
+ repository, path, parseCommit(repository, objectId).getTree())) {
return Optional.ofNullable(w)
.filter(walk -> (walk.getRawMode(0) & TYPE_MASK) == TYPE_FILE)
