opendev/gerrit
Code Issues Proposed changes

Remove StorageException catch clauses from httpd

Browse Source 
Some of these are the antipattern of returning an empty result instead
of letting a fatal error propagate. In others, the purpose of the catch
clause is to do additional logging or add context information to the
exception message. In these cases, switch to catching RuntimeException
instead, under the assumption that the additional context is useful
regardless of the type of unexpected error.

Change-Id: I21b94ae2c00b9c09c2b81c5fc8cd1b66bb491bc9
This commit is contained in:
Dave Borowitz
2019-01-15 19:34:26 -08:00
parent 4737b47546
commit 40814dd46d
7 changed files with 17 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.httpd;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Change;
@@ -61,7 +60,7 @@ public class NumericChangeIdRedirectServlet extends HttpServlet {
} catch (ResourceConflictException | ResourceNotFoundException e) {
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
} catch (StorageException | PermissionBackendException e) {
} catch (PermissionBackendException | RuntimeException e) {
throw new IOException("Unable to lookup change " + id.id, e);
}
String path =

3
java/com/google/gerrit/httpd/RunAsFilter.java
View File

@@ -19,7 +19,6 @@ import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -110,7 +109,7 @@ class RunAsFilter implements Filter {
} catch (UnprocessableEntityException e) {
replyError(req, res, SC_FORBIDDEN, "no account matches " + RUN_AS, null);
return;
} catch (StorageException | IOException | ConfigInvalidException e) {
} catch (IOException | ConfigInvalidException | RuntimeException e) {
logger.atWarning().withCause(e).log("cannot resolve account for %s", RUN_AS);
replyError(req, res, SC_INTERNAL_SERVER_ERROR, "cannot resolve " + RUN_AS, e);
return;

32
java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java
View File

@@ -18,7 +18,6 @@ import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USE
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_UUID;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.httpd.HtmlDomUtil;
import com.google.gerrit.httpd.LoginUrlToken;
@@ -188,33 +187,20 @@ class BecomeAnyAccountLoginServlet extends HttpServlet {
}
private AuthResult byUserName(String userName) {
try {
List<AccountState> accountStates =
queryProvider.get().byExternalId(SCHEME_USERNAME, userName);
if (accountStates.isEmpty()) {
getServletContext().log("No accounts with username " + userName + " found");
return null;
}
if (accountStates.size() > 1) {
getServletContext().log("Multiple accounts with username " + userName + " found");
return null;
}
return auth(accountStates.get(0).getAccount().getId());
} catch (StorageException e) {
getServletContext().log("cannot query account index", e);
List<AccountState> accountStates = queryProvider.get().byExternalId(SCHEME_USERNAME, userName);
if (accountStates.isEmpty()) {
getServletContext().log("No accounts with username " + userName + " found");
return null;
}
if (accountStates.size() > 1) {
getServletContext().log("Multiple accounts with username " + userName + " found");
return null;
}
return auth(accountStates.get(0).getAccount().getId());
}
private Optional<AuthResult> byPreferredEmail(String email) {
try {
Optional<AccountState> match =
queryProvider.get().byPreferredEmail(email).stream().findFirst();
return auth(match);
} catch (StorageException e) {
getServletContext().log("cannot query database", e);
return Optional.empty();
}
return auth(queryProvider.get().byPreferredEmail(email).stream().findFirst());
}
private Optional<AuthResult> byAccountId(String idStr) {

3
java/com/google/gerrit/httpd/auth/container/HttpLoginServlet.java
View File

@@ -19,7 +19,6 @@ import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.httpd.CanonicalWebUrl;
import com.google.gerrit.httpd.HtmlDomUtil;
@@ -128,7 +127,7 @@ class HttpLoginServlet extends HttpServlet {
logger.atFine().log(
"Associating external identity \"%s\" to user \"%s\"", remoteExternalId, user);
updateRemoteExternalId(arsp, remoteExternalId);
} catch (AccountException | StorageException | ConfigInvalidException e) {
} catch (AccountException | ConfigInvalidException e) {
logger.atSevere().withCause(e).log(
"Unable to associate external identity \"%s\" to user \"%s\"", remoteExternalId, user);
rsp.sendError(HttpServletResponse.SC_FORBIDDEN);

5
java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java
View File

@@ -19,7 +19,6 @@ import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
import com.google.common.base.CharMatcher;
import com.google.common.base.Strings;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.auth.oauth.OAuthServiceProvider;
import com.google.gerrit.extensions.auth.oauth.OAuthToken;
import com.google.gerrit.extensions.auth.oauth.OAuthUserInfo;
@@ -188,7 +187,7 @@ class OAuthSession {
logger.atInfo().log("OAuth2: linking claimed identity to %s", claimedId.get().toString());
try {
accountManager.link(claimedId.get(), req);
} catch (StorageException | ConfigInvalidException e) {
} catch (ConfigInvalidException e) {
logger.atSevere().log(
"Cannot link: %s to user identity:\n Claimed ID: %s is %s",
user.getExternalId(), claimedId.get(), claimedIdentifier);
@@ -203,7 +202,7 @@ class OAuthSession {
throws AccountException, IOException {
try {
accountManager.link(identifiedUser.get().getAccountId(), areq);
} catch (StorageException | ConfigInvalidException e) {
} catch (ConfigInvalidException e) {
logger.atSevere().log(
"Cannot link: %s to user identity: %s",
user.getExternalId(), identifiedUser.get().getAccountId());

5
java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java
View File

@@ -18,7 +18,6 @@ import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
import com.google.common.base.Strings;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.auth.oauth.OAuthServiceProvider;
import com.google.gerrit.extensions.auth.oauth.OAuthToken;
import com.google.gerrit.extensions.auth.oauth.OAuthUserInfo;
@@ -164,7 +163,7 @@ class OAuthSessionOverOpenID {
logger.atFine().log("Claimed account already exists: link to it.");
try {
accountManager.link(claimedId.get(), areq);
} catch (StorageException | ConfigInvalidException e) {
} catch (ConfigInvalidException e) {
logger.atSevere().log(
"Cannot link: %s to user identity:\n Claimed ID: %s is %s",
user.getExternalId(), claimedId.get(), claimedIdentifier);
@@ -178,7 +177,7 @@ class OAuthSessionOverOpenID {
try {
logger.atFine().log("Linking \"%s\" to \"%s\"", user.getExternalId(), accountId);
accountManager.link(accountId, areq);
} catch (StorageException | ConfigInvalidException e) {
} catch (ConfigInvalidException e) {
logger.atSevere().log(
"Cannot link: %s to user identity: %s", user.getExternalId(), accountId);
rsp.sendError(HttpServletResponse.SC_FORBIDDEN);

3
java/com/google/gerrit/httpd/raw/CatServlet.java
View File

@@ -14,7 +14,6 @@
package com.google.gerrit.httpd.raw;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Url;
@@ -145,7 +144,7 @@ public class CatServlet extends HttpServlet {
} catch (ResourceConflictException | NoSuchChangeException | AuthException e) {
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
} catch (StorageException | PermissionBackendException | IOException e) {
} catch (PermissionBackendException | IOException e) {
getServletContext().log("Cannot query database", e);
rsp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
return;