Merge "Propagate RuntimeExceptions in DefaultQuotaBackend"
diff --git a/java/com/google/gerrit/server/quota/DefaultQuotaBackend.java b/java/com/google/gerrit/server/quota/DefaultQuotaBackend.java
index baa7677..c6e67ca 100644
--- a/java/com/google/gerrit/server/quota/DefaultQuotaBackend.java
+++ b/java/com/google/gerrit/server/quota/DefaultQuotaBackend.java
@@ -74,8 +74,12 @@
responses.add(enforcer.call(p -> p.dryRun(quotaGroup, requestContext, numTokens)));
}
} catch (RuntimeException e) {
- logger.atSevere().withCause(e).log("exception while enforcing quota");
- responses.add(QuotaResponse.error("failed to request quota tokens"));
+ // Roll back the quota request for all enforcers that deducted the quota. Rethrow the
+ // exception to adhere to the API contract.
+ if (deduct) {
+ refillAfterErrorOrException(enforcers, responses, quotaGroup, requestContext, numTokens);
+ }
+ throw e;
}
}
@@ -83,11 +87,7 @@
// Roll back the quota request for all enforcers that deducted the quota (= the request
// succeeded). Don't touch failed enforcers as the interface contract said that failed
// requests should not be deducted.
- for (int i = 0; i < responses.size(); i++) {
- if (responses.get(i).status().isOk()) {
- enforcers.get(i).run(p -> p.refill(quotaGroup, requestContext, numTokens));
- }
- }
+ refillAfterErrorOrException(enforcers, responses, quotaGroup, requestContext, numTokens);
}
logger.atFine().log(
@@ -100,6 +100,19 @@
return QuotaResponse.Aggregated.create(ImmutableList.copyOf(responses));
}
+ private static void refillAfterErrorOrException(
+ List<PluginSetEntryContext<QuotaEnforcer>> enforcers,
+ List<QuotaResponse> collectedResponses,
+ String quotaGroup,
+ QuotaRequestContext requestContext,
+ long numTokens) {
+ for (int i = 0; i < collectedResponses.size(); i++) {
+ if (collectedResponses.get(i).status().isOk()) {
+ enforcers.get(i).run(p -> p.refill(quotaGroup, requestContext, numTokens));
+ }
+ }
+ }
+
static class WithUser extends WithResource implements QuotaBackend.WithUser {
WithUser(PluginSetContext<QuotaEnforcer> quotaEnforcers, CurrentUser user) {
super(quotaEnforcers, QuotaRequestContext.builder().user(user).build());
diff --git a/javatests/com/google/gerrit/acceptance/server/quota/DefaultQuotaBackendIT.java b/javatests/com/google/gerrit/acceptance/server/quota/DefaultQuotaBackendIT.java
index f60d301..dea83ca 100644
--- a/javatests/com/google/gerrit/acceptance/server/quota/DefaultQuotaBackendIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/quota/DefaultQuotaBackendIT.java
@@ -139,17 +139,13 @@
}
@Test
- public void requestTokenPluginThrowsResultsInError() throws Exception {
+ public void requestTokenPluginThrowsAndRethrows() {
QuotaRequestContext ctx = QuotaRequestContext.builder().user(identifiedAdmin).build();
expect(quotaEnforcer.requestTokens("testGroup", ctx, 1)).andThrow(new NullPointerException());
replay(quotaEnforcer);
- QuotaResponse.Aggregated result = quotaBackend.user(identifiedAdmin).requestToken("testGroup");
- assertThat(result)
- .isEqualTo(singletonAggregation(QuotaResponse.error("failed to request quota tokens")));
- exception.expect(QuotaException.class);
- exception.expectMessage("failed to request quota tokens");
- result.throwOnError();
+ exception.expect(NullPointerException.class);
+ quotaBackend.user(identifiedAdmin).requestToken("testGroup");
}
private static QuotaResponse.Aggregated singletonAggregation(QuotaResponse response) {
diff --git a/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java b/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java
index ba69d7c..31a8808 100644
--- a/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java
@@ -19,6 +19,7 @@
import static org.easymock.EasyMock.expectLastCall;
import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.resetToStrict;
+import static org.easymock.EasyMock.verify;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -87,20 +88,24 @@
@Test
public void refillsOnException() {
+ NullPointerException exception = new NullPointerException();
QuotaRequestContext ctx = QuotaRequestContext.builder().user(identifiedAdmin).build();
- expect(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).andReturn(QuotaResponse.ok());
- expect(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).andThrow(new NullPointerException());
- quotaEnforcerA.refill("testGroup", ctx, 1);
+ expect(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).andThrow(exception);
+ expect(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).andReturn(QuotaResponse.ok());
+ quotaEnforcerB.refill("testGroup", ctx, 1);
expectLastCall();
replay(quotaEnforcerA);
replay(quotaEnforcerB);
- assertThat(quotaBackend.user(identifiedAdmin).requestToken("testGroup"))
- .isEqualTo(
- QuotaResponse.Aggregated.create(
- ImmutableList.of(
- QuotaResponse.ok(), QuotaResponse.error("failed to request quota tokens"))));
+ try {
+ quotaBackend.user(identifiedAdmin).requestToken("testGroup");
+ fail("expected a NullPointerException");
+ } catch (NullPointerException e) {
+ assertThat(exception).isEqualTo(e);
+ }
+
+ verify(quotaEnforcerA);
}
@Test
