Issue 1855044: Refactoring ProxyHandler to also use UriUtils methods (Closed)
|
Created:
15 years, 6 months ago by anupama.dutta Modified:
15 years, 5 months ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk Visibility:
Public. |
DescriptionThis patch involves the following changes:
- ProxyHandler and AccelHandler use similar code for handling the headers, status codes and rewrite-mime-types. This patch refactors ProxyHandler to use the common code placed in UriUtils for all of the above. The disallowed-headers-list has also been consolidated into UriUtils.
- As a side effect of the refactoring, the header names and values will be validated (via the UriUtils method) for the ProxyHandler flow as well.
- A bug in the handling of the "rewrite-mime-type" logic has also been fixed here. Earlier the response mime type was only checked for its prefix match with the rewrite mime type upto the "/" marker and not including the "/" marker - this has been fixed now.
- Tests have been added for the "rewrite-mime-type" handling.
Patch Set 1 #Patch Set 2 : Adding ProxyHandlerTest #
Total comments: 4
Patch Set 3 : Adding UriUtilsTest for rewriteContentType method and fixing a bug in the same method #Patch Set 4 : Merging with submitted changes in head #
Total comments: 4
Patch Set 5 : Addressing Gagan's comments #
Total comments: 2
Patch Set 6 : Addressing TODO comment #
Total comments: 13
Patch Set 7 : Syncing and addressing comments #
Total comments: 2
Patch Set 8 : Syncing again and addressing Gagan's comment #Patch Set 9 : Adding isValidHeaderValue code and also catching IllegalArgumentException #
Total comments: 6
Patch Set 10 : Addressing nitpicks pointed to by Gagan. Fargo/Ziv, please review. #MessagesTotal messages: 35
Adding ProxyHandlerTest
Sign in to reply to this message.
permission denied. Thanks Gagan On Sun, Jul 18, 2010 at 9:23 AM, <anupama.dutta@gmail.com> wrote: > http://codereview.appspot.com/1855044/show >
Sign in to reply to this message.
Changed to Public. Thanks, Anupama. On Sun, Jul 18, 2010 at 9:35 AM, Gagandeep Singh <gagansingh@google.com>wrote: > permission denied. > > Thanks > Gagan > > > > > > On Sun, Jul 18, 2010 at 9:23 AM, <anupama.dutta@gmail.com> wrote: > >> http://codereview.appspot.com/1855044/show >> > >
Sign in to reply to this message.
On 2010/07/18 04:07:01, anupama.dutta wrote: > Changed to Public. > > Thanks, > Anupama. > > On Sun, Jul 18, 2010 at 9:35 AM, Gagandeep Singh <gagansingh@google.com>wrote: > > > permission denied. > > > > Thanks > > Gagan > > > > > > > > > > > > On Sun, Jul 18, 2010 at 9:23 AM, <mailto:anupama.dutta@gmail.com> wrote: > > > >> http://codereview.appspot.com/1855044/show > >> > > > > > overall looks good. let me go through it again to find any possible comments if i can. had i been a committer, i would have committed this right away :)
Sign in to reply to this message.
> > overall looks good. let me go through it again to find any possible > comments if i can. > had i been a committer, i would have committed this right away :) > > > http://codereview.appspot.com/1855044/show > Thanks :) Two specific things that I would like you to double check are: the http status code handling and the rewrite-mime-type handling. I feel there is going to be a small change in both of these because of using the UriUtils method, but I currently think the new behavior is the desirable one.
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/7001/1006 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/7001/1006#newcode49 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:49: CACHING_DIRECTIVES(ImmutableSet.of("vary", "expires", "date", "pragma", i hope no 1 is using this already http://codereview.appspot.com/1855044/diff/7001/1006#newcode141 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:141: public static void rewriteContentType(HttpRequest req, HttpServletResponse response) { this is a pretty complicated function. would be nice to add a test case for this: required = text/* response = text/css required = text/* response = nontext required = text/html response = text/plain and probably a few bad cases (if you want to): required = null response = null with required = text/*
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/7001/1006 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/7001/1006#newcode49 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:49: CACHING_DIRECTIVES(ImmutableSet.of("vary", "expires", "date", "pragma", On 2010/07/18 04:26:16, gagansingh wrote: > i hope no 1 is using this already Atleast in the current codebase, there are no existing references to this. Since this UriUtils class was added very recently, I think we can change this fairly confidently. http://codereview.appspot.com/1855044/diff/7001/1006#newcode141 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:141: public static void rewriteContentType(HttpRequest req, HttpServletResponse response) { On 2010/07/18 04:26:16, gagansingh wrote: > this is a pretty complicated function. would be nice to add a test case for > this: > required = text/* response = text/css > required = text/* response = nontext > required = text/html response = text/plain > > and probably a few bad cases (if you want to): > required = null > response = null with required = text/* Good idea. Will add these and get back to you.
Sign in to reply to this message.
Another test case which exposes a probable bug: required = text/* response = text123/html Thanks Gagan On Sun, Jul 18, 2010 at 10:02 AM, <anupama.dutta@gmail.com> wrote: > > http://codereview.appspot.com/1855044/diff/7001/1006 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java > (right): > > http://codereview.appspot.com/1855044/diff/7001/1006#newcode49 > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:49: > CACHING_DIRECTIVES(ImmutableSet.of("vary", "expires", "date", "pragma", > On 2010/07/18 04:26:16, gagansingh wrote: > >> i hope no 1 is using this already >> > > Atleast in the current codebase, there are no existing references to > this. Since this UriUtils class was added very recently, I think we can > change this fairly confidently. > > > http://codereview.appspot.com/1855044/diff/7001/1006#newcode141 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:141: > public static void rewriteContentType(HttpRequest req, > HttpServletResponse response) { > On 2010/07/18 04:26:16, gagansingh wrote: > >> this is a pretty complicated function. would be nice to add a test >> > case for > >> this: >> required = text/* response = text/css >> required = text/* response = nontext >> required = text/html response = text/plain >> > > and probably a few bad cases (if you want to): >> required = null >> response = null with required = text/* >> > > Good idea. Will add these and get back to you. > > > http://codereview.appspot.com/1855044/show >
Sign in to reply to this message.
Adding UriUtilsTest for rewriteContentType method and fixing a bug in the same method
Sign in to reply to this message.
Merging with submitted changes in head
Sign in to reply to this message.
lgtm http://codereview.appspot.com/1855044/diff/28001/2002 File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java (right): http://codereview.appspot.com/1855044/diff/28001/2002#newcode239 java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:239: .addHeader("Content-Type", "text/html") just add content-length header also and ensure that it is not passed on. http://codereview.appspot.com/1855044/diff/28001/2003 File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java (right): http://codereview.appspot.com/1855044/diff/28001/2003#newcode76 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java:76: verifyMime("", "image/png; charset=UTF-8", "image/png; charset=UTF-8"); just add verifyMime(null, "image/png; charset=UTF-8", "image/png; charset=UTF-8");
Sign in to reply to this message.
Addressing Gagan's comments
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/28001/2002 File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java (right): http://codereview.appspot.com/1855044/diff/28001/2002#newcode239 java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:239: .addHeader("Content-Type", "text/html") On 2010/07/20 06:40:27, gagan.goku wrote: > just add content-length header also and ensure that it is not passed on. Done. http://codereview.appspot.com/1855044/diff/28001/2003 File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java (right): http://codereview.appspot.com/1855044/diff/28001/2003#newcode76 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java:76: verifyMime("", "image/png; charset=UTF-8", "image/png; charset=UTF-8"); On 2010/07/20 06:40:27, gagan.goku wrote: > just add > verifyMime(null, "image/png; charset=UTF-8", "image/png; charset=UTF-8"); Done.
Sign in to reply to this message.
Sign in to reply to this message.
lgtm ++ http://codereview.appspot.com/1855044/diff/38001/39006 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/38001/39006#newcode223 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:223: response.setContentType(requiredType); please add a comment / log message / todo here saying that we are settung the content type to x/*
Sign in to reply to this message.
Addressing TODO comment
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/38001/39006 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/38001/39006#newcode223 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:223: response.setContentType(requiredType); On 2010/07/23 02:03:25, gagan.goku wrote: > please add a comment / log message / todo here saying that we are settung the > content type to x/* > Done.
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/45001/46004 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (left): http://codereview.appspot.com/1855044/diff/45001/46004#oldcode60 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java:60: "vary", "expires", "date", "pragma", "cache-control", "transfer-encoding", "www-authenticate" Please make sure you don't loose this values as part of the change. http://codereview.appspot.com/1855044/diff/45001/46005 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1855044/diff/45001/46005#newcode154 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:154: IOUtils.copy(results.getResponse(), response.getOutputStream()); You probably don't want to copy data in case of error.
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/45001/46005 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1855044/diff/45001/46005#newcode154 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:154: IOUtils.copy(results.getResponse(), response.getOutputStream()); On 2010/07/26 18:40:22, zhoresh wrote: > You probably don't want to copy data in case of error. actually we do. If the original site returns an informative error message, we should send it back to the user shouldnt we ?
Sign in to reply to this message.
Thanks for the review Ziv. http://codereview.appspot.com/1855044/diff/45001/46004 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (left): http://codereview.appspot.com/1855044/diff/45001/46004#oldcode60 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java:60: "vary", "expires", "date", "pragma", "cache-control", "transfer-encoding", "www-authenticate" On 2010/07/26 18:40:22, zhoresh wrote: > Please make sure you don't loose this values as part of the change. We made sure that we covered all of these headers. Anupama had added "etag" and "last-modified" headers to CACHING_DIRECTIVES. I think her latest patch got out of sync somehow. Resync would fix this.
Sign in to reply to this message.
Happy to see logic slowly consolidating. http://codereview.appspot.com/1855044/diff/45001/46005 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1855044/diff/45001/46005#newcode146 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:146: UriUtils.DisallowedHeaders.CLIENT_STATE_DIRECTIVES); These look to me like the right header groups. The full delta, as of this patch revision, is: Previously allowed, now disallowed: "server" (from OUTPUT_TRANSFER_DIRECTIVES) Previously disallowed, now passed through: "etag", "last-modified". Presumably these are the ones you mention are coming in an updated patch#? (CLIENT_STATE_DIRECTIVES additions) http://codereview.appspot.com/1855044/diff/45001/46005#newcode154 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:154: IOUtils.copy(results.getResponse(), response.getOutputStream()); Alas, not so. Per: http://java.sun.com/products/servlet/2.3/javadoc/javax/servlet/http/HttpServl... "After using this method, the response should be considered to be committed and should not be written to." What we really want is: response.sendError(code, results.getResponseAsString()); On 2010/07/26 18:46:46, gagan.goku wrote: > On 2010/07/26 18:40:22, zhoresh wrote: > > You probably don't want to copy data in case of error. > > actually we do. If the original site returns an informative error message, we > should send it back to the user shouldnt we ? http://codereview.appspot.com/1855044/diff/45001/46006 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/45001/46006#newcode216 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:216: public static void rewriteContentType(HttpRequest req, HttpServletResponse response) { nit: I'd prefer this to be maybeRewriteContentType, to reflect the if-condition.
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/45001/46006 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/45001/46006#newcode144 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:144: resp.sendError(HttpResponse.SC_BAD_GATEWAY); do you intend to merge: http://codereview.appspot.com/1811042/diff/54001/55005?context=50&column_widt... (setStatus vs. sendError) in this CL as well? That would accommodate the sendError(...) concern to a large degree. Per (http://blogs.atlassian.com/developer/2007/07/responsesenderror_vs_responses.html) [first resource I was able to find on the topic] sendError differs from setStatus only in that sendError sends a preconfigured error page. setStatus presumes you're going to provide the error context itself. So setStatus seems reasonable for most if not all Shindig use cases I've found.
Sign in to reply to this message.
Syncing and addressing comments
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/45001/46004 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (left): http://codereview.appspot.com/1855044/diff/45001/46004#oldcode60 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java:60: "vary", "expires", "date", "pragma", "cache-control", "transfer-encoding", "www-authenticate" On 2010/07/26 18:56:17, gagan.goku wrote: > On 2010/07/26 18:40:22, zhoresh wrote: > > Please make sure you don't loose this values as part of the change. > > We made sure that we covered all of these headers. Anupama had added "etag" and > "last-modified" headers to CACHING_DIRECTIVES. I think her latest patch got out > of sync somehow. Resync would fix this. Right. Sorry about the confusion. Added these back into UriUtils.java. http://codereview.appspot.com/1855044/diff/45001/46005 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1855044/diff/45001/46005#newcode146 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:146: UriUtils.DisallowedHeaders.CLIENT_STATE_DIRECTIVES); On 2010/07/26 21:25:02, johnfargo wrote: > These look to me like the right header groups. > > The full delta, as of this patch revision, is: > > Previously allowed, now disallowed: "server" (from OUTPUT_TRANSFER_DIRECTIVES) > > Previously disallowed, now passed through: "etag", "last-modified". Presumably > these are the ones you mention are coming in an updated patch#? > (CLIENT_STATE_DIRECTIVES additions) Right. Could you confirm that is it ok to have "server" in the disallowed header list - or was there a specific reason why it was not included in ProxyBase earlier? http://codereview.appspot.com/1855044/diff/45001/46005#newcode154 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:154: IOUtils.copy(results.getResponse(), response.getOutputStream()); On 2010/07/26 21:25:02, johnfargo wrote: > Alas, not so. Per: > http://java.sun.com/products/servlet/2.3/javadoc/javax/servlet/http/HttpServl...) > "After using this method, the response should be considered to be committed and > should not be written to." > > What we really want is: > response.sendError(code, results.getResponseAsString()); I have changed this flow to not use sendError at all and use the copy-to-output-stream approach always. Does this sound fine? http://codereview.appspot.com/1855044/diff/45001/46006 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/45001/46006#newcode144 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:144: resp.sendError(HttpResponse.SC_BAD_GATEWAY); On 2010/07/26 21:58:21, johnfargo wrote: > do you intend to merge: > http://codereview.appspot.com/1811042/diff/54001/55005?context=50&column_widt... > > (setStatus vs. sendError) in this CL as well? That would accommodate the > sendError(...) concern to a large degree. > > Per > (http://blogs.atlassian.com/developer/2007/07/responsesenderror_vs_responses.html) > [first resource I was able to find on the topic] > > sendError differs from setStatus only in that sendError sends a preconfigured > error page. setStatus presumes you're going to provide the error context itself. > So setStatus seems reasonable for most if not all Shindig use cases I've found. Yes. Synced now to get the setStatus change made by Gagan. http://codereview.appspot.com/1855044/diff/45001/46006#newcode216 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:216: public static void rewriteContentType(HttpRequest req, HttpServletResponse response) { On 2010/07/26 21:25:02, johnfargo wrote: > nit: I'd prefer this to be maybeRewriteContentType, to reflect the if-condition. Done.
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/58001/55007 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1855044/diff/58001/55007#newcode69 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:69: @Named("shindig.accelerate.remapInternalServerError") you might not want the naming shindig.accelerate.remapInternalServerError maybe shindig.proxy.remapInternalServerError ? or just assume true as fargo says against clients returning same error message as original site.
Sign in to reply to this message.
Syncing again and addressing Gagan's comment
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/58001/55007 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1855044/diff/58001/55007#newcode69 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:69: @Named("shindig.accelerate.remapInternalServerError") On 2010/07/28 16:40:10, gagan.goku wrote: > you might not want the naming shindig.accelerate.remapInternalServerError > maybe shindig.proxy.remapInternalServerError ? > or just assume true as fargo says against clients returning same error message > as original site. Done.
Sign in to reply to this message.
Adding isValidHeaderValue code and also catching IllegalArgumentException
Sign in to reply to this message.
LGTM++++++++++ ps: very small nitpicks. http://codereview.appspot.com/1855044/diff/67001/63010 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/67001/63010#newcode79 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:79: * NOTE: RFC 822 section 3.1.2 describes the structure of header fields. Would be awesome if you could a 1 line english description of what values are allowed, like "all printable ascii characters other than : are allowed". http://codereview.appspot.com/1855044/diff/67001/63010#newcode101 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:101: * @param val The header value. Would be awesome if you could a 1 line english description of what values are allowed, like "all ascii characters other than CR and LF are allowed". http://codereview.appspot.com/1855044/diff/67001/63010#newcode158 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:158: LOG.info("Skipping illegal header: " + entry.getKey() + ":" + entry.getValue()); LOG.warning ?
Sign in to reply to this message.
Addressing nitpicks pointed to by Gagan. Fargo/Ziv, please review.
Sign in to reply to this message.
http://codereview.appspot.com/1855044/diff/67001/63010 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/67001/63010#newcode79 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:79: * NOTE: RFC 822 section 3.1.2 describes the structure of header fields. On 2010/07/29 17:49:09, gagan.goku wrote: > Would be awesome if you could a 1 line english description of what values are > allowed, like "all printable ascii characters other than : are allowed". Done. http://codereview.appspot.com/1855044/diff/67001/63010#newcode101 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:101: * @param val The header value. On 2010/07/29 17:49:09, gagan.goku wrote: > Would be awesome if you could a 1 line english description of what values are > allowed, like "all ascii characters other than CR and LF are allowed". Done. http://codereview.appspot.com/1855044/diff/67001/63010#newcode158 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:158: LOG.info("Skipping illegal header: " + entry.getKey() + ":" + entry.getValue()); On 2010/07/29 17:49:09, gagan.goku wrote: > LOG.warning ? Done.
Sign in to reply to this message.
LGTM, committed as r981703. On 2010/07/29 18:03:22, anupama.dutta wrote: > http://codereview.appspot.com/1855044/diff/67001/63010 > File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java > (right): > > http://codereview.appspot.com/1855044/diff/67001/63010#newcode79 > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:79: * > NOTE: RFC 822 section 3.1.2 describes the structure of header fields. > On 2010/07/29 17:49:09, gagan.goku wrote: > > Would be awesome if you could a 1 line english description of what values are > > allowed, like "all printable ascii characters other than : are allowed". > > Done. > > http://codereview.appspot.com/1855044/diff/67001/63010#newcode101 > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:101: * > @param val The header value. > On 2010/07/29 17:49:09, gagan.goku wrote: > > Would be awesome if you could a 1 line english description of what values are > > allowed, like "all ascii characters other than CR and LF are allowed". > > Done. > > http://codereview.appspot.com/1855044/diff/67001/63010#newcode158 > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:158: > LOG.info("Skipping illegal header: " + entry.getKey() + ":" + > entry.getValue()); > On 2010/07/29 17:49:09, gagan.goku wrote: > > LOG.warning ? > > Done.
Sign in to reply to this message.
|
