From 16832323d096f6ad45f2593b322d9ba04a1c1496 Mon Sep 17 00:00:00 2001 From: nickfelt Date: Wed, 22 Feb 2017 11:48:38 -0800 Subject: [PATCH] Make ListObjectsAction return 200 when sending JSON error This fixes a bug in the interaction between ListObjectsAction and ListObjectsCommand/AppEngineConnection. ListObjectsAction was returning HTTP status code 400 when it caught an IAE, but also attempting to return a JSON response payload of {"status": "error", "error": ""}. However, AppEngineConnection treats any HTTP error response as more like a crash on the server side - it attempts to scrape the error message out of the autogenerated HTML that AppEngine produces for uncaught exceptions, and throws an exception, killing ListObjectsCommand before it can extract the JSON which contains the nicer error (that stating the missing field, etc versus just "400 Bad Request"). The fix is just to have ListObjectsAction return a 200 and the error message so that ListObjectsCommand can correctly handle it. I also de-scoped the catch to only catching IAE, since catching Exception was overbroad, and the only "expected" exception to be thrown is an IAE from the checkArgument() that tests if the requested fields all exist. Any other kinds of exceptions should actually just bubble up and kill the action, and get the regular AppEngineConnection error treatment. I also added "billingId" as an alias for "billingIdentifier", parallel to clientId/clientIdentifier, since that's why I came across this issue in the first place. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=148248834 --- .../tools/server/ListObjectsAction.java | 18 +++++++++--------- .../tools/server/ListRegistrarsAction.java | 1 + .../tools/server/ListActionTestCase.java | 5 ++--- .../tools/server/ListDomainsActionTest.java | 10 +++------- .../tools/server/ListHostsActionTest.java | 4 +--- .../server/ListPremiumListsActionTest.java | 4 +--- .../tools/server/ListRegistrarsActionTest.java | 4 +--- .../server/ListReservedListsActionTest.java | 4 +--- .../tools/server/ListTldsActionTest.java | 4 +--- 9 files changed, 20 insertions(+), 34 deletions(-) diff --git a/java/google/registry/tools/server/ListObjectsAction.java b/java/google/registry/tools/server/ListObjectsAction.java index 8902e4e3e..904718213 100644 --- a/java/google/registry/tools/server/ListObjectsAction.java +++ b/java/google/registry/tools/server/ListObjectsAction.java @@ -14,9 +14,8 @@ package google.registry.tools.server; +import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; -import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; -import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import com.google.common.base.Function; import com.google.common.base.Functions; @@ -36,6 +35,7 @@ import com.google.common.collect.Ordering; import google.registry.model.ImmutableObject; import google.registry.request.JsonResponse; import google.registry.request.Parameter; +import google.registry.util.FormattingLogger; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -53,6 +53,8 @@ import javax.inject.Inject; */ public abstract class ListObjectsAction implements Runnable { + private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass(); + public static final String FIELDS_PARAM = "fields"; public static final String PRINT_HEADER_ROW_PARAM = "printHeaderRow"; public static final String FULL_FIELD_NAMES_PARAM = "fullFieldNames"; @@ -117,13 +119,11 @@ public abstract class ListObjectsAction implements Ru response.setPayload(ImmutableMap.of( "lines", lines, "status", "success")); - } catch (Exception e) { - String message = e.getMessage(); - if (message == null) { - message = e.getClass().getName(); - } - response.setStatus( - e instanceof IllegalArgumentException ? SC_BAD_REQUEST : SC_INTERNAL_SERVER_ERROR); + } catch (IllegalArgumentException e) { + String message = firstNonNull(e.getMessage(), e.getClass().getName()); + logger.warning(e, message); + // Don't return a non-200 response, since that will cause RegistryTool to barf instead of + // letting ListObjectsCommand parse the JSON response and return a clean error. response.setPayload(ImmutableMap.of( "error", message, "status", "error")); diff --git a/java/google/registry/tools/server/ListRegistrarsAction.java b/java/google/registry/tools/server/ListRegistrarsAction.java index 4cd74ff90..afc9ac302 100644 --- a/java/google/registry/tools/server/ListRegistrarsAction.java +++ b/java/google/registry/tools/server/ListRegistrarsAction.java @@ -45,6 +45,7 @@ public final class ListRegistrarsAction extends ListObjectsAction { @Override public ImmutableBiMap getFieldAliases() { return ImmutableBiMap.of( + "billingId", "billingIdentifier", "clientId", "clientIdentifier", "premiumNames", "blockPremiumNames"); } diff --git a/javatests/google/registry/tools/server/ListActionTestCase.java b/javatests/google/registry/tools/server/ListActionTestCase.java index 75ef4f596..4c719ae73 100644 --- a/javatests/google/registry/tools/server/ListActionTestCase.java +++ b/javatests/google/registry/tools/server/ListActionTestCase.java @@ -78,11 +78,10 @@ public class ListActionTestCase { Optional fields, Optional printHeaderRow, Optional fullFieldNames, - String expectedErrorPattern, - int expectedResponseStatus) { + String expectedErrorPattern) { assertThat(expectedErrorPattern).isNotNull(); runAction(action, fields, printHeaderRow, fullFieldNames); - assertThat(response.getStatus()).isEqualTo(expectedResponseStatus); + assertThat(response.getStatus()).isEqualTo(SC_OK); assertThat(response.getResponseMap().get("status")).isEqualTo("error"); Object obj = response.getResponseMap().get("error"); assertThat(obj).isInstanceOf(String.class); diff --git a/javatests/google/registry/tools/server/ListDomainsActionTest.java b/javatests/google/registry/tools/server/ListDomainsActionTest.java index 4bcf5b753..b5902cc7c 100644 --- a/javatests/google/registry/tools/server/ListDomainsActionTest.java +++ b/javatests/google/registry/tools/server/ListDomainsActionTest.java @@ -17,7 +17,6 @@ package google.registry.tools.server; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.createTlds; import static google.registry.testing.DatastoreHelper.persistActiveDomain; -import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import com.google.common.base.Optional; import com.google.common.collect.ImmutableSet; @@ -51,8 +50,7 @@ public class ListDomainsActionTest extends ListActionTestCase { null, null, null, - "^Must specify TLDs to query$", - SC_BAD_REQUEST); + "^Must specify TLDs to query$"); } @Test @@ -63,8 +61,7 @@ public class ListDomainsActionTest extends ListActionTestCase { null, null, null, - "^TLD %%%badtld%%% does not exist$", - SC_BAD_REQUEST); + "^TLD %%%badtld%%% does not exist$"); } @Test @@ -232,7 +229,6 @@ public class ListDomainsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:", - SC_BAD_REQUEST); + "^Field 'badfield' not found - recognized fields are:"); } } diff --git a/javatests/google/registry/tools/server/ListHostsActionTest.java b/javatests/google/registry/tools/server/ListHostsActionTest.java index 7047e20d8..a4f5e2efd 100644 --- a/javatests/google/registry/tools/server/ListHostsActionTest.java +++ b/javatests/google/registry/tools/server/ListHostsActionTest.java @@ -16,7 +16,6 @@ package google.registry.tools.server; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistActiveHost; -import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import com.google.common.base.Optional; import google.registry.testing.FakeClock; @@ -104,7 +103,6 @@ public class ListHostsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:", - SC_BAD_REQUEST); + "^Field 'badfield' not found - recognized fields are:"); } } diff --git a/javatests/google/registry/tools/server/ListPremiumListsActionTest.java b/javatests/google/registry/tools/server/ListPremiumListsActionTest.java index 34109a5bf..3ebcdaa24 100644 --- a/javatests/google/registry/tools/server/ListPremiumListsActionTest.java +++ b/javatests/google/registry/tools/server/ListPremiumListsActionTest.java @@ -15,7 +15,6 @@ package google.registry.tools.server; import static google.registry.testing.DatastoreHelper.persistPremiumList; -import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import com.google.common.base.Optional; import google.registry.model.registry.label.PremiumList; @@ -87,7 +86,6 @@ public class ListPremiumListsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:", - SC_BAD_REQUEST); + "^Field 'badfield' not found - recognized fields are:"); } } diff --git a/javatests/google/registry/tools/server/ListRegistrarsActionTest.java b/javatests/google/registry/tools/server/ListRegistrarsActionTest.java index 991452a88..352080ce1 100644 --- a/javatests/google/registry/tools/server/ListRegistrarsActionTest.java +++ b/javatests/google/registry/tools/server/ListRegistrarsActionTest.java @@ -16,7 +16,6 @@ package google.registry.tools.server; import static google.registry.testing.DatastoreHelper.createTlds; import static google.registry.testing.DatastoreHelper.persistResource; -import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import com.google.common.base.Optional; import com.google.common.collect.ImmutableSet; @@ -96,7 +95,6 @@ public class ListRegistrarsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:", - SC_BAD_REQUEST); + "^Field 'badfield' not found - recognized fields are:"); } } diff --git a/javatests/google/registry/tools/server/ListReservedListsActionTest.java b/javatests/google/registry/tools/server/ListReservedListsActionTest.java index 9c2c54739..9290feab1 100644 --- a/javatests/google/registry/tools/server/ListReservedListsActionTest.java +++ b/javatests/google/registry/tools/server/ListReservedListsActionTest.java @@ -17,7 +17,6 @@ package google.registry.tools.server; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistReservedList; import static google.registry.testing.DatastoreHelper.persistResource; -import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import com.google.common.base.Optional; import google.registry.model.registry.Registry; @@ -88,7 +87,6 @@ public class ListReservedListsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:", - SC_BAD_REQUEST); + "^Field 'badfield' not found - recognized fields are:"); } } diff --git a/javatests/google/registry/tools/server/ListTldsActionTest.java b/javatests/google/registry/tools/server/ListTldsActionTest.java index e0decbe76..46af004e4 100644 --- a/javatests/google/registry/tools/server/ListTldsActionTest.java +++ b/javatests/google/registry/tools/server/ListTldsActionTest.java @@ -15,7 +15,6 @@ package google.registry.tools.server; import static google.registry.testing.DatastoreHelper.createTld; -import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import com.google.common.base.Optional; import google.registry.testing.FakeClock; @@ -76,7 +75,6 @@ public class ListTldsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:", - SC_BAD_REQUEST); + "^Field 'badfield' not found - recognized fields are:"); } }