> On Dec. 4, 2013, 1:20 p.m., Stanton Sievers wrote: > > trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2TokenHandler.java, > > line 98 > > <https://reviews.apache.org/r/15958/diff/1/?file=392992#file392992line98> > > > > I know you're simply fixing this code and didn't write it, but I'm not > > a huge fan of the approach used here. It would be much cleaner in this > > case to use a Google common's "Joiner.on(",").join(list);" > > > > That one line could replace this whole method.
Didn't know about Joiner, nice. The null check would still required though, but I'll update the patch in a minute to use that. Btw: I have another issue with the method: the spec is absolutely unclear on which token should be used here for joining, and I'm leaning towards reading the spec as 'space separated', which is the same as the input format. I'm currently waiting for an answer from the OAuth list on that topic, and then will revisit that area to try to pull a bit more of our custom code for generically handling scopes. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15958/#review29722 ----------------------------------------------------------- On Dec. 3, 2013, 11:25 a.m., Andreas Kohn wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15958/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2013, 11:25 a.m.) > > > Review request for shindig. > > > Bugs: SHINDIG-1958 > https://issues.apache.org/jira/browse/SHINDIG-1958 > > > Repository: shindig > > > Description > ------- > > Fix for > > SEVERE: Servlet.service() for servlet [OAuth2Servlet] in context with path > [/api] threw exception [java.lang.StringIndexOutOfBoundsException: String > index out of range: 34] with root cause > java.lang.StringIndexOutOfBoundsException: String index out of range: 34 > at > java.lang.AbstractStringBuilder.deleteCharAt(AbstractStringBuilder.java:762) > at java.lang.StringBuilder.deleteCharAt(StringBuilder.java:258) > at > org.apache.shindig.social.core.oauth2.OAuth2TokenHandler.listToString(OAuth2TokenHandler.java:94) > at > org.apache.shindig.social.core.oauth2.OAuth2TokenHandler.handle(OAuth2TokenHandler.java:73) > at > org.apache.shindig.social.core.oauth2.OAuth2Servlet.doGet(OAuth2Servlet.java:71) > at javax.servlet.http.HttpServlet.service(HttpServlet.java:621) > at javax.servlet.http.HttpServlet.service(HttpServlet.java:728) > > Happens when the OAuth2Code has a non-null/non-empty scope set. > > > Diffs > ----- > > > trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2TokenHandler.java > 1547331 > > trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth2/OAuth2TokenHandlerTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/15958/diff/ > > > Testing > ------- > > * Unit test in the patch. > * Use in our application which uses OAuth scopes > > > Thanks, > > Andreas Kohn > >
