-
Notifications
You must be signed in to change notification settings - Fork 10
Forward Response Content AND Status #17
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,43 @@ | ||
/** | ||
* | ||
*/ | ||
package edu.wisc.my.restproxy.dao; | ||
|
||
import java.io.IOException; | ||
|
||
import org.springframework.http.client.ClientHttpRequest; | ||
import org.springframework.http.client.ClientHttpResponse; | ||
import org.springframework.web.client.ResponseErrorHandler; | ||
|
||
/** | ||
* {@link ResponseErrorHandler} implementation that does nothing and considers every | ||
* {@link ClientHttpRequest} a success. | ||
* | ||
* This class does nothing because RestProxy is responsible soley for relaying requests and | ||
* responses. We don't care what's in the response, we just forward it on. It's up to the client to deal | ||
* with responses, whether they're successes or errors. | ||
* | ||
* @author Collin Cudd | ||
*/ | ||
public class RestProxyResponseErrorHandler implements ResponseErrorHandler { | ||
|
||
/* | ||
* (non-Javadoc) | ||
* | ||
* @see | ||
* org.springframework.web.client.ResponseErrorHandler#handleError(org.springframework.http.client | ||
* .ClientHttpResponse) | ||
*/ | ||
@Override | ||
public void handleError(ClientHttpResponse response) throws IOException { | ||
//no-op | ||
} | ||
|
||
/* (non-Javadoc) | ||
* @see org.springframework.web.client.ResponseErrorHandler#hasError(org.springframework.http.client.ClientHttpResponse) | ||
*/ | ||
@Override | ||
public boolean hasError(ClientHttpResponse response) throws IOException { | ||
return false; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/** | ||
* | ||
*/ | ||
package edu.wisc.my.restproxy; | ||
|
||
/** | ||
* Just an example of an object that could be retruned by rest api. | ||
* @author Collin Cudd | ||
*/ | ||
public class ValidationResult { | ||
|
||
boolean success; | ||
String message; | ||
/** | ||
* @param success | ||
* @param message | ||
*/ | ||
public ValidationResult(boolean success, String message) { | ||
this.success = success; | ||
this.message = message; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,14 @@ | |
import org.mockito.InjectMocks; | ||
import org.mockito.Mock; | ||
import org.mockito.runners.MockitoJUnitRunner; | ||
import org.springframework.http.HttpStatus; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.mock.env.MockEnvironment; | ||
import org.springframework.mock.web.MockHttpServletRequest; | ||
import org.springframework.web.servlet.HandlerMapping; | ||
|
||
import edu.wisc.my.restproxy.ProxyRequestContext; | ||
import edu.wisc.my.restproxy.ValidationResult; | ||
import edu.wisc.my.restproxy.dao.RestProxyDao; | ||
|
||
/** | ||
|
@@ -45,7 +48,7 @@ public void setup() { | |
*/ | ||
@Test | ||
public void proxyRequest_control() { | ||
final Object result = new Object(); | ||
final ResponseEntity<Object> result = new ResponseEntity<Object>(new Object(), HttpStatus.OK); | ||
|
||
MockHttpServletRequest request = new MockHttpServletRequest(); | ||
request.setMethod("GET"); | ||
|
@@ -59,13 +62,34 @@ public void proxyRequest_control() { | |
assertEquals(result, proxy.proxyRequest("control", request)); | ||
} | ||
|
||
/** | ||
* Test simulates a proxy request which fails with a http 400 error. | ||
* An error like this could be encountered if you were to post invalid data to a form. | ||
* Test verifies the HttpStatus AND the body (which likely contins validation error message) are passed back to the client. | ||
*/ | ||
@Test | ||
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. I call BS on these unit tests... I replaced the whole
and the test passes! :P Some other tests in here make underhanded assertion via the mock matchers themselves, which is not better, in my opinion. I wouldn't want to hold up the PR, but I'm making a note to add integration tests on the standalone service, or if possible, to refactor such that this much mocking is not needed. 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. I agree the tests are a bit weak, since they are just doing a I'll second the need for better testing - see issue #18. This gets a 👍 from me, I'll merge and release it. |
||
public void proxyRequest_failsWithBadRequest() { | ||
ValidationResult validationFailure = new ValidationResult(false, "you didn't check the box"); | ||
final ResponseEntity<Object> expectedResult = new ResponseEntity<Object>(validationFailure, HttpStatus.BAD_REQUEST); | ||
MockHttpServletRequest request = new MockHttpServletRequest(); | ||
request.setMethod("POST"); | ||
request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "/control/foo"); | ||
request.setContent("{ \"hello\": \"world\" }".getBytes()); | ||
request.addHeader("Content-Type", "application/json"); | ||
env.setProperty("control.uri", "http://destination"); | ||
when(proxyDao.proxyRequest(any(ProxyRequestContext.class))).thenReturn(expectedResult); | ||
ResponseEntity<Object> result = proxy.proxyRequest("control", request); | ||
assertEquals(expectedResult, result); | ||
assertEquals(validationFailure, result.getBody()); | ||
} | ||
|
||
/** | ||
* Experiment for {@link RestProxyServiceImpl#proxyRequest(String, HttpServletRequest)}, confirms | ||
* expected behavior when the configuration contains credentials. | ||
*/ | ||
@Test | ||
public void proxyRequest_withCredentials() { | ||
final Object result = new Object(); | ||
final ResponseEntity<Object> result = new ResponseEntity<Object>(new Object(), HttpStatus.OK); | ||
|
||
MockHttpServletRequest request = new MockHttpServletRequest(); | ||
request.setMethod("GET"); | ||
|
@@ -85,7 +109,7 @@ public void proxyRequest_withCredentials() { | |
*/ | ||
@Test | ||
public void proxyRequest_withAdditionalHeader() { | ||
final Object result = new Object(); | ||
final ResponseEntity<Object> result = new ResponseEntity<Object>(new Object(), HttpStatus.OK); | ||
|
||
MockHttpServletRequest request = new MockHttpServletRequest(); | ||
request.setAttribute("wiscedupvi", "UW111A111"); | ||
|
@@ -105,7 +129,7 @@ public void proxyRequest_withAdditionalHeader() { | |
*/ | ||
@Test | ||
public void proxyRequest_withAdditionalHeaders_andPlaceholders() { | ||
final Object result = new Object(); | ||
final ResponseEntity<Object> result = new ResponseEntity<Object>(new Object(), HttpStatus.OK); | ||
|
||
MockHttpServletRequest request = new MockHttpServletRequest(); | ||
request.setAttribute("wiscedupvi", "UW111A111"); | ||
|
@@ -125,7 +149,7 @@ public void proxyRequest_withAdditionalHeaders_andPlaceholders() { | |
*/ | ||
@Test | ||
public void proxyRequest_withAdditionalPath() { | ||
final Object result = new Object(); | ||
final ResponseEntity<Object> result = new ResponseEntity<Object>(new Object(), HttpStatus.OK); | ||
|
||
MockHttpServletRequest request = new MockHttpServletRequest(); | ||
request.setMethod("GET"); | ||
|
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.
👍 Looks like it should fix the brittleness around content type (e.g.
application/json;charset=utf-8
)