Merge "Improve detection of LockFailureException"
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiMetrics.java b/java/com/google/gerrit/httpd/restapi/RestApiMetrics.java
index a378fa4..aa1b921 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiMetrics.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiMetrics.java
@@ -19,7 +19,7 @@
import com.google.gerrit.extensions.registration.PluginName;
import com.google.gerrit.httpd.restapi.RestApiServlet.ViewData;
import com.google.gerrit.metrics.Counter1;
-import com.google.gerrit.metrics.Counter2;
+import com.google.gerrit.metrics.Counter3;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
import com.google.gerrit.metrics.Field;
@@ -37,7 +37,7 @@
};
final Counter1<String> count;
- final Counter2<String, Integer> errorCount;
+ final Counter3<String, Integer, String> errorCount;
final Timer1<String> serverLatency;
final Histogram1<String> responseBytes;
@@ -60,6 +60,9 @@
viewField,
Field.ofInteger("error_code", Metadata.Builder::httpStatus)
.description("HTTP status code")
+ .build(),
+ Field.ofString("cause", Metadata.Builder::cause)
+ .description("The cause of the error.")
.build());
serverLatency =
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 1514df8..87b4aa2 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -312,6 +312,7 @@
res.setHeader("X-Content-Type-Options", "nosniff");
int status = SC_OK;
long responseBytes = -1;
+ Optional<Exception> cause = Optional.empty();
Response<?> response = null;
QueryParams qp = null;
Object inputRequestBody = null;
@@ -593,22 +594,28 @@
}
}
} catch (MalformedJsonException | JsonParseException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req, res, status = SC_BAD_REQUEST, "Invalid " + JSON_TYPE + " in request", e);
} catch (BadRequestException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req, res, status = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e.caching(), e);
} catch (AuthException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(req, res, status = SC_FORBIDDEN, messageOr(e, "Forbidden"), e.caching(), e);
} catch (AmbiguousViewException e) {
+ cause = Optional.of(e);
responseBytes = replyError(req, res, status = SC_NOT_FOUND, messageOr(e, "Ambiguous"), e);
} catch (ResourceNotFoundException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(req, res, status = SC_NOT_FOUND, messageOr(e, "Not Found"), e.caching(), e);
} catch (MethodNotAllowedException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req,
@@ -618,9 +625,11 @@
e.caching(),
e);
} catch (ResourceConflictException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(req, res, status = SC_CONFLICT, messageOr(e, "Conflict"), e.caching(), e);
} catch (PreconditionFailedException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req,
@@ -630,6 +639,7 @@
e.caching(),
e);
} catch (UnprocessableEntityException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req,
@@ -639,10 +649,12 @@
e.caching(),
e);
} catch (NotImplementedException e) {
+ cause = Optional.of(e);
logger.atSevere().withCause(e).log("Error in %s %s", req.getMethod(), uriForLogging(req));
responseBytes =
replyError(req, res, status = SC_NOT_IMPLEMENTED, messageOr(e, "Not Implemented"), e);
} catch (UpdateException e) {
+ cause = Optional.of(e);
Throwable t = e.getCause();
if (t instanceof LockFailureException) {
logger.atSevere().withCause(t).log("Error in %s %s", req.getMethod(), uriForLogging(req));
@@ -652,6 +664,7 @@
responseBytes = handleException(e, req, res);
}
} catch (QuotaException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req,
@@ -661,13 +674,15 @@
e.caching(),
e);
} catch (Exception e) {
+ cause = Optional.of(e);
status = SC_INTERNAL_SERVER_ERROR;
responseBytes = handleException(e, req, res);
} finally {
String metric = getViewName(viewData);
+ String formattedCause = cause.map(globals.retryHelper::formatCause).orElse("_none");
globals.metrics.count.increment(metric);
if (status >= SC_BAD_REQUEST) {
- globals.metrics.errorCount.increment(metric, status);
+ globals.metrics.errorCount.increment(metric, status, formattedCause);
}
if (responseBytes != -1) {
globals.metrics.responseBytes.record(metric, responseBytes);
diff --git a/java/com/google/gerrit/server/restapi/group/DeleteMembers.java b/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
index 9bae2e2..fa1c5c6 100644
--- a/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
+++ b/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.restapi.group;
+import com.google.common.base.Strings;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.entities.Account;
@@ -68,6 +69,9 @@
Set<Account.Id> membersToRemove = new HashSet<>();
for (String nameOrEmail : input.members) {
+ if (Strings.isNullOrEmpty(nameOrEmail)) {
+ continue;
+ }
membersToRemove.add(accountResolver.resolve(nameOrEmail).asUnique().account().id());
}
AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index 6f8ef12..3dfa088 100644
--- a/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/java/com/google/gerrit/server/update/RetryHelper.java
@@ -367,7 +367,7 @@
}
}
- private String formatCause(Throwable t) {
+ public String formatCause(Throwable t) {
while ((t instanceof UpdateException
|| t instanceof StorageException
|| t instanceof ExecutionException)
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
index e543976..988580e 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -187,6 +187,24 @@
}
@Test
+ public void removeMember_nullInMemberInputDoesNotCauseFailure() throws Exception {
+ AccountGroup.UUID group =
+ groupOperations.newGroup().addMember(admin.id()).addMember(user.id()).create();
+ gApi.groups().id(group.get()).removeMembers(user.id().toString(), null);
+ ImmutableSet<Account.Id> members = groupOperations.group(group).get().members();
+ assertThat(members).containsExactly(admin.id());
+ }
+
+ @Test
+ public void removeMember_emptyStringInMemberInputDoesNotCauseFailure() throws Exception {
+ AccountGroup.UUID group =
+ groupOperations.newGroup().addMember(admin.id()).addMember(user.id()).create();
+ gApi.groups().id(group.get()).removeMembers(user.id().toString(), "");
+ ImmutableSet<Account.Id> members = groupOperations.group(group).get().members();
+ assertThat(members).containsExactly(admin.id());
+ }
+
+ @Test
public void cachedGroupsForMemberAreUpdatedOnMemberAdditionAndRemoval() throws Exception {
String username = name("user");
Account.Id accountId = accountOperations.newAccount().username(username).create();
