From b096b2ab5a7ca9582806d8e9fe3c54c0df10c1b8 Mon Sep 17 00:00:00 2001 From: Ben Rohlfs Date: Wed, 13 Nov 2019 18:00:28 -0800 Subject: [PATCH 1/5] Fine tune the change metadata rows to all have the same height Affects "Reviewers", "CC" and "Parent" rows. Screenshots: https://imgur.com/a/ouPCzKt Mostly getting rid of vertical padding of buttons such that these rows also have 24px height as the others already have. Also cleaning up gr-reviewer-list.html: Removed obsolete css rules. Change-Id: I1eb97bc33a40bffaea6c81d35bf277df0bd35fd5 (cherry picked from commit ac5e7b66af7c366ca9aa2c741fb968a548cf15f9) --- .../gr-reviewer-list/gr-reviewer-list.html | 56 +++++++------------ .../gr-copy-clipboard/gr-copy-clipboard.html | 6 ++ 2 files changed, 26 insertions(+), 36 deletions(-) diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.html b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.html index 8dbdde1acb..a5875ab1c7 100644 --- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.html +++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.html @@ -32,51 +32,35 @@ limitations under the License. opacity: .8; pointer-events: none; } - .autocompleteContainer { - position: relative; - } - .hiddenReviewers { - margin-top: var(--spacing-s); - } - .inputContainer { - display: flex; - margin-top: var(--spacing-xs); - } - .inputContainer input { - flex: 1; - } - gr-account-chip { + .container > :not(:first-child) { margin-top: var(--spacing-s); } gr-button { --gr-button: { - padding: 5px 0px; - } - } - @media screen and (max-width: 50em), screen and (min-width: 75em) { - gr-account-chip:first-of-type { - margin-top: 0; + padding: 0px 0px; } } - - and [[_hiddenReviewerCount]] more -
+
+ [[_addLabel]] + hidden$="[[!_hiddenReviewerCount]]" + on-click="_handleViewAll">and [[_hiddenReviewerCount]] more +
+ [[_addLabel]] +
diff --git a/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard.html b/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard.html index 0d8d163dfc..f58db39ff4 100644 --- a/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard.html +++ b/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard.html @@ -47,6 +47,12 @@ limitations under the License. height: 1.2em; width: 1.2em; } + gr-button { + --gr-button: { + padding: 1px 4px; + } + } +
Date: Fri, 15 Nov 2019 10:39:10 +0100 Subject: [PATCH 2/5] DeleteBranches: Disallow deletion of refs/meta/config branch The DeleteBranch REST endpoint disallows to delete the refs/meta/config branch, but using the DeleteBranches REST endpoint one could still delete it. Change-Id: I90518a118152be2330c7e533cddd36df9ef774c1 Signed-off-by: Edwin Kempin (cherry picked from commit 2b80772030a3ecb67a5267ad35567119431b2b23) --- .../server/restapi/project/DeleteBranches.java | 8 ++++++++ .../acceptance/rest/project/DeleteBranchIT.java | 11 +++++++++++ .../acceptance/rest/project/DeleteBranchesIT.java | 12 ++++++++++++ 3 files changed, 31 insertions(+) diff --git a/java/com/google/gerrit/server/restapi/project/DeleteBranches.java b/java/com/google/gerrit/server/restapi/project/DeleteBranches.java index 6e60193f8b..2e31c90bed 100644 --- a/java/com/google/gerrit/server/restapi/project/DeleteBranches.java +++ b/java/com/google/gerrit/server/restapi/project/DeleteBranches.java @@ -19,9 +19,11 @@ import static org.eclipse.jgit.lib.Constants.R_HEADS; import com.google.common.collect.ImmutableSet; import com.google.gerrit.extensions.api.projects.DeleteBranchesInput; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ProjectResource; import com.google.gwtorm.server.OrmException; @@ -44,6 +46,12 @@ public class DeleteBranches implements RestModifyView branch(new Branch.NameKey(allUsers, RefNames.REFS_CONFIG)).delete()); + assertThat(thrown).hasMessageThat().contains("not allowed to delete branch refs/meta/config"); + } + private void blockForcePush() throws Exception { block("refs/heads/*", Permission.PUSH, ANONYMOUS_USERS).setForce(true); } diff --git a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java index c1bd8f1ca9..b4a95e363d 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.rest.project; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; +import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static java.util.stream.Collectors.toList; import static org.eclipse.jgit.lib.Constants.R_HEADS; import static org.eclipse.jgit.lib.Constants.R_REFS; @@ -30,6 +31,7 @@ import com.google.gerrit.extensions.api.projects.DeleteBranchesInput; import com.google.gerrit.extensions.api.projects.ProjectApi; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.RefNames; import java.util.HashMap; @@ -157,6 +159,16 @@ public class DeleteBranchesIT extends AbstractDaemonTest { project().deleteBranches(input); } + @Test + public void cannotDeleteRefsMetaConfig() throws Exception { + DeleteBranchesInput input = new DeleteBranchesInput(); + input.branches = Lists.newArrayList(); + input.branches.add(RefNames.REFS_CONFIG); + MethodNotAllowedException thrown = + assertThrows(MethodNotAllowedException.class, () -> project().deleteBranches(input)); + assertThat(thrown).hasMessageThat().contains("not allowed to delete branch refs/meta/config"); + } + private String errorMessageForBranches(List branches) { StringBuilder message = new StringBuilder(); for (String branch : branches) { From 4b2b9b4036ec1040d87edc0e2b4ac230d114615d Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 15 Nov 2019 11:08:17 +0100 Subject: [PATCH 3/5] DeleteBranch/DeleteBranches: Disallow deletion of HEAD to prevent NPE Attempting to delete HEAD fails with a NullPointerException [1]. This is because 'HEAD' gets prefixed with 'refs/heads/' and then the ref is not found. We may consider to support deleting HEAD, but that's outside the scope of this change. E.g. deleting HEAD may rather be separate REST endpoint that lives next to SetHead since setting HEAD requires a different permission than deleting a ref. [1] java.lang.NullPointerException at com.google.gerrit.server.restapi.project.DeleteRef.deleteSingleRef(DeleteRef.java:122) at com.google.gerrit.server.restapi.project.DeleteBranch.apply(DeleteBranch.java:59) at com.google.gerrit.server.restapi.project.DeleteBranch.apply(DeleteBranch.java:34) at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestModifyViewWithRetry$4(RestApiServlet.java:740) at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78) at com.github.rholder.retry.Retryer.call(Retryer.java:160) at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:417) at com.google.gerrit.server.update.RetryHelper.executeWithAttemptAndTimeoutCount(RetryHelper.java:368) at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:271) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:820) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestModifyViewWithRetry(RestApiServlet.java:735) at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:511) at javax.servlet.http.HttpServlet.service(HttpServlet.java:717) ... Change-Id: I44c23f4553f05300b7c79bf257b417ec0b496b09 Signed-off-by: Edwin Kempin (cherry picked from commit 40e1ab1bf084f0b7da66e84b26a84cc4604cbeeb) --- .../gerrit/server/restapi/project/DeleteBranch.java | 5 ++++- .../gerrit/server/restapi/project/DeleteBranches.java | 4 +++- .../gerrit/acceptance/rest/project/DeleteBranchIT.java | 9 +++++++++ .../acceptance/rest/project/DeleteBranchesIT.java | 10 ++++++++++ 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/project/DeleteBranch.java b/java/com/google/gerrit/server/restapi/project/DeleteBranch.java index 0134ce3fca..1d327ca1f8 100644 --- a/java/com/google/gerrit/server/restapi/project/DeleteBranch.java +++ b/java/com/google/gerrit/server/restapi/project/DeleteBranch.java @@ -23,6 +23,7 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.BranchResource; import com.google.gerrit.server.query.change.InternalChangeQuery; @@ -47,7 +48,9 @@ public class DeleteBranch implements RestModifyView { @Override public Response apply(BranchResource rsrc, Input input) throws RestApiException, OrmException, IOException, PermissionBackendException { - if (isConfigRef(rsrc.getBranchKey().get())) { + if (RefNames.HEAD.equals(rsrc.getBranchKey().get())) { + throw new MethodNotAllowedException("not allowed to delete HEAD"); + } else if (isConfigRef(rsrc.getBranchKey().get())) { // Never allow to delete the meta config branch. throw new MethodNotAllowedException( "not allowed to delete branch " + rsrc.getBranchKey().get()); diff --git a/java/com/google/gerrit/server/restapi/project/DeleteBranches.java b/java/com/google/gerrit/server/restapi/project/DeleteBranches.java index 2e31c90bed..28c05017aa 100644 --- a/java/com/google/gerrit/server/restapi/project/DeleteBranches.java +++ b/java/com/google/gerrit/server/restapi/project/DeleteBranches.java @@ -47,7 +47,9 @@ public class DeleteBranches implements RestModifyView branch(new Branch.NameKey(allUsers, RefNames.HEAD)).delete()); + assertThat(thrown).hasMessageThat().contains("not allowed to delete HEAD"); + } + private void blockForcePush() throws Exception { block("refs/heads/*", Permission.PUSH, ANONYMOUS_USERS).setForce(true); } diff --git a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java index b4a95e363d..ca7517cc36 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java @@ -169,6 +169,16 @@ public class DeleteBranchesIT extends AbstractDaemonTest { assertThat(thrown).hasMessageThat().contains("not allowed to delete branch refs/meta/config"); } + @Test + public void cannotDeleteHead() throws Exception { + DeleteBranchesInput input = new DeleteBranchesInput(); + input.branches = Lists.newArrayList(); + input.branches.add(RefNames.HEAD); + MethodNotAllowedException thrown = + assertThrows(MethodNotAllowedException.class, () -> project().deleteBranches(input)); + assertThat(thrown).hasMessageThat().contains("not allowed to delete HEAD"); + } + private String errorMessageForBranches(List branches) { StringBuilder message = new StringBuilder(); for (String branch : branches) { From 6f74a43b810f9442aa21f3ec9b5e72a1b504b2aa Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 15 Nov 2019 12:18:42 +0100 Subject: [PATCH 4/5] MergeSuperSet: Include change ID into error message This makes the investigation of such issues easier. Change-Id: I53557825fe11f2b594ba7b21a0a8c16b310bd183 Signed-off-by: Edwin Kempin (cherry picked from commit 4891360e4a95c306e567b50ce72f5df3630b0873) --- java/com/google/gerrit/server/submit/MergeSuperSet.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/submit/MergeSuperSet.java b/java/com/google/gerrit/server/submit/MergeSuperSet.java index 7a600a14c6..f444fcb3e3 100644 --- a/java/com/google/gerrit/server/submit/MergeSuperSet.java +++ b/java/com/google/gerrit/server/submit/MergeSuperSet.java @@ -101,7 +101,11 @@ public class MergeSuperSet { closeOrm = true; } List cds = queryProvider.get().byLegacyChangeId(change.getId()); - checkState(cds.size() == 1, "Expected exactly one ChangeData, got " + cds.size()); + checkState( + cds.size() == 1, + "Expected exactly one ChangeData for change ID %s, got %s", + change.getId(), + cds.size()); ChangeData cd = Iterables.getFirst(cds, null); boolean visible = false; From 68961912b7fd4a7edec9e625b9b37e7e1a6bcb1e Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 15 Nov 2019 12:58:29 +0100 Subject: [PATCH 5/5] QueryAccount/QueryGroups: Enable retries If there is an error in QueryAccount/QueryGroups we automatically retry the request, but the retry always fails because AccountQueryProcessor/GroupQueryProcessor is not reusable [1]. Allow retries by creating a fresh query processor instance on each try. [1] AutoRetry: auto-retry of restapi.account.QueryAccounts has failed [CONTEXT forced=true TRACE_ID="retry-on-failure-1573791374279-221012e6" ] java.lang.IllegalStateException: AccountQueryProcessor has already been used at com.google.common.base.Preconditions.checkState(Preconditions.java:589) at com.google.gerrit.index.query.QueryProcessor.query(QueryProcessor.java:206) at com.google.gerrit.index.query.QueryProcessor.query(QueryProcessor.java:194) at com.google.gerrit.index.query.QueryProcessor.query(QueryProcessor.java:178) at com.google.gerrit.server.restapi.account.QueryAccounts.apply(QueryAccounts.java:211) at com.google.gerrit.server.restapi.account.QueryAccounts.apply(QueryAccounts.java:55) ... Change-Id: I2f7b02962d65d98e83531ffb5b81d4e9287d6fbb Signed-off-by: Edwin Kempin (cherry picked from commit bfce38654f1eb352cbcbd24d9bc18957770864af) --- .../server/restapi/account/QueryAccounts.java | 15 +++++++++++---- .../gerrit/server/restapi/group/QueryGroups.java | 11 ++++++++--- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java index 2c0512c196..7f27edda60 100644 --- a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java +++ b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java @@ -42,6 +42,7 @@ import com.google.gerrit.server.query.account.AccountQueryBuilder; import com.google.gerrit.server.query.account.AccountQueryProcessor; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import com.google.inject.Provider; import java.util.Collections; import java.util.EnumSet; import java.util.LinkedHashMap; @@ -57,12 +58,13 @@ public class QueryAccounts implements RestReadView { private final PermissionBackend permissionBackend; private final AccountLoader.Factory accountLoaderFactory; private final AccountQueryBuilder queryBuilder; - private final AccountQueryProcessor queryProcessor; + private final Provider queryProcessorProvider; private final boolean suggestConfig; private final int suggestFrom; private AccountLoader accountLoader; private boolean suggest; + private Integer limit; private int suggestLimit = 10; private String query; private Integer start; @@ -79,7 +81,7 @@ public class QueryAccounts implements RestReadView { metaVar = "CNT", usage = "maximum number of users to return") public void setLimit(int n) { - queryProcessor.setUserProvidedLimit(n); + this.limit = n; if (n < 0) { suggestLimit = 10; @@ -123,12 +125,12 @@ public class QueryAccounts implements RestReadView { PermissionBackend permissionBackend, AccountLoader.Factory accountLoaderFactory, AccountQueryBuilder queryBuilder, - AccountQueryProcessor queryProcessor, + Provider queryProcessorProvider, @GerritServerConfig Config cfg) { this.permissionBackend = permissionBackend; this.accountLoaderFactory = accountLoaderFactory; this.queryBuilder = queryBuilder; - this.queryProcessor = queryProcessor; + this.queryProcessorProvider = queryProcessorProvider; this.suggestFrom = cfg.getInt("suggest", null, "from", 0); this.options = EnumSet.noneOf(ListAccountsOption.class); @@ -185,10 +187,15 @@ public class QueryAccounts implements RestReadView { } accountLoader = accountLoaderFactory.create(fillOptions); + AccountQueryProcessor queryProcessor = queryProcessorProvider.get(); if (queryProcessor.isDisabled()) { throw new MethodNotAllowedException("query disabled"); } + if (limit != null) { + queryProcessor.setUserProvidedLimit(limit); + } + if (start != null) { queryProcessor.setStart(start); } diff --git a/java/com/google/gerrit/server/restapi/group/QueryGroups.java b/java/com/google/gerrit/server/restapi/group/QueryGroups.java index fa9285dfdb..b11664636a 100644 --- a/java/com/google/gerrit/server/restapi/group/QueryGroups.java +++ b/java/com/google/gerrit/server/restapi/group/QueryGroups.java @@ -31,6 +31,7 @@ import com.google.gerrit.server.query.group.GroupQueryBuilder; import com.google.gerrit.server.query.group.GroupQueryProcessor; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import com.google.inject.Provider; import java.util.ArrayList; import java.util.EnumSet; import java.util.List; @@ -38,7 +39,7 @@ import org.kohsuke.args4j.Option; public class QueryGroups implements RestReadView { private final GroupQueryBuilder queryBuilder; - private final GroupQueryProcessor queryProcessor; + private final Provider queryProcessorProvider; private final GroupJson json; private String query; @@ -87,9 +88,11 @@ public class QueryGroups implements RestReadView { @Inject protected QueryGroups( - GroupQueryBuilder queryBuilder, GroupQueryProcessor queryProcessor, GroupJson json) { + GroupQueryBuilder queryBuilder, + Provider queryProcessorProvider, + GroupJson json) { this.queryBuilder = queryBuilder; - this.queryProcessor = queryProcessor; + this.queryProcessorProvider = queryProcessorProvider; this.json = json; } @@ -101,6 +104,8 @@ public class QueryGroups implements RestReadView { throw new BadRequestException("missing query field"); } + GroupQueryProcessor queryProcessor = queryProcessorProvider.get(); + if (queryProcessor.isDisabled()) { throw new MethodNotAllowedException("query disabled"); }