Review for SHINDIG-624. Thanks to Michael for the patch!
http://codereview.appspot.com/7259/diff/1/3 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java (right): http://codereview.appspot.com/7259/diff/1/3#newcode46 Line 46: static final String GET_REQUEST_CALLBACK_PATTERN = "[A-Za-z\\.]+"; This should be a java.util.regex.Pattern to avoid recompiling it on every match. http://codereview.appspot.com/7259/diff/1/3#newcode61 Line 61: HttpServletResponse response) throws IOException { Same arg indentation comment as below. http://codereview.appspot.com/7259/diff/1/3#newcode67 Line 67: request, GET_REQUEST_REQ_PARAM, null); do all these fit in 100 chars? http://codereview.appspot.com/7259/diff/1/3#newcode121 Line 121: String validPattern) throws Shindig common style is to put as many args as fit in 100 chars on the first line, then wrap w/ 4-char indent. http://codereview.appspot.com/7259/diff/1/3#newcode126 Line 126: String[] values = request.getParameterValues(parameter); request.getParameter() does the values[0] piece for you - using it would simplify the code (and test) significantly http://codereview.appspot.com/7259/diff/1/3#newcode136 Line 136: parameter + "' specified. Expected: " + validPattern); You could just use IllegalArgumentException here and save some code. http://codereview.appspot.com/7259/diff/1/3#newcode139 Line 139: return result; Since this is used for only one argument, it seems to me that you could probably just inline the appropriate code into doGet() http://codereview.appspot.com/7259/diff/1/3#newcode145 Line 145: byte[] body) { same http://codereview.appspot.com/7259/diff/1/2 File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/RpcServletTest.java (right): http://codereview.appspot.com/7259/diff/1/2#newcode48 Line 48: } No need for this ctor. I'd suggest replacing it with helper methods: void expectNormalRequest(String req, String resp) void expectRpcException(RpcException) void expectJsonException(JsonException) ...which reinitialize fields as appropriate. http://codereview.appspot.com/7259/diff/1/2#newcode71 Line 71: private void setupGet(String[] reqParamValues, String[] callbackParamValues) { I'd suggest having this as a helper method that returns an HttpServletRequest rather than relying on it having that side effect. Then each of your tests would look pretty simple: HttpServletRequest req = makeRequestObj(req, callback); handler.set[Expectation(...)] servlet.doGet(req, recorder) assertX() if appropriate, or fail() if exception expected http://codereview.appspot.com/7259
