Repository: brooklyn-server Updated Branches: refs/heads/master 8e20de068 -> 94c6a3082
populate error field of `ApiError`, and better logging on REST errors Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/4807c626 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/4807c626 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/4807c626 Branch: refs/heads/master Commit: 4807c626ac5d011878f15c50f41ac9464d47355e Parents: cddaf4a Author: Alex Heneveld <[email protected]> Authored: Mon Feb 27 12:48:59 2017 +0000 Committer: Alex Heneveld <[email protected]> Committed: Fri Mar 3 09:06:50 2017 +0000 ---------------------------------------------------------------------- .../apache/brooklyn/rest/domain/ApiError.java | 14 +++++++------ .../rest/resources/ApplicationResource.java | 12 ++++++++++- .../rest/resources/ErrorResponseTest.java | 21 +++++++++++++------- 3 files changed, 33 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4807c626/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java ---------------------------------------------------------------------- diff --git a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java index a13f619..bec524b 100644 --- a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java +++ b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java @@ -30,8 +30,8 @@ import javax.ws.rs.core.Response.Status; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.text.Strings; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.databind.annotation.JsonSerialize; import com.google.common.base.Throwables; public class ApiError implements Serializable { @@ -134,10 +134,10 @@ public class ApiError implements Serializable { private final String message; - @JsonSerialize(include= JsonSerialize.Inclusion.NON_EMPTY) + @JsonInclude(JsonInclude.Include.NON_EMPTY) private final String details; - @JsonSerialize(include= JsonSerialize.Inclusion.NON_NULL) + @JsonInclude(JsonInclude.Include.NON_NULL) private final Integer error; public ApiError(String message) { this(message, null); } @@ -167,10 +167,12 @@ public class ApiError implements Serializable { return asResponse(Status.BAD_REQUEST, MediaType.APPLICATION_JSON_TYPE); } - public Response asResponse(Status defaultStatus, MediaType type) { - return Response.status(error!=null ? error : defaultStatus!=null ? defaultStatus.getStatusCode() : Status.INTERNAL_SERVER_ERROR.getStatusCode()) + public Response asResponse(Status defaultStatusIfNoErrorCodeAlready, MediaType type) { + boolean hasErrorAlready = this.getError()!=null && this.getError()!=0; + int errorCode = hasErrorAlready ? error : defaultStatusIfNoErrorCodeAlready!=null ? defaultStatusIfNoErrorCodeAlready.getStatusCode() : Status.INTERNAL_SERVER_ERROR.getStatusCode(); + return Response.status(errorCode) .type(type) - .entity(this) + .entity(hasErrorAlready ? this: builder().copy(this).errorCode(errorCode).build()) .build(); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4807c626/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java ---------------------------------------------------------------------- diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java index e5f96d6..1ad0f63 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java @@ -275,6 +275,7 @@ public class ApplicationResource extends AbstractBrooklynRestResource implements spec = createEntitySpecForApplication(yaml); } catch (Exception e) { Exceptions.propagateIfFatal(e); + log.warn("Failed REST deployment, could not create spec: "+e); UserFacingException userFacing = Exceptions.getFirstThrowableOfType(e, UserFacingException.class); if (userFacing!=null) { log.debug("Throwing "+userFacing+", wrapped in "+e); @@ -291,6 +292,8 @@ public class ApplicationResource extends AbstractBrooklynRestResource implements try { return launch(yaml, spec); } catch (Exception e) { + Exceptions.propagateIfFatal(e); + log.warn("Failed REST deployment launching "+spec+": "+e); throw WebResourceUtils.badRequest(e, "Error launching blueprint"); } } @@ -366,6 +369,7 @@ public class ApplicationResource extends AbstractBrooklynRestResource implements spec = createEntitySpecForApplication(potentialYaml); } catch (Exception e) { Exceptions.propagateIfFatal(e); + log.warn("Failed REST deployment, could not create spec (autodetecting): "+e); // TODO if not yaml/json - try ZIP, etc @@ -374,7 +378,13 @@ public class ApplicationResource extends AbstractBrooklynRestResource implements if (spec != null) { - return launch(potentialYaml, spec); + try { + return launch(potentialYaml, spec); + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + log.warn("Failed REST deployment launching "+spec+": "+e); + throw WebResourceUtils.badRequest(e, "Error launching blueprint (autodetecting)"); + } } else if (looksLikeLegacy) { throw Exceptions.propagate(legacyFormatException); } else { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4807c626/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/ErrorResponseTest.java ---------------------------------------------------------------------- diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/ErrorResponseTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/ErrorResponseTest.java index 15f3fcf..d1c4f2d 100644 --- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/ErrorResponseTest.java +++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/ErrorResponseTest.java @@ -18,17 +18,17 @@ */ package org.apache.brooklyn.rest.resources; -import com.google.common.collect.ImmutableMap; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertTrue; +import java.io.IOException; +import java.io.InputStream; + import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; -import org.testng.annotations.BeforeClass; -import org.testng.annotations.Test; - import org.apache.brooklyn.rest.domain.ApiError; import org.apache.brooklyn.rest.domain.ApplicationSpec; import org.apache.brooklyn.rest.domain.EntitySpec; @@ -36,9 +36,12 @@ import org.apache.brooklyn.rest.domain.PolicySummary; import org.apache.brooklyn.rest.testing.BrooklynRestResourceTest; import org.apache.brooklyn.rest.testing.mocks.RestMockSimpleEntity; import org.apache.brooklyn.rest.testing.mocks.RestMockSimplePolicy; +import org.testng.Assert; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import javax.ws.rs.core.Response; @Test( // by using a different suite name we disallow interleaving other tests between the methods of this test class, which wrecks the test fixtures suiteName = "ErrorResponseTest") @@ -80,10 +83,12 @@ public class ErrorResponseTest extends BrooklynRestResourceTest { ApiError error = response.readEntity(ApiError.class); assertTrue(error.getMessage().toLowerCase().contains("cannot coerce")); + assertNotNull(error.getError()); + assertEquals(error.getError(), response.getStatus(), Status.BAD_REQUEST.getStatusCode()); } @Test - public void testResponseToWrongMethod() { + public void testResponseToWrongMethod() throws IOException { String resource = "/applications/simple-app/entities/simple-ent/policies/"+policyId+"/config/" + RestMockSimplePolicy.INTEGER_CONFIG.getName() + "/set"; @@ -93,6 +98,8 @@ public class ErrorResponseTest extends BrooklynRestResourceTest { .get(); assertEquals(response.getStatus(), 405); - // Can we assert anything about the content type? + // no input stream; not an API Error + InputStream entity = (InputStream) response.getEntity(); + Assert.assertEquals(entity.read(), -1); } }
