Skip to content

Commit

Permalink
Harmonize http status code usage (#2451)
Browse files Browse the repository at this point in the history
Given that we run servlets, it makes sense to always use the status
code contants from the servlet class.
  • Loading branch information
jianglai committed May 24, 2024
1 parent 0781010 commit e88ff77
Show file tree
Hide file tree
Showing 28 changed files with 157 additions and 175 deletions.
23 changes: 8 additions & 15 deletions config/presubmits.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,23 +173,11 @@ def fails(self, file):
):
"JavaScript files should not include console logging.",
PresubmitCheck(
r".*org\.testcontainers\.shaded.*",
r".*\nimport (static )?.*\.shaded\..*",
"java",
{"/node_modules/"},
):
"Do not use shaded dependencies from testcontainers.",
PresubmitCheck(
r".*autovalue\.shaded.*",
"java",
{"/node_modules/"},
):
"Do not use shaded dependencies from autovalue.",
PresubmitCheck(
r".*avro\.shaded.*",
"java",
{"/node_modules/"},
):
"Do not use shaded dependencies from avro.",
"Do not use shaded dependencies",
PresubmitCheck(
r".*com\.google\.common\.truth\.Truth8.*",
"java",
Expand All @@ -202,7 +190,12 @@ def fails(self, file):
{"/node_modules/", "JpaTransactionManagerImpl.java"},
):
"Do not use java.util.Date. Use classes in java.time package instead.",

PresubmitCheck(
r".*com\.google\.api\.client\.http\.HttpStatusCodes.*",
"java",
{"/node_modules/"},
):
"Use status code from jakarta.servlet.http.HttpServletResponse.",
}

# Note that this regex only works for one kind of Flyway file. If we want to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@
import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm;
import static google.registry.request.Action.Method.GET;
import static google.registry.request.Action.Method.POST;
import static jakarta.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
import static java.nio.charset.StandardCharsets.US_ASCII;

