You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2017/05/09 13:27:16 UTC
[2/6] brooklyn-client git commit: lets BrooklynApi.getEntity
understand some errors, and add better method
lets BrooklynApi.getEntity understand some errors, and add better method
getEntityOnSuccess which requires a success code from the server.
triggered by observing downstream usages which use getEntity on results
without checking them, and thus have an empty TaskSummary following a severe server error
(the server error is lost and the code typically fails later with no indication of cause)
also tidy deprecations/warnings, toString
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-client/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-client/commit/23fdb122
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-client/tree/23fdb122
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-client/diff/23fdb122
Branch: refs/heads/master
Commit: 23fdb1226a71ddfe304b48bea2d3db81cadf8c7e
Parents: 1203c09
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Mon Feb 27 13:16:29 2017 +0000
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Mon Feb 27 13:16:29 2017 +0000
----------------------------------------------------------------------
.../brooklyn/rest/client/BrooklynApi.java | 125 +++++++++++++---
.../brooklyn/rest/client/BrooklynApiUtil.java | 2 +-
.../util/http/BuiltResponsePreservingError.java | 15 +-
.../rest/client/BrooklynApiGetEntityTest.java | 143 +++++++++++++++++++
4 files changed, 256 insertions(+), 29 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/brooklyn-client/blob/23fdb122/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java
----------------------------------------------------------------------
diff --git a/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java b/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java
index 822e296..466827c 100644
--- a/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java
+++ b/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java
@@ -26,12 +26,32 @@ import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.net.MalformedURLException;
import java.net.URL;
+import java.util.Map;
import javax.annotation.Nullable;
import javax.ws.rs.core.Response;
-import com.google.common.base.Supplier;
-import com.google.common.base.Suppliers;
+import org.apache.brooklyn.rest.api.AccessApi;
+import org.apache.brooklyn.rest.api.ActivityApi;
+import org.apache.brooklyn.rest.api.ApplicationApi;
+import org.apache.brooklyn.rest.api.CatalogApi;
+import org.apache.brooklyn.rest.api.EffectorApi;
+import org.apache.brooklyn.rest.api.EntityApi;
+import org.apache.brooklyn.rest.api.EntityConfigApi;
+import org.apache.brooklyn.rest.api.LocationApi;
+import org.apache.brooklyn.rest.api.PolicyApi;
+import org.apache.brooklyn.rest.api.PolicyConfigApi;
+import org.apache.brooklyn.rest.api.ScriptApi;
+import org.apache.brooklyn.rest.api.SensorApi;
+import org.apache.brooklyn.rest.api.ServerApi;
+import org.apache.brooklyn.rest.api.UsageApi;
+import org.apache.brooklyn.rest.api.VersionApi;
+import org.apache.brooklyn.rest.client.util.http.BuiltResponsePreservingError;
+import org.apache.brooklyn.rest.domain.ApiError;
+import org.apache.brooklyn.util.collections.MutableMap;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.apache.brooklyn.util.javalang.AggregateClassLoader;
+import org.apache.brooklyn.util.net.Urls;
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.Credentials;
import org.apache.http.auth.UsernamePasswordCredentials;
@@ -52,29 +72,10 @@ import org.jboss.resteasy.util.GenericType;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
import com.google.gson.Gson;
-import org.apache.brooklyn.rest.api.AccessApi;
-import org.apache.brooklyn.rest.api.ActivityApi;
-import org.apache.brooklyn.rest.api.ApplicationApi;
-import org.apache.brooklyn.rest.api.CatalogApi;
-import org.apache.brooklyn.rest.api.EffectorApi;
-import org.apache.brooklyn.rest.api.EntityApi;
-import org.apache.brooklyn.rest.api.EntityConfigApi;
-import org.apache.brooklyn.rest.api.LocationApi;
-import org.apache.brooklyn.rest.api.PolicyApi;
-import org.apache.brooklyn.rest.api.PolicyConfigApi;
-import org.apache.brooklyn.rest.api.ScriptApi;
-import org.apache.brooklyn.rest.api.SensorApi;
-import org.apache.brooklyn.rest.api.ServerApi;
-import org.apache.brooklyn.rest.api.UsageApi;
-import org.apache.brooklyn.rest.api.VersionApi;
-import org.apache.brooklyn.rest.client.util.http.BuiltResponsePreservingError;
-import org.apache.brooklyn.util.collections.MutableMap;
-import org.apache.brooklyn.util.exceptions.Exceptions;
-import org.apache.brooklyn.util.javalang.AggregateClassLoader;
-import org.apache.brooklyn.util.net.Urls;
-
import io.swagger.annotations.ApiOperation;
/**
@@ -380,6 +381,14 @@ public class BrooklynApi {
return proxy(AccessApi.class);
}
+ /** Extracts an instance of the given type from the response, including JSON strings in there.
+ * Forgives most errors except for obviously incompatible ones.
+ * To fail on any server error, use {@link #getEntityOnSuccess(Response, Class)}.
+ * <p>
+ * This method will coerce and empty map "{}" to a no-arg contructed instance of the target class.
+ * This method will also ignore most errors in the response.
+ * <p>
+ * It has changed to identify the most obvious errors. */
public static <T> T getEntity(Response response, Class<T> type) {
if (response instanceof ClientResponse) {
ClientResponse<?> clientResponse = (ClientResponse<?>) response;
@@ -387,13 +396,55 @@ public class BrooklynApi {
} else if (response instanceof BuiltResponse) {
// Handle BuiltResponsePreservingError turning objects into Strings
if (response.getEntity() instanceof String && !type.equals(String.class)) {
+ failSomeErrors(response, type, true);
return new Gson().fromJson(response.getEntity().toString(), type);
}
}
// Last-gasp attempt.
return type.cast(response.getEntity());
}
+
+ /** As {@link #getEntity(Response, Class)} but fails if the response is an error of any sort. */
+ public static <T> T getEntityOnSuccess(Response response, Class<T> type) {
+ failSomeErrors(response, type, false);
+ return getEntity(response, type);
+ }
+
+ /** This fails if it is clearly an ApiError response which the caller did not want.
+ * To fail on any error (probably better), use */
+ private static <T> void failSomeErrors(Response response, Class<?> type, boolean onlyIfItLooksLikeApiError) {
+ if (response.getStatus()<400) {
+ // not an error
+ return;
+ }
+ if (onlyIfItLooksLikeApiError && type.isAssignableFrom(ApiError.class) && !Map.class.isAssignableFrom(type)) {
+ // if user wanted a map or an ApiError, don't fail (for legacy compatibility)
+ return;
+ }
+
+ Object obj = new Gson().fromJson(response.getEntity().toString(), Object.class);
+
+ if (onlyIfItLooksLikeApiError && !(obj instanceof Map)) {
+ // only handle maps
+ return;
+ }
+
+ @SuppressWarnings("rawtypes")
+ Map m = (Map)obj;
+ Object error = m.get("error");
+ if (onlyIfItLooksLikeApiError && (error==null || new Integer(0).equals(error) || "".equals(error))) {
+ // error should be non-zero for "ApiError"
+ return;
+ }
+
+ Object message = m.get("message");
+ if (message==null) message = m.get("detail");
+
+ throw new IllegalArgumentException("Server error "+response.getStatus()+" cannot be converted to "+type.getName()+
+ (message!=null ? ": "+message : ""));
+ }
+ /** As {@link #getEntity(Response, Class)} */
public static <T> T getEntity(Response response, GenericType<T> type) {
if (response instanceof ClientResponse) {
ClientResponse<?> clientResponse = (ClientResponse<?>) response;
@@ -401,6 +452,7 @@ public class BrooklynApi {
} else if (response instanceof BuiltResponse) {
// Handle BuiltResponsePreservingError turning objects into Strings
if (response.getEntity() instanceof String) {
+ failSomeErrors(response, type.getType(), true);
return new Gson().fromJson(response.getEntity().toString(), type.getGenericType());
}
}
@@ -408,6 +460,33 @@ public class BrooklynApi {
return type.getType().cast(response.getEntity());
}
+
+ /** As {@link #getEntity(Response, GenericType)} but fails if the response is an error of any sort. */
+ public static <T> T getEntityOnSuccess(Response response, GenericType<T> type) {
+ failSomeErrors(response, type.getType(), false);
+ return getEntity(response, type);
+ }
+
+ /** As {@link #getEntity(Response, Class)} */
+ @SuppressWarnings("unchecked")
+ public static <T> T getEntity(Response response, javax.ws.rs.core.GenericType<T> type) {
+ if (response instanceof BuiltResponse) {
+ // Handle BuiltResponsePreservingError turning objects into Strings
+ if (response.getEntity() instanceof String) {
+ failSomeErrors(response, type.getRawType(), true);
+ return new Gson().fromJson(response.getEntity().toString(), type.getType());
+ }
+ }
+ return (T) getEntity(response, type.getRawType());
+ }
+
+
+ /** As {@link #getEntity(Response, javax.ws.rs.core.GenericType)} but fails if the response is an error of any sort. */
+ public static <T> T getEntityOnSuccess(Response response, javax.ws.rs.core.GenericType<T> type) {
+ failSomeErrors(response, type.getRawType(), false);
+ return getEntity(response, type);
+ }
+
/**
* @deprecated since 0.8.0-incubating. Use {@link #getEntity(Response, GenericType)} instead.
*/
http://git-wip-us.apache.org/repos/asf/brooklyn-client/blob/23fdb122/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiUtil.java
----------------------------------------------------------------------
diff --git a/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiUtil.java b/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiUtil.java
index ce57da5..0412c49 100644
--- a/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiUtil.java
+++ b/java/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiUtil.java
@@ -103,7 +103,7 @@ public class BrooklynApiUtil {
final AtomicReference<Status> appStatus = new AtomicReference<>(Status.UNKNOWN);
final boolean shortcutOnError = !Status.ERROR.equals(desiredStatus) && !Status.UNKNOWN.equals(desiredStatus);
LOG.info("Waiting " + timeout + " from " + new Date() + " for application " + application + " to be " + desiredStatus);
- boolean finalAppStatusKnown = Repeater.create("Waiting for application " + application + " status to be " + desiredStatus)
+ Repeater.create("Waiting for application " + application + " status to be " + desiredStatus)
.every(pollPeriod)
.limitTimeTo(timeout)
.rethrowExceptionImmediately()
http://git-wip-us.apache.org/repos/asf/brooklyn-client/blob/23fdb122/java/src/main/java/org/apache/brooklyn/rest/client/util/http/BuiltResponsePreservingError.java
----------------------------------------------------------------------
diff --git a/java/src/main/java/org/apache/brooklyn/rest/client/util/http/BuiltResponsePreservingError.java b/java/src/main/java/org/apache/brooklyn/rest/client/util/http/BuiltResponsePreservingError.java
index d011172..f8477ca 100644
--- a/java/src/main/java/org/apache/brooklyn/rest/client/util/http/BuiltResponsePreservingError.java
+++ b/java/src/main/java/org/apache/brooklyn/rest/client/util/http/BuiltResponsePreservingError.java
@@ -23,7 +23,6 @@ import java.lang.annotation.Annotation;
import javax.ws.rs.core.Response;
import org.apache.brooklyn.util.exceptions.Exceptions;
-import org.jboss.resteasy.client.core.BaseClientResponse;
import org.jboss.resteasy.core.Headers;
import org.jboss.resteasy.specimpl.BuiltResponse;
@@ -51,8 +50,8 @@ public class BuiltResponsePreservingError extends BuiltResponse {
Object entity = null;
try {
status = source.getStatus();
- if (source instanceof BaseClientResponse)
- headers.putAll(((BaseClientResponse<?>)source).getMetadata());
+ if (source instanceof org.jboss.resteasy.client.core.BaseClientResponse)
+ headers.putAll(((org.jboss.resteasy.client.core.BaseClientResponse<?>)source).getMetadata());
if (source instanceof org.jboss.resteasy.client.ClientResponse) {
entity = ((org.jboss.resteasy.client.ClientResponse<?>)source).getEntity(type);
} else {
@@ -63,8 +62,8 @@ public class BuiltResponsePreservingError extends BuiltResponse {
Exceptions.propagateIfFatal(e);
return new BuiltResponsePreservingError(status, headers, entity, new Annotation[0], e);
} finally {
- if (source instanceof BaseClientResponse)
- ((BaseClientResponse<?>)source).close();
+ if (source instanceof org.jboss.resteasy.client.core.BaseClientResponse)
+ ((org.jboss.resteasy.client.core.BaseClientResponse<?>)source).close();
}
}
@@ -76,4 +75,10 @@ public class BuiltResponsePreservingError extends BuiltResponse {
return super.getEntity();
}
+ @Override
+ public String toString() {
+ return "brooklyn-rest-client:BuiltResponse["+error+":"+getStatus()+
+ (!isClosed() && error==null ? ":"+getEntity() : "")+"]";
+ }
+
}
http://git-wip-us.apache.org/repos/asf/brooklyn-client/blob/23fdb122/java/src/test/java/org/apache/brooklyn/rest/client/BrooklynApiGetEntityTest.java
----------------------------------------------------------------------
diff --git a/java/src/test/java/org/apache/brooklyn/rest/client/BrooklynApiGetEntityTest.java b/java/src/test/java/org/apache/brooklyn/rest/client/BrooklynApiGetEntityTest.java
new file mode 100644
index 0000000..4c5babe
--- /dev/null
+++ b/java/src/test/java/org/apache/brooklyn/rest/client/BrooklynApiGetEntityTest.java
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.rest.client;
+
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.core.Response.Status;
+
+import org.apache.brooklyn.rest.domain.ApiError;
+import org.apache.brooklyn.rest.domain.TaskSummary;
+import org.apache.brooklyn.test.Asserts;
+import org.jboss.resteasy.specimpl.BuiltResponse;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class BrooklynApiGetEntityTest {
+
+ @Test(expectedExceptions=NullPointerException.class)
+ public void testGetEntityDisallowsNull() {
+ Assert.assertNull( BrooklynApi.getEntity(null, TaskSummary.class) );
+ }
+
+ @Test(expectedExceptions=Exception.class)
+ public void testGetEntityFailsOnNonJson() {
+ BrooklynApi.getEntity(
+ new BuiltResponse(200, null, "I'm a string not JSON", null),
+ TaskSummary.class);
+ }
+
+ @Test
+ public void testGetEntityAllowsEmptyMaps() {
+ BrooklynApi.getEntity(
+ new BuiltResponse(200, null, "{}", null),
+ TaskSummary.class);
+ }
+
+ @Test
+ public void testGetEntityIgnoresExtraFields() {
+ BrooklynApi.getEntity(
+ new BuiltResponse(200, null, "{ foo: \"This should cause an error\" }", null),
+ TaskSummary.class);
+ }
+
+ @Test
+ public void testGetEntityAllowsNull() {
+ BrooklynApi.getEntity(
+ new BuiltResponse(200, null, null, null),
+ TaskSummary.class);
+ }
+
+ @Test
+ public void testGetEntityIgnoresErrorResponseCodeInBuiltResponse() {
+ BrooklynApi.getEntity(
+ new BuiltResponse(400, null, "{}", null),
+ TaskSummary.class);
+ }
+
+ @Test
+ public void testGetEntityIgnoresErrorResponseCodeInBasicResponse() {
+ BrooklynApi.getEntity(
+ newBadRequestResponse("{}"),
+ TaskSummary.class);
+ }
+
+ @Test(expectedExceptions=Exception.class)
+ public void testGetEntityFailsIfLooksLikeApiError() {
+ BrooklynApi.getEntity(
+ newBadRequestResponse("{ error: 400 }"),
+ TaskSummary.class);
+ }
+
+ @Test
+ public void testGetEntityFailsWithMessageIfLooksLikeApiErrorWithMessage() {
+ try {
+ BrooklynApi.getEntity(
+ newBadRequestResponse("{ error: 400, message: \"Foo\" }"),
+ TaskSummary.class);
+ Asserts.shouldHaveFailedPreviously();
+ } catch (Exception e) {
+ Asserts.expectedFailureContains(e, "Foo");
+ }
+ }
+
+ @Test
+ public void testGetEntitySucceedsIfLooksLikeApiErrorWhenWantingApiError() {
+ BrooklynApi.getEntity(
+ newBadRequestResponse("{ error: 400 }"),
+ ApiError.class);
+ }
+
+ @Test(expectedExceptions=Exception.class)
+ public void testGetSuccessfulEntityFailsOnAnyError() {
+ BrooklynApi.getEntityOnSuccess(
+ newBadRequestResponse("{ error: 400 }"),
+ ApiError.class);
+ }
+
+ @Test
+ public void testGetEntitySucceedsIfNoErrorCodeWhenApiError() {
+ BrooklynApi.getEntity(
+ newJsonResponse(Status.OK, "{ error: 400 }"),
+ TaskSummary.class);
+ }
+
+ @Test
+ public void testGetSuccessfulEntitySucceedsOnNoErrorApiError() {
+ BrooklynApi.getEntityOnSuccess(
+ newJsonResponse(Status.OK, "{ error: 400 }"),
+ ApiError.class);
+ }
+
+ public Response newBadRequestResponse(Object entity) {
+ return newJsonResponse(Status.BAD_REQUEST, entity);
+ }
+
+ public static Response newJsonResponse(Status status, Object entity) {
+ return newResponse(status, MediaType.APPLICATION_JSON_TYPE, entity);
+ }
+
+ public static Response newResponse(Status status, MediaType type, Object entity) {
+ return Response.status(status)
+ .type(type)
+ .entity(entity)
+ .build();
+ }
+
+}