From 5f32d1bbeb302f569bed43f548afd91f568d2f6b Mon Sep 17 00:00:00 2001 From: mcilwain Date: Tue, 29 Nov 2016 16:30:19 -0800 Subject: [PATCH] Correctly set HTTP error status codes when list objects fails This makes the associated nomulus tool commands correctly return error exit codes when the server-side component fails. This improves scriptability. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=140543216 --- .../registry/tools/server/ListObjectsAction.java | 4 ++++ .../registry/tools/server/ListActionTestCase.java | 8 +++++--- .../registry/tools/server/ListDomainsActionTest.java | 10 +++++++--- .../registry/tools/server/ListHostsActionTest.java | 4 +++- .../tools/server/ListPremiumListsActionTest.java | 4 +++- .../tools/server/ListRegistrarsActionTest.java | 4 +++- .../tools/server/ListReservedListsActionTest.java | 4 +++- .../registry/tools/server/ListTldsActionTest.java | 4 +++- 8 files changed, 31 insertions(+), 11 deletions(-) diff --git a/java/google/registry/tools/server/ListObjectsAction.java b/java/google/registry/tools/server/ListObjectsAction.java index 2dfacbefc..9d6012928 100644 --- a/java/google/registry/tools/server/ListObjectsAction.java +++ b/java/google/registry/tools/server/ListObjectsAction.java @@ -15,6 +15,8 @@ package google.registry.tools.server; 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; @@ -120,6 +122,8 @@ public abstract class ListObjectsAction implements Ru if (message == null) { message = e.getClass().getName(); } + response.setStatus( + e instanceof IllegalArgumentException ? SC_BAD_REQUEST : SC_INTERNAL_SERVER_ERROR); response.setPayload(ImmutableMap.of( "error", message, "status", "error")); diff --git a/javatests/google/registry/tools/server/ListActionTestCase.java b/javatests/google/registry/tools/server/ListActionTestCase.java index 36471c041..f85951883 100644 --- a/javatests/google/registry/tools/server/ListActionTestCase.java +++ b/javatests/google/registry/tools/server/ListActionTestCase.java @@ -40,7 +40,7 @@ public class ListActionTestCase { private FakeJsonResponse response; - void runAction( + private void runAction( ListObjectsAction action, Optional fields, Optional printHeaderRow, @@ -51,7 +51,6 @@ public class ListActionTestCase { action.printHeaderRow = printHeaderRow; action.fullFieldNames = fullFieldNames; action.run(); - assertThat(response.getStatus()).isEqualTo(SC_OK); } void testRunSuccess( @@ -62,6 +61,7 @@ public class ListActionTestCase { String ... expectedLinePatterns) { assertThat(expectedLinePatterns).isNotNull(); runAction(action, fields, printHeaderRow, fullFieldNames); + assertThat(response.getStatus()).isEqualTo(SC_OK); assertThat(response.getResponseMap().get("status")).isEqualTo("success"); Object obj = response.getResponseMap().get("lines"); assertThat(obj).isInstanceOf(List.class); @@ -78,9 +78,11 @@ public class ListActionTestCase { Optional fields, Optional printHeaderRow, Optional fullFieldNames, - String expectedErrorPattern) { + String expectedErrorPattern, + int expectedResponseStatus) { assertThat(expectedErrorPattern).isNotNull(); runAction(action, fields, printHeaderRow, fullFieldNames); + assertThat(response.getStatus()).isEqualTo(expectedResponseStatus); 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 51ba81922..aa5da0cc1 100644 --- a/javatests/google/registry/tools/server/ListDomainsActionTest.java +++ b/javatests/google/registry/tools/server/ListDomainsActionTest.java @@ -17,6 +17,7 @@ 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; @@ -50,7 +51,8 @@ public class ListDomainsActionTest extends ListActionTestCase { null, null, null, - "^Must specify TLDs to query$"); + "^Must specify TLDs to query$", + SC_BAD_REQUEST); } @Test @@ -61,7 +63,8 @@ public class ListDomainsActionTest extends ListActionTestCase { null, null, null, - "^TLD %%%badtld%%% does not exist$"); + "^TLD %%%badtld%%% does not exist$", + SC_BAD_REQUEST); } @Test @@ -229,6 +232,7 @@ public class ListDomainsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:"); + "^Field 'badfield' not found - recognized fields are:", + SC_BAD_REQUEST); } } diff --git a/javatests/google/registry/tools/server/ListHostsActionTest.java b/javatests/google/registry/tools/server/ListHostsActionTest.java index d70e60cdd..8145f6b99 100644 --- a/javatests/google/registry/tools/server/ListHostsActionTest.java +++ b/javatests/google/registry/tools/server/ListHostsActionTest.java @@ -16,6 +16,7 @@ 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; @@ -103,6 +104,7 @@ public class ListHostsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:"); + "^Field 'badfield' not found - recognized fields are:", + SC_BAD_REQUEST); } } diff --git a/javatests/google/registry/tools/server/ListPremiumListsActionTest.java b/javatests/google/registry/tools/server/ListPremiumListsActionTest.java index dccd9fb7c..29e4e7f48 100644 --- a/javatests/google/registry/tools/server/ListPremiumListsActionTest.java +++ b/javatests/google/registry/tools/server/ListPremiumListsActionTest.java @@ -15,6 +15,7 @@ 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; @@ -86,6 +87,7 @@ public class ListPremiumListsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:"); + "^Field 'badfield' not found - recognized fields are:", + SC_BAD_REQUEST); } } diff --git a/javatests/google/registry/tools/server/ListRegistrarsActionTest.java b/javatests/google/registry/tools/server/ListRegistrarsActionTest.java index 2ca146073..a96ca1199 100644 --- a/javatests/google/registry/tools/server/ListRegistrarsActionTest.java +++ b/javatests/google/registry/tools/server/ListRegistrarsActionTest.java @@ -16,6 +16,7 @@ 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; @@ -95,6 +96,7 @@ public class ListRegistrarsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:"); + "^Field 'badfield' not found - recognized fields are:", + SC_BAD_REQUEST); } } diff --git a/javatests/google/registry/tools/server/ListReservedListsActionTest.java b/javatests/google/registry/tools/server/ListReservedListsActionTest.java index 4ae295799..0ff5c41a2 100644 --- a/javatests/google/registry/tools/server/ListReservedListsActionTest.java +++ b/javatests/google/registry/tools/server/ListReservedListsActionTest.java @@ -17,6 +17,7 @@ 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; @@ -87,6 +88,7 @@ public class ListReservedListsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:"); + "^Field 'badfield' not found - recognized fields are:", + SC_BAD_REQUEST); } } diff --git a/javatests/google/registry/tools/server/ListTldsActionTest.java b/javatests/google/registry/tools/server/ListTldsActionTest.java index f8fcaa97e..680d872b7 100644 --- a/javatests/google/registry/tools/server/ListTldsActionTest.java +++ b/javatests/google/registry/tools/server/ListTldsActionTest.java @@ -15,6 +15,7 @@ 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; @@ -75,6 +76,7 @@ public class ListTldsActionTest extends ListActionTestCase { Optional.of("badfield"), null, null, - "^Field 'badfield' not found - recognized fields are:"); + "^Field 'badfield' not found - recognized fields are:", + SC_BAD_REQUEST); } }