Merge "Reduce usage of RefControl"
diff --git a/Documentation/cmd-apropos.txt b/Documentation/cmd-apropos.txt
index 31d21c1..2ef71bf 100644
--- a/Documentation/cmd-apropos.txt
+++ b/Documentation/cmd-apropos.txt
@@ -15,7 +15,7 @@
from the matched documents.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-create-project.txt b/Documentation/cmd-create-project.txt
index 503bd12..4d1ea05 100644
--- a/Documentation/cmd-create-project.txt
+++ b/Documentation/cmd-create-project.txt
@@ -102,6 +102,7 @@
* FAST_FORWARD_ONLY: produces a strictly linear history.
* MERGE_IF_NECESSARY: create a merge commit when required.
* REBASE_IF_NECESSARY: rebase the commit when required.
+* REBASE_ALWAYS: always rebase the commit including dependencies.
* MERGE_ALWAYS: always create a merge commit.
* CHERRY_PICK: always cherry-pick the commit.
diff --git a/Documentation/cmd-ls-groups.txt b/Documentation/cmd-ls-groups.txt
index d8eef8b..6d4bdc5 100644
--- a/Documentation/cmd-ls-groups.txt
+++ b/Documentation/cmd-ls-groups.txt
@@ -23,7 +23,7 @@
all groups are listed.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-ls-members.txt b/Documentation/cmd-ls-members.txt
index a6d492c..273451b 100644
--- a/Documentation/cmd-ls-members.txt
+++ b/Documentation/cmd-ls-members.txt
@@ -16,7 +16,7 @@
shown tab-separated.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts. Output is either an error
diff --git a/Documentation/cmd-ls-projects.txt b/Documentation/cmd-ls-projects.txt
index e2e71ff..486ca44 100644
--- a/Documentation/cmd-ls-projects.txt
+++ b/Documentation/cmd-ls-projects.txt
@@ -25,7 +25,7 @@
group, all projects are listed.
== ACCESS
-Any user who has configured an SSH key, or by an user over HTTP.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-query.txt b/Documentation/cmd-query.txt
index 1faf1b0..90e5cdd 100644
--- a/Documentation/cmd-query.txt
+++ b/Documentation/cmd-query.txt
@@ -108,7 +108,7 @@
will be used to cut the result set.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-receive-pack.txt b/Documentation/cmd-receive-pack.txt
index 798f872..b62b9a9 100644
--- a/Documentation/cmd-receive-pack.txt
+++ b/Documentation/cmd-receive-pack.txt
@@ -37,7 +37,7 @@
Deprecated, use `refs/for/branch%cc=address` instead.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== EXAMPLES
diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt
index 4e24701..8f40d6c 100644
--- a/Documentation/cmd-review.txt
+++ b/Documentation/cmd-review.txt
@@ -150,7 +150,7 @@
invocations of the SSH command are required.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-set-members.txt b/Documentation/cmd-set-members.txt
index ae44843..5fb2bb9 100644
--- a/Documentation/cmd-set-members.txt
+++ b/Documentation/cmd-set-members.txt
@@ -49,7 +49,7 @@
order: `--remove`, `--exclude`, `--add`, `--include`
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-set-project.txt b/Documentation/cmd-set-project.txt
index 62d6e92..7282e28 100644
--- a/Documentation/cmd-set-project.txt
+++ b/Documentation/cmd-set-project.txt
@@ -53,6 +53,7 @@
* FAST_FORWARD_ONLY: produces a strictly linear history.
* MERGE_IF_NECESSARY: create a merge commit when required.
* REBASE_IF_NECESSARY: rebase the commit when required.
+* REBASE_ALWAYS: always rebase the commit including dependencies.
* MERGE_ALWAYS: always create a merge commit.
* CHERRY_PICK: always cherry-pick the commit.
diff --git a/Documentation/cmd-set-reviewers.txt b/Documentation/cmd-set-reviewers.txt
index 3d53456..0a757fd 100644
--- a/Documentation/cmd-set-reviewers.txt
+++ b/Documentation/cmd-set-reviewers.txt
@@ -47,7 +47,7 @@
Display site-specific usage information
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-version.txt b/Documentation/cmd-version.txt
index cc797cc..85b0491 100644
--- a/Documentation/cmd-version.txt
+++ b/Documentation/cmd-version.txt
@@ -26,7 +26,7 @@
`<n>` is computed.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index a82370c..fccc32e 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -3650,7 +3650,9 @@
+
The default submit type for newly created projects. Supported values
are `MERGE_IF_NECESSARY`, `FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`,
-`MERGE_ALWAYS` and `CHERRY_PICK`.
+`REBASE_ALWAYS`, `MERGE_ALWAYS` and `CHERRY_PICK`.
++
+For more details see link:project-configuration.html#submit_type[Submit Types].
+
By default, `MERGE_IF_NECESSARY`.
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index e4b9a83..532b8c42 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -11,11 +11,10 @@
[[label_Code-Review]]
== Label: Code-Review
-The code review label is the second of two default labels that is
-configured upon the creation of a Gerrit instance. It may have any
-meaning the project desires. It was originally invented by the Android
-Open Source Project to mean 'I read the code and it seems reasonably
-correct'.
+The Code-Review label is configured upon the creation of a Gerrit
+instance. It may have any meaning the project desires. It was
+originally invented by the Android Open Source Project to mean
+'I read the code and it seems reasonably correct'.
The range of values is:
@@ -87,8 +86,10 @@
Project to mean 'compiles, passes basic unit tests'. Some CI tools
expect to use the Verified label to vote on a change after running.
-Administrators can install the Verified label by adding the following
-text to `project.config`:
+During site initialization the administrator may have chosen to
+configure the default Verified label for all projects. In case it is
+desired to configure it at a later time, administrators can do this by
+adding the following to `project.config` in `All-Projects`:
----
[label "Verified"]
@@ -96,6 +97,7 @@
value = -1 Fails
value = 0 No score
value = +1 Verified
+ copyAllScoresIfNoCodeChange = true
----
The range of values is:
@@ -315,8 +317,8 @@
the commit message is different. This can be used to enable sticky
approvals on labels that only depend on the code, reducing turn-around
if only the commit message is changed prior to submitting a change.
-For the Verified label that is installed by the link:pgm-init.html[init]
-site program this is enabled by default.
+For the Verified label that is optionally installed by the
+link:pgm-init.html[init] site program this is enabled by default.
Defaults to false.
diff --git a/Documentation/error-permission-denied.txt b/Documentation/error-permission-denied.txt
index 574818d..879273d 100644
--- a/Documentation/error-permission-denied.txt
+++ b/Documentation/error-permission-denied.txt
@@ -3,15 +3,20 @@
With this error message an SSH command to Gerrit is rejected if the
SSH authentication is not successful.
-The link:http://en.wikipedia.org/wiki/Secure_Shell[SSH] protocol uses link:http://en.wikipedia.org/wiki/Public-key_cryptography[Public-key Cryptography] for authentication.
-This means for a successful SSH authentication you need your private
-SSH key and the corresponding public SSH key must be known to Gerrit.
+The link:http://en.wikipedia.org/wiki/Secure_Shell[SSH] protocol can use
+link:http://en.wikipedia.org/wiki/Public-key_cryptography[Public-key Cryptography]
+for authentication.
+In general configurations, Gerrit will authenticate you by the public keys
+known to you. Optionally, it can be configured by the administrator to allow
+for link:config-gerrit.html#sshd.kerberosKeytab[kerberos] authentication
+instead.
-If you are facing this problem, do the following:
+In any case, verify that you are using the correct username for the SSH command
+and that it is typed correctly (case sensitive). You can look up your username
+in the Gerrit Web UI under 'Settings' -> 'Profile'.
-. Verify that you are using the correct username for the SSH command
- and that it is typed correctly (case sensitive). You can look up
- your username in the Gerrit Web UI under 'Settings' -> 'Profile'.
+If you are facing this problem and using an SSH keypair, do the following:
+
. Verify that you have uploaded your public SSH key for your Gerrit
account. To do this go in the Gerrit Web UI to 'Settings' ->
'SSH Public Keys' and check that your public SSH key is there. If
@@ -21,6 +26,19 @@
described below. From the trace you should see which private SSH
key is used.
+Debugging kerberos issues can be quite hard given the complexity of the
+protocol. In case you are using kerberos authentication, do the following:
+
+. Verify that you have acquired a valid initial ticket. On a Linux machine, you
+ can acquire one using the `kinit` command. List all your tickets using the
+ `klist` command. It should list all principals for which you have acquired a
+ ticket and include a principal name corresponding to your Gerrit server, for
+ example `HOST/gerrit.mydomain.tld@MYDOMAIN.TLD`.
+ Note that tickets can expire and require you to re-run `kinit` periodically.
+. Verify that your SSH client is using kerberos authentication. For OpenSSH
+ clients this can be controlled using the `GSSAPIAuthentication` setting.
+ For more information see
+ link:user-upload.html#configure_ssh_kerberos[SSH kerberos configuration].
== Test SSH authentication
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt
index 9aa0a3b..948ec25 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -56,8 +56,8 @@
and the link:user-upload.html#http[HTTP/HTTPS] protocols.
[NOTE]
-To use SSH you must link:user-upload.html#configure_ssh[generate an SSH
-key pair and upload the public SSH key to Gerrit].
+To use SSH you may need to link:user-upload.html#ssh[configure your SSH public
+key in your `Settings`].
[[code-review]]
== Code Review Workflow
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index 8cddce2..901f15a 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -86,6 +86,10 @@
* `batch_update/execute_change_ops`: BatchUpdate change update latency,
excluding reindexing
+* `batch_update/retry_attempt_counts`: Distribution of number of attempts made
+by RetryHelper (1 == single attempt, no retry)
+* `batch_update/retry_timeout_count`: Number of executions of RetryHelper that
+ultimately timed out
=== NoteDb
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index cb161af..09fea83 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6418,7 +6418,7 @@
|Field Name ||Description
|`submit_type` ||
Submit type used for this change, can be `MERGE_IF_NECESSARY`,
-`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `MERGE_ALWAYS` or
+`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `REBASE_ALWAYS`, `MERGE_ALWAYS` or
`CHERRY_PICK`.
|`strategy` |optional|
The strategy of the merge, can be `recursive`, `resolve`,
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index 54682ed..7ee7336 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -2582,7 +2582,7 @@
MaxObjectSizeLimitInfo] entity.
|`submit_type` ||
The default submit type of the project, can be `MERGE_IF_NECESSARY`,
-`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `MERGE_ALWAYS` or
+`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `REBASE_ALWAYS`, `MERGE_ALWAYS` or
`CHERRY_PICK`.
|`match_author_to_committer_date` |optional|
link:#inherited-boolean-info[InheritedBooleanInfo] that indicates whether
@@ -2660,7 +2660,7 @@
If not set, this setting is not updated.
|`submit_type` |optional|
The default submit type of the project, can be `MERGE_IF_NECESSARY`,
-`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `MERGE_ALWAYS` or
+`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `REBASE_ALWAYS`, `MERGE_ALWAYS` or
`CHERRY_PICK`. +
If not set, the submit type is not updated.
|`state` |optional|
@@ -2966,8 +2966,8 @@
Whether an empty initial commit should be created.
|`submit_type` |optional|
The submit type that should be set for the project
-(`MERGE_IF_NECESSARY`, `REBASE_IF_NECESSARY`, `FAST_FORWARD_ONLY`,
-`MERGE_ALWAYS`, `CHERRY_PICK`). +
+(`MERGE_IF_NECESSARY`, `REBASE_IF_NECESSARY`, `REBASE_ALWAYS`,
+`FAST_FORWARD_ONLY`, `MERGE_ALWAYS`, `CHERRY_PICK`). +
If not set, `MERGE_IF_NECESSARY` is set as submit type unless
link:config-gerrit.html#repository.name.defaultSubmitType[
repository.<name>.defaultSubmitType] is set to a different value.
diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt
index deec660..cb00a84 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -9,8 +9,8 @@
All three methods rely on authentication, which must first be configured
by the uploading user.
-Gerrit supports two methods of authenticating the uploading user. SSH
-public key, and HTTP/HTTPS.
+Gerrit supports two protocols for uploading changes; SSH and HTTP/HTTPS. These
+may not all be available for you, depending on the server configuration.
[[http]]
== HTTP/HTTPS
@@ -41,13 +41,15 @@
[[ssh]]
== SSH
-Each user uploading changes to Gerrit must configure one or more SSH
-public keys. The per-user SSH key list can be accessed over the web
-within Gerrit by `Settings`, and then accessing the `SSH Public Keys`
-tab.
+To upload changes over SSH, Gerrit supports two forms of authentication: a
+user's public key or kerberos.
-[[configure_ssh]]
-=== Configuration
+Unless your Gerrit instance is configured to support
+link:config-gerrit.html#sshd.kerberosKeytab[kerberos] in your domain, only
+public key authentication can be used.
+
+[[configure_ssh_public_keys]]
+=== Public keys
To register a new SSH key for use with Gerrit, paste the contents of
your `id_rsa.pub` or `id_dsa.pub` file into the text box and click
@@ -79,10 +81,29 @@
documentation, for more details on configuration of the agent
process and how to add the private key.
+[[configure_ssh_kerberos]]
+=== Kerberos
+
+A kerberos-enabled server configuration allows for zero configuration in an
+existing single-sign-on environment.
+
+Your SSH client should be configured to enable kerberos authentication. For
+OpenSSH clients, this is controlled by the option `GSSAPIAuthentication` which
+should be set to `yes`.
+
+Some Linux distributions have packaged OpenSSH to enable this by default (e.g.
+Debian, Ubuntu). If this is not the case for your distribution, enable it for
+Gerrit with this entry in your local SSH configuration:
+
+----
+Host gerrit.mydomain.tld
+ GSSAPIAuthentication yes
+----
+
[[test_ssh]]
=== Testing Connections
-To verify your SSH key is working correctly, try using an SSH client
+To verify your SSH authentication is working correctly, try using an SSH client
to connect to Gerrit's SSHD port. By default Gerrit runs on
port 29418, using the same hostname as the web server:
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 6733380..6a1e3b9 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
@@ -1134,6 +1134,12 @@
return name;
}
+ protected String createAccount(String name, String group) throws Exception {
+ name = name(name);
+ accountCreator.create(name, group);
+ return name;
+ }
+
protected RevCommit getHead(Repository repo, String name) throws Exception {
try (RevWalk rw = new RevWalk(repo)) {
Ref r = repo.exactRef(name);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index f472b50..0d68f4a 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -31,6 +31,7 @@
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import static org.junit.Assert.fail;
@@ -1313,6 +1314,26 @@
}
}
+ @Test
+ public void groups() throws Exception {
+ assertGroups(
+ admin.username, ImmutableList.of("Anonymous Users", "Registered Users", "Administrators"));
+
+ //TODO: update when test user is fixed to be included in "Anonymous Users" and
+ // "Registered Users" groups
+ assertGroups(user.username, ImmutableList.of());
+
+ String group = createGroup("group");
+ String newUser = createAccount("user1", group);
+ assertGroups(newUser, ImmutableList.of(group));
+ }
+
+ private void assertGroups(String user, List<String> expected) throws Exception {
+ List<String> actual =
+ gApi.accounts().id(user).getGroups().stream().map(g -> g.name).collect(toList());
+ assertThat(actual).containsExactlyElementsIn(expected);
+ }
+
private void assertSequenceNumbers(List<SshKeyInfo> sshKeys) {
int seq = 1;
for (SshKeyInfo key : sshKeys) {
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 320faa0..69315a2 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
@@ -1797,7 +1797,7 @@
setApiUser(user);
exception.expect(AuthException.class);
- exception.expectMessage("delete reviewer not permitted");
+ exception.expectMessage("remove reviewer not permitted");
gApi.changes().id(r.getChangeId()).reviewer(admin.getId().toString()).remove();
}
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 26383e5..1b5e544a 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
@@ -687,12 +687,6 @@
return groupCache.get(new AccountGroup.NameKey(name));
}
- private String createAccount(String name, String group) throws Exception {
- name = name(name);
- accountCreator.create(name, group);
- return name;
- }
-
private void setCreatedOnToNull(AccountGroup.UUID groupUuid) throws Exception {
groupsUpdateProvider.get().updateGroup(db, groupUuid, group -> group.setCreatedOn(null));
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
index 33ff5a4..e5816dd 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
@@ -25,19 +25,15 @@
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.common.data.GlobalCapability;
-import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupDescriptions;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
-import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.group.CreateGroup;
-import com.google.gerrit.server.group.GroupsCollection;
import com.google.inject.Inject;
import java.util.Arrays;
import java.util.List;
@@ -48,8 +44,6 @@
public class SuggestReviewersIT extends AbstractDaemonTest {
@Inject private CreateGroup.Factory createGroupFactory;
- @Inject private GroupsCollection groups;
-
private AccountGroup group1;
private AccountGroup group2;
private AccountGroup group3;
@@ -432,8 +426,7 @@
private AccountGroup group(String name) throws Exception {
GroupInfo group = createGroupFactory.create(name(name)).apply(TopLevelResource.INSTANCE, null);
- GroupDescription.Basic d = groups.parseInternal(Url.decode(group.id));
- return GroupDescriptions.toAccountGroup(d);
+ return groupCache.get(new AccountGroup.UUID(group.id));
}
private TestAccount user(String name, String fullName, String emailName, AccountGroup... groups)
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescription.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescription.java
index 62a8544..ef3cfd0 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescription.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescription.java
@@ -16,6 +16,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import java.sql.Timestamp;
/** Group methods exposed by the GroupBackend. */
public class GroupDescription {
@@ -42,10 +43,18 @@
String getUrl();
}
- /** The extended information exposed by internal groups backed by an AccountGroup. */
+ /** The extended information exposed by internal groups. */
public interface Internal extends Basic {
- /** @return the backing AccountGroup. */
- AccountGroup getAccountGroup();
+
+ AccountGroup.Id getId();
+
+ String getDescription();
+
+ AccountGroup.UUID getOwnerGroupUUID();
+
+ boolean isVisibleToAll();
+
+ Timestamp getCreatedOn();
}
private GroupDescription() {}
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescriptions.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescriptions.java
index 0c06868..b7a06c5 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescriptions.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescriptions.java
@@ -17,18 +17,11 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import java.sql.Timestamp;
/** Utility class for building GroupDescription objects. */
public class GroupDescriptions {
- @Nullable
- public static AccountGroup toAccountGroup(GroupDescription.Basic group) {
- if (group instanceof GroupDescription.Internal) {
- return ((GroupDescription.Internal) group).getAccountGroup();
- }
- return null;
- }
-
public static GroupDescription.Internal forAccountGroup(AccountGroup group) {
return new GroupDescription.Internal() {
@Override
@@ -42,21 +35,40 @@
}
@Override
- public AccountGroup getAccountGroup() {
- return group;
- }
-
- @Override
@Nullable
public String getEmailAddress() {
return null;
}
@Override
- @Nullable
public String getUrl() {
return "#" + PageLinks.toGroup(getGroupUUID());
}
+
+ @Override
+ public AccountGroup.Id getId() {
+ return group.getId();
+ }
+
+ @Override
+ public String getDescription() {
+ return group.getDescription();
+ }
+
+ @Override
+ public AccountGroup.UUID getOwnerGroupUUID() {
+ return group.getOwnerGroupUUID();
+ }
+
+ @Override
+ public boolean isVisibleToAll() {
+ return group.isVisibleToAll();
+ }
+
+ @Override
+ public Timestamp getCreatedOn() {
+ return group.getCreatedOn();
+ }
};
}
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupInfo.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupInfo.java
index 5de0aad..2b5bf1b 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupInfo.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupInfo.java
@@ -46,8 +46,7 @@
url = a.getUrl();
if (a instanceof GroupDescription.Internal) {
- AccountGroup group = ((GroupDescription.Internal) a).getAccountGroup();
- description = group.getDescription();
+ description = ((GroupDescription.Internal) a).getDescription();
}
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
index b88097c..912ad64 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
@@ -25,6 +25,7 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.EmailInfo;
import com.google.gerrit.extensions.common.GpgKeyInfo;
+import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.SshKeyInfo;
import com.google.gerrit.extensions.restapi.NotImplementedException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -69,6 +70,8 @@
List<ChangeInfo> getStarredChanges() throws RestApiException;
+ List<GroupInfo> getGroups() throws RestApiException;
+
List<EmailInfo> getEmails() throws RestApiException;
void addEmail(EmailInput input) throws RestApiException;
@@ -197,6 +200,11 @@
}
@Override
+ public List<GroupInfo> getGroups() throws RestApiException {
+ throw new NotImplementedException();
+ }
+
+ @Override
public List<EmailInfo> getEmails() throws RestApiException {
throw new NotImplementedException();
}
diff --git a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
index c86d804..7044547 100644
--- a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
+++ b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
@@ -37,6 +37,17 @@
}
@Test
+ public void changeRefs() throws Exception {
+ String changeMetaRef = RefNames.changeMetaRef(changeId);
+ assertThat(changeMetaRef).isEqualTo("refs/changes/73/67473/meta");
+ assertThat(RefNames.isNoteDbMetaRef(changeMetaRef)).isTrue();
+
+ String robotCommentsRef = RefNames.robotCommentsRef(changeId);
+ assertThat(robotCommentsRef).isEqualTo("refs/changes/73/67473/robot-comments");
+ assertThat(RefNames.isNoteDbMetaRef(robotCommentsRef)).isTrue();
+ }
+
+ @Test
public void refsUsers() throws Exception {
assertThat(RefNames.refsUsers(accountId)).isEqualTo("refs/users/23/1011123");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
index 1812ef4..70cbb6d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
@@ -18,7 +18,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GlobalCapability;
-import com.google.gerrit.common.data.GroupDescriptions;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.InvalidSshKeyException;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.annotations.RequiresCapability;
@@ -211,8 +211,8 @@
Set<AccountGroup.UUID> groupUuids = new HashSet<>();
if (groups != null) {
for (String g : groups) {
- AccountGroup group = GroupDescriptions.toAccountGroup(groupsCollection.parseInternal(g));
- groupUuids.add(group.getGroupUUID());
+ GroupDescription.Internal internalGroup = groupsCollection.parseInternal(g);
+ groupUuids.add(internalGroup.getGroupUUID());
}
}
return groupUuids;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
index 56f8d63..5af4898 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
@@ -142,12 +142,15 @@
}
public boolean isOwner() {
- AccountGroup accountGroup = GroupDescriptions.toAccountGroup(group);
- if (accountGroup == null) {
- isOwner = false;
- } else if (isOwner == null) {
- AccountGroup.UUID ownerUUID = accountGroup.getOwnerGroupUUID();
+ if (isOwner != null) {
+ return isOwner;
+ }
+
+ if (group instanceof GroupDescription.Internal) {
+ AccountGroup.UUID ownerUUID = ((GroupDescription.Internal) group).getOwnerGroupUUID();
isOwner = getUser().getEffectiveGroups().contains(ownerUUID) || canAdministrateServer();
+ } else {
+ isOwner = false;
}
return isOwner;
}
@@ -189,7 +192,9 @@
}
private boolean canSeeMembers() {
- AccountGroup accountGroup = GroupDescriptions.toAccountGroup(group);
- return (accountGroup != null && accountGroup.isVisibleToAll()) || isOwner();
+ if (group instanceof GroupDescription.Internal) {
+ return ((GroupDescription.Internal) group).isVisibleToAll() || isOwner();
+ }
+ return false;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
index a42362c..9764d80 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
@@ -85,6 +85,6 @@
@Override
public boolean isVisibleToAll(AccountGroup.UUID uuid) {
GroupDescription.Internal g = get(uuid);
- return g != null && g.getAccountGroup().isVisibleToAll();
+ return g != null && g.isVisibleToAll();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
index 64760a65..f8539d9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
@@ -33,6 +33,7 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.EmailInfo;
import com.google.gerrit.extensions.common.GpgKeyInfo;
+import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.SshKeyInfo;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.Response;
@@ -54,6 +55,7 @@
import com.google.gerrit.server.account.GetEditPreferences;
import com.google.gerrit.server.account.GetEmails;
import com.google.gerrit.server.account.GetExternalIds;
+import com.google.gerrit.server.account.GetGroups;
import com.google.gerrit.server.account.GetPreferences;
import com.google.gerrit.server.account.GetSshKeys;
import com.google.gerrit.server.account.GetWatchedProjects;
@@ -70,6 +72,7 @@
import com.google.gerrit.server.account.Stars;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.List;
@@ -116,6 +119,7 @@
private final GetExternalIds getExternalIds;
private final DeleteExternalIds deleteExternalIds;
private final PutStatus putStatus;
+ private final GetGroups getGroups;
@Inject
AccountApiImpl(
@@ -153,6 +157,7 @@
GetExternalIds getExternalIds,
DeleteExternalIds deleteExternalIds,
PutStatus putStatus,
+ GetGroups getGroups,
@Assisted AccountResource account) {
this.account = account;
this.accountLoaderFactory = ailf;
@@ -189,6 +194,7 @@
this.getExternalIds = getExternalIds;
this.deleteExternalIds = deleteExternalIds;
this.putStatus = putStatus;
+ this.getGroups = getGroups;
}
@Override
@@ -363,6 +369,15 @@
}
@Override
+ public List<GroupInfo> getGroups() throws RestApiException {
+ try {
+ return getGroups.apply(account);
+ } catch (OrmException e) {
+ throw asRestApiException("Cannot get groups", e);
+ }
+ }
+
+ @Override
public List<EmailInfo> getEmails() {
return getEmails.apply(account);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 6471788..3ceeb24 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -116,7 +116,9 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
@@ -212,6 +214,7 @@
private final ChangeKindCache changeKindCache;
private final ChangeIndexCollection indexes;
private final ApprovalsUtil approvalsUtil;
+ private final RemoveReviewerControl removeReviewerControl;
private boolean lazyLoad = true;
private AccountLoader accountLoader;
@@ -243,6 +246,7 @@
ChangeKindCache changeKindCache,
ChangeIndexCollection indexes,
ApprovalsUtil approvalsUtil,
+ RemoveReviewerControl removeReviewerControl,
@Assisted Iterable<ListChangesOption> options) {
this.db = db;
this.userProvider = user;
@@ -267,6 +271,7 @@
this.changeKindCache = changeKindCache;
this.indexes = indexes;
this.approvalsUtil = approvalsUtil;
+ this.removeReviewerControl = removeReviewerControl;
this.options = Sets.immutableEnumSet(options);
}
@@ -1100,7 +1105,8 @@
return result;
}
- private Collection<AccountInfo> removableReviewers(ChangeControl ctl, ChangeInfo out) {
+ private Collection<AccountInfo> removableReviewers(ChangeControl ctl, ChangeInfo out)
+ throws PermissionBackendException, NoSuchChangeException {
// Although this is called removableReviewers, this method also determines
// which CCs are removable.
//
@@ -1120,7 +1126,9 @@
}
for (ApprovalInfo ai : label.all) {
Account.Id id = new Account.Id(ai._accountId);
- if (ctl.canRemoveReviewer(id, MoreObjects.firstNonNull(ai.value, 0))) {
+
+ if (removeReviewerControl.testRemoveReviewer(
+ ctl.getNotes(), ctl.getUser(), id, MoreObjects.firstNonNull(ai.value, 0))) {
removable.add(id);
} else {
fixed.add(id);
@@ -1137,7 +1145,7 @@
for (AccountInfo ai : ccs) {
if (ai._accountId != null) {
Account.Id id = new Account.Id(ai._accountId);
- if (ctl.canRemoveReviewer(id, 0)) {
+ if (removeReviewerControl.testRemoveReviewer(ctl.getNotes(), ctl.getUser(), id, 0)) {
removable.add(id);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index df4b435..933705a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -38,6 +38,8 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.BatchUpdateReviewDb;
import com.google.gerrit.server.update.ChangeContext;
@@ -70,6 +72,7 @@
private final DeleteReviewerSender.Factory deleteReviewerSenderFactory;
private final NotesMigration migration;
private final NotifyUtil notifyUtil;
+ private final RemoveReviewerControl removeReviewerControl;
private final Account reviewer;
private final DeleteReviewerInput input;
@@ -91,6 +94,7 @@
DeleteReviewerSender.Factory deleteReviewerSenderFactory,
NotesMigration migration,
NotifyUtil notifyUtil,
+ RemoveReviewerControl removeReviewerControl,
@Assisted Account reviewerAccount,
@Assisted DeleteReviewerInput input) {
this.approvalsUtil = approvalsUtil;
@@ -102,6 +106,7 @@
this.deleteReviewerSenderFactory = deleteReviewerSenderFactory;
this.migration = migration;
this.notifyUtil = notifyUtil;
+ this.removeReviewerControl = removeReviewerControl;
this.reviewer = reviewerAccount;
this.input = input;
@@ -109,7 +114,7 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws AuthException, ResourceNotFoundException, OrmException {
+ throws AuthException, ResourceNotFoundException, OrmException, PermissionBackendException {
Account.Id reviewerId = reviewer.getId();
if (!approvalsUtil.getReviewers(ctx.getDb(), ctx.getNotes()).all().contains(reviewerId)) {
throw new ResourceNotFoundException();
@@ -130,21 +135,18 @@
List<PatchSetApproval> del = new ArrayList<>();
boolean votesRemoved = false;
for (PatchSetApproval a : approvals(ctx, reviewerId)) {
- if (ctx.getControl().canRemoveReviewer(a)) {
- del.add(a);
- if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) {
- oldApprovals.put(a.getLabel(), a.getValue());
- removedVotesMsg
- .append("* ")
- .append(a.getLabel())
- .append(formatLabelValue(a.getValue()))
- .append(" by ")
- .append(userFactory.create(a.getAccountId()).getNameEmail())
- .append("\n");
- votesRemoved = true;
- }
- } else {
- throw new AuthException("delete reviewer not permitted");
+ removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
+ del.add(a);
+ if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) {
+ oldApprovals.put(a.getLabel(), a.getValue());
+ removedVotesMsg
+ .append("* ")
+ .append(a.getLabel())
+ .append(formatLabelValue(a.getValue()))
+ .append(" by ")
+ .append(userFactory.create(a.getAccountId()).getNameEmail())
+ .append("\n");
+ votesRemoved = true;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
index c31f72d..7029101 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
@@ -40,7 +40,9 @@
import com.google.gerrit.server.extensions.events.VoteDeleted;
import com.google.gerrit.server.mail.send.DeleteVoteSender;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -72,6 +74,7 @@
private final VoteDeleted voteDeleted;
private final DeleteVoteSender.Factory deleteVoteSenderFactory;
private final NotifyUtil notifyUtil;
+ private final RemoveReviewerControl removeReviewerControl;
@Inject
DeleteVote(
@@ -83,7 +86,8 @@
IdentifiedUser.GenericFactory userFactory,
VoteDeleted voteDeleted,
DeleteVoteSender.Factory deleteVoteSenderFactory,
- NotifyUtil notifyUtil) {
+ NotifyUtil notifyUtil,
+ RemoveReviewerControl removeReviewerControl) {
super(retryHelper);
this.db = db;
this.approvalsUtil = approvalsUtil;
@@ -93,6 +97,7 @@
this.voteDeleted = voteDeleted;
this.deleteVoteSenderFactory = deleteVoteSenderFactory;
this.notifyUtil = notifyUtil;
+ this.removeReviewerControl = removeReviewerControl;
}
@Override
@@ -143,7 +148,8 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws OrmException, AuthException, ResourceNotFoundException, IOException {
+ throws OrmException, AuthException, ResourceNotFoundException, IOException,
+ PermissionBackendException {
ChangeControl ctl = ctx.getControl();
change = ctl.getChange();
PatchSet.Id psId = change.currentPatchSetId();
@@ -166,8 +172,12 @@
// Populate map for non-matching labels, needed by VoteDeleted.
newApprovals.put(a.getLabel(), a.getValue());
continue;
- } else if (!ctl.canRemoveReviewer(a)) {
- throw new AuthException("delete vote not permitted");
+ } else {
+ try {
+ removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
+ } catch (AuthException e) {
+ throw new AuthException("delete vote not permitted", e);
+ }
}
// Set the approval to 0 if vote is being removed.
newApprovals.put(a.getLabel(), (short) 0);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java
index 109baa8..27692c8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java
@@ -87,10 +87,8 @@
public List<GroupInfo> apply(GroupResource resource, Input input)
throws MethodNotAllowedException, AuthException, UnprocessableEntityException, OrmException,
ResourceNotFoundException {
- AccountGroup group = resource.toAccountGroup();
- if (group == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal group =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
input = Input.init(input);
GroupControl control = resource.getControl();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java
index 15bcc18..96024d2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java
@@ -17,6 +17,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.client.AuthType;
import com.google.gerrit.extensions.common.AccountInfo;
@@ -111,10 +112,8 @@
public List<AccountInfo> apply(GroupResource resource, Input input)
throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException,
IOException, ConfigInvalidException, ResourceNotFoundException {
- AccountGroup internalGroup = resource.toAccountGroup();
- if (internalGroup == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal internalGroup =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
input = Input.init(input);
GroupControl control = resource.getControl();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
index 19974c9..804d3e2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
@@ -166,8 +166,8 @@
private AccountGroup.Id owner(GroupInput input) throws UnprocessableEntityException {
if (input.ownerId != null) {
- GroupDescription.Basic d = groups.parseInternal(Url.decode(input.ownerId));
- return GroupDescriptions.toAccountGroup(d).getId();
+ GroupDescription.Internal d = groups.parseInternal(Url.decode(input.ownerId));
+ return d.getId();
}
return null;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java
index ebdb12d..64618c1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java
@@ -54,10 +54,8 @@
public Response<?> apply(GroupResource resource, Input input)
throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException,
ResourceNotFoundException {
- AccountGroup internalGroup = resource.toAccountGroup();
- if (internalGroup == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal internalGroup =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
input = Input.init(input);
final GroupControl control = resource.getControl();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java
index babff37..1069e1c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteMembers.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.group;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
@@ -56,10 +57,8 @@
public Response<?> apply(GroupResource resource, Input input)
throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException,
IOException, ConfigInvalidException, ResourceNotFoundException {
- AccountGroup internalGroup = resource.toAccountGroup();
- if (internalGroup == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal internalGroup =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
input = Input.init(input);
final GroupControl control = resource.getControl();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetAuditLog.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetAuditLog.java
index e2a467a..4ea7041 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetAuditLog.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetAuditLog.java
@@ -64,10 +64,9 @@
@Override
public List<? extends GroupAuditEventInfo> apply(GroupResource rsrc)
throws AuthException, MethodNotAllowedException, OrmException {
- AccountGroup group = rsrc.toAccountGroup();
- if (group == null) {
- throw new MethodNotAllowedException();
- } else if (!rsrc.getControl().isOwner()) {
+ GroupDescription.Internal group =
+ rsrc.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
+ if (!rsrc.getControl().isOwner()) {
throw new AuthException("Not group owner");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetDescription.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetDescription.java
index 6900b83..0610843 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetDescription.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetDescription.java
@@ -15,19 +15,17 @@
package com.google.gerrit.server.group;
import com.google.common.base.Strings;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.inject.Singleton;
@Singleton
public class GetDescription implements RestReadView<GroupResource> {
@Override
public String apply(GroupResource resource) throws MethodNotAllowedException {
- AccountGroup group = resource.toAccountGroup();
- if (group == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal group =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
return Strings.nullToEmpty(group.getDescription());
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetOwner.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetOwner.java
index 464be18..03d0788 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetOwner.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetOwner.java
@@ -14,12 +14,12 @@
package com.google.gerrit.server.group;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.account.GroupControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -40,10 +40,8 @@
@Override
public GroupInfo apply(GroupResource resource)
throws MethodNotAllowedException, ResourceNotFoundException, OrmException {
- AccountGroup group = resource.toAccountGroup();
- if (group == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal group =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
try {
GroupControl c = controlFactory.validateFor(group.getOwnerGroupUUID());
return json.format(c.getGroup());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupJson.java
index 0be167d..639ee55 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupJson.java
@@ -19,7 +19,6 @@
import com.google.common.base.Strings;
import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupDescriptions;
import com.google.gerrit.extensions.client.ListGroupsOption;
import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.GroupOptionsInfo;
@@ -37,8 +36,7 @@
public class GroupJson {
public static GroupOptionsInfo createOptions(GroupDescription.Basic group) {
GroupOptionsInfo options = new GroupOptionsInfo();
- AccountGroup ag = GroupDescriptions.toAccountGroup(group);
- if (ag != null && ag.isVisibleToAll()) {
+ if (isInternalGroup(group) && ((GroupDescription.Internal) group).isVisibleToAll()) {
options.visibleToAll = true;
}
return options;
@@ -96,25 +94,30 @@
info.url = Strings.emptyToNull(group.getUrl());
info.options = createOptions(group);
- AccountGroup g = GroupDescriptions.toAccountGroup(group);
- if (g != null) {
- info.description = Strings.emptyToNull(g.getDescription());
- info.groupId = g.getId().get();
- if (g.getOwnerGroupUUID() != null) {
- info.ownerId = Url.encode(g.getOwnerGroupUUID().get());
- GroupDescription.Basic o = groupBackend.get(g.getOwnerGroupUUID());
+ if (isInternalGroup(group)) {
+ GroupDescription.Internal internalGroup = (GroupDescription.Internal) group;
+ info.description = Strings.emptyToNull(internalGroup.getDescription());
+ info.groupId = internalGroup.getId().get();
+ AccountGroup.UUID ownerGroupUUID = internalGroup.getOwnerGroupUUID();
+ if (ownerGroupUUID != null) {
+ info.ownerId = Url.encode(ownerGroupUUID.get());
+ GroupDescription.Basic o = groupBackend.get(ownerGroupUUID);
if (o != null) {
info.owner = o.getName();
}
}
- info.createdOn = g.getCreatedOn();
+ info.createdOn = internalGroup.getCreatedOn();
}
return info;
}
+ private static boolean isInternalGroup(GroupDescription.Basic group) {
+ return group instanceof GroupDescription.Internal;
+ }
+
private GroupInfo initMembersAndIncludes(GroupResource rsrc, GroupInfo info) throws OrmException {
- if (rsrc.toAccountGroup() == null) {
+ if (!rsrc.isInternalGroup()) {
return info;
}
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupResource.java
index 54fc787..44e770f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupResource.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupResource.java
@@ -15,12 +15,11 @@
package com.google.gerrit.server.group;
import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupDescriptions;
import com.google.gerrit.extensions.restapi.RestResource;
import com.google.gerrit.extensions.restapi.RestView;
-import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.account.GroupControl;
import com.google.inject.TypeLiteral;
+import java.util.Optional;
public class GroupResource implements RestResource {
public static final TypeLiteral<RestView<GroupResource>> GROUP_KIND =
@@ -44,12 +43,17 @@
return getGroup().getName();
}
- public AccountGroup.UUID getGroupUUID() {
- return getGroup().getGroupUUID();
+ public boolean isInternalGroup() {
+ GroupDescription.Basic group = getGroup();
+ return group instanceof GroupDescription.Internal;
}
- public AccountGroup toAccountGroup() {
- return GroupDescriptions.toAccountGroup(getGroup());
+ public Optional<GroupDescription.Internal> asInternalGroup() {
+ GroupDescription.Basic group = getGroup();
+ if (group instanceof GroupDescription.Internal) {
+ return Optional.of((GroupDescription.Internal) group);
+ }
+ return Optional.empty();
}
public GroupControl getControl() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java
index 397bf08..c8ab8f7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java
@@ -16,7 +16,6 @@
import com.google.common.collect.ListMultimap;
import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupDescriptions;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -142,12 +141,13 @@
* @throws UnprocessableEntityException thrown if the group ID cannot be resolved, if the group is
* not visible to the calling user or if it's an external group
*/
- public GroupDescription.Basic parseInternal(String id) throws UnprocessableEntityException {
+ public GroupDescription.Internal parseInternal(String id) throws UnprocessableEntityException {
GroupDescription.Basic group = parse(id);
- if (GroupDescriptions.toAccountGroup(group) == null) {
- throw new UnprocessableEntityException(String.format("External Group Not Allowed: %s", id));
+ if (group instanceof GroupDescription.Internal) {
+ return (GroupDescription.Internal) group;
}
- return group;
+
+ throw new UnprocessableEntityException(String.format("External Group Not Allowed: %s", id));
}
/**
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java
index 1fecc38..d48acee 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/IncludedGroupsCollection.java
@@ -25,7 +25,6 @@
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
-import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.group.AddIncludedGroups.PutIncludedGroup;
import com.google.gwtorm.server.OrmException;
@@ -67,10 +66,8 @@
@Override
public IncludedGroupResource parse(GroupResource resource, IdString id)
throws MethodNotAllowedException, AuthException, ResourceNotFoundException, OrmException {
- AccountGroup parent = resource.toAccountGroup();
- if (parent == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal parent =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
GroupDescription.Basic member =
groupsCollection.parse(TopLevelResource.INSTANCE, id).getGroup();
@@ -80,7 +77,7 @@
throw new ResourceNotFoundException(id);
}
- private boolean isMember(AccountGroup parent, GroupDescription.Basic member)
+ private boolean isMember(GroupDescription.Internal parent, GroupDescription.Basic member)
throws OrmException, ResourceNotFoundException {
try {
return groups.isIncluded(dbProvider.get(), parent.getGroupUUID(), member.getGroupUUID());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/Index.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/Index.java
index b7b98b2..5d076d9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/Index.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/Index.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.group;
-import com.google.gerrit.common.data.GroupDescriptions;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -44,14 +43,15 @@
throw new AuthException("not allowed to index group");
}
- AccountGroup group = GroupDescriptions.toAccountGroup(rsrc.getGroup());
- if (group == null) {
+ AccountGroup.UUID groupUuid = rsrc.getGroup().getGroupUUID();
+ if (!rsrc.isInternalGroup()) {
throw new UnprocessableEntityException(
- String.format("External Group Not Allowed: %s", rsrc.getGroupUUID().get()));
+ String.format("External Group Not Allowed: %s", groupUuid.get()));
}
+ AccountGroup accountGroup = groupCache.get(groupUuid);
// evicting the group from the cache, reindexes the group
- groupCache.evict(group);
+ groupCache.evict(accountGroup);
return Response.none();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java
index 9004a8a..33d9b57 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java
@@ -16,6 +16,7 @@
import static com.google.common.base.Strings.nullToEmpty;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
@@ -51,14 +52,13 @@
@Override
public List<GroupInfo> apply(GroupResource rsrc) throws MethodNotAllowedException, OrmException {
- if (rsrc.toAccountGroup() == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal group =
+ rsrc.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
boolean ownerOfParent = rsrc.getControl().isOwner();
List<GroupInfo> included = new ArrayList<>();
Collection<AccountGroup.UUID> includedGroupUuids =
- groupIncludeCache.subgroupsOf(rsrc.toAccountGroup().getGroupUUID());
+ groupIncludeCache.subgroupsOf(group.getGroupUUID());
for (AccountGroup.UUID includedGroupUuid : includedGroupUuids) {
try {
GroupControl i = controlFactory.controlFor(includedGroupUuid);
@@ -69,7 +69,7 @@
log.warn(
String.format(
"Group %s no longer available, included into %s",
- includedGroupUuid, rsrc.getGroup().getName()));
+ includedGroupUuid, group.getName()));
continue;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java
index 3d383a0..0f8aa40 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.group;
import com.google.common.collect.Lists;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.common.AccountInfo;
@@ -61,11 +62,9 @@
@Override
public List<AccountInfo> apply(GroupResource resource)
throws MethodNotAllowedException, OrmException {
- if (resource.toAccountGroup() == null) {
- throw new MethodNotAllowedException();
- }
-
- return apply(resource.getGroupUUID());
+ GroupDescription.Internal group =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
+ return apply(group.getGroupUUID());
}
public List<AccountInfo> apply(AccountGroup group) throws OrmException {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java
index 66f5b26..4c0d458 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.group;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AcceptsCreate;
@@ -71,10 +72,8 @@
public MemberResource parse(GroupResource parent, IdString id)
throws MethodNotAllowedException, AuthException, ResourceNotFoundException, OrmException,
IOException, ConfigInvalidException {
- AccountGroup group = parent.toAccountGroup();
- if (group == null) {
- throw new MethodNotAllowedException();
- }
+ GroupDescription.Internal group =
+ parent.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
IdentifiedUser user = accounts.parse(TopLevelResource.INSTANCE, id).getUser();
if (parent.getControl().canSeeMember(user.getAccountId()) && isMember(group, user)) {
@@ -83,7 +82,7 @@
throw new ResourceNotFoundException(id);
}
- private boolean isMember(AccountGroup group, IdentifiedUser user)
+ private boolean isMember(GroupDescription.Internal group, IdentifiedUser user)
throws OrmException, ResourceNotFoundException {
AccountGroup.UUID groupUuid = group.getGroupUUID();
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java
index f0709b4..3d6feea 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutDescription.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.group;
import com.google.common.base.Strings;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.DefaultInput;
@@ -56,16 +57,15 @@
input = new Input(); // Delete would set description to null.
}
- AccountGroup accountGroup = resource.toAccountGroup();
- if (accountGroup == null) {
- throw new MethodNotAllowedException();
- } else if (!resource.getControl().isOwner()) {
+ GroupDescription.Internal internalGroup =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
+ if (!resource.getControl().isOwner()) {
throw new AuthException("Not group owner");
}
String newDescription = Strings.emptyToNull(input.description);
- if (!Objects.equals(accountGroup.getDescription(), newDescription)) {
- AccountGroup.UUID groupUuid = resource.getGroupUUID();
+ if (!Objects.equals(internalGroup.getDescription(), newDescription)) {
+ AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
try {
groupsUpdateProvider
.get()
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java
index 9ef8d8727..75a7eb5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutName.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.group;
import com.google.common.base.Strings;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NameAlreadyUsedException;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -52,9 +53,9 @@
public String apply(GroupResource rsrc, Input input)
throws MethodNotAllowedException, AuthException, BadRequestException,
ResourceConflictException, ResourceNotFoundException, OrmException, IOException {
- if (rsrc.toAccountGroup() == null) {
- throw new MethodNotAllowedException();
- } else if (!rsrc.getControl().isOwner()) {
+ GroupDescription.Internal internalGroup =
+ rsrc.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
+ if (!rsrc.getControl().isOwner()) {
throw new AuthException("Not group owner");
} else if (input == null || Strings.isNullOrEmpty(input.name)) {
throw new BadRequestException("name is required");
@@ -64,15 +65,15 @@
throw new BadRequestException("name is required");
}
- if (rsrc.toAccountGroup().getName().equals(newName)) {
+ if (internalGroup.getName().equals(newName)) {
return newName;
}
- renameGroup(rsrc.toAccountGroup(), newName);
+ renameGroup(internalGroup, newName);
return newName;
}
- private void renameGroup(AccountGroup group, String newName)
+ private void renameGroup(GroupDescription.Internal group, String newName)
throws ResourceConflictException, ResourceNotFoundException, OrmException, IOException {
AccountGroup.UUID groupUuid = group.getGroupUUID();
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOptions.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOptions.java
index 69ce64b..1ea018f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOptions.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOptions.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.group;
+import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.common.GroupOptionsInfo;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -44,10 +45,9 @@
public GroupOptionsInfo apply(GroupResource resource, GroupOptionsInfo input)
throws MethodNotAllowedException, AuthException, BadRequestException,
ResourceNotFoundException, OrmException, IOException {
- AccountGroup accountGroup = resource.toAccountGroup();
- if (accountGroup == null) {
- throw new MethodNotAllowedException();
- } else if (!resource.getControl().isOwner()) {
+ GroupDescription.Internal internalGroup =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
+ if (!resource.getControl().isOwner()) {
throw new AuthException("Not group owner");
}
@@ -58,8 +58,8 @@
input.visibleToAll = false;
}
- if (accountGroup.isVisibleToAll() != input.visibleToAll) {
- AccountGroup.UUID groupUuid = accountGroup.getGroupUUID();
+ if (internalGroup.isVisibleToAll() != input.visibleToAll) {
+ AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
try {
groupsUpdateProvider
.get()
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOwner.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOwner.java
index 6dd0809..20e1dbe 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOwner.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/PutOwner.java
@@ -61,10 +61,9 @@
public GroupInfo apply(GroupResource resource, Input input)
throws ResourceNotFoundException, MethodNotAllowedException, AuthException,
BadRequestException, UnprocessableEntityException, OrmException, IOException {
- AccountGroup accountGroup = resource.toAccountGroup();
- if (accountGroup == null) {
- throw new MethodNotAllowedException();
- } else if (!resource.getControl().isOwner()) {
+ GroupDescription.Internal internalGroup =
+ resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
+ if (!resource.getControl().isOwner()) {
throw new AuthException("Not group owner");
}
@@ -73,8 +72,8 @@
}
GroupDescription.Basic owner = groupsCollection.parse(input.owner);
- if (!accountGroup.getOwnerGroupUUID().equals(owner.getGroupUUID())) {
- AccountGroup.UUID groupUuid = resource.getGroupUUID();
+ if (!internalGroup.getOwnerGroupUUID().equals(owner.getGroupUUID())) {
+ AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
try {
groupsUpdateProvider
.get()
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java
index 278cd86..d1434bc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java
@@ -16,7 +16,6 @@
import com.google.common.base.Strings;
import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupDescriptions;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.index.query.Predicate;
@@ -175,12 +174,12 @@
continue;
}
- AccountGroup ig = GroupDescriptions.toAccountGroup(group);
- if (ig == null) {
+ if (!(group instanceof GroupDescription.Internal)) {
// Non-internal groups cannot be expanded by the server.
continue;
}
+ GroupDescription.Internal ig = (GroupDescription.Internal) group;
try {
args.groups.getMembers(db, ig.getGroupUUID()).forEach(matching.accounts::add);
} catch (NoSuchGroupException e) {
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 f1ebf64..38fd144 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
@@ -331,7 +331,7 @@
}
/** Is this user the owner of the change? */
- private boolean isOwner() {
+ boolean isOwner() {
if (getUser().isIdentifiedUser()) {
Account.Id id = getUser().asIdentifiedUser().getAccountId();
return id.equals(getChange().getOwner());
@@ -358,40 +358,6 @@
return false;
}
- /** @return true if the user is allowed to remove this reviewer. */
- public boolean canRemoveReviewer(PatchSetApproval approval) {
- return canRemoveReviewer(approval.getAccountId(), approval.getValue());
- }
-
- public boolean canRemoveReviewer(Account.Id reviewer, int value) {
- if (getChange().getStatus().isOpen()) {
- // A user can always remove themselves.
- //
- if (getUser().isIdentifiedUser()) {
- if (getUser().getAccountId().equals(reviewer)) {
- return true; // can remove self
- }
- }
-
- // The change owner may remove any zero or positive score.
- //
- if (isOwner() && 0 <= value) {
- return true;
- }
-
- // Users with the remove reviewer permission, the branch owner, project
- // owner and site admin can remove anyone
- if (getRefControl().canRemoveReviewer() // has removal permissions
- || getRefControl().isOwner() // branch owner
- || getProjectControl().isOwner() // project owner
- || getProjectControl().isAdmin()) {
- return true;
- }
- }
-
- return false;
- }
-
/** Can this user edit the topic name? */
private boolean canEditTopicName() {
if (getChange().getStatus().isOpen()) {
@@ -553,7 +519,7 @@
case SUBMIT:
return getRefControl().canSubmit(isOwner());
- case REMOVE_REVIEWER: // TODO Honor specific removal filters?
+ case REMOVE_REVIEWER:
case SUBMIT_AS:
return getRefControl().canPerform(perm.permissionName().get());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RemoveReviewerControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RemoveReviewerControl.java
new file mode 100644
index 0000000..591fcc2
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RemoveReviewerControl.java
@@ -0,0 +1,102 @@
+// 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.server.project;
+
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.reviewdb.client.Account;
+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.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+@Singleton
+public class RemoveReviewerControl {
+ private final PermissionBackend permissionBackend;
+ private final Provider<ReviewDb> dbProvider;
+ private final ChangeControl.GenericFactory changeControlFactory;
+
+ @Inject
+ RemoveReviewerControl(
+ PermissionBackend permissionBackend,
+ Provider<ReviewDb> dbProvider,
+ ChangeControl.GenericFactory changeControlFactory) {
+ this.permissionBackend = permissionBackend;
+ this.dbProvider = dbProvider;
+ this.changeControlFactory = changeControlFactory;
+ }
+
+ /** @throws AuthException if this user is not allowed to remove this approval. */
+ public void checkRemoveReviewer(
+ ChangeNotes notes, CurrentUser currentUser, PatchSetApproval approval)
+ throws PermissionBackendException, AuthException, NoSuchChangeException {
+ if (canRemoveReviewerWithoutPermissionCheck(
+ notes, currentUser, approval.getAccountId(), approval.getValue())) {
+ return;
+ }
+
+ permissionBackend
+ .user(currentUser)
+ .change(notes)
+ .database(dbProvider)
+ .check(ChangePermission.REMOVE_REVIEWER);
+ }
+
+ /** @return true if the user is allowed to remove this reviewer. */
+ public boolean testRemoveReviewer(
+ ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
+ throws PermissionBackendException, NoSuchChangeException {
+ if (canRemoveReviewerWithoutPermissionCheck(notes, currentUser, reviewer, value)) {
+ return true;
+ }
+ return permissionBackend
+ .user(currentUser)
+ .change(notes)
+ .database(dbProvider)
+ .test(ChangePermission.REMOVE_REVIEWER);
+ }
+
+ private boolean canRemoveReviewerWithoutPermissionCheck(
+ ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
+ throws NoSuchChangeException {
+ ChangeControl changeControl = changeControlFactory.controlFor(notes, currentUser);
+ if (!changeControl.getChange().getStatus().isOpen()) {
+ return false;
+ }
+ // A user can always remove themselves.
+ if (changeControl.getUser().isIdentifiedUser()) {
+ if (changeControl.getUser().getAccountId().equals(reviewer)) {
+ return true; // can remove self
+ }
+ }
+ // The change owner may remove any zero or positive score.
+ if (changeControl.isOwner() && 0 <= value) {
+ return true;
+ }
+ // Users with the remove reviewer permission, the branch owner, project
+ // owner and site admin can remove anyone
+ if (changeControl.getRefControl().isOwner() // branch owner
+ || changeControl.getProjectControl().isOwner() // project owner
+ || changeControl.getProjectControl().isAdmin()) { // project admin
+ return true;
+ }
+ return false;
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDb.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDb.java
index ed18a86..6c5dd35 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDb.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDb.java
@@ -40,6 +40,7 @@
b.append(port(dbs.optional("port")));
b.append("/");
b.append(dbs.required("database"));
+ b.append("?useBulkStmts=false");
return b.toString();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java
index d8a5278..4cbaffd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java
@@ -18,6 +18,7 @@
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
+import com.github.rholder.retry.Attempt;
import com.github.rholder.retry.RetryException;
import com.github.rholder.retry.RetryListener;
import com.github.rholder.retry.RetryerBuilder;
@@ -28,6 +29,10 @@
import com.google.common.base.Throwables;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.metrics.Counter0;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Histogram0;
+import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.LockFailureException;
import com.google.gerrit.server.notedb.NotesMigration;
@@ -75,6 +80,30 @@
}
}
+ @Singleton
+ private static class Metrics {
+ final Histogram0 attemptCounts;
+ final Counter0 timeoutCount;
+
+ @Inject
+ Metrics(MetricMaker metricMaker) {
+ attemptCounts =
+ metricMaker.newHistogram(
+ "batch_update/retry_attempt_counts",
+ new Description(
+ "Distribution of number of attempts made by RetryHelper"
+ + " (1 == single attempt, no retry)")
+ .setCumulative()
+ .setUnit("attempts"));
+ timeoutCount =
+ metricMaker.newCounter(
+ "batch_update/retry_timeout_count",
+ new Description("Number of executions of RetryHelper that ultimately timed out")
+ .setCumulative()
+ .setUnit("timeouts"));
+ }
+ }
+
public static Options.Builder options() {
return new AutoValue_RetryHelper_Options.Builder();
}
@@ -84,6 +113,7 @@
}
private final NotesMigration migration;
+ private final Metrics metrics;
private final BatchUpdate.Factory updateFactory;
private final Duration defaultTimeout;
private final WaitStrategy waitStrategy;
@@ -91,9 +121,11 @@
@Inject
RetryHelper(
@GerritServerConfig Config cfg,
+ Metrics metrics,
NotesMigration migration,
ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory,
NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) {
+ this.metrics = metrics;
this.migration = migration;
this.updateFactory =
new BatchUpdate.Factory(migration, reviewDbBatchUpdateFactory, noteDbBatchUpdateFactory);
@@ -117,18 +149,18 @@
}
public <T> T execute(Action<T> action, Options opts) throws RestApiException, UpdateException {
+ MetricListener listener = null;
try {
RetryerBuilder<T> builder = RetryerBuilder.newBuilder();
if (migration.disableChangeReviewDb()) {
+ listener = new MetricListener(opts.listener());
builder
+ .withRetryListener(listener)
.withStopStrategy(
StopStrategies.stopAfterDelay(
firstNonNull(opts.timeout(), defaultTimeout).toMillis(), MILLISECONDS))
.withWaitStrategy(waitStrategy)
.retryIfException(RetryHelper::isLockFailure);
- if (opts.listener() != null) {
- builder.withRetryListener(opts.listener());
- }
} else {
// Either we aren't full-NoteDb, or the underlying ref storage doesn't support atomic
// transactions. Either way, retrying a partially-failed operation is not idempotent, so
@@ -136,11 +168,18 @@
}
return builder.build().call(() -> action.call(updateFactory));
} catch (ExecutionException | RetryException e) {
+ if (e instanceof RetryException) {
+ metrics.timeoutCount.increment();
+ }
if (e.getCause() != null) {
Throwables.throwIfInstanceOf(e.getCause(), UpdateException.class);
Throwables.throwIfInstanceOf(e.getCause(), RestApiException.class);
}
throw new UpdateException(e);
+ } finally {
+ if (listener != null) {
+ metrics.attemptCounts.record(listener.getAttemptCount());
+ }
}
}
@@ -150,4 +189,26 @@
}
return t instanceof LockFailureException;
}
+
+ private static class MetricListener implements RetryListener {
+ private final RetryListener delegate;
+ private long attemptCount;
+
+ MetricListener(@Nullable RetryListener delegate) {
+ this.delegate = delegate;
+ attemptCount = 1;
+ }
+
+ @Override
+ public <V> void onRetry(Attempt<V> attempt) {
+ attemptCount = attempt.getAttemptNumber();
+ if (delegate != null) {
+ delegate.onRetry(attempt);
+ }
+ }
+
+ long getAttemptCount() {
+ return attemptCount;
+ }
+ }
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/util/LabelVoteTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/util/LabelVoteTest.java
index d3cb927..9069928 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/util/LabelVoteTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/util/LabelVoteTest.java
@@ -14,79 +14,48 @@
package com.google.gerrit.server.util;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
+import static com.google.gerrit.server.util.LabelVote.parse;
+import static com.google.gerrit.server.util.LabelVote.parseWithEquals;
import org.junit.Test;
public class LabelVoteTest {
@Test
- public void parse() {
- LabelVote l;
- l = LabelVote.parse("Code-Review-2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) -2, l.value());
- l = LabelVote.parse("Code-Review-1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) -1, l.value());
- l = LabelVote.parse("-Code-Review");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 0, l.value());
- l = LabelVote.parse("Code-Review");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 1, l.value());
- l = LabelVote.parse("Code-Review+1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 1, l.value());
- l = LabelVote.parse("Code-Review+2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 2, l.value());
+ public void labelVoteParse() {
+ assertLabelVoteEquals(parse("Code-Review-2"), "Code-Review", -2);
+ assertLabelVoteEquals(parse("Code-Review-1"), "Code-Review", -1);
+ assertLabelVoteEquals(parse("-Code-Review"), "Code-Review", 0);
+ assertLabelVoteEquals(parse("Code-Review"), "Code-Review", 1);
+ assertLabelVoteEquals(parse("Code-Review+1"), "Code-Review", 1);
+ assertLabelVoteEquals(parse("Code-Review+2"), "Code-Review", 2);
}
@Test
- public void format() {
- assertEquals("Code-Review-2", LabelVote.parse("Code-Review-2").format());
- assertEquals("Code-Review-1", LabelVote.parse("Code-Review-1").format());
- assertEquals("-Code-Review", LabelVote.parse("-Code-Review").format());
- assertEquals("Code-Review+1", LabelVote.parse("Code-Review+1").format());
- assertEquals("Code-Review+2", LabelVote.parse("Code-Review+2").format());
+ public void labelVoteFormat() {
+ assertThat(parse("Code-Review-2").format()).isEqualTo("Code-Review-2");
+ assertThat(parse("Code-Review-1").format()).isEqualTo("Code-Review-1");
+ assertThat(parse("-Code-Review").format()).isEqualTo("-Code-Review");
+ assertThat(parse("Code-Review+1").format()).isEqualTo("Code-Review+1");
+ assertThat(parse("Code-Review+2").format()).isEqualTo("Code-Review+2");
}
@Test
- public void parseWithEquals() {
- LabelVote l;
- l = LabelVote.parseWithEquals("Code-Review=-2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) -2, l.value());
- l = LabelVote.parseWithEquals("Code-Review=-1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) -1, l.value());
- l = LabelVote.parseWithEquals("Code-Review=0");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 0, l.value());
- l = LabelVote.parseWithEquals("Code-Review=1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 1, l.value());
- l = LabelVote.parseWithEquals("Code-Review=+1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 1, l.value());
- l = LabelVote.parseWithEquals("Code-Review=2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 2, l.value());
- l = LabelVote.parseWithEquals("Code-Review=+2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 2, l.value());
- l = LabelVote.parseWithEquals("R=0");
- assertEquals("R", l.label());
- assertEquals((short) 0, l.value());
+ public void labelVoteParseWithEquals() {
+ assertLabelVoteEquals(parseWithEquals("Code-Review=-2"), "Code-Review", -2);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=-1"), "Code-Review", -1);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=0"), "Code-Review", 0);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=1"), "Code-Review", 1);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=+1"), "Code-Review", 1);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=2"), "Code-Review", 2);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=+2"), "Code-Review", 2);
+ assertLabelVoteEquals(parseWithEquals("R=0"), "R", 0);
String longName = "A-loooooooooooooooooooooooooooooooooooooooooooooooooong-label";
// Regression test: an old bug passed the string length as a radix to Short#parseShort.
- assertTrue(longName.length() > Character.MAX_RADIX);
- l = LabelVote.parseWithEquals(longName + "=+1");
- assertEquals(longName, l.label());
- assertEquals((short) 1, l.value());
+ assertThat(longName.length()).isGreaterThan(Character.MAX_RADIX);
+ assertLabelVoteEquals(parseWithEquals(longName + "=+1"), longName, 1);
assertParseWithEqualsFails(null);
assertParseWithEqualsFails("");
@@ -99,18 +68,23 @@
}
@Test
- public void formatWithEquals() {
- assertEquals("Code-Review=-2", LabelVote.parseWithEquals("Code-Review=-2").formatWithEquals());
- assertEquals("Code-Review=-1", LabelVote.parseWithEquals("Code-Review=-1").formatWithEquals());
- assertEquals("Code-Review=0", LabelVote.parseWithEquals("Code-Review=0").formatWithEquals());
- assertEquals("Code-Review=+1", LabelVote.parseWithEquals("Code-Review=+1").formatWithEquals());
- assertEquals("Code-Review=+2", LabelVote.parseWithEquals("Code-Review=+2").formatWithEquals());
+ public void labelVoteFormatWithEquals() {
+ assertThat(parseWithEquals("Code-Review=-2").formatWithEquals()).isEqualTo("Code-Review=-2");
+ assertThat(parseWithEquals("Code-Review=-1").formatWithEquals()).isEqualTo("Code-Review=-1");
+ assertThat(parseWithEquals("Code-Review=0").formatWithEquals()).isEqualTo("Code-Review=0");
+ assertThat(parseWithEquals("Code-Review=+1").formatWithEquals()).isEqualTo("Code-Review=+1");
+ assertThat(parseWithEquals("Code-Review=+2").formatWithEquals()).isEqualTo("Code-Review=+2");
+ }
+
+ private void assertLabelVoteEquals(LabelVote actual, String expectedLabel, int expectedValue) {
+ assertThat(actual.label()).isEqualTo(expectedLabel);
+ assertThat((int) actual.value()).isEqualTo(expectedValue);
}
private void assertParseWithEqualsFails(String value) {
try {
- LabelVote.parseWithEquals(value);
- fail("expected IllegalArgumentException when parsing \"" + value + "\"");
+ parseWithEquals(value);
+ assert_().fail("expected IllegalArgumentException when parsing \"%s\"", value);
} catch (IllegalArgumentException e) {
// Expected.
}
diff --git a/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.html b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.html
new file mode 100644
index 0000000..c2b28d5
--- /dev/null
+++ b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.html
@@ -0,0 +1,140 @@
+<!--
+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.
+-->
+
+<link rel="import" href="../../../bower_components/polymer/polymer.html">
+
+<link rel="import" href="../../../behaviors/gr-access-behavior/gr-access-behavior.html">
+<link rel="import" href="../../../bower_components/iron-input/iron-input.html">
+<link rel="import" href="../../../styles/gr-form-styles.html">
+<link rel="import" href="../../../styles/shared-styles.html">
+<link rel="import" href="../../shared/gr-button/gr-button.html">
+<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
+<link rel="import" href="../gr-permission/gr-permission.html">
+
+<script src="../../../scripts/util.js"></script>
+
+<dom-module id="gr-access-section">
+ <template>
+ <style include="shared-styles">
+ :host {
+ display: block;
+ margin-bottom: 1em;
+ }
+ fieldset {
+ border: 1px solid #d1d2d3;
+ }
+ .header,
+ .editingRef .editContainer,
+ #deletedContainer {
+ align-items: baseline;
+ background: #f6f6f6;
+ border-bottom: 1px dotted #d1d2d3;
+ display: flex;
+ justify-content: space-between;
+ padding: .7em .7em;
+ }
+ #deletedContainer {
+ border-bottom: 0;
+ }
+ .sectionContent {
+ padding: .7em;
+ }
+ #deletedContainer,
+ .deleted #mainContainer,
+ .global,
+ #addPermission,
+ #updateBtns,
+ .editingRef .header,
+ .editContainer {
+ display: none;
+ }
+ .deleted #deletedContainer,
+ #mainContainer,
+ .editing #addPermission,
+ .editing #updateBtns {
+ display: block;
+ }
+ .editingRef .editContainer {
+ display: flex;
+ }
+ </style>
+ <style include="gr-form-styles"></style>
+ <fieldset id="section"
+ class$="gr-form-styles [[_computeSectionClass(editing, _editingRef, _deleted)]]">
+ <div id="mainContainer">
+ <div class="header">
+ <span class="name">
+ <h3>[[_computeSectionName(section.id)]]</h3>
+ </span>
+ <div id="updateBtns">
+ <gr-button
+ id="editBtn"
+ class$="[[_computeEditBtnClass(section.id)]]"
+ on-tap="_handleEditReference">Edit Reference</gr-button>
+ <gr-button
+ id="deleteBtn"
+ on-tap="_handleRemoveReference">Remove</gr-button>
+ </div><!-- end updateBtns -->
+ </div><!-- end header -->
+ <div class="editContainer">
+ <input
+ id="editRefInput"
+ bind-value="{{section.id}}"
+ is="iron-input"
+ type="text">
+ <gr-button
+ id="undoEdit"
+ on-tap="_undoReferenceEdit">Undo</gr-button>
+ </div><!-- end editContainer -->
+ <div class="sectionContent">
+ <template
+ is="dom-repeat"
+ items="{{_permissions}}"
+ as="permission">
+ <gr-permission
+ name="[[_computePermissionName(section.id, permission, permissionValues, capabilities)]]"
+ permission="{{permission}}"
+ labels="[[labels]]"
+ section="[[section.id]]"
+ editing="[[editing]]">
+ </gr-permission>
+ </template>
+ <div id="addPermission">
+ Add permission:
+ <select id="permissionSelect">
+ <!-- called with a third parameter so that permissions update
+ after a new section is added. -->
+ <template
+ is="dom-repeat"
+ items="[[_computePermissions(section.id, capabilities, labels, section.value.permissions.*)]]">
+ <option value="[[item.value.id]]">[[item.value.name]]</option>
+ </template>
+ </select>
+ <gr-button id="addBtn" on-tap="_handleAddPermission">Add</gr-button>
+ </div><!-- end addPermission -->
+ </div><!-- end sectionContent -->
+ </div><!-- end mainContainer -->
+ <div id="deletedContainer">
+ [[_computeSectionName(section.id)]] was deleted
+ <gr-button
+ id="undoRemoveBtn"
+ on-tap="_handleUndoRemove">Undo</gr-button>
+ </div><!-- end deletedContainer -->
+ </fieldset>
+ <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
+ </template>
+ <script src="gr-access-section.js"></script>
+</dom-module>
diff --git a/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.js b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.js
new file mode 100644
index 0000000..16e6207
--- /dev/null
+++ b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.js
@@ -0,0 +1,199 @@
+// 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.
+(function() {
+ 'use strict';
+
+ const GLOBAL_NAME = 'GLOBAL_CAPABILITIES';
+
+ // The name that gets automatically input when a new reference is added.
+ const NEW_NAME = 'refs/heads/*';
+ const REFS_NAME = 'refs/';
+ const ON_BEHALF_OF = '(On Behalf Of)';
+ const LABEL = 'Label';
+
+ Polymer({
+ is: 'gr-access-section',
+
+ properties: {
+ capabilities: Object,
+ /** @type {?} */
+ section: {
+ type: Object,
+ notify: true,
+ observer: '_sectionChanged',
+ },
+ labels: Object,
+ editing: {
+ type: Boolean,
+ value: false,
+ },
+ _originalId: String,
+ _editingRef: {
+ type: Boolean,
+ value: false,
+ },
+ _deleted: {
+ type: Boolean,
+ value: false,
+ },
+ _permissions: Array,
+ },
+
+ behaviors: [
+ Gerrit.AccessBehavior,
+ ],
+
+ _sectionChanged(section) {
+ this._permissions = this.toSortedArray(section.value.permissions);
+ this._originalId = section.id;
+ },
+
+ _computePermissions(name, capabilities, labels) {
+ let allPermissions;
+ if (name === GLOBAL_NAME) {
+ allPermissions = this.toSortedArray(capabilities);
+ } else {
+ const labelOptions = this._computeLabelOptions(labels);
+ allPermissions = labelOptions.concat(
+ this.toSortedArray(this.permissionValues));
+ }
+ return allPermissions.filter(permission => {
+ return !this.section.value.permissions[permission.id];
+ });
+ },
+
+ _computeLabelOptions(labels) {
+ const labelOptions = [];
+ for (const labelName of Object.keys(labels)) {
+ labelOptions.push({
+ id: 'label-' + labelName,
+ value: {
+ name: `${LABEL} ${labelName}`,
+ id: 'label-' + labelName,
+ },
+ });
+ labelOptions.push({
+ id: 'labelAs-' + labelName,
+ value: {
+ name: `${LABEL} ${labelName} ${ON_BEHALF_OF}`,
+ id: 'labelAs-' + labelName,
+ },
+ });
+ }
+ return labelOptions;
+ },
+
+ _computePermissionName(name, permission, permissionValues, capabilities) {
+ if (name === GLOBAL_NAME) {
+ return capabilities[permission.id].name;
+ } else if (permissionValues[permission.id]) {
+ return permissionValues[permission.id].name;
+ } else if (permission.value.label) {
+ let behalfOf = '';
+ if (permission.id.startsWith('labelAs-')) {
+ behalfOf = ON_BEHALF_OF;
+ }
+ return `${LABEL} ${permission.value.label}${behalfOf}`;
+ }
+ },
+
+ _computeSectionName(name) {
+ // When a new section is created, it doesn't yet have a ref. Set into
+ // edit mode so that the user can input one.
+ if (!name) {
+ this._editingRef = true;
+ // Needed for the title value. This is the same default as GWT.
+ name = NEW_NAME;
+ // Needed for the input field value.
+ this.set('section.id', name);
+ }
+ if (name === GLOBAL_NAME) {
+ return 'Global Capabilities';
+ } else if (name.startsWith(REFS_NAME)) {
+ return `Reference: ${name}`;
+ }
+ return name;
+ },
+
+ _handleRemoveReference() {
+ this._deleted = true;
+ this.set('section.value.deleted', true);
+ },
+
+ _handleUndoRemove() {
+ this._deleted = false;
+ delete this.section.value.deleted;
+ },
+
+ _handleEditReference() {
+ this._editingRef = true;
+ },
+
+ _undoReferenceEdit() {
+ this._editingRef = false;
+ this.set('section.id', this._originalId);
+ },
+
+ _computeSectionClass(editing, editingRef, deleted) {
+ const classList = [];
+ if (editing) {
+ classList.push('editing');
+ }
+ if (editingRef) {
+ classList.push('editingRef');
+ }
+ if (deleted) {
+ classList.push('deleted');
+ }
+ return classList.join(' ');
+ },
+
+ _computeEditBtnClass(name) {
+ return name === GLOBAL_NAME ? 'global' : '';
+ },
+
+ _handleAddPermission() {
+ const value = this.$.permissionSelect.value;
+ const permission = {
+ id: value,
+ value: {rules: {}},
+ };
+
+ // This is needed to update the 'label' property of the
+ // 'label-<label-name>' permission.
+ //
+ // The value from the add permission dropdown will either be
+ // label-<label-name> or labelAs-<labelName>.
+ // But, the format of the API response is as such:
+ // "permissions": {
+ // "label-Code-Review": {
+ // "label": "Code-Review",
+ // "rules": {...}
+ // }
+ // }
+ // }
+ // When we add a new item, we have to push the new permission in the same
+ // format as the ones that have been returned by the API.
+ if (value.startsWith('label')) {
+ permission.value.label =
+ value.replace('label-', '').replace('labelAs-', '');
+ }
+ // Add to the end of the array (used in dom-repeat) and also to the
+ // section object that is two way bound with its parent element.
+ this.push('_permissions', permission);
+ this.set(['section.value.permissions', permission.id],
+ permission.value);
+ },
+ });
+})();
\ No newline at end of file
diff --git a/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section_test.html b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section_test.html
new file mode 100644
index 0000000..38c5d67
--- /dev/null
+++ b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section_test.html
@@ -0,0 +1,466 @@
+<!DOCTYPE html>
+<!--
+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.
+-->
+
+<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
+<title>gr-access-section</title>
+
+<script src="../../../bower_components/page/page.js"></script>
+<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
+<script src="../../../bower_components/web-component-tester/browser.js"></script>
+<link rel="import" href="../../../test/common-test-setup.html"/>
+<link rel="import" href="gr-access-section.html">
+
+<script>void(0);</script>
+
+<test-fixture id="basic">
+ <template>
+ <gr-access-section></gr-access-section>
+ </template>
+</test-fixture>
+
+<script>
+ suite('gr-access-section tests', () => {
+ let element;
+ let sandbox;
+
+ setup(() => {
+ sandbox = sinon.sandbox.create();
+ element = fixture('basic');
+ });
+
+ teardown(() => {
+ sandbox.restore();
+ });
+
+ suite('unit tests', () => {
+ setup(() => {
+ element.section = {
+ id: 'refs/*',
+ value: {
+ permissions: {
+ read: {
+ rules: {},
+ },
+ },
+ },
+ };
+ element.capabilities = {
+ accessDatabase: {
+ id: 'accessDatabase',
+ name: 'Access Database',
+ },
+ administrateServer: {
+ id: 'administrateServer',
+ name: 'Administrate Server',
+ },
+ batchChangesLimit: {
+ id: 'batchChangesLimit',
+ name: 'Batch Changes Limit',
+ },
+ createAccount: {
+ id: 'createAccount',
+ name: 'Create Account',
+ },
+ };
+ element.labels = {
+ 'Code-Review': {
+ values: {
+ ' 0': 'No score',
+ '-1': 'I would prefer this is not merged as is',
+ '-2': 'This shall not be merged',
+ '+1': 'Looks good to me, but someone else must approve',
+ '+2': 'Looks good to me, approved',
+ },
+ default_value: 0,
+ },
+ };
+ element._sectionChanged(element.section);
+ flushAsynchronousOperations();
+ });
+
+ test('_sectionChanged', () => {
+ // _sectionChanged was called in setup, so just make assertions.
+ const expectedPermissions = [
+ {
+ id: 'read',
+ value: {
+ rules: {},
+ },
+ },
+ ];
+ assert.deepEqual(element._permissions, expectedPermissions);
+ assert.equal(element._originalId, element.section.id);
+ });
+
+ test('_computeLabelOptions', () => {
+ const expectedLabelOptions = [
+ {
+ id: 'label-Code-Review',
+ value: {
+ name: 'Label Code-Review',
+ id: 'label-Code-Review',
+ },
+ },
+ {
+ id: 'labelAs-Code-Review',
+ value: {
+ name: 'Label Code-Review (On Behalf Of)',
+ id: 'labelAs-Code-Review',
+ },
+ },
+ ];
+
+ assert.deepEqual(element._computeLabelOptions(element.labels),
+ expectedLabelOptions);
+ });
+
+ test('_computePermissions', () => {
+ sandbox.stub(element, 'toSortedArray').returns(
+ [{
+ id: 'push',
+ value: {
+ rules: {},
+ },
+ },
+ {
+ id: 'read',
+ value: {
+ rules: {},
+ },
+ },
+ ]);
+
+ const expectedPermissions = [{
+ id: 'push',
+ value: {
+ rules: {},
+ },
+ },
+ ];
+ const labelOptions = [
+ {
+ id: 'label-Code-Review',
+ value: {
+ name: 'Label Code-Review',
+ id: 'label-Code-Review',
+ },
+ },
+ {
+ id: 'labelAs-Code-Review',
+ value: {
+ name: 'Label Code-Review (On Behalf Of)',
+ id: 'labelAs-Code-Review',
+ },
+ },
+ ];
+
+ // For global capabilities, just return the sorted array filtered by
+ // existing permissions.
+ let name = 'GLOBAL_CAPABILITIES';
+ assert.deepEqual(element._computePermissions(name, element.capabilities,
+ element.labels), expectedPermissions);
+
+ // Uses the capabilities array to come up with possible values.
+ assert.isTrue(element.toSortedArray.lastCall.
+ calledWithExactly(element.capabilities));
+
+
+ // For everything else, include possible label values before filtering.
+ name = 'refs/for/*';
+ assert.deepEqual(element._computePermissions(name, element.capabilities,
+ element.labels), labelOptions.concat(expectedPermissions));
+
+ // Uses permissionValues (defined in gr-access-behavior) to come up with
+ // possible values.
+ assert.isTrue(element.toSortedArray.lastCall.
+ calledWithExactly(element.permissionValues));
+ });
+
+ test('_computePermissionName', () => {
+ let name = 'GLOBAL_CAPABILITIES';
+ let permission = {
+ id: 'administrateServer',
+ value: {},
+ };
+ assert.equal(element._computePermissionName(name, permission,
+ element.permissionValues, element.capabilities),
+ element.capabilities[permission.id].name);
+
+ name = 'refs/for/*';
+ permission = {
+ id: 'abandon',
+ value: {},
+ };
+
+ assert.equal(element._computePermissionName(
+ name, permission, element.permissionValues, element.capabilities),
+ element.permissionValues[permission.id].name);
+
+ name = 'refs/for/*';
+ permission = {
+ id: 'label-Code-Review',
+ value: {
+ label: 'Code-Review',
+ },
+ };
+
+ assert.equal(element._computePermissionName(name, permission,
+ element.permissionValues, element.capabilities),
+ 'Label Code-Review');
+
+ permission = {
+ id: 'labelAs-Code-Review',
+ value: {
+ label: 'Code-Review',
+ },
+ };
+
+ assert.equal(element._computePermissionName(name, permission,
+ element.permissionValues, element.capabilities),
+ 'Label Code-Review(On Behalf Of)');
+ });
+
+ test('_computeSectionName', () => {
+ let name;
+ // When computing the section name for an undefined name, it means a
+ // new section is being added. In this case, it should defualt to
+ // 'refs/heads/*'.
+ element._editingRef = false;
+ assert.equal(element._computeSectionName(name),
+ 'Reference: refs/heads/*');
+ assert.isTrue(element._editingRef);
+ assert.equal(element.section.id, 'refs/heads/*');
+
+ // Reset editing to false.
+ element._editingRef = false;
+ name = 'GLOBAL_CAPABILITIES';
+ assert.equal(element._computeSectionName(name), 'Global Capabilities');
+ assert.isFalse(element._editingRef);
+
+ name = 'refs/for/*';
+ assert.equal(element._computeSectionName(name),
+ 'Reference: refs/for/*');
+ assert.isFalse(element._editingRef);
+ });
+
+ test('_handleEditReference', () => {
+ element._handleEditReference();
+ assert.isTrue(element._editingRef);
+ });
+
+ test('_undoReferenceEdit', () => {
+ element._originalId = 'refs/for/old';
+ element.section.id = 'refs/for/new';
+ element.editing = true;
+ element._undoReferenceEdit();
+ assert.isFalse(element._editingRef);
+ assert.equal(element.section.id, 'refs/for/old');
+ });
+
+ test('_computeSectionClass', () => {
+ let editingRef = false;
+ let editing = false;
+ let deleted = false;
+ assert.equal(element._computeSectionClass(editing, editingRef, deleted),
+ '');
+
+ editing = true;
+ assert.equal(element._computeSectionClass(editing, editingRef, deleted),
+ 'editing');
+
+ editingRef = true;
+ assert.equal(element._computeSectionClass(editing, editingRef, deleted),
+ 'editing editingRef');
+
+ deleted = true;
+ assert.equal(element._computeSectionClass(editing, editingRef, deleted),
+ 'editing editingRef deleted');
+
+ editingRef = false;
+ assert.equal(element._computeSectionClass(editing, editingRef, deleted),
+ 'editing deleted');
+ });
+
+ test('_computeEditBtnClass', () => {
+ let name = 'GLOBAL_CAPABILITIES';
+ assert.equal(element._computeEditBtnClass(name), 'global');
+ name = 'refs/for/*';
+ assert.equal(element._computeEditBtnClass(name), '');
+ });
+ });
+
+ suite('interactive tests', () => {
+ setup(() => {
+ element.labels = {
+ 'Code-Review': {
+ values: {
+ ' 0': 'No score',
+ '-1': 'I would prefer this is not merged as is',
+ '-2': 'This shall not be merged',
+ '+1': 'Looks good to me, but someone else must approve',
+ '+2': 'Looks good to me, approved',
+ },
+ default_value: 0,
+ },
+ };
+ });
+ suite('Global section', () => {
+ setup(() => {
+ element.section = {
+ id: 'GLOBAL_CAPABILITIES',
+ value: {
+ permissions: {
+ accessDatabase: {
+ rules: {},
+ },
+ },
+ },
+ };
+ element.capabilities = {
+ accessDatabase: {
+ id: 'accessDatabase',
+ name: 'Access Database',
+ },
+ administrateServer: {
+ id: 'administrateServer',
+ name: 'Administrate Server',
+ },
+ batchChangesLimit: {
+ id: 'batchChangesLimit',
+ name: 'Batch Changes Limit',
+ },
+ createAccount: {
+ id: 'createAccount',
+ name: 'Create Account',
+ },
+ };
+ element._sectionChanged(element.section);
+ flushAsynchronousOperations();
+ });
+
+ test('classes are assigned correctly', () => {
+ assert.isFalse(element.$.section.classList.contains('editing'));
+ assert.isFalse(element.$.section.classList.contains('deleted'));
+ assert.isTrue(element.$.editBtn.classList.contains('global'));
+ });
+ });
+
+ suite('Non-global section', () => {
+ setup(() => {
+ element.section = {
+ id: 'refs/*',
+ value: {
+ permissions: {
+ read: {
+ rules: {},
+ },
+ },
+ },
+ };
+ element.capabilities = {};
+ element._sectionChanged(element.section);
+ flushAsynchronousOperations();
+ });
+
+ test('classes are assigned correctly', () => {
+ assert.isFalse(element.$.section.classList.contains('editing'));
+ assert.isFalse(element.$.section.classList.contains('deleted'));
+ assert.isFalse(element.$.editBtn.classList.contains('global'));
+ });
+
+ test('add permission', () => {
+ element.$.permissionSelect.value = 'label-Code-Review';
+ assert.equal(element._permissions.length, 1);
+ assert.equal(Object.keys(element.section.value.permissions).length,
+ 1);
+ MockInteractions.tap(element.$.addBtn);
+ flushAsynchronousOperations();
+
+ // The permission is added to both the permissions array and also
+ // the section's permission object.
+ assert.equal(element._permissions.length, 2);
+ let permission = {
+ id: 'label-Code-Review',
+ value: {
+ label: 'Code-Review',
+ rules: {},
+ },
+ };
+ assert.equal(element._permissions.length, 2);
+ assert.deepEqual(element._permissions[1], permission);
+ assert.equal(Object.keys(element.section.value.permissions).length,
+ 2);
+ assert.deepEqual(
+ element.section.value.permissions['label-Code-Review'],
+ permission.value);
+
+
+ element.$.permissionSelect.value = 'abandon';
+ MockInteractions.tap(element.$.addBtn);
+ flushAsynchronousOperations();
+
+ permission = {
+ id: 'abandon',
+ value: {
+ rules: {},
+ },
+ };
+
+ assert.equal(element._permissions.length, 3);
+ assert.deepEqual(element._permissions[2], permission);
+ assert.equal(Object.keys(element.section.value.permissions).length,
+ 3);
+ assert.deepEqual(element.section.value.permissions['abandon'],
+ permission.value);
+ });
+
+ test('edit section reference', () => {
+ assert.isFalse(element.$.section.classList.contains('editing'));
+ element.editing = true;
+ assert.isTrue(element.$.section.classList.contains('editing'));
+ assert.isFalse(element._editingRef);
+ MockInteractions.tap(element.$.editBtn);
+ element.$.editRefInput.bindValue='new/ref';
+ flushAsynchronousOperations();
+ assert.equal(element.section.id, 'new/ref');
+ assert.isTrue(element._editingRef);
+ assert.isTrue(element.$.section.classList.contains('editingRef'));
+ MockInteractions.tap(element.$.undoEdit);
+ flushAsynchronousOperations();
+ assert.isFalse(element._editingRef);
+ assert.isFalse(element.$.section.classList.contains('editingRef'));
+ assert.equal(element.section.id, 'refs/*');
+ });
+
+ test('remove section', () => {
+ element.editing = true;
+ assert.isFalse(element._deleted);
+ MockInteractions.tap(element.$.deleteBtn);
+ flushAsynchronousOperations();
+ assert.isTrue(element._deleted);
+ assert.isTrue(element.$.section.classList.contains('deleted'));
+ assert.isTrue(element.section.value.deleted);
+
+ MockInteractions.tap(element.$.undoRemoveBtn);
+ flushAsynchronousOperations();
+ assert.isFalse(element._deleted);
+ assert.isNotOk(element.section.value.deleted);
+ });
+ });
+ });
+ });
+</script>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.html b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.html
index d841fbc..35537e7 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.html
@@ -74,18 +74,17 @@
</tr>
<template is="dom-repeat" items="[[sections]]" as="changeSection"
index-as="sectionIndex">
- <template is="dom-if" if="[[_sectionTitle(sectionIndex)]]">
+ <template is="dom-if" if="[[changeSection.sectionName]]">
<tr class="groupHeader">
<td class="cell"
colspan$="[[_computeColspan(changeTableColumns, labelNames)]]">
- <a
- href$="[[_sectionHref(sectionIndex)]]">
- [[_sectionTitle(sectionIndex)]]
+ <a href$="[[_sectionHref(changeSection.query)]]">
+ [[changeSection.sectionName]]
</a>
</td>
</tr>
</template>
- <template is="dom-if" if="[[!changeSection.length]]">
+ <template is="dom-if" if="[[!changeSection.results.length]]">
<tr class="noChanges">
<td class="cell"
colspan$="[[_computeColspan(changeTableColumns, labelNames)]]">
@@ -93,7 +92,7 @@
</td>
</tr>
</template>
- <template is="dom-repeat" items="[[changeSection]]" as="change">
+ <template is="dom-repeat" items="[[changeSection.results]]" as="change">
<gr-change-list-item
selected$="[[_computeItemSelected(index, sectionIndex, selectedIndex)]]"
assigned$="[[_computeItemAssigned(account, change)]]"
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js
index d302120..63b42bb 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js
@@ -55,15 +55,17 @@
/**
* ChangeInfo objects grouped into arrays. The sections and changes
* properties should not be used together.
+ *
+ * @type {!Array<{
+ * sectionName: string,
+ * query: string,
+ * results: !Array<!Object>
+ * }>}
*/
sections: {
type: Array,
value() { return []; },
},
- sectionMetadata: {
- type: Array,
- value() { return []; },
- },
labelNames: {
type: Array,
computed: '_computeLabelNames(sections)',
@@ -152,10 +154,9 @@
const nonExistingLabel = function(item) {
return !labels.includes(item);
};
- for (let i = 0; i < sections.length; i++) {
- const section = sections[i];
- for (let j = 0; j < section.length; j++) {
- const change = section[j];
+ for (const section of sections) {
+ if (!section.results) { continue; }
+ for (const change of section.results) {
if (!change.labels) { continue; }
const currentLabels = Object.keys(change.labels);
labels = labels.concat(currentLabels.filter(nonExistingLabel));
@@ -171,17 +172,10 @@
},
_changesChanged(changes) {
- this.sections = changes ? [changes] : [];
+ this.sections = changes ? [{results: changes}] : [];
},
- _sectionTitle(sectionIndex) {
- if (sectionIndex > this.sectionMetadata.length - 1) { return null; }
- return this.sectionMetadata[sectionIndex].name;
- },
-
- _sectionHref(sectionIndex) {
- if (sectionIndex > this.sectionMetadata.length - 1) { return null; }
- const query = this.sectionMetadata[sectionIndex].query;
+ _sectionHref(query) {
return `${this.getBaseUrl()}/q/${this.encodeURL(query, true)}`;
},
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html
index d758475..582a559 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html
@@ -107,14 +107,26 @@
test('computed fields', () => {
assert.equal(element._computeLabelNames(
- [[{_number: 0, labels: {}}]]).length, 0);
- assert.equal(element._computeLabelNames([[
- {_number: 0, labels: {Verified: {approved: {}}}},
- {_number: 1, labels: {
- 'Verified': {approved: {}}, 'Code-Review': {approved: {}}}},
- {_number: 2, labels: {
- 'Verified': {approved: {}}, 'Library-Compliance': {approved: {}}}},
- ]]).length, 3);
+ [{results: [{_number: 0, labels: {}}]}]).length, 0);
+ assert.equal(element._computeLabelNames([
+ {results: [
+ {_number: 0, labels: {Verified: {approved: {}}}},
+ {
+ _number: 1,
+ labels: {
+ 'Verified': {approved: {}},
+ 'Code-Review': {approved: {}},
+ },
+ },
+ {
+ _number: 2,
+ labels: {
+ 'Verified': {approved: {}},
+ 'Library-Compliance': {approved: {}},
+ },
+ },
+ ]},
+ ]).length, 3);
assert.equal(element._computeLabelShortcut('Code-Review'), 'CR');
assert.equal(element._computeLabelShortcut('Verified'), 'V');
@@ -244,7 +256,7 @@
});
test('empty sections', () => {
- element.sections = [[], []];
+ element.sections = [{results: []}, {results: []}];
flushAsynchronousOperations();
const listItems = Polymer.dom(element.root).querySelectorAll(
'gr-change-list-item');
@@ -382,31 +394,26 @@
test('keyboard shortcuts', () => {
element.selectedIndex = 0;
element.sections = [
- [
- {_number: 0},
- {_number: 1},
- {_number: 2},
- ],
- [
- {_number: 3},
- {_number: 4},
- {_number: 5},
- ],
- [
- {_number: 6},
- {_number: 7},
- {_number: 8},
- ],
- ];
- element.sectionMetadata = [
{
- name: 'Group 1',
+ results: [
+ {_number: 0},
+ {_number: 1},
+ {_number: 2},
+ ],
},
{
- name: 'Group 2',
+ results: [
+ {_number: 3},
+ {_number: 4},
+ {_number: 5},
+ ],
},
{
- name: 'Group 3',
+ results: [
+ {_number: 6},
+ {_number: 7},
+ {_number: 8},
+ ],
},
];
flushAsynchronousOperations();
@@ -469,14 +476,12 @@
});
test('_sectionHref', () => {
- element.sectionMetadata = [
- {query: 'is:open owner:self'},
- {query: 'is:open ((reviewer:self -is:ignored) OR assignee:self)'},
- ];
-
- assert.equal(element._sectionHref(10), null);
- assert.equal(element._sectionHref(0), '/q/is:open+owner:self');
- assert.equal(element._sectionHref(1),
+ assert.equal(
+ element._sectionHref('is:open owner:self'),
+ '/q/is:open+owner:self');
+ assert.equal(
+ element._sectionHref(
+ 'is:open ((reviewer:self -is:ignored) OR assignee:self)'),
'/q/is:open+((reviewer:self+-is:ignored)+OR+assignee:self)');
});
});
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.html b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.html
index 63a6e0b..bec7429 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.html
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.html
@@ -46,8 +46,7 @@
show-reviewed-state
account="[[account]]"
selected-index="{{viewState.selectedChangeIndex}}"
- sections="{{_results}}"
- section-metadata="[[sectionMetadata]]"></gr-change-list>
+ sections="[[_results]]"></gr-change-list>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
</template>
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.js b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.js
index 69550b9..6c5bad3 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.js
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.js
@@ -17,20 +17,22 @@
const DEFAULT_SECTIONS = [
{
name: 'Work in progress',
- query: 'is:open owner:self is:wip',
+ query: 'is:open owner:${user} is:wip',
+ selfOnly: true,
},
{
name: 'Outgoing reviews',
- query: 'is:open owner:self -is:wip',
+ query: 'is:open owner:${user} -is:wip',
},
{
name: 'Incoming reviews',
- query: 'is:open ((reviewer:self -owner:self -is:ignored) OR ' +
- 'assignee:self) -is:wip',
+ query: 'is:open ((reviewer:${user} -owner:${user} -is:ignored) OR ' +
+ 'assignee:${user}) -is:wip',
},
{
name: 'Recently closed',
- query: 'is:closed (owner:self OR reviewer:self OR assignee:self)',
+ query: 'is:closed (owner:${user} OR reviewer:${user} OR ' +
+ 'assignee:${user})',
suffixForDashboard: '-age:4w limit:10',
},
];
@@ -53,11 +55,10 @@
viewState: Object,
params: {
type: Object,
- observer: '_paramsChanged',
},
_results: Array,
- sectionMetadata: {
+ _sectionMetadata: {
type: Array,
value() { return DEFAULT_SECTIONS; },
},
@@ -71,6 +72,10 @@
},
},
+ observers: [
+ '_userChanged(params.user)',
+ ],
+
behaviors: [
Gerrit.RESTClientBehavior,
],
@@ -83,38 +88,53 @@
);
},
- attached() {
- this.fire('title-change', {title: 'My Reviews'});
+ _computeTitle(user) {
+ if (user === 'self') {
+ return 'My Reviews';
+ }
+ return 'Dashboard for ' + user;
},
/**
* Allows a refresh if menu item is selected again.
*/
- _paramsChanged() {
+ _userChanged(user) {
+ if (!user) { return; }
+
+ // NOTE: This method may be called before attachment. Fire title-change
+ // in an async so that attachment to the DOM can take place first.
+ this.async(
+ () => this.fire('title-change', {title: this._computeTitle(user)}));
+
this._loading = true;
- this._getChanges().then(results => {
- this._results = results;
- this._loading = false;
- }).catch(err => {
- this._loading = false;
- console.warn(err.message);
- });
+ const sections = this._sectionMetadata.filter(
+ section => (user === 'self' || !section.selfOnly));
+ const queries =
+ sections.map(
+ section => this._dashboardQueryForSection(section, user));
+ this.$.restAPI.getChanges(null, queries, null, this.options)
+ .then(results => {
+ this._results = sections.map((section, i) => {
+ return {
+ sectionName: section.name,
+ query: queries[i],
+ results: results[i],
+ };
+ });
+ this._loading = false;
+ }).catch(err => {
+ this._loading = false;
+ console.warn(err.message);
+ });
},
- _getChanges() {
- return this.$.restAPI.getChanges(
- null,
- this.sectionMetadata.map(
- section => this._dashboardQueryForSection(section)),
- null,
- this.options);
+ _dashboardQueryForSection(section, user) {
+ const query =
+ section.suffixForDashboard ?
+ section.query + ' ' + section.suffixForDashboard :
+ section.query;
+ return query.replace(/\$\{user\}/g, user);
},
- _dashboardQueryForSection(section) {
- if (section.suffixForDashboard) {
- return section.query + ' ' + section.suffixForDashboard;
- }
- return section.query;
- },
});
})();
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.html b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.html
index 0e73342..2edf26f 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.html
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.html
@@ -39,30 +39,62 @@
setup(() => {
element = fixture('basic');
sandbox = sinon.sandbox.create();
+ getChangesStub = sandbox.stub(element.$.restAPI, 'getChanges',
+ () => Promise.resolve());
});
teardown(() => {
sandbox.restore();
});
- test('content is refreshed with same dropdown selected twice', () => {
- const getChangesStub = sandbox.stub(element, '_getChanges',
- () => Promise.resolve());
+ test('nothing happens when user param is falsy', () => {
+ element.params = {};
+ flushAsynchronousOperations();
+ assert.equal(getChangesStub.callCount, 0);
- element.params = {view: Gerrit.Nav.View.DASHBOARD};
+ element.params = {user: ''};
+ flushAsynchronousOperations();
+ assert.equal(getChangesStub.callCount, 0);
+ });
+ test('content is refreshed when user param is updated', () => {
+ element.params = {user: 'self'};
+ flushAsynchronousOperations();
assert.equal(getChangesStub.callCount, 1);
- element.params = {view: Gerrit.Nav.View.DASHBOARD};
- assert.equal(getChangesStub.callCount, 2);
+ });
+
+ test('viewing another user\'s dashboard omits selfOnly sections', () => {
+ element._sectionMetadata = [
+ {query: '1'},
+ {query: '2', selfOnly: true},
+ ];
+
+ element.params = {user: 'self'};
+ flushAsynchronousOperations();
+ assert.isTrue(
+ getChangesStub.calledWith(null, ['1', '2'], null, element.options));
+
+ element.params = {user: 'user'};
+ flushAsynchronousOperations();
+ assert.isTrue(
+ getChangesStub.calledWith(null, ['1'], null, element.options));
});
test('_dashboardQueryForSection', () => {
- const query = 'query';
- const suffixForDashboard = 'suffix';
- assert.equal(element._dashboardQueryForSection({query}), 'query');
+ const query = 'query for ${user}';
+ const suffixForDashboard = 'suffix for ${user}';
assert.equal(
- element._dashboardQueryForSection({query, suffixForDashboard}),
- 'query suffix');
+ element._dashboardQueryForSection({query}, 'user'),
+ 'query for user');
+ assert.equal(
+ element._dashboardQueryForSection(
+ {query, suffixForDashboard}, 'user'),
+ 'query for user suffix for user');
+ });
+
+ test('_computeTitle', () => {
+ assert.equal(element._computeTitle('self'), 'My Reviews');
+ assert.equal(element._computeTitle('not self'), 'Dashboard for not self');
});
});
</script>
diff --git a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
index 1242699..7089b2e 100644
--- a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
+++ b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
@@ -258,8 +258,8 @@
*/
getUrlForOwner(owner) {
return this._getUrlFor({
- view: Gerrit.Nav.View.SEARCH,
- owner,
+ view: Gerrit.Nav.View.DASHBOARD,
+ user: owner,
});
},
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.js b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
index e3358d4..1e2cc70f 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
@@ -108,6 +108,8 @@
} else {
url = `/c/${params.changeNum}${range}`;
}
+ } else if (params.view === Gerrit.Nav.View.DASHBOARD) {
+ url = `/dashboard/${params.user || 'self'}`;
} else if (params.view === Gerrit.Nav.View.DIFF) {
let range = this._getPatchRangeExpression(params);
if (range.length) { range = '/' + range; }
@@ -196,6 +198,17 @@
page('/login/' + encodeURIComponent(data.substring(basePath.length)));
},
+ /**
+ * Hashes parsed by page.js exclude "inner" hashes, so a URL like "/a#b#c"
+ * is parsed to have a hash of "b" rather than "b#c". Instead, this method
+ * parses hashes correctly. Will return an empty string if there is no hash.
+ * @param {!string} canonicalPath
+ * @return {!string} Everything after the first '#' ("a#b#c" -> "b#c").
+ */
+ _getHashFromCanonicalPath(canonicalPath) {
+ return canonicalPath.split('#').slice(1).join('#');
+ },
+
_startRouter() {
const base = window.Gerrit.BaseUrlBehavior.getBaseUrl();
if (base) {
@@ -233,22 +246,22 @@
// Close child window on redirect after login.
window.close();
}
+ let hash = this._getHashFromCanonicalPath(data.canonicalPath);
// For backward compatibility with GWT links.
- if (data.hash) {
+ if (hash) {
// In certain login flows the server may redirect to a hash without
// a leading slash, which page.js doesn't handle correctly.
- if (data.hash[0] !== '/') {
- data.hash = '/' + data.hash;
+ if (hash[0] !== '/') {
+ hash = '/' + hash;
}
- if (data.hash.includes('/ /') && data.canonicalPath.includes('/+/')) {
+ if (hash.includes('/ /') && data.canonicalPath.includes('/+/')) {
// Path decodes all '+' to ' ' -- this breaks project-based URLs.
// See Issue 6888.
- data.hash = data.hash.replace('/ /', '/+/');
+ hash = hash.replace('/ /', '/+/');
}
- const hash = data.hash;
let newUrl = base + hash;
if (hash.startsWith('/VE/')) {
- newUrl = base + '/settings' + data.hash;
+ newUrl = base + '/settings' + hash;
}
this._redirect(newUrl);
return;
@@ -263,12 +276,22 @@
});
page('/dashboard/(.*)', loadUser, data => {
+ if (!data.params[0]) {
+ page.redirect('/dashboard/self');
+ return;
+ }
this._restAPI.getLoggedIn().then(loggedIn => {
- if (loggedIn) {
- data.params.view = Gerrit.Nav.View.DASHBOARD;
- this._setParams(data.params);
+ if (!loggedIn) {
+ if (data.params[0].toLowerCase() === 'self') {
+ this._redirectToLogin(data.canonicalPath);
+ } else {
+ this._redirect('/q/owner:' + data.params[0]);
+ }
} else {
- this._redirectToLogin(data.canonicalPath);
+ this._setParams({
+ view: Gerrit.Nav.View.DASHBOARD,
+ user: data.params[0],
+ });
}
});
});
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
index 51fcfc1..031cf85 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
@@ -43,6 +43,28 @@
teardown(() => { sandbox.restore(); });
+ test('_getHashFromCanonicalPath', () => {
+ let url = '/foo/bar';
+ let hash = element._getHashFromCanonicalPath(url);
+ assert.equal(hash, '');
+
+ url = '';
+ hash = element._getHashFromCanonicalPath(url);
+ assert.equal(hash, '');
+
+ url = '/foo#bar';
+ hash = element._getHashFromCanonicalPath(url);
+ assert.equal(hash, 'bar');
+
+ url = '/foo#bar#baz';
+ hash = element._getHashFromCanonicalPath(url);
+ assert.equal(hash, 'bar#baz');
+
+ url = '#foo#bar#baz';
+ hash = element._getHashFromCanonicalPath(url);
+ assert.equal(hash, 'foo#bar#baz');
+ });
+
suite('generateUrl', () => {
test('search', () => {
let params = {
diff --git a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.js b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.js
index 9a287fc..a90158f 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.js
+++ b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.js
@@ -31,7 +31,8 @@
_computeOwnerLink(account) {
if (!account) { return; }
- return Gerrit.Nav.getUrlForOwner(account.email || account._account_id);
+ return Gerrit.Nav.getUrlForOwner(
+ account.email || account.username || account._account_id);
},
_computeShowEmail(account) {
diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html
index ad9ba8a..3a3c242 100644
--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@@ -30,6 +30,7 @@
// This seemed to be flakey when it was farther down the list. Keep at the
// beginning.
'gr-app_test.html',
+ 'admin/gr-access-section/gr-access-section_test.html',
'admin/gr-admin-group-list/gr-admin-group-list_test.html',
'admin/gr-admin-project-list/gr-admin-project-list_test.html',
'admin/gr-admin-view/gr-admin-view_test.html',
diff --git a/polygerrit-ui/server.go b/polygerrit-ui/server.go
index f7cab2e..79cf4bf 100644
--- a/polygerrit-ui/server.go
+++ b/polygerrit-ui/server.go
@@ -200,7 +200,7 @@
// Any path prefixes that should resolve to index.html.
var (
- fePaths = []string{"/q/", "/c/", "/dashboard/"}
+ fePaths = []string{"/q/", "/c/", "/dashboard/", "/admin/"}
issueNumRE = regexp.MustCompile(`^\/\d+\/?$`)
)
diff --git a/tools/bzl/classpath.bzl b/tools/bzl/classpath.bzl
index 5b9242e..9448ed1 100644
--- a/tools/bzl/classpath.bzl
+++ b/tools/bzl/classpath.bzl
@@ -1,5 +1,5 @@
def _classpath_collector(ctx):
- all = set()
+ all = depset()
for d in ctx.attr.deps:
if hasattr(d, 'java'):
all += d.java.transitive_runtime_deps
diff --git a/tools/bzl/gwt.bzl b/tools/bzl/gwt.bzl
index ef182bf..b0e250d 100644
--- a/tools/bzl/gwt.bzl
+++ b/tools/bzl/gwt.bzl
@@ -189,7 +189,7 @@
)
def _get_transitive_closure(ctx):
- deps = set()
+ deps = depset()
for dep in ctx.attr.module_deps:
deps += dep.java.transitive_runtime_deps
deps += dep.java.transitive_source_jars
diff --git a/tools/bzl/javadoc.bzl b/tools/bzl/javadoc.bzl
index 341b9c1..18ca129 100644
--- a/tools/bzl/javadoc.bzl
+++ b/tools/bzl/javadoc.bzl
@@ -17,8 +17,8 @@
def _impl(ctx):
zip_output = ctx.outputs.zip
- transitive_jar_set = set()
- source_jars = set()
+ transitive_jar_set = depset()
+ source_jars = depset()
for l in ctx.attr.libs:
source_jars += l.java.source_jars
transitive_jar_set += l.java.transitive_deps
diff --git a/tools/bzl/js.bzl b/tools/bzl/js.bzl
index 788301c..39d6acf 100644
--- a/tools/bzl/js.bzl
+++ b/tools/bzl/js.bzl
@@ -124,18 +124,18 @@
)
def _bower_component_impl(ctx):
- transitive_zipfiles = set([ctx.file.zipfile])
+ transitive_zipfiles = depset([ctx.file.zipfile])
for d in ctx.attr.deps:
transitive_zipfiles += d.transitive_zipfiles
- transitive_licenses = set()
+ transitive_licenses = depset()
if ctx.file.license:
- transitive_licenses += set([ctx.file.license])
+ transitive_licenses += depset([ctx.file.license])
for d in ctx.attr.deps:
transitive_licenses += d.transitive_licenses
- transitive_versions = set(ctx.files.version_json)
+ transitive_versions = depset(ctx.files.version_json)
for d in ctx.attr.deps:
transitive_versions += d.transitive_versions
@@ -173,13 +173,13 @@
command = cmd,
mnemonic = "GenBowerZip")
- licenses = set()
+ licenses = depset()
if ctx.file.license:
- licenses += set([ctx.file.license])
+ licenses += depset([ctx.file.license])
return struct(
transitive_zipfiles=list([ctx.outputs.zip]),
- transitive_versions=set([]),
+ transitive_versions=depset(),
transitive_licenses=licenses)
js_component = rule(
@@ -219,15 +219,15 @@
def _bower_component_bundle_impl(ctx):
"""A bunch of bower components zipped up."""
- zips = set([])
+ zips = depset()
for d in ctx.attr.deps:
zips += d.transitive_zipfiles
- versions = set([])
+ versions = depset()
for d in ctx.attr.deps:
versions += d.transitive_versions
- licenses = set([])
+ licenses = depset()
for d in ctx.attr.deps:
licenses += d.transitive_versions
diff --git a/tools/bzl/pkg_war.bzl b/tools/bzl/pkg_war.bzl
index edaaab0..ebb632f 100644
--- a/tools/bzl/pkg_war.bzl
+++ b/tools/bzl/pkg_war.bzl
@@ -73,7 +73,7 @@
]
# Add lib
- transitive_lib_deps = set()
+ transitive_lib_deps = depset()
for l in ctx.attr.libs:
if hasattr(l, 'java'):
transitive_lib_deps += l.java.transitive_runtime_deps
@@ -85,7 +85,7 @@
inputs.append(dep)
# Add pgm lib
- transitive_pgmlib_deps = set()
+ transitive_pgmlib_deps = depset()
for l in ctx.attr.pgmlibs:
transitive_pgmlib_deps += l.java.transitive_runtime_deps
@@ -95,7 +95,7 @@
inputs.append(dep)
# Add context
- transitive_context_deps = set()
+ transitive_context_deps = depset()
if ctx.attr.context:
for jar in ctx.attr.context:
if hasattr(jar, 'java'):
diff --git a/tools/bzl/plugin.bzl b/tools/bzl/plugin.bzl
index 11ac572..1223f02 100644
--- a/tools/bzl/plugin.bzl
+++ b/tools/bzl/plugin.bzl
@@ -59,7 +59,7 @@
if gwt_module:
native.java_library(
name = name + '__gwt_module',
- resources = list(set(srcs + resources)),
+ resources = depset(srcs + resources).to_list(),
runtime_deps = deps + GWT_PLUGIN_DEPS,
visibility = ['//visibility:public'],
**kwargs
