From 72f2652dca861a743103a8f6d8a3fe0cfdf5b00d Mon Sep 17 00:00:00 2001 From: Ignasi Barrera Date: Thu, 25 Feb 2016 12:23:10 +0100 Subject: [PATCH] Properly handler ProfitBricks service errors --- .../ProfitBricksComputeServiceAdapter.java | 13 ++- .../profitbricks/domain/ServiceFault.java | 89 +++++++++++-------- .../ProfitBricksHttpErrorHandler.java | 14 ++- ...FromPayloadHttpCommandExecutorService.java | 11 ++- .../parser/ServiceFaultResponseHandler.java | 27 ++++-- .../ServiceFaultResponseHandlerTest.java | 32 +++++-- .../src/test/resources/fault-500.xml | 9 ++ 7 files changed, 136 insertions(+), 59 deletions(-) create mode 100644 providers/profitbricks/src/test/resources/fault-500.xml diff --git a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/ProfitBricksComputeServiceAdapter.java b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/ProfitBricksComputeServiceAdapter.java index fe02c41589..2b4df3dbde 100644 --- a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/ProfitBricksComputeServiceAdapter.java +++ b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/ProfitBricksComputeServiceAdapter.java @@ -302,11 +302,16 @@ public class ProfitBricksComputeServiceAdapter implements ComputeServiceAdapter< public Provisionable getImage(String id) { // try search images logger.trace("<< searching for image with id=%s", id); - Image image = api.imageApi().getImage(id); - if (image != null) { - logger.trace(">> found image [%s].", image.name()); - return image; + try { + Image image = api.imageApi().getImage(id); + if (image != null) { + logger.trace(">> found image [%s].", image.name()); + return image; + } + } catch (Exception ex) { + logger.warn(ex, ">> unexpected error getting image. Trying to get as a snapshot..."); } + // try search snapshots logger.trace("<< not found from images. searching for snapshot with id=%s", id); Snapshot snapshot = api.snapshotApi().getSnapshot(id); diff --git a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/domain/ServiceFault.java b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/domain/ServiceFault.java index bcf9774ab4..d429981456 100644 --- a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/domain/ServiceFault.java +++ b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/domain/ServiceFault.java @@ -16,55 +16,68 @@ */ package org.jclouds.profitbricks.domain; +import org.jclouds.javax.annotation.Nullable; + import com.google.auto.value.AutoValue; import com.google.common.base.Enums; @AutoValue public abstract class ServiceFault { - public enum FaultCode { - - BAD_REQUEST, - UNEXPECTED, - UNAUTHORIZED, - RESOURCE_NOT_FOUND, - RESOURCE_DELETED, - PROVISIONING_IN_PROCESS, - PROVISIONING_NO_CHANGES, - OVER_LIMIT_SETTING, - SERVER_EXCEED_CAPACITY, - SERVICE_UNAVAILABLE, - UNRECOGNIZED; - - public static FaultCode fromValue(String v) { - return Enums.getIfPresent(FaultCode.class, v).or(UNRECOGNIZED); - } - } - - public abstract FaultCode faultCode(); - - public abstract int httpCode(); - - public abstract String message(); - - public abstract int requestId(); - + public abstract String faultCode(); + public abstract String faultString(); + @Nullable public abstract Details details(); + public static Builder builder() { return new AutoValue_ServiceFault.Builder(); } - + @AutoValue.Builder public abstract static class Builder { - - public abstract Builder faultCode(FaultCode faultCode); - - public abstract Builder httpCode(int httpCode); - - public abstract Builder message(String message); - - public abstract Builder requestId(int requestId); - + public abstract Builder faultCode(String faultCode); + public abstract Builder faultString(String faultString); + public abstract Builder details(Details details); public abstract ServiceFault build(); - + } + + @AutoValue + public abstract static class Details { + + public enum FaultCode { + + BAD_REQUEST, + UNEXPECTED, + UNAUTHORIZED, + RESOURCE_NOT_FOUND, + RESOURCE_DELETED, + PROVISIONING_IN_PROCESS, + PROVISIONING_NO_CHANGES, + OVER_LIMIT_SETTING, + SERVER_EXCEED_CAPACITY, + SERVICE_UNAVAILABLE, + UNRECOGNIZED; + + public static FaultCode fromValue(String v) { + return Enums.getIfPresent(FaultCode.class, v).or(UNRECOGNIZED); + } + } + + public abstract FaultCode faultCode(); + public abstract int httpCode(); + public abstract String message(); + public abstract int requestId(); + + public static Builder builder() { + return new AutoValue_ServiceFault_Details.Builder(); + } + + @AutoValue.Builder + public abstract static class Builder { + public abstract Builder faultCode(FaultCode faultCode); + public abstract Builder httpCode(int httpCode); + public abstract Builder message(String message); + public abstract Builder requestId(int requestId); + public abstract Details build(); + } } } diff --git a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/handlers/ProfitBricksHttpErrorHandler.java b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/handlers/ProfitBricksHttpErrorHandler.java index a4ecb50667..eaf26fa72b 100644 --- a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/handlers/ProfitBricksHttpErrorHandler.java +++ b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/handlers/ProfitBricksHttpErrorHandler.java @@ -16,6 +16,7 @@ */ package org.jclouds.profitbricks.handlers; +import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream; import static org.jclouds.util.Closeables2.closeQuietly; import javax.inject.Singleton; @@ -39,7 +40,15 @@ public class ProfitBricksHttpErrorHandler implements HttpErrorHandler { @Override public void handleError(final HttpCommand command, final HttpResponse response) { - Exception exception = null; + // it is important to always read fully and close streams + byte[] data = closeClientButKeepContentStream(response); + String message = data != null ? new String(data) : null; + + Exception exception = message != null ? new HttpResponseException(command, response, message) + : new HttpResponseException(command, response); + message = message != null ? message : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(), + response.getStatusLine()); + try { switch (response.getStatusCode()) { case 400: @@ -49,6 +58,9 @@ public class ProfitBricksHttpErrorHandler implements HttpErrorHandler { case 401: exception = new AuthorizationException("This request requires authentication.", exception); break; + case 403: + exception = new AuthorizationException(response.getMessage(), exception); + break; case 402: case 409: exception = new IllegalStateException(response.getMessage(), exception); diff --git a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/http/ResponseStatusFromPayloadHttpCommandExecutorService.java b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/http/ResponseStatusFromPayloadHttpCommandExecutorService.java index 00ca238037..795d460398 100644 --- a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/http/ResponseStatusFromPayloadHttpCommandExecutorService.java +++ b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/http/ResponseStatusFromPayloadHttpCommandExecutorService.java @@ -94,10 +94,13 @@ public class ResponseStatusFromPayloadHttpCommandExecutorService extends JavaUrl try { if (isSoapPayload(in)) { ServiceFault fault = faultHandler.parse(in); - if (fault != null) - responseBuilder - .statusCode(fault.httpCode()) - .message(fault.message()); + if (fault != null) { + if (fault.details() != null) { + responseBuilder.statusCode(fault.details().httpCode()).message(fault.details().message()); + } else { + responseBuilder.message(fault.faultString()); + } + } } } catch (Exception ex) { // ignore diff --git a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandler.java b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandler.java index f21c32e632..8b790ed0a1 100644 --- a/providers/profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandler.java +++ b/providers/profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandler.java @@ -17,27 +17,40 @@ package org.jclouds.profitbricks.http.parser; import org.jclouds.profitbricks.domain.ServiceFault; +import org.xml.sax.Attributes; import org.xml.sax.SAXException; public class ServiceFaultResponseHandler extends BaseProfitBricksResponseHandler { private final ServiceFault.Builder builder; + private ServiceFault.Details.Builder detailsBuilder; private boolean done = false; ServiceFaultResponseHandler() { this.builder = ServiceFault.builder(); } + + @Override + public void startElement(String uri, String localName, String qName, Attributes attributes) throws SAXException { + if ("detail".equals(qName)) { + detailsBuilder = ServiceFault.Details.builder(); + } + } @Override protected void setPropertyOnEndTag(String qName) { - if ("faultCode".equals(qName)) - builder.faultCode(ServiceFault.FaultCode.fromValue(textToStringValue())); + if ("faultcode".equals(qName)) + builder.faultCode(textToStringValue()); + else if ("faultstring".equals(qName)) + builder.faultString(textToStringValue()); + else if ("faultCode".equals(qName)) + detailsBuilder.faultCode(ServiceFault.Details.FaultCode.fromValue(textToStringValue())); else if ("httpCode".equals(qName)) - builder.httpCode(textToIntValue()); + detailsBuilder.httpCode(textToIntValue()); else if ("message".equals(qName)) - builder.message(textToStringValue()); + detailsBuilder.message(textToStringValue()); else if ("requestId".equals(qName)) - builder.requestId(textToIntValue()); + detailsBuilder.requestId(textToIntValue()); } @Override @@ -45,8 +58,10 @@ public class ServiceFaultResponseHandler extends BaseProfitBricksResponseHandler if (done) return; setPropertyOnEndTag(qName); - if ("detail".equals(qName)) + if ("S:Fault".equals(qName)) done = true; + if ("detail".equals(qName)) + builder.details(detailsBuilder.build()); clearTextBuffer(); } diff --git a/providers/profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandlerTest.java b/providers/profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandlerTest.java index 2b54dd5820..f8c36f6100 100644 --- a/providers/profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandlerTest.java +++ b/providers/profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandlerTest.java @@ -21,6 +21,7 @@ import static org.testng.Assert.assertNotNull; import org.jclouds.http.functions.ParseSax; import org.jclouds.profitbricks.domain.ServiceFault; +import org.jclouds.profitbricks.domain.ServiceFault.Details; import org.testng.annotations.Test; @Test(groups = "unit", testName = "ServiceFaultResponseHandlerTest") @@ -37,12 +38,31 @@ public class ServiceFaultResponseHandlerTest extends BaseResponseHandlerTest parser = createParser(); + ServiceFault actual = parser.parse(payloadFromResource("/fault-500.xml")); + assertNotNull(actual, "Parsed content returned null"); + + ServiceFault expected = ServiceFault.builder().faultCode("S:Server").faultString("javax.ejb.EJBException") + .build(); assertEquals(expected, actual); } diff --git a/providers/profitbricks/src/test/resources/fault-500.xml b/providers/profitbricks/src/test/resources/fault-500.xml new file mode 100644 index 0000000000..e9964740ec --- /dev/null +++ b/providers/profitbricks/src/test/resources/fault-500.xml @@ -0,0 +1,9 @@ + + + + + S:Server + javax.ejb.EJBException + + + \ No newline at end of file