Merge "Merge branch 'stable-2.14' into stable-2.15" into stable-2.15
diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt
index 3f6acf6..59ed6ff 100644
--- a/Documentation/cmd-review.txt
+++ b/Documentation/cmd-review.txt
@@ -120,15 +120,10 @@
Votes that are not permitted for the user are silently ignored.
--label::
- Set a label by name to the value 'N'. Invalid votes (invalid label
- or invalid value) and votes that are not permitted for the user are
- silently ignored.
-
---strict-labels::
- Require ability to vote on all specified labels before reviewing change.
- If the vote is invalid (invalid label or invalid name), the vote is not
- permitted for the user, or the vote is on an outdated or closed patch set,
- return an error instead of silently discarding the vote.
+ Set a label by name to the value 'N'. The ability to vote on all specified
+ labels is required. If the vote is invalid (invalid label or invalid name),
+ the vote is not permitted for the user, or the vote is on an outdated or
+ closed patch set, return an error instead of silently discarding the vote.
--tag::
-t::
diff --git a/Documentation/cmd-show-caches.txt b/Documentation/cmd-show-caches.txt
index e47ae81..6a1f554 100644
--- a/Documentation/cmd-show-caches.txt
+++ b/Documentation/cmd-show-caches.txt
@@ -61,11 +61,12 @@
adv_bases | | | |
changes | | 27.1ms | 0% |
groups | 5646 | 11.8ms | 97% |
- groups_byinclude | 230 | 2.4ms | 62% |
+ groups_bymember | | | |
groups_byname | | | |
+ groups_bysubgroup | 230 | 2.4ms | 62% |
groups_byuuid | 5612 | 29.2ms | 99% |
groups_external | 1 | 1.5s | 98% |
- groups_members | 5714 | 19.7ms | 99% |
+ groups_subgroups | 5714 | 19.7ms | 99% |
ldap_group_existence | | | |
ldap_groups | 650 | 680.5ms | 99% |
ldap_groups_byinclude | 1024 | | 83% |
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 2898a23..d241b98 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -767,13 +767,8 @@
cache `"accounts"`::
+
Cache entries contain important details of an active user, including
-their display name, preferences, known email addresses, and group
-memberships. Entry information is obtained from the following
-database tables:
-+
-* `accounts`
-+
-* `account_group_members`
+their display name, preferences, and known email addresses. Entry
+information is obtained from the `accounts` database table.
+
If direct updates are made to any of these database tables, this
@@ -849,16 +844,21 @@
Caches the basic group information from the `account_groups` table,
including the group owner, name, and description.
+
-Gerrit group membership obtained from the `account_group_members`
-table is cached under the `"accounts"` cache, above. External group
-membership obtained from LDAP is cached under `"ldap_groups"`.
+External group membership obtained from LDAP is cached under
+`"ldap_groups"`.
-cache `"groups_byinclude"`::
+cache `"groups_bymember"`::
+
-Caches group inclusions in other groups. If direct updates are made
+Caches the groups which contain a specific member (account). If direct
+updates are made to the `account_group_members` table, this cache should
+be flushed.
+
+cache `"groups_bysubgroups"`::
++
+Caches the parent groups of a subgroup. If direct updates are made
to the `account_group_includes` table, this cache should be flushed.
-cache `"groups_members"`::
+cache `"groups_subgroups"`::
+
Caches subgroups. If direct updates are made to the
`account_group_includes` table, this cache should be flushed.
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index ea577f3..618ad3f 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6695,13 +6695,6 @@
|`robot_comments` |optional|
The robot comments that should be added as a map that maps a file path
to a list of link:#robot-comment-input[RobotCommentInput] entities.
-|`strict_labels` |`true` if not set|
-Whether all labels are required to be within the user's permitted ranges
-based on access controls. +
-If `true`, attempting to use a label not granted to the user will fail
-the entire modify operation early. +
-If `false`, the operation will execute anyway, but the proposed labels
-will be modified to be the "best" value allowed by the access controls.
|`drafts` |optional|
Draft handling that defines how draft comments are handled that are
already in the database but that were not also described in this
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index b64af53..320e848 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -333,7 +333,7 @@
"mem": 12
}
},
- "groups_byinclude": {
+ "groups_bymember": {
"type": "MEM",
"entries": {},
"hit_ratio": {}
@@ -343,6 +343,11 @@
"entries": {},
"hit_ratio": {}
},
+ "groups_bysubgroup": {
+ "type": "MEM",
+ "entries": {},
+ "hit_ratio": {}
+ },
"groups_byuuid": {
"type": "MEM",
"entries": {
@@ -358,7 +363,7 @@
"entries": {},
"hit_ratio": {}
},
- groups_members": {
+ groups_subgroups": {
"type": "MEM",
"entries": {
"mem": 4
@@ -467,11 +472,12 @@
"diff_intraline",
"git_tags",
"groups",
- "groups_byinclude",
+ "groups_bymember",
"groups_byname",
+ "groups_bysubgroup",
"groups_byuuid",
"groups_external",
- "groups_members",
+ "groups_subgroups",
"permission_sort",
"plugin_resources",
"project_list",
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index abbecd0..a801669 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -21,6 +21,8 @@
import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.project.Util.category;
+import static com.google.gerrit.server.project.Util.value;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.HEAD;
@@ -37,6 +39,9 @@
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.LabelFunction;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.api.GerritApi;
@@ -1398,4 +1403,19 @@
config.getProject().setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE);
saveProjectConfig(project, config);
}
+
+ protected void configLabel(String label, LabelFunction func) throws Exception {
+ configLabel(
+ project, label, func, value(1, "Passes"), value(0, "No score"), value(-1, "Failed"));
+ }
+
+ protected void configLabel(
+ Project.NameKey project, String label, LabelFunction func, LabelValue... value)
+ throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ LabelType labelType = category(label, value);
+ labelType.setFunction(func);
+ cfg.getLabelSections().put(labelType.getName(), labelType);
+ saveProjectConfig(project, cfg);
+ }
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 780c2d7..d3389c5 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -70,6 +70,7 @@
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
@@ -133,6 +134,7 @@
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.server.update.BatchUpdate;
@@ -1582,7 +1584,6 @@
// Exact request format made by GWT UI at ddc6b7160fe416fed9e7e3180489d44c82fd64f8.
ReviewInput in = new ReviewInput();
in.labels = ImmutableMap.of("Code-Review", (short) 0);
- in.strictLabels = true;
in.drafts = DraftHandling.PUBLISH_ALL_REVISIONS;
in.message = "comment";
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in);
@@ -3189,6 +3190,68 @@
gApi.changes().id(changeId).topic(topic);
}
+ @Test
+ public void submittableAfterLosingPermissions_MaxWithBlock() throws Exception {
+ configLabel("Label", LabelFunction.MAX_WITH_BLOCK);
+ submittableAfterLosingPermissions("Label");
+ }
+
+ @Test
+ public void submittableAfterLosingPermissions_AnyWithBlock() throws Exception {
+ configLabel("Label", LabelFunction.ANY_WITH_BLOCK);
+ submittableAfterLosingPermissions("Label");
+ }
+
+ public void submittableAfterLosingPermissions(String label) throws Exception {
+ String codeReviewLabel = "Code-Review";
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ AccountGroup.UUID registered = SystemGroupBackend.REGISTERED_USERS;
+ Util.allow(cfg, Permission.forLabel(label), -1, +1, registered, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(codeReviewLabel), -2, +2, registered, "refs/heads/*");
+ saveProjectConfig(cfg);
+
+ setApiUser(user);
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Verify user's permitted range.
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertPermitted(change, label, -1, 0, 1);
+ assertPermitted(change, codeReviewLabel, -2, -1, 0, 1, 2);
+
+ ReviewInput input = new ReviewInput();
+ input.label(codeReviewLabel, 2);
+ input.label(label, 1);
+ gApi.changes().id(changeId).current().review(input);
+
+ assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().keySet())
+ .containsExactly(codeReviewLabel, label);
+ assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().values())
+ .containsExactly((short) 2, (short) 1);
+ assertThat(gApi.changes().id(changeId).get().submittable).isTrue();
+
+ setApiUser(admin);
+ // Remove user's permission for 'Label'.
+ Util.remove(cfg, Permission.forLabel(label), registered, "refs/heads/*");
+ // Update user's permitted range for 'Code-Review' to be -1...+1.
+ Util.remove(cfg, Permission.forLabel(codeReviewLabel), registered, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(codeReviewLabel), -1, +1, registered, "refs/heads/*");
+ saveProjectConfig(cfg);
+
+ // Verify user's new permitted range.
+ setApiUser(user);
+ change = gApi.changes().id(changeId).get();
+ assertPermitted(change, label);
+ assertPermitted(change, codeReviewLabel, -1, 0, 1);
+
+ assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().values())
+ .containsExactly((short) 2, (short) 1);
+ assertThat(gApi.changes().id(changeId).get().submittable).isTrue();
+
+ setApiUser(admin);
+ gApi.changes().id(changeId).current().submit();
+ }
+
private String getCommitMessage(String changeId) throws RestApiException, IOException {
return gApi.changes().id(changeId).current().file("/COMMIT_MSG").content().asString();
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 305a2b0..3f18f64 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -46,6 +46,7 @@
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.account.GroupIncludeCache;
import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.GroupsUpdate;
import com.google.gerrit.server.group.InternalGroup;
@@ -56,6 +57,7 @@
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
@@ -65,6 +67,7 @@
public class GroupsIT extends AbstractDaemonTest {
@Inject @ServerInitiated private Provider<GroupsUpdate> groupsUpdateProvider;
@Inject private Groups groups;
+ @Inject private GroupIncludeCache groupIncludeCache;
@Test
public void systemGroupCanBeRetrievedFromIndex() throws Exception {
@@ -95,6 +98,26 @@
}
@Test
+ public void cachedGroupsForMemberAreUpdatedOnMemberAdditionAndRemoval() throws Exception {
+ // Fill the cache for the observed account.
+ groupIncludeCache.getGroupsWithMember(user.getId());
+ String groupName = createGroup("users");
+ AccountGroup.UUID groupUuid = new AccountGroup.UUID(gApi.groups().id(groupName).get().id);
+
+ gApi.groups().id(groupName).addMembers(user.fullName);
+
+ Collection<AccountGroup.UUID> groupsWithMemberAfterAddition =
+ groupIncludeCache.getGroupsWithMember(user.getId());
+ assertThat(groupsWithMemberAfterAddition).contains(groupUuid);
+
+ gApi.groups().id(groupName).removeMembers(user.fullName);
+
+ Collection<AccountGroup.UUID> groupsWithMemberAfterRemoval =
+ groupIncludeCache.getGroupsWithMember(user.getId());
+ assertThat(groupsWithMemberAfterRemoval).doesNotContain(groupUuid);
+ }
+
+ @Test
public void addExistingMember_OK() throws Exception {
String g = "Administrators";
assertMembers(g, admin);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index 36843a5..9b88e0d 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -146,7 +146,6 @@
ReviewInput in = new ReviewInput();
in.onBehalfOf = user.id.toString();
- in.strictLabels = true;
in.label("Not-A-Label", 5);
exception.expect(BadRequestException.class);
@@ -155,23 +154,6 @@
}
@Test
- public void voteOnBehalfOfInvalidLabelIgnoredWithoutStrictLabels() throws Exception {
- allowCodeReviewOnBehalfOf();
- PushOneCommit.Result r = createChange();
- RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
-
- ReviewInput in = new ReviewInput();
- in.onBehalfOf = user.id.toString();
- in.strictLabels = false;
- in.label("Code-Review", 1);
- in.label("Not-A-Label", 5);
-
- revision.review(in);
-
- assertThat(gApi.changes().id(r.getChangeId()).get().labels).doesNotContainKey("Not-A-Label");
- }
-
- @Test
public void voteOnBehalfOfLabelNotPermitted() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
LabelType verified = Util.verified();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
index 8388ed0..37d3e1e 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.MoveInput;
@@ -33,6 +34,7 @@
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.Util;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.PersonIdent;
@@ -227,6 +229,59 @@
move(r.getChangeId(), newBranch.get());
}
+ @Test
+ public void moveChangeOnlyKeepVetoVotes() throws Exception {
+ // A vote for a label will be kept after moving if the label's function is *WithBlock and the
+ // vote holds the minimum value.
+ createBranch(new Branch.NameKey(project, "foo"));
+
+ String codeReviewLabel = "Code-Review"; // 'Code-Review' uses 'MaxWithBlock' function.
+ String testLabelA = "Label-A";
+ String testLabelB = "Label-B";
+ String testLabelC = "Label-C";
+ configLabel(testLabelA, LabelFunction.ANY_WITH_BLOCK);
+ configLabel(testLabelB, LabelFunction.MAX_NO_BLOCK);
+ configLabel(testLabelC, LabelFunction.NO_BLOCK);
+
+ AccountGroup.UUID registered = SystemGroupBackend.REGISTERED_USERS;
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ Util.allow(cfg, Permission.forLabel(testLabelA), -1, +1, registered, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(testLabelB), -1, +1, registered, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(testLabelC), -1, +1, registered, "refs/heads/*");
+ saveProjectConfig(cfg);
+
+ String changeId = createChange().getChangeId();
+ gApi.changes().id(changeId).current().review(ReviewInput.reject());
+
+ amendChange(changeId);
+
+ ReviewInput input = new ReviewInput();
+ input.label(testLabelA, -1);
+ input.label(testLabelB, -1);
+ input.label(testLabelC, -1);
+ gApi.changes().id(changeId).current().review(input);
+
+ assertThat(gApi.changes().id(changeId).current().reviewer(admin.email).votes().keySet())
+ .containsExactly(codeReviewLabel, testLabelA, testLabelB, testLabelC);
+ assertThat(gApi.changes().id(changeId).current().reviewer(admin.email).votes().values())
+ .containsExactly((short) -2, (short) -1, (short) -1, (short) -1);
+
+ // Move the change to the 'foo' branch.
+ assertThat(gApi.changes().id(changeId).get().branch).isEqualTo("master");
+ move(changeId, "foo");
+ assertThat(gApi.changes().id(changeId).get().branch).isEqualTo("foo");
+
+ // 'Code-Review -2' and 'Label-A -1' will be kept.
+ assertThat(gApi.changes().id(changeId).current().reviewer(admin.email).votes().values())
+ .containsExactly((short) -2, (short) -1, (short) 0, (short) 0);
+
+ // Move the change back to 'master'.
+ move(changeId, "master");
+ assertThat(gApi.changes().id(changeId).get().branch).isEqualTo("master");
+ assertThat(gApi.changes().id(changeId).current().reviewer(admin.email).votes().values())
+ .containsExactly((short) -2, (short) -1, (short) 0, (short) 0);
+ }
+
private void move(int changeNum, String destination) throws RestApiException {
gApi.changes().id(changeNum).move(destination);
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
index 38ff3c7..b02ae7b 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
@@ -15,6 +15,10 @@
package com.google.gerrit.acceptance.server.project;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.common.data.LabelFunction.ANY_WITH_BLOCK;
+import static com.google.gerrit.common.data.LabelFunction.MAX_NO_BLOCK;
+import static com.google.gerrit.common.data.LabelFunction.NO_BLOCK;
+import static com.google.gerrit.common.data.LabelFunction.NO_OP;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.project.Util.category;
import static com.google.gerrit.server.project.Util.value;
@@ -80,7 +84,7 @@
@Test
public void customLabelNoOp_NegativeVoteNotBlock() throws Exception {
- label.setFunctionName("NoOp");
+ label.setFunction(NO_OP);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@@ -93,7 +97,7 @@
@Test
public void customLabelNoBlock_NegativeVoteNotBlock() throws Exception {
- label.setFunctionName("NoBlock");
+ label.setFunction(NO_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@@ -106,7 +110,7 @@
@Test
public void customLabelMaxNoBlock_NegativeVoteNotBlock() throws Exception {
- label.setFunctionName("MaxNoBlock");
+ label.setFunction(MAX_NO_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@@ -119,7 +123,7 @@
@Test
public void customLabelAnyWithBlock_NegativeVoteBlock() throws Exception {
- label.setFunctionName("AnyWithBlock");
+ label.setFunction(ANY_WITH_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@@ -133,7 +137,7 @@
@Test
public void customLabelAnyWithBlock_Addreviewer_ZeroVote() throws Exception {
- P.setFunctionName("AnyWithBlock");
+ P.setFunction(ANY_WITH_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
AddReviewerInput in = new AddReviewerInput();
@@ -168,9 +172,9 @@
@Test
public void customLabel_DisallowPostSubmit() throws Exception {
- label.setFunctionName("NoOp");
+ label.setFunction(NO_OP);
label.setAllowPostSubmit(false);
- P.setFunctionName("NoOp");
+ P.setFunction(NO_OP);
saveLabelConfig();
PushOneCommit.Result r = createChange();
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelFunction.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelFunction.java
new file mode 100644
index 0000000..0ce2c29
--- /dev/null
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelFunction.java
@@ -0,0 +1,71 @@
+// Copyright (C) 2017 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.common.data;
+
+import com.google.gerrit.common.Nullable;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * Functions for determining submittability based on label votes.
+ *
+ * <p>Only describes built-in label functions. Admins can extend the logic arbitrarily using Prolog
+ * rules, in which case the choice of function in the project config is ignored.
+ *
+ * <p>Function semantics are documented in {@code config-labels.txt}, and actual behavior is
+ * implemented in Prolog in {@code gerrit_common.pl}.
+ */
+public enum LabelFunction {
+ MAX_WITH_BLOCK("MaxWithBlock", true),
+ ANY_WITH_BLOCK("AnyWithBlock", true),
+ MAX_NO_BLOCK("MaxNoBlock", false),
+ NO_BLOCK("NoBlock", false),
+ NO_OP("NoOp", false),
+ PATCH_SET_LOCK("PatchSetLock", false);
+
+ public static final Map<String, LabelFunction> ALL;
+
+ static {
+ Map<String, LabelFunction> all = new LinkedHashMap<>();
+ for (LabelFunction f : values()) {
+ all.put(f.getFunctionName(), f);
+ }
+ ALL = Collections.unmodifiableMap(all);
+ }
+
+ public static Optional<LabelFunction> parse(@Nullable String str) {
+ return Optional.ofNullable(ALL.get(str));
+ }
+
+ private final String name;
+ private final boolean isBlock;
+
+ private LabelFunction(String name, boolean isBlock) {
+ this.name = name;
+ this.isBlock = isBlock;
+ }
+
+ /** The function name as defined in documentation and {@code project.config}. */
+ public String getFunctionName() {
+ return name;
+ }
+
+ /** Whether the label is a "block" label, meaning a minimum vote will prevent submission. */
+ public boolean isBlock() {
+ return isBlock;
+ }
+}
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
index c90e1fd..7bfd22e 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
@@ -14,6 +14,7 @@
package com.google.gerrit.common.data;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.LabelId;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import java.util.ArrayList;
@@ -22,6 +23,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
public class LabelType {
public static final boolean DEF_ALLOW_POST_SUBMIT = true;
@@ -97,7 +99,9 @@
protected String name;
+ // String rather than LabelFunction for backwards compatibility with GWT JSON interface.
protected String functionName;
+
protected boolean copyMinScore;
protected boolean copyMaxScore;
protected boolean copyAllScoresOnMergeFirstParentUpdate;
@@ -124,7 +128,7 @@
values = sortValues(valueList);
defaultValue = 0;
- functionName = "MaxWithBlock";
+ functionName = LabelFunction.MAX_WITH_BLOCK.getFunctionName();
maxNegative = Short.MIN_VALUE;
maxPositive = Short.MAX_VALUE;
@@ -154,12 +158,19 @@
return psa.getLabelId().get().equalsIgnoreCase(name);
}
- public String getFunctionName() {
- return functionName;
+ public LabelFunction getFunction() {
+ if (functionName == null) {
+ return null;
+ }
+ Optional<LabelFunction> f = LabelFunction.parse(functionName);
+ if (!f.isPresent()) {
+ throw new IllegalStateException("Unsupported functionName: " + functionName);
+ }
+ return f.get();
}
- public void setFunctionName(String functionName) {
- this.functionName = functionName;
+ public void setFunction(@Nullable LabelFunction function) {
+ this.functionName = function != null ? function.getFunctionName() : null;
}
public boolean canOverride() {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
index 113651b..f851d5e 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
@@ -60,7 +60,6 @@
private native void init() /*-{
this.labels = {};
- this.strict_labels = true;
}-*/;
public final native void prePost() /*-{
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java
index 9e9f1c8..2340a97 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java
@@ -20,11 +20,8 @@
import static java.util.stream.Collectors.toList;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
-import com.google.gerrit.elasticsearch.ElasticIndexModule;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.lifecycle.LifecycleManager;
-import com.google.gerrit.lucene.LuceneIndexModule;
import com.google.gerrit.pgm.util.BatchProgramModule;
import com.google.gerrit.pgm.util.RuntimeShutdown;
import com.google.gerrit.pgm.util.SiteProgram;
@@ -33,13 +30,12 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
-import com.google.gerrit.server.index.IndexModule;
+import com.google.gerrit.server.index.DummyIndexModule;
import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
import com.google.gerrit.server.notedb.rebuild.NoteDbMigrator;
import com.google.gerrit.server.schema.DataSourceType;
import com.google.inject.Inject;
import com.google.inject.Injector;
-import com.google.inject.Module;
import com.google.inject.Provider;
import java.util.ArrayList;
import java.util.List;
@@ -194,23 +190,12 @@
public void configure() {
install(dbInjector.getInstance(BatchProgramModule.class));
bind(GitReferenceUpdated.class).toInstance(GitReferenceUpdated.DISABLED);
- install(getIndexModule());
+ install(new DummyIndexModule());
factory(ChangeResource.Factory.class);
}
});
}
- private Module getIndexModule() {
- switch (IndexModule.getIndexType(dbInjector)) {
- case LUCENE:
- return LuceneIndexModule.singleVersionWithExplicitVersions(ImmutableMap.of(), threads);
- case ELASTICSEARCH:
- return ElasticIndexModule.singleVersionWithExplicitVersions(ImmutableMap.of(), threads);
- default:
- throw new IllegalStateException("unsupported index.type");
- }
- }
-
private void stop() {
try {
LifecycleManager m = sysManager;
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitAdminUser.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitAdminUser.java
index 9a81c52..d02de6c 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitAdminUser.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitAdminUser.java
@@ -44,7 +44,6 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
-import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import org.apache.commons.validator.routines.EmailValidator;
@@ -142,12 +141,7 @@
}
AccountState as =
- new AccountState(
- new AllUsersName(allUsers.get()),
- a,
- Collections.singleton(adminGroup.getGroupUUID()),
- extIds,
- new HashMap<>());
+ new AccountState(new AllUsersName(allUsers.get()), a, extIds, new HashMap<>());
for (AccountIndex accountIndex : accountIndexCollection.getWriteIndexes()) {
accountIndex.replace(as);
}
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java
index 60fd60f..c7309f8 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java
@@ -14,6 +14,8 @@
package com.google.gerrit.pgm.init;
+import static com.google.gerrit.common.data.LabelFunction.MAX_WITH_BLOCK;
+
import com.google.gerrit.pgm.init.api.AllProjectsConfig;
import com.google.gerrit.pgm.init.api.ConsoleUI;
import com.google.gerrit.pgm.init.api.InitStep;
@@ -54,7 +56,7 @@
public void postRun() throws Exception {
Config cfg = allProjectsConfig.load().getConfig();
if (installVerified) {
- cfg.setString(KEY_LABEL, LABEL_VERIFIED, KEY_FUNCTION, "MaxWithBlock");
+ cfg.setString(KEY_LABEL, LABEL_VERIFIED, KEY_FUNCTION, MAX_WITH_BLOCK.getFunctionName());
cfg.setStringList(
KEY_LABEL,
LABEL_VERIFIED,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
index 6971b48..c1f89e2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
@@ -31,7 +31,6 @@
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
@@ -213,7 +212,7 @@
}
}
return labelNormalizer.normalize(notes, user, byUser.values()).getNormalized();
- } catch (IOException | PermissionBackendException e) {
+ } catch (IOException e) {
throw new OrmException(e);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
index d062842..9894751 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -14,34 +14,19 @@
package com.google.gerrit.server.account;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Streams;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.account.WatchConfig.NotifyType;
-import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.group.Groups;
-import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.index.account.AccountIndexer;
-import com.google.gerrit.server.index.group.GroupField;
-import com.google.gerrit.server.index.group.GroupIndex;
-import com.google.gerrit.server.index.group.GroupIndexCollection;
-import com.google.gerrit.server.query.group.InternalGroupQuery;
-import com.google.gwtorm.server.OrmException;
-import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.Provider;
@@ -52,9 +37,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Optional;
-import java.util.Set;
import java.util.concurrent.ExecutionException;
-import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -147,64 +130,37 @@
private AccountState missing(Account.Id accountId) {
Account account = new Account(accountId, TimeUtil.nowTs());
account.setActive(false);
- Set<AccountGroup.UUID> anon = ImmutableSet.of();
- return new AccountState(
- allUsersName,
- account,
- anon,
- Collections.emptySet(),
- new HashMap<ProjectWatchKey, Set<NotifyType>>());
+ return new AccountState(allUsersName, account, Collections.emptySet(), new HashMap<>());
}
static class ByIdLoader extends CacheLoader<Account.Id, Optional<AccountState>> {
- private final SchemaFactory<ReviewDb> schema;
private final AllUsersName allUsersName;
private final Accounts accounts;
- private final Provider<GroupIndex> groupIndexProvider;
- private final Provider<InternalGroupQuery> groupQueryProvider;
- private final GroupCache groupCache;
private final GeneralPreferencesLoader loader;
private final Provider<WatchConfig.Accessor> watchConfig;
private final ExternalIds externalIds;
@Inject
ByIdLoader(
- SchemaFactory<ReviewDb> sf,
AllUsersName allUsersName,
Accounts accounts,
- GroupIndexCollection groupIndexCollection,
- Provider<InternalGroupQuery> groupQueryProvider,
- GroupCache groupCache,
GeneralPreferencesLoader loader,
Provider<WatchConfig.Accessor> watchConfig,
ExternalIds externalIds) {
- this.schema = sf;
this.allUsersName = allUsersName;
this.accounts = accounts;
- this.groupIndexProvider = groupIndexCollection::getSearchIndex;
- this.groupQueryProvider = groupQueryProvider;
- this.groupCache = groupCache;
this.loader = loader;
this.watchConfig = watchConfig;
this.externalIds = externalIds;
}
@Override
- public Optional<AccountState> load(Account.Id key) throws Exception {
- try (ReviewDb db = schema.open()) {
- return load(db, key);
- }
- }
-
- private Optional<AccountState> load(ReviewDb db, Account.Id who)
- throws OrmException, IOException, ConfigInvalidException {
+ public Optional<AccountState> load(Account.Id who) throws Exception {
Account account = accounts.get(who);
if (account == null) {
return Optional.empty();
}
- Set<AccountGroup.UUID> internalGroups = getGroupsWithMember(db, who);
-
try {
account.setGeneralPreferences(loader.load(who));
} catch (IOException | ConfigInvalidException e) {
@@ -216,25 +172,8 @@
new AccountState(
allUsersName,
account,
- internalGroups,
externalIds.byAccount(who),
watchConfig.get().getProjectWatches(who)));
}
-
- private ImmutableSet<AccountGroup.UUID> getGroupsWithMember(ReviewDb db, Account.Id memberId)
- throws OrmException {
- Stream<InternalGroup> internalGroupStream;
- if (groupIndexProvider.get() != null
- && groupIndexProvider.get().getSchema().hasField(GroupField.MEMBER)) {
- internalGroupStream = groupQueryProvider.get().byMember(memberId).stream();
- } else {
- internalGroupStream =
- Groups.getGroupsWithMemberFromReviewDb(db, memberId)
- .map(groupCache::get)
- .flatMap(Streams::stream);
- }
-
- return internalGroupStream.map(InternalGroup::getGroupUUID).collect(toImmutableSet());
- }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java
index dd523a9..36bfb7e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java
@@ -23,7 +23,6 @@
import com.google.common.cache.CacheBuilder;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.CurrentUser.PropertyKey;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.WatchConfig.NotifyType;
@@ -46,7 +45,6 @@
private final AllUsersName allUsersName;
private final Account account;
- private final Set<AccountGroup.UUID> internalGroups;
private final Collection<ExternalId> externalIds;
private final Map<ProjectWatchKey, Set<NotifyType>> projectWatches;
private Cache<IdentifiedUser.PropertyKey<Object>, Object> properties;
@@ -54,12 +52,10 @@
public AccountState(
AllUsersName allUsersName,
Account account,
- Set<AccountGroup.UUID> actualGroups,
Collection<ExternalId> externalIds,
Map<ProjectWatchKey, Set<NotifyType>> projectWatches) {
this.allUsersName = allUsersName;
this.account = account;
- this.internalGroups = actualGroups;
this.externalIds = externalIds;
this.projectWatches = projectWatches;
this.account.setUserName(getUserName(externalIds));
@@ -117,11 +113,6 @@
return projectWatches;
}
- /** The set of groups maintained directly within the Gerrit database. */
- public Set<AccountGroup.UUID> getInternalGroups() {
- return internalGroups;
- }
-
public static String getUserName(Collection<ExternalId> ids) {
for (ExternalId extId : ids) {
if (extId.isScheme(SCHEME_USERNAME)) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
index c702aef..157afb8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
@@ -14,20 +14,42 @@
package com.google.gerrit.server.account;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import java.util.Collection;
/** Tracks group inclusions in memory for efficient access. */
public interface GroupIncludeCache {
- /** @return groups directly a member of the passed group. */
+
+ /**
+ * Returns the UUIDs of all groups of which the specified account is a direct member.
+ *
+ * @param memberId the ID of the account
+ * @return the UUIDs of all groups having the account as member
+ */
+ Collection<AccountGroup.UUID> getGroupsWithMember(Account.Id memberId);
+
+ /**
+ * Returns the subgroups of a group.
+ *
+ * @param group the UUID of the group
+ * @return the UUIDs of all direct subgroups
+ */
Collection<AccountGroup.UUID> subgroupsOf(AccountGroup.UUID group);
- /** @return any groups the passed group belongs to. */
+ /**
+ * Returns the parent groups of a subgroup.
+ *
+ * @param groupId the UUID of the subgroup
+ * @return the UUIDs of all direct parent groups
+ */
Collection<AccountGroup.UUID> parentGroupsOf(AccountGroup.UUID groupId);
/** @return set of any UUIDs that are not internal groups. */
Collection<AccountGroup.UUID> allExternalMembers();
+ void evictGroupsWithMember(Account.Id memberId);
+
void evictSubgroupsOf(AccountGroup.UUID groupId);
void evictParentGroupsOf(AccountGroup.UUID groupId);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
index 2691bc1..e128627 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
@@ -15,12 +15,15 @@
package com.google.gerrit.server.account;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.gerrit.common.errors.NoSuchGroupException;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
@@ -41,6 +44,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.concurrent.ExecutionException;
+import java.util.stream.Stream;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -48,8 +52,9 @@
@Singleton
public class GroupIncludeCacheImpl implements GroupIncludeCache {
private static final Logger log = LoggerFactory.getLogger(GroupIncludeCacheImpl.class);
- private static final String PARENT_GROUPS_NAME = "groups_byinclude";
- private static final String SUBGROUPS_NAME = "groups_members";
+ private static final String PARENT_GROUPS_NAME = "groups_bysubgroup";
+ private static final String SUBGROUPS_NAME = "groups_subgroups";
+ private static final String GROUPS_WITH_MEMBER_NAME = "groups_bymember";
private static final String EXTERNAL_NAME = "groups_external";
public static Module module() {
@@ -57,6 +62,12 @@
@Override
protected void configure() {
cache(
+ GROUPS_WITH_MEMBER_NAME,
+ Account.Id.class,
+ new TypeLiteral<ImmutableSet<AccountGroup.UUID>>() {})
+ .loader(GroupsWithMemberLoader.class);
+
+ cache(
PARENT_GROUPS_NAME,
AccountGroup.UUID.class,
new TypeLiteral<ImmutableList<AccountGroup.UUID>>() {})
@@ -77,23 +88,37 @@
};
}
+ private final LoadingCache<Account.Id, ImmutableSet<AccountGroup.UUID>> groupsWithMember;
private final LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> subgroups;
private final LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> parentGroups;
private final LoadingCache<String, ImmutableList<AccountGroup.UUID>> external;
@Inject
GroupIncludeCacheImpl(
+ @Named(GROUPS_WITH_MEMBER_NAME)
+ LoadingCache<Account.Id, ImmutableSet<AccountGroup.UUID>> groupsWithMember,
@Named(SUBGROUPS_NAME)
LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> subgroups,
@Named(PARENT_GROUPS_NAME)
LoadingCache<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> parentGroups,
@Named(EXTERNAL_NAME) LoadingCache<String, ImmutableList<AccountGroup.UUID>> external) {
+ this.groupsWithMember = groupsWithMember;
this.subgroups = subgroups;
this.parentGroups = parentGroups;
this.external = external;
}
@Override
+ public Collection<AccountGroup.UUID> getGroupsWithMember(Account.Id memberId) {
+ try {
+ return groupsWithMember.get(memberId);
+ } catch (ExecutionException e) {
+ log.warn(String.format("Cannot load groups containing %d as member", memberId.get()));
+ return ImmutableSet.of();
+ }
+ }
+
+ @Override
public Collection<AccountGroup.UUID> subgroupsOf(AccountGroup.UUID groupId) {
try {
return subgroups.get(groupId);
@@ -114,6 +139,13 @@
}
@Override
+ public void evictGroupsWithMember(Account.Id memberId) {
+ if (memberId != null) {
+ groupsWithMember.invalidate(memberId);
+ }
+ }
+
+ @Override
public void evictSubgroupsOf(AccountGroup.UUID groupId) {
if (groupId != null) {
subgroups.invalidate(groupId);
@@ -141,6 +173,46 @@
}
}
+ static class GroupsWithMemberLoader
+ extends CacheLoader<Account.Id, ImmutableSet<AccountGroup.UUID>> {
+ private final SchemaFactory<ReviewDb> schema;
+ private final Provider<GroupIndex> groupIndexProvider;
+ private final Provider<InternalGroupQuery> groupQueryProvider;
+ private final GroupCache groupCache;
+
+ @Inject
+ GroupsWithMemberLoader(
+ SchemaFactory<ReviewDb> schema,
+ GroupIndexCollection groupIndexCollection,
+ Provider<InternalGroupQuery> groupQueryProvider,
+ GroupCache groupCache) {
+ this.schema = schema;
+ groupIndexProvider = groupIndexCollection::getSearchIndex;
+ this.groupQueryProvider = groupQueryProvider;
+ this.groupCache = groupCache;
+ }
+
+ @Override
+ public ImmutableSet<AccountGroup.UUID> load(Account.Id memberId)
+ throws OrmException, NoSuchGroupException {
+
+ Stream<InternalGroup> internalGroupStream;
+ GroupIndex groupIndex = groupIndexProvider.get();
+ if (groupIndex != null && groupIndex.getSchema().hasField(GroupField.MEMBER)) {
+ internalGroupStream = groupQueryProvider.get().byMember(memberId).stream();
+ } else {
+ try (ReviewDb db = schema.open()) {
+ internalGroupStream =
+ Groups.getGroupsWithMemberFromReviewDb(db, memberId)
+ .map(groupCache::get)
+ .flatMap(Streams::stream);
+ }
+ }
+
+ return internalGroupStream.map(InternalGroup::getGroupUUID).collect(toImmutableSet());
+ }
+ }
+
static class SubgroupsLoader
extends CacheLoader<AccountGroup.UUID, ImmutableList<AccountGroup.UUID>> {
private final SchemaFactory<ReviewDb> schema;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
index ae28e1c..a077629 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
@@ -22,6 +22,7 @@
import com.google.gerrit.server.group.InternalGroup;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
@@ -54,12 +55,7 @@
this.groupCache = groupCache;
this.includeCache = includeCache;
this.user = user;
-
- Set<AccountGroup.UUID> groups = user.state().getInternalGroups();
- memberOf = new ConcurrentHashMap<>(groups.size());
- for (AccountGroup.UUID g : groups) {
- memberOf.put(g, true);
- }
+ memberOf = new ConcurrentHashMap<>();
}
@Override
@@ -97,6 +93,10 @@
if (!group.isPresent()) {
continue;
}
+ if (group.get().getMembers().contains(user.getAccountId())) {
+ memberOf.put(id, true);
+ return true;
+ }
if (search(group.get().getSubgroups())) {
memberOf.put(id, true);
return true;
@@ -124,7 +124,8 @@
private ImmutableSet<AccountGroup.UUID> computeKnownGroups() {
GroupMembership membership = user.getEffectiveGroups();
- Set<AccountGroup.UUID> direct = user.state().getInternalGroups();
+ Collection<AccountGroup.UUID> direct = includeCache.getGroupsWithMember(user.getAccountId());
+ direct.forEach(groupUuid -> memberOf.put(groupUuid, true));
Set<AccountGroup.UUID> r = Sets.newHashSet(direct);
r.remove(null);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
index 2f3855c..27d4eb1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
@@ -21,6 +21,7 @@
import com.google.common.base.Strings;
import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.extensions.api.changes.MoveInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -31,18 +32,24 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.LabelId;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -55,7 +62,8 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import java.util.ArrayList;
+import java.util.List;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -71,6 +79,9 @@
private final Provider<InternalChangeQuery> queryProvider;
private final ChangeMessagesUtil cmUtil;
private final PatchSetUtil psUtil;
+ private final ApprovalsUtil approvalsUtil;
+ private final ProjectCache projectCache;
+ private final Provider<CurrentUser> userProvider;
@Inject
Move(
@@ -81,7 +92,10 @@
Provider<InternalChangeQuery> queryProvider,
ChangeMessagesUtil cmUtil,
RetryHelper retryHelper,
- PatchSetUtil psUtil) {
+ PatchSetUtil psUtil,
+ ApprovalsUtil approvalsUtil,
+ ProjectCache projectCache,
+ Provider<CurrentUser> userProvider) {
super(retryHelper);
this.permissionBackend = permissionBackend;
this.dbProvider = dbProvider;
@@ -90,6 +104,9 @@
this.queryProvider = queryProvider;
this.cmUtil = cmUtil;
this.psUtil = psUtil;
+ this.approvalsUtil = approvalsUtil;
+ this.projectCache = projectCache;
+ this.userProvider = userProvider;
}
@Override
@@ -138,7 +155,7 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws OrmException, ResourceConflictException, RepositoryNotFoundException, IOException {
+ throws OrmException, ResourceConflictException, IOException {
change = ctx.getChange();
if (change.getStatus() != Status.NEW) {
throw new ResourceConflictException("Change is " + ChangeUtil.status(change));
@@ -188,10 +205,13 @@
throw new ResourceConflictException("Patch set is not current");
}
- ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
+ PatchSet.Id psId = change.currentPatchSetId();
+ ChangeUpdate update = ctx.getUpdate(psId);
update.setBranch(newDestKey.get());
change.setDest(newDestKey);
+ updateApprovals(ctx, update, psId, projectKey);
+
StringBuilder msgBuf = new StringBuilder();
msgBuf.append("Change destination moved from ");
msgBuf.append(changePrevDest.getShortName());
@@ -207,6 +227,46 @@
return true;
}
+
+ /**
+ * We have a long discussion about how to deal with its votes after moving a change from one
+ * branch to another. In the end, we think only keeping the veto votes is the best way since
+ * it's simple for us and less confusing for our users. See the discussion in the following
+ * proposal: https://gerrit-review.googlesource.com/c/gerrit/+/129171
+ */
+ private void updateApprovals(
+ ChangeContext ctx, ChangeUpdate update, PatchSet.Id psId, Project.NameKey project)
+ throws IOException, OrmException {
+ List<PatchSetApproval> approvals = new ArrayList<>();
+ for (PatchSetApproval psa :
+ approvalsUtil.byPatchSet(
+ ctx.getDb(),
+ ctx.getNotes(),
+ userProvider.get(),
+ psId,
+ ctx.getRevWalk(),
+ ctx.getRepoView().getConfig())) {
+ ProjectState projectState = projectCache.checkedGet(project);
+ LabelType type =
+ projectState.getLabelTypes(ctx.getNotes(), ctx.getUser()).byLabel(psa.getLabelId());
+ // Only keep veto votes, defined as votes where:
+ // 1- the label function allows minimum values to block submission.
+ // 2- the vote holds the minimum value.
+ if (type.isMaxNegative(psa) && type.getFunction().isBlock()) {
+ continue;
+ }
+
+ // Remove votes from NoteDb.
+ update.removeApprovalFor(psa.getAccountId(), psa.getLabel());
+ approvals.add(
+ new PatchSetApproval(
+ new PatchSetApproval.Key(psId, psa.getAccountId(), new LabelId(psa.getLabel())),
+ (short) 0,
+ ctx.getWhen()));
+ }
+ // Remove votes from ReviewDb.
+ ctx.getDb().patchSetApprovals().upsert(approvals);
+ }
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 58634a5..0022656 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -233,7 +233,7 @@
input.drafts = DraftHandling.DELETE;
}
if (input.labels != null) {
- checkLabels(revision, labelTypes, input.strictLabels, input.labels);
+ checkLabels(revision, labelTypes, input.labels);
}
if (input.comments != null) {
cleanUpComments(input.comments);
@@ -443,12 +443,9 @@
while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next();
LabelType type = labelTypes.byLabel(ent.getKey());
- if (type == null && in.strictLabels) {
+ if (type == null) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
- } else if (type == null) {
- itr.remove();
- continue;
}
if (!caller.isInternalUser()) {
@@ -479,8 +476,7 @@
changeResourceFactory.create(rev.getNotes(), reviewer), rev.getPatchSet());
}
- private void checkLabels(
- RevisionResource rsrc, LabelTypes labelTypes, boolean strict, Map<String, Short> labels)
+ private void checkLabels(RevisionResource rsrc, LabelTypes labelTypes, Map<String, Short> labels)
throws BadRequestException, AuthException, PermissionBackendException {
PermissionBackend.ForChange perm = rsrc.permissions();
Iterator<Map.Entry<String, Short>> itr = labels.entrySet().iterator();
@@ -488,12 +484,8 @@
Map.Entry<String, Short> ent = itr.next();
LabelType lt = labelTypes.byLabel(ent.getKey());
if (lt == null) {
- if (strict) {
- throw new BadRequestException(
- String.format("label \"%s\" is not a configured label", ent.getKey()));
- }
- itr.remove();
- continue;
+ throw new BadRequestException(
+ String.format("label \"%s\" is not a configured label", ent.getKey()));
}
if (ent.getValue() == null || ent.getValue() == 0) {
@@ -503,23 +495,16 @@
}
if (lt.getValue(ent.getValue()) == null) {
- if (strict) {
- throw new BadRequestException(
- String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue()));
- }
- itr.remove();
- continue;
+ throw new BadRequestException(
+ String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue()));
}
short val = ent.getValue();
try {
perm.check(new LabelPermission.WithValue(lt, val));
} catch (AuthException e) {
- if (strict) {
- throw new AuthException(
- String.format("Applying label \"%s\": %d is restricted", lt.getName(), val));
- }
- ent.setValue(perm.squashThenCheck(lt, val));
+ throw new AuthException(
+ String.format("Applying label \"%s\": %d is restricted", lt.getName(), val));
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
index 6a332cd..73cda7f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
@@ -24,32 +24,26 @@
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.LabelValue;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.permissions.LabelPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Collection;
import java.util.List;
/**
- * Normalizes votes on labels according to project config and permissions.
+ * Normalizes votes on labels according to project config.
*
* <p>Votes are recorded in the database for a user based on the state of the project at that time:
- * what labels are defined for the project, and what the user is allowed to vote on. Both of those
- * can change between the time a vote is originally made and a later point, for example when a
- * change is submitted. This class normalizes old votes against current project configuration.
+ * what labels are defined for the project. The label definition can change between the time a vote
+ * is originally made and a later point, for example when a change is submitted. This class
+ * normalizes old votes against current project configuration.
*/
@Singleton
public class LabelNormalizer {
@@ -77,33 +71,24 @@
}
}
- private final Provider<ReviewDb> db;
private final IdentifiedUser.GenericFactory userFactory;
- private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
@Inject
- LabelNormalizer(
- Provider<ReviewDb> db,
- IdentifiedUser.GenericFactory userFactory,
- PermissionBackend permissionBackend,
- ProjectCache projectCache) {
- this.db = db;
+ LabelNormalizer(IdentifiedUser.GenericFactory userFactory, ProjectCache projectCache) {
this.userFactory = userFactory;
- this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
}
/**
* @param notes change containing the given approvals.
* @param approvals list of approvals.
- * @return copies of approvals normalized to the defined ranges for the label type and permissions
- * for the user. Approvals for unknown labels are not included in the output, nor are
- * approvals where the user has no permissions for that label.
+ * @return copies of approvals normalized to the defined ranges for the label type. Approvals for
+ * unknown labels are not included in the output.
* @throws OrmException
*/
public Result normalize(ChangeNotes notes, Collection<PatchSetApproval> approvals)
- throws OrmException, PermissionBackendException, IOException {
+ throws OrmException, IOException {
IdentifiedUser user = userFactory.create(notes.getChange().getOwner());
return normalize(notes, user, approvals);
}
@@ -112,13 +97,12 @@
* @param notes change notes containing the given approvals.
* @param user current user.
* @param approvals list of approvals.
- * @return copies of approvals normalized to the defined ranges for the label type and permissions
- * for the user. Approvals for unknown labels are not included in the output, nor are
- * approvals where the user has no permissions for that label.
+ * @return copies of approvals normalized to the defined ranges for the label type. Approvals for
+ * unknown labels are not included in the output.
*/
public Result normalize(
ChangeNotes notes, CurrentUser user, Collection<PatchSetApproval> approvals)
- throws PermissionBackendException, IOException {
+ throws IOException {
List<PatchSetApproval> unchanged = Lists.newArrayListWithCapacity(approvals.size());
List<PatchSetApproval> updated = Lists.newArrayListWithCapacity(approvals.size());
List<PatchSetApproval> deleted = Lists.newArrayListWithCapacity(approvals.size());
@@ -142,9 +126,7 @@
}
PatchSetApproval copy = copy(psa);
applyTypeFloor(label, copy);
- if (!applyRightFloor(notes, label, copy)) {
- deleted.add(psa);
- } else if (copy.getValue() != psa.getValue()) {
+ if (copy.getValue() != psa.getValue()) {
updated.add(copy);
} else {
unchanged.add(psa);
@@ -157,26 +139,6 @@
return new PatchSetApproval(src.getPatchSetId(), src);
}
- private boolean applyRightFloor(ChangeNotes notes, LabelType lt, PatchSetApproval a)
- throws PermissionBackendException {
- PermissionBackend.ForChange forChange =
- permissionBackend.user(userFactory.create(a.getAccountId())).database(db).change(notes);
- // Check if the user is allowed to vote on the label at all
- try {
- forChange.check(new LabelPermission(lt.getName()));
- } catch (AuthException e) {
- return false;
- }
- // Squash vote to nearest allowed value
- try {
- forChange.check(new LabelPermission.WithValue(lt.getName(), a.getValue()));
- return true;
- } catch (AuthException e) {
- a.setValue(forChange.squashThenCheck(lt, a.getValue()));
- return true;
- }
- }
-
private void applyTypeFloor(LabelType lt, PatchSetApproval a) {
LabelValue atMin = lt.getMin();
if (atMin != null && a.getValue() < atMin.getValue()) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
index 33f9b7d..ed5695c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
@@ -19,11 +19,9 @@
import com.google.common.base.CharMatcher;
import com.google.common.base.Joiner;
-import com.google.common.base.MoreObjects;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
@@ -33,6 +31,7 @@
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
@@ -67,6 +66,7 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
+import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@@ -156,9 +156,6 @@
private static final String KEY_VALUE = "value";
private static final String KEY_CAN_OVERRIDE = "canOverride";
private static final String KEY_BRANCH = "branch";
- private static final ImmutableSet<String> LABEL_FUNCTIONS =
- ImmutableSet.of(
- "MaxWithBlock", "AnyWithBlock", "MaxNoBlock", "NoBlock", "NoOp", "PatchSetLock");
private static final String REVIEWER = "reviewer";
private static final String KEY_ENABLE_REVIEWER_BY_EMAIL = "enableByEmail";
@@ -868,19 +865,20 @@
continue;
}
- String functionName =
- MoreObjects.firstNonNull(rc.getString(LABEL, name, KEY_FUNCTION), "MaxWithBlock");
- if (LABEL_FUNCTIONS.contains(functionName)) {
- label.setFunctionName(functionName);
- } else {
+ String functionName = rc.getString(LABEL, name, KEY_FUNCTION);
+ Optional<LabelFunction> function =
+ functionName != null
+ ? LabelFunction.parse(functionName)
+ : Optional.of(LabelFunction.MAX_WITH_BLOCK);
+ if (!function.isPresent()) {
error(
new ValidationError(
PROJECT_CONFIG,
String.format(
"Invalid %s for label \"%s\". Valid names are: %s",
- KEY_FUNCTION, name, Joiner.on(", ").join(LABEL_FUNCTIONS))));
- label.setFunctionName(null);
+ KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet()))));
}
+ label.setFunction(function.orElse(null));
if (!values.isEmpty()) {
short dv = (short) rc.getInt(LABEL, name, KEY_DEFAULT_VALUE, 0);
@@ -1367,7 +1365,7 @@
String name = e.getKey();
LabelType label = e.getValue();
toUnset.remove(name);
- rc.setString(LABEL, name, KEY_FUNCTION, label.getFunctionName());
+ rc.setString(LABEL, name, KEY_FUNCTION, label.getFunction().getFunctionName());
rc.setInt(LABEL, name, KEY_DEFAULT_VALUE, label.getDefaultValue());
setBooleanConfigKey(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
index 152d398..9a362d4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
@@ -45,7 +45,6 @@
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.git.SubmoduleException;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -330,7 +329,7 @@
}
private void setApproval(ChangeContext ctx, IdentifiedUser user)
- throws OrmException, IOException, PermissionBackendException {
+ throws OrmException, IOException {
Change.Id id = ctx.getChange().getId();
List<SubmitRecord> records = args.commitStatus.getSubmitRecords(id);
PatchSet.Id oldPsId = toMerge.getPatchsetId();
@@ -352,7 +351,7 @@
}
private LabelNormalizer.Result approve(ChangeContext ctx, ChangeUpdate update)
- throws OrmException, IOException, PermissionBackendException {
+ throws OrmException, IOException {
PatchSet.Id psId = update.getPatchSetId();
Map<PatchSetApproval.Key, PatchSetApproval> byKey = new HashMap<>();
for (PatchSetApproval psa :
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
index 736eeec..f31e383 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
@@ -32,7 +32,6 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.GroupIncludeCache;
import com.google.gerrit.server.git.RenameGroupOp;
@@ -77,7 +76,6 @@
private final GroupCache groupCache;
private final GroupIncludeCache groupIncludeCache;
private final AuditService auditService;
- private final AccountCache accountCache;
private final RenameGroupOp.Factory renameGroupOpFactory;
@Nullable private final IdentifiedUser currentUser;
private final PersonIdent committerIdent;
@@ -88,7 +86,6 @@
GroupCache groupCache,
GroupIncludeCache groupIncludeCache,
AuditService auditService,
- AccountCache accountCache,
RenameGroupOp.Factory renameGroupOpFactory,
@GerritPersonIdent PersonIdent serverIdent,
@Assisted @Nullable IdentifiedUser currentUser) {
@@ -96,7 +93,6 @@
this.groupCache = groupCache;
this.groupIncludeCache = groupIncludeCache;
this.auditService = auditService;
- this.accountCache = accountCache;
this.renameGroupOpFactory = renameGroupOpFactory;
this.currentUser = currentUser;
committerIdent = getCommitterIdent(serverIdent, currentUser);
@@ -285,7 +281,7 @@
db.accountGroupMembers().insert(newMembers);
groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
for (AccountGroupMember newMember : newMembers) {
- accountCache.evict(newMember.getAccountId());
+ groupIncludeCache.evictGroupsWithMember(newMember.getAccountId());
}
}
@@ -324,7 +320,7 @@
db.accountGroupMembers().delete(membersToRemove);
groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
for (AccountGroupMember member : membersToRemove) {
- accountCache.evict(member.getAccountId());
+ groupIncludeCache.evictGroupsWithMember(member.getAccountId());
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
index 6db9357..c87e8e4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -429,65 +429,5 @@
.map((v) -> new LabelPermission.WithValue(label, v))
.collect(toSet());
}
-
- /**
- * Squash a label value to the nearest allowed value.
- *
- * <p>For multi-valued labels like Code-Review with values -2..+2 a user may try to use +2, but
- * only have permission for the -1..+1 range. The caller should have already tried:
- *
- * <pre>
- * check(new LabelPermission.WithValue("Code-Review", 2));
- * </pre>
- *
- * and caught {@link AuthException}. {@code squashThenCheck} will use {@link #test(LabelType)}
- * to determine potential values of Code-Review the user can use, and select the nearest value
- * along the same sign, e.g. -1 for -2 and +1 for +2.
- *
- * @param label definition of the label to test values of.
- * @param val previously denied value the user attempted.
- * @return nearest allowed value, or {@code 0} if no value was allowed.
- * @throws PermissionBackendException backend cannot run test or check.
- */
- public short squashThenCheck(LabelType label, short val) throws PermissionBackendException {
- short s = squashByTest(label, val);
- if (s == 0 || s == val) {
- return 0;
- }
- try {
- check(new LabelPermission.WithValue(label, s));
- return s;
- } catch (AuthException e) {
- return 0;
- }
- }
-
- /**
- * Squash a label value to the nearest allowed value using only test methods.
- *
- * <p>Tests all possible values and selects the closet available to {@code val} while matching
- * the sign of {@code val}. Unlike {@code #squashThenCheck(LabelType, short)} this method only
- * uses {@code test} methods and should not be used in contexts like a review handler without
- * checking the resulting score.
- *
- * @param label definition of the label to test values of.
- * @param val previously denied value the user attempted.
- * @return nearest likely allowed value, or {@code 0} if no value was identified.
- * @throws PermissionBackendException backend cannot run test.
- */
- public short squashByTest(LabelType label, short val) throws PermissionBackendException {
- return nearest(test(label), val);
- }
-
- private static short nearest(Iterable<LabelPermission.WithValue> possible, short wanted) {
- short s = 0;
- for (LabelPermission.WithValue v : possible) {
- if ((wanted < 0 && v.value() < 0 && wanted <= v.value() && v.value() < s)
- || (wanted > 0 && v.value() > 0 && wanted >= v.value() && v.value() > s)) {
- s = v.value();
- }
- }
- return s;
- }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index 2203f77..484cd0c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -22,6 +22,7 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -300,7 +301,7 @@
.byLabel(ap.getLabel());
if (type != null
&& ap.getValue() == 1
- && type.getFunctionName().equalsIgnoreCase("PatchSetLock")) {
+ && type.getFunction() == LabelFunction.PATCH_SET_LOCK) {
return true;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
index 1917d6f..785ae38 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -79,7 +79,7 @@
for (PatchSetApproval p : object.currentApprovals()) {
if (labelType.matches(p)) {
hasVote = true;
- if (match(object, p.getValue(), p.getAccountId(), labelType)) {
+ if (match(object, p.getValue(), p.getAccountId())) {
return true;
}
}
@@ -105,7 +105,7 @@
return null;
}
- protected boolean match(ChangeData cd, short value, Account.Id approver, LabelType type) {
+ protected boolean match(ChangeData cd, short value, Account.Id approver) {
if (value != expVal) {
return false;
}
@@ -119,11 +119,11 @@
return false;
}
- // Double check the value is still permitted for the user.
+ // Check the user has 'READ' permission.
try {
PermissionBackend.ForChange perm =
permissionBackend.user(reviewer).database(dbProvider).change(cd);
- return perm.test(ChangePermission.READ) && expVal == perm.squashByTest(type, value);
+ return perm.test(ChangePermission.READ);
} catch (PermissionBackendException e) {
return false;
}
diff --git a/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java b/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java
index 9bfcc61..2c76999 100644
--- a/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java
+++ b/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java
@@ -78,7 +78,7 @@
return new StructureTerm(
symLabelType,
SymbolTerm.intern(type.getName()),
- SymbolTerm.intern(type.getFunctionName()),
+ SymbolTerm.intern(type.getFunction().getFunctionName()),
min != null ? new IntegerTerm(min.getValue()) : NONE,
max != null ? new IntegerTerm(max.getValue()) : NONE);
}
diff --git a/gerrit-server/src/main/prolog/gerrit_common.pl b/gerrit-server/src/main/prolog/gerrit_common.pl
index 4671e0d..8fd0657 100644
--- a/gerrit-server/src/main/prolog/gerrit_common.pl
+++ b/gerrit-server/src/main/prolog/gerrit_common.pl
@@ -279,12 +279,12 @@
!,
max_with_block(Label, Min, Max, S).
max_with_block(Label, Min, Max, reject(Who)) :-
- check_label_range_permission(Label, Min, ok(Who)),
+ commit_label(label(Label, Min), Who),
!
.
max_with_block(Label, Min, Max, ok(Who)) :-
- \+ check_label_range_permission(Label, Min, ok(_)),
- check_label_range_permission(Label, Max, ok(Who)),
+ \+ commit_label(label(Label, Min), _),
+ commit_label(label(Label, Max), Who),
!
.
max_with_block(Label, Min, Max, need(Max)) :-
@@ -306,7 +306,7 @@
%%
any_with_block(Label, Min, reject(Who)) :-
Min < 0,
- check_label_range_permission(Label, Min, ok(Who)),
+ commit_label(label(Label, Min), Who),
!
.
any_with_block(Label, Min, may(_)).
@@ -321,7 +321,7 @@
!,
max_no_block(Label, Max, S).
max_no_block(Label, Max, ok(Who)) :-
- check_label_range_permission(Label, Max, ok(Who)),
+ commit_label(label(Label, Max), Who),
!
.
max_no_block(Label, Max, need(Max)) :-
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
index 75b00fd..5453fad 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
@@ -150,7 +150,7 @@
}
@Test
- public void normalizeByPermission() throws Exception {
+ public void noNormalizeByPermission() throws Exception {
ProjectConfig pc = loadAllProjects();
allow(pc, forLabel("Code-Review"), -1, 1, REGISTERED_USERS, "refs/heads/*");
allow(pc, forLabel("Verified"), -1, 1, REGISTERED_USERS, "refs/heads/*");
@@ -158,8 +158,7 @@
PatchSetApproval cr = psa(userId, "Code-Review", 2);
PatchSetApproval v = psa(userId, "Verified", 1);
- assertEquals(
- Result.create(list(v), list(copy(cr, 1)), list()), norm.normalize(notes, list(cr, v)));
+ assertEquals(Result.create(list(cr, v), list(), list()), norm.normalize(notes, list(cr, v)));
}
@Test
@@ -177,10 +176,10 @@
}
@Test
- public void emptyPermissionRangeOmitsResult() throws Exception {
+ public void emptyPermissionRangeKeepsResult() throws Exception {
PatchSetApproval cr = psa(userId, "Code-Review", 1);
PatchSetApproval v = psa(userId, "Verified", 1);
- assertEquals(Result.create(list(), list(), list(cr, v)), norm.normalize(notes, list(cr, v)));
+ assertEquals(Result.create(list(cr, v), list(), list()), norm.normalize(notes, list(cr, v)));
}
@Test
@@ -191,7 +190,7 @@
PatchSetApproval cr = psa(userId, "Code-Review", 0);
PatchSetApproval v = psa(userId, "Verified", 0);
- assertEquals(Result.create(list(cr), list(), list(v)), norm.normalize(notes, list(cr, v)));
+ assertEquals(Result.create(list(cr, v), list(), list()), norm.normalize(notes, list(cr, v)));
}
private ProjectConfig loadAllProjects() throws Exception {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/account/AccountFieldTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/account/AccountFieldTest.java
index 7eed034..6d4f122 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/index/account/AccountFieldTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/account/AccountFieldTest.java
@@ -44,12 +44,7 @@
List<String> values =
toStrings(
AccountField.REF_STATE.get(
- new AccountState(
- allUsersName,
- account,
- ImmutableSet.of(),
- ImmutableSet.of(),
- ImmutableMap.of())));
+ new AccountState(allUsersName, account, ImmutableSet.of(), ImmutableMap.of())));
assertThat(values).hasSize(1);
String expectedValue =
allUsersName.get() + ":" + RefNames.refsUsers(account.getId()) + ":" + metaId;
@@ -78,11 +73,7 @@
toStrings(
AccountField.EXTERNAL_ID_STATE.get(
new AccountState(
- null,
- account,
- ImmutableSet.of(),
- ImmutableSet.of(extId1, extId2),
- ImmutableMap.of())));
+ null, account, ImmutableSet.of(extId1, extId2), ImmutableMap.of())));
String expectedValue1 = extId1.key().sha1().name() + ":" + extId1.blobId().name();
String expectedValue2 = extId2.key().sha1().name() + ":" + extId2.blobId().name();
assertThat(values).containsExactly(expectedValue1, expectedValue2);
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java
index 5215561..d65dd47 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java
@@ -388,7 +388,6 @@
new AllUsersName(AllUsersNameProvider.DEFAULT),
account,
Collections.emptySet(),
- Collections.emptySet(),
new HashMap<>());
}
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
index 5a72d5c..6604641 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
@@ -17,6 +17,7 @@
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
@@ -47,7 +48,7 @@
public static final LabelType patchSetLock() {
LabelType label =
category("Patch-Set-Lock", value(1, "Patch Set Locked"), value(0, "Patch Set Unlocked"));
- label.setFunctionName("PatchSetLock");
+ label.setFunction(LabelFunction.PATCH_SET_LOCK);
return label;
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java
index 7eda3cc..3cd1696 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java
@@ -18,6 +18,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.LabelValue;
@@ -105,7 +106,7 @@
assertThat(codeReview).isNotNull();
assertThat(codeReview.getName()).isEqualTo("Code-Review");
assertThat(codeReview.getDefaultValue()).isEqualTo(0);
- assertThat(codeReview.getFunctionName()).isEqualTo("MaxWithBlock");
+ assertThat(codeReview.getFunction()).isEqualTo(LabelFunction.MAX_WITH_BLOCK);
assertThat(codeReview.isCopyMinScore()).isTrue();
assertValueRange(codeReview, 2, 1, 0, -1, -2);
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java
index 51f5268..b1711e2 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java
@@ -79,7 +79,6 @@
new AllUsersName(AllUsersNameProvider.DEFAULT),
account,
ImmutableSet.of(),
- ImmutableSet.of(),
new HashMap<>());
}
}
diff --git a/gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl b/gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl
index c993394..a7df2b9 100644
--- a/gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl
+++ b/gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl
@@ -65,7 +65,7 @@
test(default_submit_fails) :-
findall(P, default_submit(P), All),
All = [submit(C, V)],
- C = label('Code-Review', ok(test_user(alice))),
+ C = label('Code-Review', ok(_)),
V = label('Verified', need(1)).
@@ -84,7 +84,7 @@
test(can_submit_not_ready) :-
can_submit(gerrit:default_submit, S),
S = not_ready(submit(C, V)),
- C = label('Code-Review', ok(test_user(alice))),
+ C = label('Code-Review', ok(_)),
V = label('Verified', need(1)).
test(can_submit_only_verified_not_ready) :-
@@ -99,7 +99,7 @@
can_submit(gerrit:default_submit, R),
filter_submit_results(filter_out_v, [R], S),
S = [ok(submit(C))],
- C = label('Code-Review', ok(test_user(alice))).
+ C = label('Code-Review', ok(_)).
test(filter_submit_add_code_review) :-
set_commit_labels([
@@ -119,7 +119,7 @@
can_submit(gerrit:default_submit, R),
arg(1, R, S),
find_label(S, 'Code-Review', L),
- L = label('Code-Review', ok(test_user(alice))).
+ L = label('Code-Review', ok(_)).
test(find_default_verified) :-
can_submit(gerrit:default_submit, R),
@@ -133,7 +133,7 @@
test(remove_default_code_review) :-
can_submit(gerrit:default_submit, R),
arg(1, R, S),
- C = label('Code-Review', ok(test_user(alice))),
+ C = label('Code-Review', ok(_)),
remove_label(S, C, Out),
Out = submit(V),
V = label('Verified', need(1)).
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
index 2a82a26..46d6dca 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -135,12 +135,6 @@
private boolean json;
@Option(
- name = "--strict-labels",
- usage = "Strictly check if the labels specified can be applied to the given patch set(s)"
- )
- private boolean strictLabels;
-
- @Option(
name = "--tag",
aliases = "-t",
usage = "applies a tag to the given review",
@@ -274,7 +268,6 @@
review.notify = notify;
review.labels = new TreeMap<>();
review.drafts = ReviewInput.DraftHandling.PUBLISH;
- review.strictLabels = strictLabels;
for (ApproveOption ao : optionList) {
Short v = ao.value();
if (v != null) {
