-
Notifications
You must be signed in to change notification settings - Fork 30
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using 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; | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.