You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by al...@apache.org on 2014/07/18 15:42:18 UTC
[07/12] git commit: Add a UserFacingException which can be used to
indicate to suppress stack traces,
and tidy up how REST errors are reported and returned
Add a UserFacingException which can be used to indicate to suppress stack traces, and tidy up how REST errors are reported and returned
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/e8f7283c
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/e8f7283c
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/e8f7283c
Branch: refs/heads/master
Commit: e8f7283c20f34a8060400e20cc9d7a5974c96917
Parents: 9d61659
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Thu Jul 17 14:55:32 2014 -0400
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Fri Jul 18 09:15:17 2014 -0400
----------------------------------------------------------------------
usage/cli/src/main/java/brooklyn/cli/Main.java | 4 +-
.../java/brooklyn/rest/domain/ApiError.java | 24 +++++++++
.../brooklyn/rest/resources/UsageResource.java | 5 +-
.../BrooklynPropertiesSecurityFilter.java | 11 ++--
.../rest/util/DefaultExceptionMapper.java | 57 ++++++++++++--------
.../brooklyn/util/exceptions/Exceptions.java | 21 ++++----
.../util/exceptions/FatalRuntimeException.java | 2 +-
.../util/exceptions/UserFacingException.java | 21 ++++++++
8 files changed, 106 insertions(+), 39 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/e8f7283c/usage/cli/src/main/java/brooklyn/cli/Main.java
----------------------------------------------------------------------
diff --git a/usage/cli/src/main/java/brooklyn/cli/Main.java b/usage/cli/src/main/java/brooklyn/cli/Main.java
index ae2696d..601ded2 100644
--- a/usage/cli/src/main/java/brooklyn/cli/Main.java
+++ b/usage/cli/src/main/java/brooklyn/cli/Main.java
@@ -66,6 +66,7 @@ import brooklyn.util.ResourceUtils;
import brooklyn.util.exceptions.Exceptions;
import brooklyn.util.exceptions.FatalConfigurationRuntimeException;
import brooklyn.util.exceptions.FatalRuntimeException;
+import brooklyn.util.exceptions.UserFacingException;
import brooklyn.util.guava.Maybe;
import brooklyn.util.javalang.Enums;
import brooklyn.util.net.Networking;
@@ -763,7 +764,8 @@ public class Main {
} catch (Exception e) { // unexpected error during command execution
log.error("Execution error: " + e.getMessage(), e);
System.err.println("Execution error: " + e.getMessage());
- e.printStackTrace();
+ if (!(e instanceof UserFacingException))
+ e.printStackTrace();
System.exit(EXECUTION_ERROR);
}
}
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/e8f7283c/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java
----------------------------------------------------------------------
diff --git a/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java b/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java
index c293294..dd3dcbc 100644
--- a/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java
+++ b/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java
@@ -20,7 +20,13 @@ package brooklyn.rest.domain;
import static com.google.common.base.Preconditions.checkNotNull;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.core.Response.Status;
+
import org.codehaus.jackson.annotate.JsonProperty;
+import org.codehaus.jackson.map.annotate.JsonSerialize;
+import org.codehaus.jackson.map.annotate.JsonSerialize.Inclusion;
import brooklyn.util.text.Strings;
@@ -76,6 +82,11 @@ public class ApiError {
return this;
}
+ /** as {@link #prefixMessage(String, String)} with default separator of `: ` */
+ public Builder prefixMessage(String prefix) {
+ return prefixMessage(prefix, ": ");
+ }
+
/** puts a prefix in front of the message, with the given separator if there is already a message;
* if there is no message, it simply sets the prefix as the message
*/
@@ -107,6 +118,8 @@ public class ApiError {
}
private final String message;
+
+ @JsonSerialize(include=Inclusion.NON_EMPTY)
private final String details;
public ApiError(
@@ -145,4 +158,15 @@ public class ApiError {
.add("details", details)
.toString();
}
+
+ public Response asBadRequestResponseJson() {
+ return asResponse(Status.BAD_REQUEST, MediaType.APPLICATION_JSON_TYPE);
+ }
+
+ public Response asResponse(Status status, MediaType type) {
+ return Response.status(status)
+ .type(type)
+ .entity(this)
+ .build();
+ }
}
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/e8f7283c/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java
----------------------------------------------------------------------
diff --git a/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java b/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java
index b8a1c78..88dfe3a 100644
--- a/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java
+++ b/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java
@@ -40,6 +40,7 @@ import brooklyn.rest.api.UsageApi;
import brooklyn.rest.domain.UsageStatistic;
import brooklyn.rest.domain.UsageStatistics;
import brooklyn.rest.transform.ApplicationTransformer;
+import brooklyn.util.exceptions.UserFacingException;
import brooklyn.util.time.Time;
import com.google.common.base.Objects;
@@ -249,8 +250,8 @@ public class UsageResource extends AbstractBrooklynRestResource implements Usage
private void checkDates(Date startDate, Date endDate) {
if (startDate.compareTo(endDate) > 0) {
- throw new IllegalArgumentException("Start must be less than or equal to end: " + startDate + " > " + endDate +
- " (" + startDate.getTime() + " > " + endDate.getTime() + ")");
+ throw new UserFacingException(new IllegalArgumentException("Start must be less than or equal to end: " + startDate + " > " + endDate +
+ " (" + startDate.getTime() + " > " + endDate.getTime() + ")"));
}
}
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/e8f7283c/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java
----------------------------------------------------------------------
diff --git a/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java b/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java
index 74ab303..1331e1f 100644
--- a/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java
+++ b/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java
@@ -95,7 +95,12 @@ public class BrooklynPropertiesSecurityFilter implements Filter {
log.debug(" REST req {} parameters: {}", uid, ((HttpServletRequest)request).getParameterMap());
}
if (((HttpServletRequest)request).getContentLength()>0) {
- log.debug(" REST req {} upload content type {}", uid, ((HttpServletRequest)request).getContentType()+" length "+((HttpServletRequest)request).getContentLength());
+ int len = ((HttpServletRequest)request).getContentLength();
+ log.debug(" REST req {} upload content type {}", uid, ((HttpServletRequest)request).getContentType()+" length "+len
+ // would be nice to do this, but the stream can only be read once;
+ // TODO figure out how we could peek at it
+// +(len>0 && len<4096 ? ""+Streams.readFullyString(((HttpServletRequest)request).getInputStream()) : "")
+ );
}
chain.doFilter(request, response);
@@ -107,9 +112,9 @@ public class BrooklynPropertiesSecurityFilter implements Filter {
} catch (Throwable e) {
// NB errors are typically already caught at this point
if (log.isDebugEnabled())
- log.debug("REST` failed processing request "+uri+" with "+entitlementContext+": "+e, e);
+ log.debug("REST failed processing request "+uri+" with "+entitlementContext+": "+e, e);
- log.warn("REST` failed processing request "+uri+" with "+entitlementContext+": "+e, e);
+ log.warn("REST failed processing request "+uri+" with "+entitlementContext+": "+e, e);
throw Exceptions.propagate(e);
} finally {
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/e8f7283c/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java
----------------------------------------------------------------------
diff --git a/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java b/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java
index 0271998..4a458ce 100644
--- a/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java
+++ b/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java
@@ -18,6 +18,8 @@
*/
package brooklyn.rest.util;
+import java.util.Set;
+
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
@@ -29,10 +31,12 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.yaml.snakeyaml.error.YAMLException;
-import com.google.common.base.Optional;
-
+import brooklyn.management.entitlement.Entitlements;
import brooklyn.rest.domain.ApiError;
import brooklyn.rest.domain.ApiError.Builder;
+import brooklyn.util.collections.MutableSet;
+import brooklyn.util.exceptions.Exceptions;
+import brooklyn.util.exceptions.UserFacingException;
import brooklyn.util.flags.ClassCoercionException;
import brooklyn.util.text.Strings;
@@ -41,6 +45,8 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> {
private static final Logger LOG = LoggerFactory.getLogger(DefaultExceptionMapper.class);
+ static Set<Class<?>> warnedUnknownExceptions = MutableSet.of();
+
/**
* Maps a throwable to a response.
* <p/>
@@ -51,38 +57,45 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> {
@Override
public Response toResponse(Throwable throwable) {
+ LOG.debug("REST request running as {} threw: {}", Entitlements.getEntitlementContext(), throwable);
if (LOG.isTraceEnabled()) {
- String message = Optional.fromNullable(throwable.getMessage()).or(throwable.getClass().getName());
- LOG.trace("Request threw: " + message);
+ LOG.trace("Full details of "+Entitlements.getEntitlementContext()+" "+throwable, throwable);
}
+ // Some methods will throw this, which gets converted automatically
if (throwable instanceof WebApplicationException) {
WebApplicationException wae = (WebApplicationException) throwable;
return wae.getResponse();
}
- // Assume ClassCoercionExceptions are caused by TypeCoercions from input parameters gone wrong.
- if (throwable instanceof ClassCoercionException)
- return responseBadRequestJson(ApiError.of(throwable));
+ // The nicest way for methods to provide errors, wrap as this, and the stack trace will be suppressed
+ if (throwable instanceof UserFacingException) {
+ return ApiError.of(throwable.getMessage()).asBadRequestResponseJson();
+ }
- if (throwable instanceof YAMLException)
- return responseBadRequestJson(ApiError.builderFromThrowable(throwable).prefixMessage("Invalid YAML", ": ").build());
+ // For everything else, a trace is supplied
+
+ // Assume ClassCoercionExceptions are caused by TypeCoercions from input parameters gone wrong
+ // And IllegalArgumentException for malformed input parameters.
+ if (throwable instanceof ClassCoercionException || throwable instanceof IllegalArgumentException) {
+ return ApiError.of(throwable).asBadRequestResponseJson();
+ }
+
+ // YAML exception
+ if (throwable instanceof YAMLException) {
+ return ApiError.builder().message(throwable.getMessage()).prefixMessage("Invalid YAML").build().asBadRequestResponseJson();
+ }
+
+ if (!Exceptions.isPrefixBoring(throwable)) {
+ if ( warnedUnknownExceptions.add( throwable.getClass() )) {
+ LOG.info("No exception mapping for " + throwable.getClass() + "; consider adding to "+getClass()+" (future warnings logged at debug)");
+ }
+ }
- LOG.info("No exception mapping for " + throwable.getClass() + ", responding 500", throwable);
Builder rb = ApiError.builderFromThrowable(throwable);
if (Strings.isBlank(rb.getMessage()))
- rb.message("Internal error. Check server logs for details.");
- return Response.status(Status.INTERNAL_SERVER_ERROR)
- .type(MediaType.APPLICATION_JSON)
- .entity(rb.build())
- .build();
- }
-
- private Response responseBadRequestJson(ApiError build) {
- return Response.status(Status.BAD_REQUEST)
- .type(MediaType.APPLICATION_JSON)
- .entity(build)
- .build();
+ rb.message("Internal error. Contact server administrator to consult logs for more details.");
+ return rb.build().asResponse(Status.INTERNAL_SERVER_ERROR, MediaType.APPLICATION_JSON_TYPE);
}
}
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/e8f7283c/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java b/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java
index 9a33566..a233b0c 100644
--- a/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java
+++ b/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java
@@ -27,7 +27,6 @@ import java.util.Collection;
import java.util.List;
import java.util.concurrent.ExecutionException;
-import brooklyn.util.collections.MutableList;
import brooklyn.util.text.Strings;
import com.google.common.base.Predicate;
@@ -38,11 +37,11 @@ import com.google.common.collect.Iterables;
public class Exceptions {
- private static final List<Class<? extends Throwable>> BORING_THROWABLES = ImmutableList.<Class<? extends Throwable>>of(
+ private static final List<Class<? extends Throwable>> BORING_THROWABLE_SUPERTYPES = ImmutableList.<Class<? extends Throwable>>of(
ExecutionException.class, InvocationTargetException.class, PropagatedRuntimeException.class);
private static boolean isBoring(Throwable t) {
- for (Class<? extends Throwable> type: BORING_THROWABLES)
+ for (Class<? extends Throwable> type: BORING_THROWABLE_SUPERTYPES)
if (type.isInstance(t)) return true;
return false;
}
@@ -54,13 +53,15 @@ public class Exceptions {
}
};
- private static List<Class<? extends Throwable>> BORING_PREFIX_THROWABLES = MutableList.copyOf(BORING_THROWABLES)
- .append(IllegalStateException.class).append(RuntimeException.class).append(CompoundRuntimeException.class)
- .toImmutable();
+ private static List<Class<? extends Throwable>> BORING_PREFIX_THROWABLE_EXACT_TYPES = ImmutableList.<Class<? extends Throwable>>of(
+ IllegalStateException.class, RuntimeException.class, CompoundRuntimeException.class);
- private static boolean isPrefixBoring(Throwable t) {
- for (Class<? extends Throwable> type: BORING_PREFIX_THROWABLES)
- if (type.isInstance(t)) return true;
+ /** Returns whether this is throwable either known to be boring or to have an unuseful prefix */
+ public static boolean isPrefixBoring(Throwable t) {
+ if (isBoring(t))
+ return true;
+ for (Class<? extends Throwable> type: BORING_PREFIX_THROWABLE_EXACT_TYPES)
+ if (t.getClass().equals(type)) return true;
return false;
}
@@ -68,7 +69,7 @@ public class Exceptions {
String was;
do {
was = s;
- for (Class<? extends Throwable> type: BORING_PREFIX_THROWABLES) {
+ for (Class<? extends Throwable> type: BORING_PREFIX_THROWABLE_EXACT_TYPES) {
s = Strings.removeAllFromStart(type.getCanonicalName(), type.getName(), type.getSimpleName(), ":", " ");
}
} while (!was.equals(s));
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/e8f7283c/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java b/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java
index 28f7828..3ef9909 100644
--- a/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java
+++ b/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java
@@ -20,7 +20,7 @@ package brooklyn.util.exceptions;
/** Exception indicating a fatal error, typically used in CLI routines.
* The message supplied here should be suitable for display in a CLI response (without stack trace / exception class). */
-public class FatalRuntimeException extends RuntimeException {
+public class FatalRuntimeException extends UserFacingException {
private static final long serialVersionUID = -3359163414517503809L;
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/e8f7283c/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java b/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java
new file mode 100644
index 0000000..1d53d11
--- /dev/null
+++ b/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java
@@ -0,0 +1,21 @@
+package brooklyn.util.exceptions;
+
+/** marker interface, to show that an exception is suitable for pretty-printing to an end-user,
+ * without including a stack trace */
+public class UserFacingException extends RuntimeException {
+
+ private static final long serialVersionUID = 2216885527195571323L;
+
+ public UserFacingException(String message) {
+ super(message);
+ }
+
+ public UserFacingException(Throwable cause) {
+ super(cause.getMessage(), cause);
+ }
+
+ public UserFacingException(String message, Throwable cause) {
+ super(message, cause);
+ }
+
+}