Issue 1032045: JSON-RPC over JSONP support (Closed)
|
Created:
15 years, 8 months ago by Paul Lindner Modified:
15 years, 8 months ago Reviewers:
johnfargo, shindig.remailer Visibility:
Public. |
Patch Set 1 #
Total comments: 8
Patch Set 2 : Updated jsonp code #
Total comments: 10
MessagesTotal messages: 9
http://codereview.appspot.com/1032045/diff/1/2 File java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java (right): http://codereview.appspot.com/1032045/diff/1/2#newcode123 java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java:123: throw new IllegalArgumentException("Wrong format for parameter 'callback' specified. Must match: " + nit: > 100char http://codereview.appspot.com/1032045/diff/1/4 File java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java (right): http://codereview.appspot.com/1032045/diff/1/4#newcode89 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:89: if (HttpUtil.isJSONP(servletRequest)) { the body of this if-statement should be a helper ie. dispatchJsonp http://codereview.appspot.com/1032045/diff/1/4#newcode95 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:95: if ((content.indexOf('[') != -1) && content.indexOf('[') < content.indexOf('{')) { duplicated elsewhere in the code: should be broken out into at least an isBatch method http://codereview.appspot.com/1032045/diff/1/4#newcode108 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:108: } this whole if/else block is identical to that found in doPost, but with a JSONP wrapper. let's just wrap ServletResponse in a helper class that either does a Jsonp wrap or not -- or break this out into a separate method that takes boolean isJsonp. http://codereview.appspot.com/1032045/diff/1/4#newcode115 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:115: throw new IllegalArgumentException("could not find data to process"); IMO would be preferable not to throw a RuntimeException
Sign in to reply to this message.
new patch coming.. http://codereview.appspot.com/1032045/diff/1/4 File java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java (right): http://codereview.appspot.com/1032045/diff/1/4#newcode95 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:95: if ((content.indexOf('[') != -1) && content.indexOf('[') < content.indexOf('{')) { On 2010/05/01 02:41:06, johnfargo wrote: > duplicated elsewhere in the code: should be broken out into at least an isBatch > method Done. http://codereview.appspot.com/1032045/diff/1/4#newcode108 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:108: } On 2010/05/01 02:41:06, johnfargo wrote: > this whole if/else block is identical to that found in doPost, but with a JSONP > wrapper. let's just wrap ServletResponse in a helper class that either does a > Jsonp wrap or not -- or break this out into a separate method that takes boolean > isJsonp. I combined the doGet/doPost methods into a single one since there's a bunch of other duplicate code too. http://codereview.appspot.com/1032045/diff/1/4#newcode115 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:115: throw new IllegalArgumentException("could not find data to process"); On 2010/05/01 02:41:06, johnfargo wrote: > IMO would be preferable not to throw a RuntimeException Done. Replaced with explicit error return
Sign in to reply to this message.
http://codereview.appspot.com/1032045/diff/6001/7001 File java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java (right): http://codereview.appspot.com/1032045/diff/6001/7001#newcode130 java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java:130: return true; why not just return the matched callback param to encapsulate logic? http://codereview.appspot.com/1032045/diff/6001/7003 File java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java (right): http://codereview.appspot.com/1032045/diff/6001/7003#newcode86 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:86: if (!("GET".equals(method) || "POST".equals(method))) { can't this just be enforced by direct use? http://codereview.appspot.com/1032045/diff/6001/7003#newcode87 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:87: sendError(servletResponse, new ResponseItem(HttpServletResponse.SC_BAD_REQUEST, "Only POST/GET")); still >100 char http://codereview.appspot.com/1032045/diff/6001/7004 File java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java (right): http://codereview.appspot.com/1032045/diff/6001/7004#newcode132 java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java:132: } nit: 1-space off indent http://codereview.appspot.com/1032045/diff/6001/7005 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java (right): http://codereview.appspot.com/1032045/diff/6001/7005#newcode65 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:65: HttpUtil.isJSONP(request); is this just for the side-effect of param validation?
Sign in to reply to this message.
fixed nits, will commit if no objections. http://codereview.appspot.com/1032045/diff/6001/7001 File java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java (right): http://codereview.appspot.com/1032045/diff/6001/7001#newcode130 java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java:130: return true; On 2010/05/03 23:04:54, johnfargo wrote: > why not just return the matched callback param to encapsulate logic? This logic has three states - callback valid, callback invalid, no callback, so just returning the param doesn't cut it. An IAE seems appropriate since malformed callbacks should be relatively rare and should result in a 400 response. http://codereview.appspot.com/1032045/diff/6001/7003 File java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java (right): http://codereview.appspot.com/1032045/diff/6001/7003#newcode86 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:86: if (!("GET".equals(method) || "POST".equals(method))) { On 2010/05/03 23:04:54, johnfargo wrote: > can't this just be enforced by direct use? not sure what you mean here. I combined doGet and doPost here to eliminate duplicate code paths. In doing so we need to reject methods we can't handle. http://codereview.appspot.com/1032045/diff/6001/7003#newcode87 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:87: sendError(servletResponse, new ResponseItem(HttpServletResponse.SC_BAD_REQUEST, "Only POST/GET")); On 2010/05/03 23:04:54, johnfargo wrote: > still >100 char Done. http://codereview.appspot.com/1032045/diff/6001/7004 File java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java (right): http://codereview.appspot.com/1032045/diff/6001/7004#newcode132 java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java:132: } On 2010/05/03 23:04:54, johnfargo wrote: > nit: 1-space off indent Done. http://codereview.appspot.com/1032045/diff/6001/7005 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java (right): http://codereview.appspot.com/1032045/diff/6001/7005#newcode65 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:65: HttpUtil.isJSONP(request); On 2010/05/03 23:04:54, johnfargo wrote: > is this just for the side-effect of param validation? Yes. Comment added to describe this
Sign in to reply to this message.
On Mon, May 3, 2010 at 6:45 PM, <lindner@inuus.com> wrote: > fixed nits, will commit if no objections. > No objections per se, but commentary. > > > > http://codereview.appspot.com/1032045/diff/6001/7001 > File > java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java > (right): > > http://codereview.appspot.com/1032045/diff/6001/7001#newcode130 > > java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java:130: > return true; > On 2010/05/03 23:04:54, johnfargo wrote: > >> why not just return the matched callback param to encapsulate logic? >> > > This logic has three states - callback valid, callback invalid, no > callback, so just returning the param doesn't cut it. An IAE seems > appropriate since malformed callbacks should be relatively rare and > should result in a 400 response. > I assumed callback invalid -> exception, so valid vs. no callback is null vs. String. > > http://codereview.appspot.com/1032045/diff/6001/7003 > File > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java > (right): > > http://codereview.appspot.com/1032045/diff/6001/7003#newcode86 > > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:86: > if (!("GET".equals(method) || "POST".equals(method))) { > On 2010/05/03 23:04:54, johnfargo wrote: > >> can't this just be enforced by direct use? >> > > not sure what you mean here. I combined doGet and doPost here to > eliminate duplicate code paths. In doing so we need to reject methods > we can't handle. > I was advocating for getting rid of the check. If you only call this from doGet and doPost, you're already implicitly checking method. > > http://codereview.appspot.com/1032045/diff/6001/7003#newcode87 > > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:87: > sendError(servletResponse, new > ResponseItem(HttpServletResponse.SC_BAD_REQUEST, "Only POST/GET")); > On 2010/05/03 23:04:54, johnfargo wrote: > >> still >100 char >> > > Done. > > http://codereview.appspot.com/1032045/diff/6001/7004 > File > > java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java > (right): > > http://codereview.appspot.com/1032045/diff/6001/7004#newcode132 > > java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java:132: > } > On 2010/05/03 23:04:54, johnfargo wrote: > >> nit: 1-space off indent >> > > Done. > > http://codereview.appspot.com/1032045/diff/6001/7005 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java > (right): > > http://codereview.appspot.com/1032045/diff/6001/7005#newcode65 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:65: > HttpUtil.isJSONP(request); > On 2010/05/03 23:04:54, johnfargo wrote: > >> is this just for the side-effect of param validation? >> > Yes. Comment added to describe this > > http://codereview.appspot.com/1032045/show >
Sign in to reply to this message.
On Mon, May 3, 2010 at 6:47 PM, John Hjelmstad <johnfargo@gmail.com> wrote: > On Mon, May 3, 2010 at 6:45 PM, <lindner@inuus.com> wrote: > > > fixed nits, will commit if no objections. > > > > No objections per se, but commentary. > > > > > > > > > > http://codereview.appspot.com/1032045/diff/6001/7001 > > File > > java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java > > (right): > > > > http://codereview.appspot.com/1032045/diff/6001/7001#newcode130 > > > > > java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java:130: > > return true; > > On 2010/05/03 23:04:54, johnfargo wrote: > > > >> why not just return the matched callback param to encapsulate logic? > >> > > > > This logic has three states - callback valid, callback invalid, no > > callback, so just returning the param doesn't cut it. An IAE seems > > appropriate since malformed callbacks should be relatively rare and > > should result in a 400 response. > > > > I assumed callback invalid -> exception, so valid vs. no callback is null > vs. String. okay, that seems fine. > > > > > http://codereview.appspot.com/1032045/diff/6001/7003 > > File > > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java > > (right): > > > > http://codereview.appspot.com/1032045/diff/6001/7003#newcode86 > > > > > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:86: > > if (!("GET".equals(method) || "POST".equals(method))) { > > On 2010/05/03 23:04:54, johnfargo wrote: > > > >> can't this just be enforced by direct use? > >> > > > > not sure what you mean here. I combined doGet and doPost here to > > eliminate duplicate code paths. In doing so we need to reject methods > > we can't handle. > > > > I was advocating for getting rid of the check. If you only call this from > doGet and doPost, you're already implicitly checking method. > > I suppose I could rename service() to something like dispatch() and have doGet/doPost call that. Seems a little cluttered imho, but I can make the change. > > > > > http://codereview.appspot.com/1032045/diff/6001/7003#newcode87 > > > > > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:87: > > sendError(servletResponse, new > > ResponseItem(HttpServletResponse.SC_BAD_REQUEST, "Only POST/GET")); > > On 2010/05/03 23:04:54, johnfargo wrote: > > > >> still >100 char > >> > > > > Done. > > > > http://codereview.appspot.com/1032045/diff/6001/7004 > > File > > > > > java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java > > (right): > > > > http://codereview.appspot.com/1032045/diff/6001/7004#newcode132 > > > > > java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java:132: > > } > > On 2010/05/03 23:04:54, johnfargo wrote: > > > >> nit: 1-space off indent > >> > > > > Done. > > > > http://codereview.appspot.com/1032045/diff/6001/7005 > > File > > > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java > > (right): > > > > http://codereview.appspot.com/1032045/diff/6001/7005#newcode65 > > > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:65: > > HttpUtil.isJSONP(request); > > On 2010/05/03 23:04:54, johnfargo wrote: > > > >> is this just for the side-effect of param validation? > >> > > Yes. Comment added to describe this > > > > http://codereview.appspot.com/1032045/show > > >
Sign in to reply to this message.
Your choice; I'll LGTM this either way. On Mon, May 3, 2010 at 6:52 PM, Paul Lindner <lindner@inuus.com> wrote: > On Mon, May 3, 2010 at 6:47 PM, John Hjelmstad <johnfargo@gmail.com>wrote: > >> On Mon, May 3, 2010 at 6:45 PM, <lindner@inuus.com> wrote: >> >> > fixed nits, will commit if no objections. >> > >> >> No objections per se, but commentary. >> >> >> > >> > >> > >> > http://codereview.appspot.com/1032045/diff/6001/7001 >> > File >> > >> java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java >> > (right): >> > >> > http://codereview.appspot.com/1032045/diff/6001/7001#newcode130 >> > >> > >> java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java:130: >> > return true; >> > On 2010/05/03 23:04:54, johnfargo wrote: >> > >> >> why not just return the matched callback param to encapsulate logic? >> >> >> > >> > This logic has three states - callback valid, callback invalid, no >> > callback, so just returning the param doesn't cut it. An IAE seems >> > appropriate since malformed callbacks should be relatively rare and >> > should result in a 400 response. >> > >> >> I assumed callback invalid -> exception, so valid vs. no callback is null >> vs. String. > > > okay, that seems fine. > > >> >> > >> > http://codereview.appspot.com/1032045/diff/6001/7003 >> > File >> > >> java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java >> > (right): >> > >> > http://codereview.appspot.com/1032045/diff/6001/7003#newcode86 >> > >> > >> java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:86: >> > if (!("GET".equals(method) || "POST".equals(method))) { >> > On 2010/05/03 23:04:54, johnfargo wrote: >> > >> >> can't this just be enforced by direct use? >> >> >> > >> > not sure what you mean here. I combined doGet and doPost here to >> > eliminate duplicate code paths. In doing so we need to reject methods >> > we can't handle. >> > >> >> I was advocating for getting rid of the check. If you only call this from >> doGet and doPost, you're already implicitly checking method. >> >> > I suppose I could rename service() to something like dispatch() and have > doGet/doPost call that. Seems a little cluttered imho, but I can make the > change. > > >> >> > >> > http://codereview.appspot.com/1032045/diff/6001/7003#newcode87 >> > >> > >> java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:87: >> > sendError(servletResponse, new >> > ResponseItem(HttpServletResponse.SC_BAD_REQUEST, "Only POST/GET")); >> > On 2010/05/03 23:04:54, johnfargo wrote: >> > >> >> still >100 char >> >> >> > >> > Done. >> > >> > http://codereview.appspot.com/1032045/diff/6001/7004 >> > File >> > >> > >> java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java >> > (right): >> > >> > http://codereview.appspot.com/1032045/diff/6001/7004#newcode132 >> > >> > >> java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java:132: >> > } >> > On 2010/05/03 23:04:54, johnfargo wrote: >> > >> >> nit: 1-space off indent >> >> >> > >> > Done. >> > >> > http://codereview.appspot.com/1032045/diff/6001/7005 >> > File >> > >> > >> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java >> > (right): >> > >> > http://codereview.appspot.com/1032045/diff/6001/7005#newcode65 >> > >> > >> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:65: >> > HttpUtil.isJSONP(request); >> > On 2010/05/03 23:04:54, johnfargo wrote: >> > >> >> is this just for the side-effect of param validation? >> >> >> > Yes. Comment added to describe this >> > >> > http://codereview.appspot.com/1032045/show >> > >> > >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
