Skip to content
This repository has been archived by the owner on Mar 30, 2023. It is now read-only.

Forward Response Content AND Status #17

Merged
merged 2 commits into from
Sep 8, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/main/java/edu/wisc/my/restproxy/dao/RestProxyDao.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
*/
package edu.wisc.my.restproxy.dao;

import org.springframework.http.ResponseEntity;

import edu.wisc.my.restproxy.ProxyRequestContext;

/**
Expand All @@ -13,9 +15,8 @@
public interface RestProxyDao {

/**
*
* @param proxyRequestContext
* @return the response of the proxied request, serialized to an {@link Object}
* @return the {@link ResponseEntity} of the proxied request.
*/
public Object proxyRequest(ProxyRequestContext proxyRequestContext);
public ResponseEntity<Object> proxyRequest(ProxyRequestContext proxyRequestContext);
}
27 changes: 18 additions & 9 deletions src/main/java/edu/wisc/my/restproxy/dao/RestProxyDaoImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.apache.commons.lang3.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.InitializingBean;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpEntity;
import org.springframework.http.HttpHeaders;
Expand All @@ -21,23 +22,33 @@
/**
* {@link RestProxyDao} implementation backed by a {@link RestTemplate}.
*
* A default instance is provided, but consumers are strongly recommended to configure
* their own instance and inject.
* A default {@link RestTemplate} instance is provided, but consumers are strongly recommended to
* configure their own instance and inject. However, this class will always use
* {@link RestProxyResponseErrorHandler} because it's the client's responsiblity to deal with
* errors.
*
* @author Nicholas Blair
*/
@Service
public class RestProxyDaoImpl implements RestProxyDao {

public class RestProxyDaoImpl implements RestProxyDao, InitializingBean {
@Autowired(required=false)
private RestTemplate restTemplate = new RestTemplate();

/* (non-Javadoc)
* @see org.springframework.beans.factory.InitializingBean#afterPropertiesSet()
*/
@Override
public void afterPropertiesSet() throws Exception {
this.restTemplate.setErrorHandler(new RestProxyResponseErrorHandler());
};

private static final Logger logger = LoggerFactory.getLogger(RestProxyDaoImpl.class);
/* (non-Javadoc)
* @see edu.wisc.my.restproxy.dao.RestProxyDao#proxyRequest(edu.wisc.my.restproxy.ProxyRequestContext)
*/
@Override
public Object proxyRequest(ProxyRequestContext context) {
public ResponseEntity<Object> proxyRequest(ProxyRequestContext context) {
HttpHeaders headers = new HttpHeaders();
if(StringUtils.isNotBlank(context.getUsername()) && null != context.getPassword()) {
StringBuffer credsBuffer = new StringBuffer(context.getUsername());
Expand All @@ -54,12 +65,10 @@ public Object proxyRequest(ProxyRequestContext context) {
}

HttpEntity<Object> request = context.getRequestBody() == null ? new HttpEntity<Object>(headers) : new HttpEntity<Object>(context.getRequestBody().getBody(), headers);

ResponseEntity<Object> response = restTemplate.exchange(context.getUri(),
context.getHttpMethod(), request, Object.class, context.getAttributes());
context.getHttpMethod(), request, Object.class, context.getAttributes());
logger.trace("completed request for {}, response= {}", context, response);
Object responseBody = response.getBody();
return responseBody;
return response;
}

}
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
Expand Up @@ -5,6 +5,8 @@

import javax.servlet.http.HttpServletRequest;

import org.springframework.http.ResponseEntity;

import edu.wisc.my.restproxy.ProxyRequestContext;

/**
Expand All @@ -19,7 +21,7 @@ public interface RestProxyService {
*
* @param resourceKey
* @param request
* @return the Object returned from the REST API, serialized to an {@link Object} (may return null)
* @return the {@link ResponseEntity} returned from the REST API (may return null)
*/
public Object proxyRequest(String resourceKey, HttpServletRequest request);
public ResponseEntity<Object> proxyRequest(String resourceKey, HttpServletRequest request);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.core.env.Environment;
import org.springframework.http.HttpMethod;
import org.springframework.http.MediaType;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Service;
import org.springframework.util.FileCopyUtils;
import org.springframework.web.servlet.HandlerMapping;
Expand Down Expand Up @@ -62,7 +62,7 @@ void setEnv(Environment env) {
* @see KeyUtils#getProxyHeaders(Environment, String, HttpServletRequest)
*/
@Override
public Object proxyRequest(final String resourceKey, final HttpServletRequest request) {
public ResponseEntity<Object> proxyRequest(final String resourceKey, final HttpServletRequest request) {
final String resourceRoot = env.getProperty(resourceKey + ".uri");
if(StringUtils.isBlank(resourceRoot)) {
logger.info("unknown resourceKey {}", resourceKey);
Expand Down Expand Up @@ -95,11 +95,15 @@ public Object proxyRequest(final String resourceKey, final HttpServletRequest re
RequestBody requestBody = null;
try {
InputStream inputStream = request.getInputStream();
final String contentType = request.getHeader(HttpHeaders.CONTENT_TYPE);
if(inputStream != null && MediaType.APPLICATION_JSON_VALUE.equals(contentType)) {
final String contentType = request.getContentType();
int contentLength = request.getContentLength();
Copy link
Contributor

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)

if(inputStream != null && contentLength > 0) {
requestBody = new RequestBody()
.setBody(FileCopyUtils.copyToByteArray(inputStream))
.setContentType(contentType);
.setBody(FileCopyUtils.copyToByteArray(inputStream));

if(StringUtils.isNotBlank(contentType)) {
requestBody.setContentType(contentType);
}

context.setRequestBody(requestBody)
.getHeaders().put(HttpHeaders.CONTENT_TYPE, contentType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.core.env.Environment;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.ResponseBody;
Expand All @@ -20,7 +23,7 @@
*
* @author Nicholas Blair
*/
@RestController
@Controller
public class ResourceProxyController {

@Autowired
Expand All @@ -38,15 +41,19 @@ void setEnv(Environment env) {
*
*/
@RequestMapping("/{key}/**")
public @ResponseBody Object proxyResource(HttpServletRequest request,
public @ResponseBody Object proxyResource(HttpServletRequest request,
HttpServletResponse response,
@PathVariable String key) {
Object result = proxyService.proxyRequest(key, request);
if(result == null) {
response.setStatus(HttpServletResponse.SC_NOT_FOUND);
return null;
} else {
return result;
ResponseEntity<Object> responseEntity = proxyService.proxyRequest(key, request);
if(responseEntity != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider refactoring into a guard clause:

if(responseEntity == null) {
  response.setStatus(HttpStatus.NOT_FOUND.value());
  return null;
}
if (...){
...
}

HttpStatus statusCode = responseEntity.getStatusCode();
response.setStatus(statusCode.value());
if(responseEntity.hasBody()) {
return responseEntity.getBody();
}
}else {
response.setStatus(HttpStatus.NOT_FOUND.value());
}
return null;
}
}
23 changes: 23 additions & 0 deletions src/test/java/edu/wisc/my/restproxy/ValidationResult.java
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
Expand Up @@ -74,9 +74,6 @@ public void proxyRequest_control() {
headers.add("Authorization", "Basic " + base64Creds);

HttpEntity<String> expectedRequest = new HttpEntity<String>(headers);
Object expectedResponseBody = Mockito.mock(Object.class);
Mockito.when(expectedResponse.getBody()).thenReturn(expectedResponseBody);

Mockito.when(
restTemplate.exchange(
Matchers.eq(context.getUri()),
Expand All @@ -95,6 +92,6 @@ public void proxyRequest_control() {
Matchers.eq(Object.class),
Matchers.eq(context.getAttributes())
);
assertEquals(expectedResponseBody, proxyResponse);
assertEquals(expectedResponse, proxyResponse);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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");
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

I call BS on these unit tests... I replaced the whole proxyRequest method with

return proxyDao.proxyRequest(null);

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 verify(proxyDao).proxyRequest(any(RequestContext.class)). To continue unit testing, we'll need equals/hashCode on RequestContext so we can confirm some expected instances are getting past. Perhaps the RequestContext construction should be isolated and tested separately.

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");
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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");
Expand Down
Loading