import com.google.api.client.http.HttpStatusCodes;
import com.google.cloud.storage.BlobId;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -190,7 +190,7 @@ boolean uploadToBsa(String unavailableDomains, DateTime runTime) {
return true;
} catch (IOException e) {
logger.atSevere().withCause(e).log("Error while attempting to upload to BSA, aborting.");
response.setStatus(HttpStatusCodes.STATUS_CODE_SERVER_ERROR);
response.setStatus(SC_INTERNAL_SERVER_ERROR);
response.setPayload("Error while attempting to upload to BSA: " + e.getMessage());
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@

package google.registry.rdap;

import static com.google.api.client.http.HttpStatusCodes.STATUS_CODE_OK;
import static com.google.common.net.HttpHeaders.ACCEPT_ENCODING;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.request.UrlConnectionUtils.gUnzipBytes;
import static google.registry.request.UrlConnectionUtils.isGZipped;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;
import static java.nio.charset.StandardCharsets.UTF_8;

import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -114,7 +114,7 @@ private ImmutableMap<String, String> getIanaIdsToUrls()
connection.setRequestProperty(ACCEPT_ENCODING, "gzip");
String csvString;
try {
if (connection.getResponseCode() != STATUS_CODE_OK) {
if (connection.getResponseCode() != SC_OK) {
throw new UrlConnectionException("Failed to load RDAP base URLs from ICANN", connection);
}
// With GZIP encoding header in the request (see above) ICANN had still sent response in plain
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/google/registry/rde/RdeReporter.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@

package google.registry.rde;

import static com.google.api.client.http.HttpStatusCodes.STATUS_CODE_BAD_REQUEST;
import static com.google.api.client.http.HttpStatusCodes.STATUS_CODE_OK;
import static google.registry.request.UrlConnectionUtils.getResponseBytes;
import static google.registry.request.UrlConnectionUtils.setBasicAuth;
import static google.registry.request.UrlConnectionUtils.setPayload;
import static google.registry.util.DomainNameUtils.canonicalizeHostname;
import static jakarta.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;
import static java.nio.charset.StandardCharsets.UTF_8;

import com.google.api.client.http.HttpMethods;
Expand Down Expand Up @@ -85,7 +85,7 @@ public void send(byte[] reportBytes) throws XmlException, GeneralSecurityExcepti

try {
responseCode = connection.getResponseCode();
if (responseCode != STATUS_CODE_OK && responseCode != STATUS_CODE_BAD_REQUEST) {
if (responseCode != SC_OK && responseCode != SC_BAD_REQUEST) {
logger.atWarning().log("Connection to RDE report server failed: %d", responseCode);
throw new UrlConnectionException("PUT failed", connection);
}
Expand All @@ -96,7 +96,7 @@ public void send(byte[] reportBytes) throws XmlException, GeneralSecurityExcepti

// We know that an HTTP 200 response can only contain a result code of
// 1000 (i. e. success), there is no need to parse it.
if (responseCode != STATUS_CODE_OK) {
if (responseCode != SC_OK) {
XjcIirdeaResult result = parseResult(responseBytes);
logger.atWarning().log(
"Rejected when trying to PUT RDE report to ICANN server: %d %s\n%s",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@

package google.registry.reporting.icann;

import static com.google.api.client.http.HttpStatusCodes.STATUS_CODE_OK;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.net.MediaType.CSV_UTF_8;
import static google.registry.model.tld.Tlds.assertTldExists;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;

import com.google.api.client.http.HttpMethods;
import com.google.common.base.Ascii;
Expand Down Expand Up @@ -95,7 +95,7 @@ public boolean send(byte[] reportBytes, String reportFilename)
try {
responseCode = connection.getResponseCode();
content = UrlConnectionUtils.getResponseBytes(connection);
if (responseCode != STATUS_CODE_OK) {
if (responseCode != SC_OK) {
XjcIirdeaResult result = parseResult(content);
logger.atWarning().log(
"PUT rejected, status code %s:\n%s\n%s",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,11 @@
package google.registry.ui.server.console;

import static google.registry.request.Action.Method.GET;
import static jakarta.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
import static jakarta.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static jakarta.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
import static jakarta.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;

import com.google.api.client.http.HttpStatusCodes;
import com.google.common.base.Throwables;
import com.google.common.flogger.FluentLogger;
import google.registry.model.console.ConsolePermission;
Expand Down Expand Up @@ -50,7 +53,7 @@ public final void run() {
AuthResult authResult = consoleApiParams.authResult();
if (authResult.userAuthInfo().isEmpty()
|| authResult.userAuthInfo().get().consoleUser().isEmpty()) {
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED);
consoleApiParams.response().setStatus(SC_UNAUTHORIZED);
return;
}
User user = consoleApiParams.authResult().userAuthInfo().get().consoleUser().get();
Expand All @@ -77,15 +80,13 @@ public final void run() {
}
} catch (ConsolePermissionForbiddenException e) {
logger.atWarning().withCause(e).log("Forbidden");
setFailedResponse("", HttpStatusCodes.STATUS_CODE_FORBIDDEN);
setFailedResponse("", SC_FORBIDDEN);
} catch (HttpException.BadRequestException | IllegalArgumentException e) {
logger.atWarning().withCause(e).log("Error in request");
setFailedResponse(
Throwables.getRootCause(e).getMessage(), HttpStatusCodes.STATUS_CODE_BAD_REQUEST);
setFailedResponse(Throwables.getRootCause(e).getMessage(), SC_BAD_REQUEST);
} catch (Throwable t) {
logger.atWarning().withCause(t).log("Internal server error");
setFailedResponse(
Throwables.getRootCause(t).getMessage(), HttpStatusCodes.STATUS_CODE_SERVER_ERROR);
setFailedResponse(Throwables.getRootCause(t).getMessage(), SC_INTERNAL_SERVER_ERROR);
}
}

Expand Down Expand Up @@ -118,7 +119,7 @@ private boolean verifyXSRF() {
.findFirst();
if (maybeCookie.isEmpty()
|| !consoleApiParams.xsrfTokenManager().validateToken(maybeCookie.get().getValue())) {
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED);
consoleApiParams.response().setStatus(SC_UNAUTHORIZED);
return false;
}
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
package google.registry.ui.server.console;

import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static jakarta.servlet.http.HttpServletResponse.SC_NOT_FOUND;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;

import com.google.api.client.http.HttpStatusCodes;
import com.google.gson.Gson;
import google.registry.model.EppResourceUtils;
import google.registry.model.console.ConsolePermission;
Expand Down Expand Up @@ -59,16 +60,16 @@ protected void getHandler(User user) {
EppResourceUtils.loadByForeignKeyCached(
Domain.class, paramDomain, tm().getTransactionTime()));
if (possibleDomain.isEmpty()) {
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_NOT_FOUND);
consoleApiParams.response().setStatus(SC_NOT_FOUND);
return;
}
Domain domain = possibleDomain.get();
if (!user.getUserRoles()
.hasPermission(domain.getCurrentSponsorRegistrarId(), ConsolePermission.DOWNLOAD_DOMAINS)) {
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_NOT_FOUND);
consoleApiParams.response().setStatus(SC_NOT_FOUND);
return;
}
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK);
consoleApiParams.response().setStatus(SC_OK);
consoleApiParams.response().setPayload(gson.toJson(domain));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
import static com.google.common.base.Preconditions.checkArgument;
import static google.registry.model.console.ConsolePermission.DOWNLOAD_DOMAINS;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;

import com.google.api.client.http.HttpStatusCodes;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Ascii;
import com.google.gson.Gson;
Expand Down Expand Up @@ -118,7 +118,7 @@ private void runInTransaction() {
consoleApiParams
.response()
.setPayload(gson.toJson(new DomainListResult(domains, checkpoint, actualTotalResults)));
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK);
consoleApiParams.response().setStatus(SC_OK);
}

/** Creates the query to get the total number of matching domains, interpolating as necessary. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@
import static com.google.common.base.Preconditions.checkArgument;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.request.Action.Method.POST;
import static jakarta.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static jakarta.servlet.http.HttpServletResponse.SC_NOT_FOUND;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;

import com.google.api.client.http.HttpStatusCodes;
import com.google.common.base.Strings;
import com.google.gson.annotations.Expose;
import google.registry.flows.EppException.AuthenticationErrorException;
Expand Down Expand Up @@ -92,14 +94,14 @@ protected void postHandler(User user) {
try {
registrar = registrarAccessor.getRegistrar(eppRequestBody.registrarId());
} catch (RegistrarAccessDeniedException e) {
setFailedResponse(e.getMessage(), HttpStatusCodes.STATUS_CODE_NOT_FOUND);
setFailedResponse(e.getMessage(), SC_NOT_FOUND);
return;
}

try {
credentials.validate(registrar, eppRequestBody.oldPassword());
} catch (AuthenticationErrorException e) {
setFailedResponse(e.getMessage(), HttpStatusCodes.STATUS_CODE_FORBIDDEN);
setFailedResponse(e.getMessage(), SC_FORBIDDEN);
return;
}

Expand All @@ -113,7 +115,7 @@ protected void postHandler(User user) {
new InternetAddress(registrar.getEmailAddress(), true)));
});

consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK);
consoleApiParams.response().setStatus(SC_OK);
}

public record EppPasswordData(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@
import static google.registry.request.RequestParameters.extractOptionalParameter;
import static google.registry.request.RequestParameters.extractRequiredParameter;
import static google.registry.ui.server.registrar.RegistryLockPostAction.VERIFICATION_EMAIL_TEMPLATE;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;
import static jakarta.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;

import com.google.api.client.http.HttpStatusCodes;
import com.google.common.collect.ImmutableList;
import com.google.gson.Gson;
import google.registry.flows.EppException;
Expand Down Expand Up @@ -90,7 +91,7 @@ public ConsoleRegistryLockAction(
protected void getHandler(User user) {
checkPermission(user, registrarId, ConsolePermission.REGISTRY_LOCK);
consoleApiParams.response().setPayload(gson.toJson(getLockedDomains()));
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK);
consoleApiParams.response().setStatus(SC_OK);
}

@Override
Expand Down Expand Up @@ -124,8 +125,7 @@ protected void postHandler(User user) {
if (!isAdmin) {
checkArgument(maybePassword.isPresent(), "No password provided");
if (!user.verifyRegistryLockPassword(maybePassword.get())) {
setFailedResponse(
"Incorrect registry lock password", HttpStatusCodes.STATUS_CODE_UNAUTHORIZED);
setFailedResponse("Incorrect registry lock password", SC_UNAUTHORIZED);
return;
}
}
Expand All @@ -147,7 +147,7 @@ protected void postHandler(User user) {
relockDurationMillis.map(Duration::new));
sendVerificationEmail(registryLock, registryLockEmail, isLock);
});
response.setStatus(HttpStatusCodes.STATUS_CODE_OK);
response.setStatus(SC_OK);
}

private void sendVerificationEmail(RegistryLock lock, String userEmail, boolean isLock) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
package google.registry.ui.server.console;

import static google.registry.request.Action.Method.GET;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;

import com.google.api.client.http.HttpStatusCodes;
import com.google.common.collect.ImmutableMap;
import google.registry.config.RegistryConfig.Config;
import google.registry.model.console.User;
Expand Down Expand Up @@ -88,6 +88,6 @@ protected void getHandler(User user) {
"technicalDocsUrl", technicalDocsUrl));

consoleApiParams.response().setPayload(json.toString());
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK);
consoleApiParams.response().setStatus(SC_OK);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.request.Action.Method.GET;
import static google.registry.request.Action.Method.POST;
import static jakarta.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;

import com.google.api.client.http.HttpStatusCodes;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Streams;
import com.google.gson.Gson;
Expand Down Expand Up @@ -72,7 +73,7 @@ public RegistrarsAction(
@Override
protected void getHandler(User user) {
if (!user.getUserRoles().hasGlobalPermission(ConsolePermission.VIEW_REGISTRARS)) {
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN);
consoleApiParams.response().setStatus(SC_FORBIDDEN);
return;
}

Expand All @@ -82,13 +83,13 @@ protected void getHandler(User user) {
.collect(ImmutableList.toImmutableList());

consoleApiParams.response().setPayload(gson.toJson(registrars));
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK);
consoleApiParams.response().setStatus(SC_OK);
}

@Override
protected void postHandler(User user) {
if (!user.getUserRoles().isAdmin()) {
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN);
consoleApiParams.response().setStatus(SC_FORBIDDEN);
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.request.Action.Method.GET;
import static google.registry.request.Action.Method.POST;
import static jakarta.servlet.http.HttpServletResponse.SC_OK;

import com.google.api.client.http.HttpStatusCodes;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
Expand Down Expand Up @@ -78,7 +78,7 @@ protected void getHandler(User user) {
.filter(r -> !r.getTypes().isEmpty())
.collect(toImmutableList()));

consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK);
consoleApiParams.response().setStatus(SC_OK);
consoleApiParams.response().setPayload(gson.toJson(am));
}

Expand Down Expand Up @@ -112,6 +112,6 @@ protected void postHandler(User user) {
}

RegistrarPoc.updateContacts(registrar, updatedContacts);
consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK);
consoleApiParams.response().setStatus(SC_OK);
}
}
Loading

0 comments on commit e88ff77

Please sign in to comment.