Re: JSONP support for /gadgets/metadata

[jo...@gmail.com]

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