Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Commit

Permalink
#14 Added builder for exception.
Browse files Browse the repository at this point in the history
  • Loading branch information
axelerod committed Sep 29, 2015
1 parent df4e9fe commit 69ec5d7
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@

public enum ApiCode
{
SUCCESS, VALIDATION_ERROR, AUTHENTICATION_ERROR, AUTHORIZATION_ERROR, RESOURCE_LOCKED, MAX_OPERATIONS_LIMIT_EXCEEDED, GENERAL_ERROR, MAINTENANCE_MODE_ERROR
SUCCESS, VALIDATION_ERROR, AUTHENTICATION_ERROR, AUTHORIZATION_ERROR, RESOURCE_LOCKED, MAX_OPERATIONS_LIMIT_EXCEEDED, GENERAL_ERROR, MAINTENANCE_MODE_ERROR, NETWORK_ERROR

This comment has been minimized.

Copy link
@axelerod

axelerod Sep 30, 2015

Author Contributor

Discussed with @dlyash , enum is bad idea. It will cause exception if server add new code, revert to String

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,51 @@
package com.smartling.api.sdk.exceptions;

// TODO(AShesterov): refactor API-SDK: rename ApiException to SmartlingApiException

import com.smartling.api.sdk.dto.ApiCode;
import org.apache.commons.lang3.StringUtils;

import java.util.ArrayList;
import java.util.List;

/**
* Thrown when an exception has occurred when using the {@link com.smartling.api.sdk.FileApiClientAdapter}.
*/
public class ApiException extends Exception
{
private static final long serialVersionUID = -397098626101615761L;

public ApiException(final String message)
private ApiCode apiCode;
private int httpCode;
private List<String> messages = new ArrayList<>();

ApiException(List<String> messages, final ApiCode apiCode, int httpCode)
{
super(message);
super(StringUtils.join(messages, " ,"));
this.messages = messages;
this.apiCode = apiCode;
this.httpCode = httpCode;
}

public ApiException(final Exception e)
ApiException(final Exception e, ApiCode apiCode)
{
super(e);
messages.add(e.getMessage());
this.apiCode = apiCode;
}

public ApiCode getApiCode()
{
return apiCode;
}

public int getHttpCode()
{
return httpCode;
}

public List<String> getMessages()
{
return messages;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package com.smartling.api.sdk.exceptions;

import com.google.gson.reflect.TypeToken;
import com.smartling.api.sdk.JsonReader;
import com.smartling.api.sdk.dto.ApiCode;
import com.smartling.api.sdk.dto.ApiResponse;
import com.smartling.api.sdk.dto.ApiResponseWrapper;
import com.smartling.api.sdk.dto.EmptyResponse;

import java.io.IOException;
import java.util.List;

public class ApiExceptionBuilder

This comment has been minimized.

Copy link
@PavloMyrotiuk

PavloMyrotiuk Sep 29, 2015

Contributor

that is not builder, in the way we used to think about it, more like a factory. I would just added several constructor to ApiException class.

This comment has been minimized.

Copy link
@dimitrystd

dimitrystd Sep 30, 2015

Contributor

I don't see a reason for this class at all. No sense to wrap simple exception into builder or factory. @dlyash , do you see overengineering here like me?

This comment has been minimized.

Copy link
@axelerod

axelerod Sep 30, 2015

Author Contributor

Agree with Pasha, it is factory. Main reason is extract JSON parsing from exception class and leave it simple and stupid.

{
public ApiException newException(String contents, int httpCode)
{
ApiResponse<EmptyResponse> apiResponse = JsonReader.getApiResponse(contents, new TypeToken<ApiResponseWrapper<EmptyResponse>>()
{
}
);
ApiCode apiCode = apiResponse.getCode();
List<String> messages = apiResponse.getMessages();
return new ApiException(messages, apiCode, httpCode);
}

public ApiException newException(IOException e) {
return new ApiException(e, ApiCode.NETWORK_ERROR);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import com.smartling.api.sdk.ProxyConfiguration;
import com.smartling.api.sdk.dto.file.StringResponse;
import com.smartling.api.sdk.exceptions.ApiException;

import com.smartling.api.sdk.exceptions.ApiExceptionBuilder;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang3.CharEncoding;
import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -85,15 +85,16 @@ public StringResponse executeHttpCall(final HttpRequestBase httpRequest, final P

final String charset = EntityUtils.getContentCharSet(response.getEntity());
final StringResponse stringResponse = inputStreamToString(response.getEntity().getContent(), charset);
if (response.getStatusLine().getStatusCode() == HttpStatus.SC_OK)
int statusCode = response.getStatusLine().getStatusCode();
if (statusCode == HttpStatus.SC_OK)
return stringResponse;

throw new ApiException(stringResponse.getContents());
throw new ApiExceptionBuilder().newException(stringResponse.getContents(), statusCode);
}
catch (final IOException ioe)
{
logger.error(String.format(LOG_MESSAGE_ERROR_TEMPLATE, ioe.getMessage()));
throw new ApiException(ioe);
throw new ApiExceptionBuilder().newException(ioe);
}
finally
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package com.smartling.api.sdk.exceptions;

import com.smartling.api.sdk.dto.ApiCode;
import org.junit.Test;

import java.io.IOException;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.hasItems;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;

public class ApiExceptionBuilderTest
{

public static final String ERROR_RESPONSE = "{\"response\":{\"data\":null,\"code\":\"VALIDATION_ERROR\",\"messages\":[\"apiKey parameter is required\",\"apiVersion parameter is required\"]}}";

private final ApiExceptionBuilder testedInstance = new ApiExceptionBuilder();

@Test
public void shouldReturnExceptionUsingContentsAndHttpCode()
{
ApiException apiException = testedInstance.newException(ERROR_RESPONSE, 0);

assertThat(apiException, instanceOf(ApiException.class));
}

@Test
public void shouldRetrieveCode()
{
ApiException apiException = testedInstance.newException(ERROR_RESPONSE, 0);

assertThat(apiException.getApiCode(), is(equalTo(ApiCode.VALIDATION_ERROR)));
}

@Test
public void shouldRetrieveMessages()
{
ApiException apiException = testedInstance.newException(ERROR_RESPONSE, 0);

assertThat(apiException.getMessages(), hasItems(equalTo("apiKey parameter is required"), equalTo("apiVersion parameter is required")));
}

@Test
public void shouldPassHttpCode()
{
ApiException apiException = testedInstance.newException(ERROR_RESPONSE, 123);

assertThat(apiException.getHttpCode(), is(123));
}

@Test
public void shouldSetNetworkErrorCodeInCaseIoException() {
ApiException apiException = testedInstance.newException(new IOException("Some exception"));

assertThat(apiException.getApiCode(), is(ApiCode.NETWORK_ERROR));
}
}
33 changes: 20 additions & 13 deletions api-sdk/src/test/java/com/smartling/api/sdk/util/HttpUtilsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,9 @@
*/
package com.smartling.api.sdk.util;

import static org.junit.Assert.*;
import static org.mockito.Mockito.*;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;

import com.smartling.api.sdk.ProxyConfiguration;
import com.smartling.api.sdk.dto.file.StringResponse;
import com.smartling.api.sdk.exceptions.ApiException;
import org.apache.http.HttpEntity;
import org.apache.http.HttpStatus;
import org.apache.http.StatusLine;
Expand All @@ -35,9 +30,19 @@
import org.junit.Test;
import org.mockito.ArgumentCaptor;

import com.smartling.api.sdk.ProxyConfiguration;
import com.smartling.api.sdk.dto.file.StringResponse;
import com.smartling.api.sdk.exceptions.ApiException;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

public class HttpUtilsTest
{
Expand All @@ -50,7 +55,9 @@ public class HttpUtilsTest
private HttpEntity httpEntity;
private StatusLine statusLine;

private static final String TEST_RESPONSE = "test response";
private static final String TEST_RESPONSE = "{\"response\":{\"data\":null,\"code\":\"VALIDATION_ERROR\",\"messages\":[\"apiKey parameter is required\"]}}";
private static final String TEST_MESSAGE = "apiKey parameter is required";

private static final String HOST = "host";
private static final String PASSWORD = "password";
private static final String PORT = "5000";
Expand Down Expand Up @@ -107,7 +114,7 @@ public void testExecuteHttpCall404() throws ApiException, ClientProtocolExceptio
}
catch (ApiException e)
{
assertEquals(TEST_RESPONSE, e.getMessage());
assertEquals(TEST_MESSAGE, e.getMessage());
}
}

Expand Down

0 comments on commit 69ec5d7

Please sign in to comment.