Issue 1696059: Catching recoverable errors in ProxyHandler and returning original results (Closed)
|
Created:
15 years, 5 months ago by gagan.goku Modified:
15 years, 5 months ago CC:
cool-shindig-committers_googlegroups.com, anupama.dutta Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionProxy servlet should not throw INTERNAL_SERVER_ERRORS whenever there is a rewriting exception.
If it is possible to return the original content and the RETURN_ORIGINAL_CONTENT_ON_ERROR is set as "true", ProxyHandler should return the original content.
To reproduce the exception, do the following:
1) Override ParseModule to use CajaHtmlParser instead of Neko.
2) Proxy a web page which caja fails on, so for example.org, load:
gadgets/proxy?container=default&gadget=test&url=http%3A%2F%2Fwww.example.org%2F
ProxyHandler then throws INTERNAL_SERVER_ERROR:
org.apache.shindig.gadgets.rewrite.RewritingException: content.getDocument is null. Content: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<!-- server: 155 mcn: in:/ci/content/current/site/index.html hostname: wci033, country: in, cluster: ind, created: 2010-08-03 14:51:45 -->
<html xmlns="http://www.w3.org/1999/xhtml" xmlns:og="http://opengraphprotocol.org/schema/" xmlns:fb="http://developers.example.com/schema/" >
<head>
<title>Example.org </title>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8" />
.....
Patch Set 1 #Patch Set 2 : catching_recoverable_errors #
Total comments: 8
Patch Set 3 : 'adding_test' #Patch Set 4 : 'adding_test' #
Total comments: 2
Patch Set 5 : updating_patch #Patch Set 6 : adding_anupamas_test #Patch Set 7 : not_returning_original_content_if_sanitize_request #Patch Set 8 : not_returning_original_content_if_sanitize_request #Patch Set 9 : reverting_clearing_headers_change #Patch Set 10 : uploading_after_syncing #Patch Set 11 : adding_return_original_content_on_error #Patch Set 12 : svn_up #Patch Set 13 : updating_test #Patch Set 14 : updating_test #
Total comments: 10
Patch Set 15 : addressing_anupamas_comments #Patch Set 16 : adding_returnOriginalContentOnError_to_more_places #
MessagesTotal messages: 30
http://codereview.appspot.com/1696059/diff/2001/3001 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1696059/diff/2001/3001#newcode141 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: LOG.log(Level.FINE, "Caught RewritingException when proxying: " "WARNING" would be better? http://codereview.appspot.com/1696059/diff/2001/3001#newcode169 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:169: * is recoverable. The rationale behind it is that errors should be thrown The rationale behind it is ... -> The rationale here is ... http://codereview.appspot.com/1696059/diff/2001/3001#newcode174 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:174: * @param results The result of rewriting. The result of rewriting -> The rewritten HttpResponse. http://codereview.appspot.com/1696059/diff/2001/3001#newcode178 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:178: return !(StringUtils.isEmpty(results.getResponseAsString()) && Could you add a test that exposes this problem?
Sign in to reply to this message.
http://codereview.appspot.com/1696059/diff/2001/3001 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1696059/diff/2001/3001#newcode141 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: LOG.log(Level.FINE, "Caught RewritingException when proxying: " On 2010/08/03 17:21:13, anupama.dutta wrote: > "WARNING" would be better? Done. http://codereview.appspot.com/1696059/diff/2001/3001#newcode169 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:169: * is recoverable. The rationale behind it is that errors should be thrown On 2010/08/03 17:21:13, anupama.dutta wrote: > The rationale behind it is ... -> The rationale here is ... Done. http://codereview.appspot.com/1696059/diff/2001/3001#newcode174 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:174: * @param results The result of rewriting. On 2010/08/03 17:21:13, anupama.dutta wrote: > The result of rewriting -> The rewritten HttpResponse. Done. http://codereview.appspot.com/1696059/diff/2001/3001#newcode178 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:178: return !(StringUtils.isEmpty(results.getResponseAsString()) && On 2010/08/03 17:21:13, anupama.dutta wrote: > Could you add a test that exposes this problem? Done.
Sign in to reply to this message.
http://codereview.appspot.com/1696059/diff/9001/10002 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1696059/diff/9001/10002#newcode141 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: if (!isRecoverable(results)) { HttpResponse is immutable, so if we get here we'll always have the original-retrieved content. In such case, why would isRecoverable depend on there being content (and no headers)?
Sign in to reply to this message.
http://codereview.appspot.com/1696059/diff/9001/10002 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1696059/diff/9001/10002#newcode141 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: if (!isRecoverable(results)) { On 2010/08/03 21:50:26, johnfargo wrote: > HttpResponse is immutable, so if we get here we'll always have the > original-retrieved content. In such case, why would isRecoverable depend on > there being content (and no headers)? Hi John The rationale here is that if we have either content or headers, we want to return these to the user so that the user can make more sense out of it. Only in the case when there is no content and no headers, we should treat it as a grave error and return an INTERNAL_SERVER_ERROR WDYT ?
Sign in to reply to this message.
On Tue, Aug 3, 2010 at 11:29 PM, <gagan.goku@gmail.com> wrote: > > http://codereview.appspot.com/1696059/diff/9001/10002 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java > (right): > > http://codereview.appspot.com/1696059/diff/9001/10002#newcode141 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: > if (!isRecoverable(results)) { > On 2010/08/03 21:50:26, johnfargo wrote: > >> HttpResponse is immutable, so if we get here we'll always have the >> original-retrieved content. In such case, why would isRecoverable >> > depend on > >> there being content (and no headers)? >> > > Hi John > > The rationale here is that if we have either content or headers, we want > to return these to the user so that the user can make more sense out of > it. > Only in the case when there is no content and no headers, we should > treat it as a grave error and return an INTERNAL_SERVER_ERROR > > WDYT ? The first half seems quite reasonable to me. But in the case of the second piece -- no content, no headers -- I wonder what the status code would be in the first place. May as well proxy that along verbatim too? --j > > > http://codereview.appspot.com/1696059/show >
Sign in to reply to this message.
On Wed, Aug 4, 2010 at 1:32 PM, John Hjelmstad <johnfargo@gmail.com> wrote: > On Tue, Aug 3, 2010 at 11:29 PM, <gagan.goku@gmail.com> wrote: > >> >> http://codereview.appspot.com/1696059/diff/9001/10002 >> File >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java >> (right): >> >> http://codereview.appspot.com/1696059/diff/9001/10002#newcode141 >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: >> if (!isRecoverable(results)) { >> On 2010/08/03 21:50:26, johnfargo wrote: >> >>> HttpResponse is immutable, so if we get here we'll always have the >>> original-retrieved content. In such case, why would isRecoverable >>> >> depend on >> >>> there being content (and no headers)? >>> >> >> Hi John >> >> The rationale here is that if we have either content or headers, we want >> to return these to the user so that the user can make more sense out of >> it. >> Only in the case when there is no content and no headers, we should >> treat it as a grave error and return an INTERNAL_SERVER_ERROR >> >> WDYT ? > > > The first half seems quite reasonable to me. But in the case of the second > piece -- no content, no headers -- I wonder what the status code would be in > the first place. May as well proxy that along verbatim too? > We have not seen any cases with no content and no headers yet. It might be better if we continue to return INTERNAL_SERVER_ERROR so that if we ever see such a page in the future, it is easier to track. Once we encounter such a page, we can decide how to appropriately handle it. > --j > > >> >> >> http://codereview.appspot.com/1696059/show >> > >
Sign in to reply to this message.
SGTM. Want me to complete the review or hold off for updates? On Wed, Aug 4, 2010 at 3:17 AM, Gagandeep Singh <gagan.goku@gmail.com>wrote: > On Wed, Aug 4, 2010 at 1:32 PM, John Hjelmstad <johnfargo@gmail.com>wrote: > >> On Tue, Aug 3, 2010 at 11:29 PM, <gagan.goku@gmail.com> wrote: >> >>> >>> http://codereview.appspot.com/1696059/diff/9001/10002 >>> File >>> >>> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java >>> (right): >>> >>> http://codereview.appspot.com/1696059/diff/9001/10002#newcode141 >>> >>> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: >>> if (!isRecoverable(results)) { >>> On 2010/08/03 21:50:26, johnfargo wrote: >>> >>>> HttpResponse is immutable, so if we get here we'll always have the >>>> original-retrieved content. In such case, why would isRecoverable >>>> >>> depend on >>> >>>> there being content (and no headers)? >>>> >>> >>> Hi John >>> >>> The rationale here is that if we have either content or headers, we want >>> to return these to the user so that the user can make more sense out of >>> it. >>> Only in the case when there is no content and no headers, we should >>> treat it as a grave error and return an INTERNAL_SERVER_ERROR >>> >>> WDYT ? >> >> >> The first half seems quite reasonable to me. But in the case of the second >> piece -- no content, no headers -- I wonder what the status code would be in >> the first place. May as well proxy that along verbatim too? >> > > We have not seen any cases with no content and no headers yet. It might be > better if we continue to return INTERNAL_SERVER_ERROR so that if we ever see > such a page in the future, it is easier to track. Once we encounter such a > page, we can decide how to appropriately handle it. > > >> --j >> >> >>> >>> >>> http://codereview.appspot.com/1696059/show >>> >> >> >
Sign in to reply to this message.
On Thu, Aug 5, 2010 at 3:32 AM, John Hjelmstad <johnfargo@gmail.com> wrote: > SGTM. Want me to complete the review or hold off for updates? I can't think of any other updates for now, so please continue the review :) . Do you have something else in mind ? > > On Wed, Aug 4, 2010 at 3:17 AM, Gagandeep Singh <gagan.goku@gmail.com>wrote: > >> On Wed, Aug 4, 2010 at 1:32 PM, John Hjelmstad <johnfargo@gmail.com>wrote: >> >>> On Tue, Aug 3, 2010 at 11:29 PM, <gagan.goku@gmail.com> wrote: >>> >>>> >>>> http://codereview.appspot.com/1696059/diff/9001/10002 >>>> File >>>> >>>> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java >>>> (right): >>>> >>>> http://codereview.appspot.com/1696059/diff/9001/10002#newcode141 >>>> >>>> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: >>>> if (!isRecoverable(results)) { >>>> On 2010/08/03 21:50:26, johnfargo wrote: >>>> >>>>> HttpResponse is immutable, so if we get here we'll always have the >>>>> original-retrieved content. In such case, why would isRecoverable >>>>> >>>> depend on >>>> >>>>> there being content (and no headers)? >>>>> >>>> >>>> Hi John >>>> >>>> The rationale here is that if we have either content or headers, we want >>>> to return these to the user so that the user can make more sense out of >>>> it. >>>> Only in the case when there is no content and no headers, we should >>>> treat it as a grave error and return an INTERNAL_SERVER_ERROR >>>> >>>> WDYT ? >>> >>> >>> The first half seems quite reasonable to me. But in the case of the >>> second piece -- no content, no headers -- I wonder what the status code >>> would be in the first place. May as well proxy that along verbatim too? >>> >> >> We have not seen any cases with no content and no headers yet. It might be >> better if we continue to return INTERNAL_SERVER_ERROR so that if we ever see >> such a page in the future, it is easier to track. Once we encounter such a >> page, we can decide how to appropriately handle it. >> >> >>> --j >>> >>> >>>> >>>> >>>> http://codereview.appspot.com/1696059/show >>>> >>> >>> >> >
Sign in to reply to this message.
Sounds good to me -- could you resolve this against current @head? I'd be happy to commit. On 2010/08/05 05:57:47, gagan.goku wrote: > On Thu, Aug 5, 2010 at 3:32 AM, John Hjelmstad <mailto:johnfargo@gmail.com> wrote: > > > SGTM. Want me to complete the review or hold off for updates? > > > I can't think of any other updates for now, so please continue the review > :) . Do you have something else in mind ? > > > > > > On Wed, Aug 4, 2010 at 3:17 AM, Gagandeep Singh <gagan.goku@gmail.com>wrote: > > > >> On Wed, Aug 4, 2010 at 1:32 PM, John Hjelmstad <johnfargo@gmail.com>wrote: > >> > >>> On Tue, Aug 3, 2010 at 11:29 PM, <mailto:gagan.goku@gmail.com> wrote: > >>> > >>>> > >>>> http://codereview.appspot.com/1696059/diff/9001/10002 > >>>> File > >>>> > >>>> > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java > >>>> (right): > >>>> > >>>> http://codereview.appspot.com/1696059/diff/9001/10002#newcode141 > >>>> > >>>> > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:141: > >>>> if (!isRecoverable(results)) { > >>>> On 2010/08/03 21:50:26, johnfargo wrote: > >>>> > >>>>> HttpResponse is immutable, so if we get here we'll always have the > >>>>> original-retrieved content. In such case, why would isRecoverable > >>>>> > >>>> depend on > >>>> > >>>>> there being content (and no headers)? > >>>>> > >>>> > >>>> Hi John > >>>> > >>>> The rationale here is that if we have either content or headers, we want > >>>> to return these to the user so that the user can make more sense out of > >>>> it. > >>>> Only in the case when there is no content and no headers, we should > >>>> treat it as a grave error and return an INTERNAL_SERVER_ERROR > >>>> > >>>> WDYT ? > >>> > >>> > >>> The first half seems quite reasonable to me. But in the case of the > >>> second piece -- no content, no headers -- I wonder what the status code > >>> would be in the first place. May as well proxy that along verbatim too? > >>> > >> > >> We have not seen any cases with no content and no headers yet. It might be > >> better if we continue to return INTERNAL_SERVER_ERROR so that if we ever see > >> such a page in the future, it is easier to track. Once we encounter such a > >> page, we can decide how to appropriately handle it. > >> > >> > >>> --j > >>> > >>> > >>>> > >>>> > >>>> http://codereview.appspot.com/1696059/show > >>>> > >>> > >>> > >> > > >
Sign in to reply to this message.
The original content shouldn't be returned if the "sanitize" parameter is set to 1 since the original content was intended to be sanitized.
Sign in to reply to this message.
On 2010/08/06 05:43:43, chirag wrote: > The original content shouldn't be returned if the "sanitize" parameter is set to > 1 since the original content was intended to be sanitized. Done :)
Sign in to reply to this message.
@Chirag, that's a great point. While it's easy to work around this particular issue, this does call into question the wisdom of breaking the assumption that rewriters will operate over provided content. Cajoled content will meet this criterion, and frankly image proxying could as well -- users may be using the service explicitly to guarantee exploit-free imagery. Thoughts? On Fri, Aug 6, 2010 at 6:55 AM, <gagan.goku@gmail.com> wrote: > On 2010/08/06 05:43:43, chirag wrote: > >> The original content shouldn't be returned if the "sanitize" parameter >> > is set to > >> 1 since the original content was intended to be sanitized. >> > > Done :) > > > http://codereview.appspot.com/1696059/show >
Sign in to reply to this message.
Gagan, thoughts on my latest response? There are several situations in which users of the proxy might reasonably be expecting that original content is _not_ returned. The examples listed are cajoled content and imagery (resized, optimized, sanitized, etc). On 2010/08/06 18:16:28, johnfargo wrote: > @Chirag, that's a great point. While it's easy to work around this > particular issue, this does call into question the wisdom of breaking the > assumption that rewriters will operate over provided content. Cajoled > content will meet this criterion, and frankly image proxying could as well > -- users may be using the service explicitly to guarantee exploit-free > imagery. > > Thoughts? > > On Fri, Aug 6, 2010 at 6:55 AM, <mailto:gagan.goku@gmail.com> wrote: > > > On 2010/08/06 05:43:43, chirag wrote: > > > >> The original content shouldn't be returned if the "sanitize" parameter > >> > > is set to > > > >> 1 since the original content was intended to be sanitized. > >> > > > > Done :) > > > > > > http://codereview.appspot.com/1696059/show > > >
Sign in to reply to this message.
I agree. I've been operating under the assumption that proxied content can only be mutated through the rewriter chain. The new CajaResponseRewriter operates under this assumption (http://codereview.appspot.com/1847052/show). On Fri, Aug 6, 2010 at 11:16 AM, John Hjelmstad <johnfargo@gmail.com> wrote: > @Chirag, that's a great point. While it's easy to work around this > particular issue, this does call into question the wisdom of breaking the > assumption that rewriters will operate over provided content. Cajoled > content will meet this criterion, and frankly image proxying could as well > -- users may be using the service explicitly to guarantee exploit-free > imagery. > Thoughts? > > On Fri, Aug 6, 2010 at 6:55 AM, <gagan.goku@gmail.com> wrote: >> >> On 2010/08/06 05:43:43, chirag wrote: >>> >>> The original content shouldn't be returned if the "sanitize" parameter >> >> is set to >>> >>> 1 since the original content was intended to be sanitized. >> >> Done :) >> >> http://codereview.appspot.com/1696059/show > >
Sign in to reply to this message.
@John I didn't realize you were asking me for comments :) On Wed, Aug 11, 2010 at 11:48 PM, <johnfargo@gmail.com> wrote: > Gagan, thoughts on my latest response? There are several situations in > which users of the proxy might reasonably be expecting that original > content is _not_ returned. The examples listed are cajoled content and > imagery (resized, optimized, sanitized, etc). Yes, if people are relying on the fact that if there is some weirdness in the original content then ProxyServlet will throw an internal server error, then its probably better to keep the existing behavior around. One case where we have seen this behavior is the sanitize request param as pointed out by Chirag. However, this leaves us with only one option. In order to maintain existing behavior, lets add another request param say returnOriginalIfError. This will default to false for everyone other than for requests that explicitly set it to true. And we put the isRecovereable check only if returnOriginalIfError is true. Slowly, people who want to return original content in case of errors will start setting it to true. What do you think ? Thanks Gagan
Sign in to reply to this message.
Soudns like a quite reasonable strategy to me. Feel free to attach this behavior to ContainerConfig (for an overridable default) as well. On Thu, Aug 12, 2010 at 12:55 AM, Gagandeep singh <gagan.goku@gmail.com>wrote: > @John > > I didn't realize you were asking me for comments :) > > On Wed, Aug 11, 2010 at 11:48 PM, <johnfargo@gmail.com> wrote: > > Gagan, thoughts on my latest response? There are several situations in >> which users of the proxy might reasonably be expecting that original >> content is _not_ returned. The examples listed are cajoled content and >> imagery (resized, optimized, sanitized, etc). > > > Yes, if people are relying on the fact that if there is some weirdness in > the original content then ProxyServlet will throw an internal server error, > then its probably better to keep the existing behavior around. One case > where we have seen this behavior is the sanitize request param as pointed > out by Chirag. > > However, this leaves us with only one option. In order to maintain existing > behavior, lets add another request param say returnOriginalIfError. This > will default to false for everyone other than for requests that explicitly > set it to true. And we put the isRecovereable check only > if returnOriginalIfError is true. > Slowly, people who want to return original content in case of errors will > start setting it to true. > > What do you think ? > > Thanks > Gagan >
Sign in to reply to this message.
PS. Pre-nit: I'd recommend a short queryparam name, since our URLs are getting long enough as-is. Anyone who cares to keep this behavior will know what to add anyway. On Thu, Aug 12, 2010 at 12:57 AM, John Hjelmstad <johnfargo@gmail.com>wrote: > Soudns like a quite reasonable strategy to me. Feel free to attach this > behavior to ContainerConfig (for an overridable default) as well. > > > On Thu, Aug 12, 2010 at 12:55 AM, Gagandeep singh <gagan.goku@gmail.com>wrote: > >> @John >> >> I didn't realize you were asking me for comments :) >> >> On Wed, Aug 11, 2010 at 11:48 PM, <johnfargo@gmail.com> wrote: >> >> Gagan, thoughts on my latest response? There are several situations in >>> which users of the proxy might reasonably be expecting that original >>> content is _not_ returned. The examples listed are cajoled content and >>> imagery (resized, optimized, sanitized, etc). >> >> >> Yes, if people are relying on the fact that if there is some weirdness in >> the original content then ProxyServlet will throw an internal server error, >> then its probably better to keep the existing behavior around. One case >> where we have seen this behavior is the sanitize request param as pointed >> out by Chirag. >> >> However, this leaves us with only one option. In order to maintain >> existing behavior, lets add another request param say returnOriginalIfError. >> This will default to false for everyone other than for requests that >> explicitly set it to true. And we put the isRecovereable check only >> if returnOriginalIfError is true. >> Slowly, people who want to return original content in case of errors will >> start setting it to true. >> >> What do you think ? >> >> Thanks >> Gagan >> > >
Sign in to reply to this message.
@John I didn't realize you were asking me for comments :) On Wed, Aug 11, 2010 at 11:48 PM, <johnfargo@gmail.com> wrote: > Gagan, thoughts on my latest response? There are several situations in > which users of the proxy might reasonably be expecting that original > content is _not_ returned. The examples listed are cajoled content and > imagery (resized, optimized, sanitized, etc). Yes, if people are relying on the fact that if there is some weirdness in the original content then ProxyServlet will throw an internal server error, then its probably better to keep the existing behavior around. One case where we have seen this behavior is the sanitize request param as pointed out by Chirag. However, this leaves us with only one option. In order to maintain existing behavior, lets add another request param say returnOriginalIfError. This will default to false for everyone other than for requests that explicitly set it to true. And we put the isRecovereable check only if returnOriginalIfError is true. Slowly, people who want to return original content in case of errors will start setting it to true. What do you think ? Thanks Gagan
Sign in to reply to this message.
Cool. Let me add the http request param in this change and make another cl for container config. On Thu, Aug 12, 2010 at 1:28 PM, John Hjelmstad <johnfargo@gmail.com> wrote: > PS. Pre-nit: I'd recommend a short queryparam name, since our URLs are > getting long enough as-is. Anyone who cares to keep this behavior will know > what to add anyway. > > > On Thu, Aug 12, 2010 at 12:57 AM, John Hjelmstad <johnfargo@gmail.com>wrote: > >> Soudns like a quite reasonable strategy to me. Feel free to attach this >> behavior to ContainerConfig (for an overridable default) as well. >> >> >> On Thu, Aug 12, 2010 at 12:55 AM, Gagandeep singh <gagan.goku@gmail.com>wrote: >> >>> @John >>> >>> I didn't realize you were asking me for comments :) >>> >>> On Wed, Aug 11, 2010 at 11:48 PM, <johnfargo@gmail.com> wrote: >>> >>> Gagan, thoughts on my latest response? There are several situations in >>>> which users of the proxy might reasonably be expecting that original >>>> content is _not_ returned. The examples listed are cajoled content and >>>> imagery (resized, optimized, sanitized, etc). >>> >>> >>> Yes, if people are relying on the fact that if there is some weirdness in >>> the original content then ProxyServlet will throw an internal server error, >>> then its probably better to keep the existing behavior around. One case >>> where we have seen this behavior is the sanitize request param as pointed >>> out by Chirag. >>> >>> However, this leaves us with only one option. In order to maintain >>> existing behavior, lets add another request param say returnOriginalIfError. >>> This will default to false for everyone other than for requests that >>> explicitly set it to true. And we put the isRecovereable check only >>> if returnOriginalIfError is true. >>> Slowly, people who want to return original content in case of errors will >>> start setting it to true. >>> >>> What do you think ? >>> >>> Thanks >>> Gagan >>> >> >> >
Sign in to reply to this message.
Cool. Let me add the http request param in this change and make another cl for container config. On Thu, Aug 12, 2010 at 1:28 PM, John Hjelmstad <johnfargo@gmail.com> wrote: > PS. Pre-nit: I'd recommend a short queryparam name, since our URLs are > getting long enough as-is. Anyone who cares to keep this behavior will know > what to add anyway. > > > On Thu, Aug 12, 2010 at 12:57 AM, John Hjelmstad <johnfargo@gmail.com>wrote: > >> Soudns like a quite reasonable strategy to me. Feel free to attach this >> behavior to ContainerConfig (for an overridable default) as well. >> >> >> On Thu, Aug 12, 2010 at 12:55 AM, Gagandeep singh <gagan.goku@gmail.com>wrote: >> >>> @John >>> >>> I didn't realize you were asking me for comments :) >>> >>> On Wed, Aug 11, 2010 at 11:48 PM, <johnfargo@gmail.com> wrote: >>> >>> Gagan, thoughts on my latest response? There are several situations in >>>> which users of the proxy might reasonably be expecting that original >>>> content is _not_ returned. The examples listed are cajoled content and >>>> imagery (resized, optimized, sanitized, etc). >>> >>> >>> Yes, if people are relying on the fact that if there is some weirdness in >>> the original content then ProxyServlet will throw an internal server error, >>> then its probably better to keep the existing behavior around. One case >>> where we have seen this behavior is the sanitize request param as pointed >>> out by Chirag. >>> >>> However, this leaves us with only one option. In order to maintain >>> existing behavior, lets add another request param say returnOriginalIfError. >>> This will default to false for everyone other than for requests that >>> explicitly set it to true. And we put the isRecovereable check only >>> if returnOriginalIfError is true. >>> Slowly, people who want to return original content in case of errors will >>> start setting it to true. >>> >>> What do you think ? >>> >>> Thanks >>> Gagan >>> >> >> >
Sign in to reply to this message.
Cool. Let me add the http request param in this change and make another cl for container config. On Thu, Aug 12, 2010 at 1:28 PM, John Hjelmstad <johnfargo@gmail.com> wrote: > PS. Pre-nit: I'd recommend a short queryparam name, since our URLs are > getting long enough as-is. Anyone who cares to keep this behavior will know > what to add anyway. > > > On Thu, Aug 12, 2010 at 12:57 AM, John Hjelmstad <johnfargo@gmail.com>wrote: > >> Soudns like a quite reasonable strategy to me. Feel free to attach this >> behavior to ContainerConfig (for an overridable default) as well. >> >> >> On Thu, Aug 12, 2010 at 12:55 AM, Gagandeep singh <gagan.goku@gmail.com>wrote: >> >>> @John >>> >>> I didn't realize you were asking me for comments :) >>> >>> On Wed, Aug 11, 2010 at 11:48 PM, <johnfargo@gmail.com> wrote: >>> >>> Gagan, thoughts on my latest response? There are several situations in >>>> which users of the proxy might reasonably be expecting that original >>>> content is _not_ returned. The examples listed are cajoled content and >>>> imagery (resized, optimized, sanitized, etc). >>> >>> >>> Yes, if people are relying on the fact that if there is some weirdness in >>> the original content then ProxyServlet will throw an internal server error, >>> then its probably better to keep the existing behavior around. One case >>> where we have seen this behavior is the sanitize request param as pointed >>> out by Chirag. >>> >>> However, this leaves us with only one option. In order to maintain >>> existing behavior, lets add another request param say returnOriginalIfError. >>> This will default to false for everyone other than for requests that >>> explicitly set it to true. And we put the isRecovereable check only >>> if returnOriginalIfError is true. >>> Slowly, people who want to return original content in case of errors will >>> start setting it to true. >>> >>> What do you think ? >>> >>> Thanks >>> Gagan >>> >> >> >
Sign in to reply to this message.
Cool. Let me add the http request param in this change and make another cl for container config. On Thu, Aug 12, 2010 at 1:28 PM, John Hjelmstad <johnfargo@gmail.com> wrote: > PS. Pre-nit: I'd recommend a short queryparam name, since our URLs are > getting long enough as-is. Anyone who cares to keep this behavior will know > what to add anyway. > > > On Thu, Aug 12, 2010 at 12:57 AM, John Hjelmstad <johnfargo@gmail.com>wrote: > >> Soudns like a quite reasonable strategy to me. Feel free to attach this >> behavior to ContainerConfig (for an overridable default) as well. >> >> >> On Thu, Aug 12, 2010 at 12:55 AM, Gagandeep singh <gagan.goku@gmail.com>wrote: >> >>> @John >>> >>> I didn't realize you were asking me for comments :) >>> >>> On Wed, Aug 11, 2010 at 11:48 PM, <johnfargo@gmail.com> wrote: >>> >>> Gagan, thoughts on my latest response? There are several situations in >>>> which users of the proxy might reasonably be expecting that original >>>> content is _not_ returned. The examples listed are cajoled content and >>>> imagery (resized, optimized, sanitized, etc). >>> >>> >>> Yes, if people are relying on the fact that if there is some weirdness in >>> the original content then ProxyServlet will throw an internal server error, >>> then its probably better to keep the existing behavior around. One case >>> where we have seen this behavior is the sanitize request param as pointed >>> out by Chirag. >>> >>> However, this leaves us with only one option. In order to maintain >>> existing behavior, lets add another request param say returnOriginalIfError. >>> This will default to false for everyone other than for requests that >>> explicitly set it to true. And we put the isRecovereable check only >>> if returnOriginalIfError is true. >>> Slowly, people who want to return original content in case of errors will >>> start setting it to true. >>> >>> What do you think ? >>> >>> Thanks >>> Gagan >>> >> >> >
Sign in to reply to this message.
Hi Anumapa, John I added a RETURN_ORIGINAL_CONTENT_ON_ERROR param to ProxyUri and HttpRequest and ProxyHandler will keep throwing INTERNAL_SERVER_ERROR unless this param is set to "true". This should keep the old behavior intact. A test has been added for the same. Please take a look. Thanks Gagan
Sign in to reply to this message.
Good tests! http://codereview.appspot.com/1696059/diff/38002/49004 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java (right): http://codereview.appspot.com/1696059/diff/38002/49004#newcode180 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java:180: req.setParam(Param.RETURN_ORIGINAL_CONTENT_ON_ERROR.getKey(), Add a newline before this to separate it from the image-related params. http://codereview.appspot.com/1696059/diff/38002/49003 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriCommon.java (right): http://codereview.appspot.com/1696059/diff/38002/49003#newcode51 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriCommon.java:51: RETURN_ORIGINAL_CONTENT_ON_ERROR("return_original"), return_orig_on_err ? http://codereview.appspot.com/1696059/diff/38002/49001 File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java (right): http://codereview.appspot.com/1696059/diff/38002/49001#newcode242 java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:242: .addHeader("Content-Type", contentType) Remove these headers, since these are not relevant to the test? http://codereview.appspot.com/1696059/diff/38002/49001#newcode293 java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:293: .addHeader("Content-Type", contentType) Same comment as above. http://codereview.appspot.com/1696059/diff/38002/49001#newcode303 java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:303: ResponseRewriter rewriter = new DomWalker.Rewriter() { Can't we pull this piece into a test-helper method?
Sign in to reply to this message.
http://codereview.appspot.com/1696059/diff/38002/49004 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java (right): http://codereview.appspot.com/1696059/diff/38002/49004#newcode180 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java:180: req.setParam(Param.RETURN_ORIGINAL_CONTENT_ON_ERROR.getKey(), On 2010/08/12 10:32:33, anupama.dutta wrote: > Add a newline before this to separate it from the image-related params. Done. http://codereview.appspot.com/1696059/diff/38002/49003 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriCommon.java (right): http://codereview.appspot.com/1696059/diff/38002/49003#newcode51 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriCommon.java:51: RETURN_ORIGINAL_CONTENT_ON_ERROR("return_original"), On 2010/08/12 10:32:33, anupama.dutta wrote: > return_orig_on_err ? Done. http://codereview.appspot.com/1696059/diff/38002/49001 File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java (right): http://codereview.appspot.com/1696059/diff/38002/49001#newcode242 java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:242: .addHeader("Content-Type", contentType) On 2010/08/12 10:32:33, anupama.dutta wrote: > Remove these headers, since these are not relevant to the test? Done. http://codereview.appspot.com/1696059/diff/38002/49001#newcode293 java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:293: .addHeader("Content-Type", contentType) On 2010/08/12 10:32:33, anupama.dutta wrote: > Same comment as above. Done. http://codereview.appspot.com/1696059/diff/38002/49001#newcode303 java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:303: ResponseRewriter rewriter = new DomWalker.Rewriter() { On 2010/08/12 10:32:33, anupama.dutta wrote: > Can't we pull this piece into a test-helper method? Done.
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
On 2010/08/12 13:24:16, anupama.dutta wrote: > LGTM. Thanks for the thorough review Anupama. @John, Chirag: cl is all yours now :)
Sign in to reply to this message.
Hi Ziv, John, Chirag Could you please take a look and commit this cl if it looks fine to you ? Thanks Gagan
Sign in to reply to this message.
LGTM. I'm making a few slight modifications, and committing. Let me know if
you would like any of these reverted to the way you did them:
1. Change RETURN_ORIG_ON_ERR param to "rooe" rather than
"return_orig_on_err", to keep proxy URLs shorter.
2. Accept values "1" or "true" for return_orig_on_err ("1" is more widely
used in Shindig, but I see no problem accepting "true").
3. For #2, use the form "true".equalsIgnoreCase(val) to avoid need for
null-checks.
4. Change setReturnOriginalContentOnError in ProxyUri to accept a boolean.
Also mark @VisibleForTesting
5. Use a new typed method boolean shouldReturnOrigOnErr() rather than
passing RETURN_ORIG_CONTENT_ON_ERR in HttpRequest.setParam/getParam(). The
get/setParam() stuff is still a little unsatisfying to me, so I'd prefer not
to expand its reach. For the time being, it's just being used for image
sizing.
--John
On Wed, Aug 18, 2010 at 3:55 AM, <gagan.goku@gmail.com> wrote:
> Hi Ziv, John, Chirag
>
> Could you please take a look and commit this cl if it looks fine to you
> ?
>
> Thanks
> Gagan
>
>
> http://codereview.appspot.com/1696059/
>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
