Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PT-694] Do not throw exception for successful created http code #69

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/main/com/sailthru/client/http/SailthruHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ public class SailthruHandler implements ResponseHandler<Object> {

/* Supported HTTP Status codes */
public static final int STATUS_OK = 200;
public static final int STATUS_CREATED = 201;
Copy link
Contributor

@kryptx kryptx May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we didn't also add 204 (this is returned by the DELETE endpoint)? Also the POST endpoint will return 409 if the watch already exists, it seems like that might be useful for a client to know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for the feedback. I was planning to release this incrementally mainly to derisk my changes. But I think its easy for me to include the 202 and 204 code here.
409 and other http codes will still be thrown back to upstream via the default switch case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye, I was thinking the switch case below would just return some kind of generic error, but you're right, they can already pull the 409 status code out of that with no further changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for 202, I cannot see a reason the client should throw if we receive it, but if you still want to follow-up with a refactor proposal then I'd say it could be left out of this PR. We currently will never return 202 from PHP API.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. So just need to add 204 code for the DELETE endpoint.

public static final int STATUS_NO_CONTENT = 204;
public static final int STATUS_BAD_REQUEST = 400;
public static final int STATUS_UNAUTHORIZED = 401;
public static final int STATUS_FORBIDDEN = 403;
Expand All @@ -59,6 +61,8 @@ public Object handleResponse(HttpResponse httpResponse) throws ClientProtocolExc

switch (statusCode) {
case STATUS_OK:
case STATUS_CREATED:
case STATUS_NO_CONTENT:
break;

case STATUS_BAD_REQUEST:
Expand Down
96 changes: 96 additions & 0 deletions src/test/com/sailthru/client/http/SailthruHandlerTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package com.sailthru.client.http;

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import com.google.gson.Gson;
import com.sailthru.client.SailthruUtil;
import com.sailthru.client.exceptions.ApiException;
import com.sailthru.client.handler.JsonHandler;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import org.apache.http.HttpStatus;
import org.apache.http.ProtocolVersion;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.entity.BasicHttpEntity;
import org.apache.http.message.BasicStatusLine;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.junit.MockitoJUnitRunner;

@RunWith(MockitoJUnitRunner.class)
public class SailthruHandlerTest {
private final SailthruHandler handler = new SailthruHandler(new JsonHandler());
private final Gson gson = SailthruUtil.createGson();
private static final String EXAMPLE_RESPONSE = "{\"sample_response\":true}";

@Test
public void testHandlingSuccessfulResponse() throws IOException {
CloseableHttpResponse httpOkResponse = getMockHttpResponseWithStatus(HttpStatus.SC_OK, "OK");
Object okResponse = handler.handleResponse(httpOkResponse);
Assert.assertEquals(EXAMPLE_RESPONSE, gson.toJson(okResponse));

CloseableHttpResponse httpCreatedResponse = getMockHttpResponseWithStatus(HttpStatus.SC_CREATED, "Created");
Object createdResponse = handler.handleResponse(httpCreatedResponse);
Assert.assertEquals(EXAMPLE_RESPONSE, gson.toJson(createdResponse));

CloseableHttpResponse httpNoContentResponse = getMockHttpResponseWithStatus(HttpStatus.SC_NO_CONTENT, "No Content");
Object noContentResponse = handler.handleResponse(httpNoContentResponse);
Assert.assertEquals(EXAMPLE_RESPONSE, gson.toJson(noContentResponse));
}

@Test
public void testHandlingClientErrorResponse() throws IOException {
CloseableHttpResponse httpBadResponse = getMockHttpResponseWithStatus(HttpStatus.SC_BAD_REQUEST, "Bad Request");
try {
handler.handleResponse(httpBadResponse);
Assert.fail("Expected an APIException to be thrown");
} catch (ApiException apiException) {
Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, apiException.getStatusCode());
}

CloseableHttpResponse httpUnauthorizedResponse = getMockHttpResponseWithStatus(HttpStatus.SC_UNAUTHORIZED, "Unauthorized");
try {
handler.handleResponse(httpUnauthorizedResponse);
Assert.fail("Expected an APIException to be thrown");
} catch (ApiException apiException) {
Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, apiException.getStatusCode());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ExceptionRule can help simplify these try-execute-fail-catch-assert patterns. https://junit.org/junit4/javadoc/4.12/org/junit/rules/ExpectedException.html

This looks like two tests in one method. Can you split them apart more? If you were to use ExceptionRule you would want to just test one response at a time.

}

@Test
public void testHandlingServerErrorResponse() throws IOException {
CloseableHttpResponse httpServerErrorResponse = getMockHttpResponseWithStatus(HttpStatus.SC_INTERNAL_SERVER_ERROR, "Internal Server Error");
try {
handler.handleResponse(httpServerErrorResponse);
Assert.fail("Expected an APIException to be thrown");
} catch (ApiException apiException) {
Assert.assertEquals(HttpStatus.SC_INTERNAL_SERVER_ERROR, apiException.getStatusCode());
}

CloseableHttpResponse httpBadGatewayResponse = getMockHttpResponseWithStatus(HttpStatus.SC_BAD_GATEWAY, "Bad Gateway");
try {
handler.handleResponse(httpBadGatewayResponse);
Assert.fail("Expected an APIException to be thrown");
} catch (ApiException apiException) {
Assert.assertEquals(HttpStatus.SC_BAD_GATEWAY, apiException.getStatusCode());
}
}

private CloseableHttpResponse getMockHttpResponseWithStatus(int statusCode, String reasonPhrase) {
CloseableHttpResponse mockHttpResponse = mock(CloseableHttpResponse.class);

BasicHttpEntity fakeEntity = new BasicHttpEntity();
fakeEntity.setContent(new ByteArrayInputStream(EXAMPLE_RESPONSE.getBytes()));
when(mockHttpResponse.getEntity()).thenReturn(fakeEntity);

when(mockHttpResponse.getStatusLine()).thenReturn(new BasicStatusLine(new ProtocolVersion("HTTP", 1, 1), statusCode, reasonPhrase));

when(mockHttpResponse.getFirstHeader("X-Rate-Limit-Limit")).thenReturn(null);
when(mockHttpResponse.getFirstHeader("X-Rate-Limit-Remaining")).thenReturn(null);
when(mockHttpResponse.getFirstHeader("X-Rate-Limit-Reset")).thenReturn(null);

return mockHttpResponse;
}
}