[BugFix]: Followup change to pass on container information correctly throughout the request serving

This is a followup change (following already committed change http://codereview.appspot.com/2004042/). The goal is to pass on the correct gadget context (which includes container) throughout the path of request serving.

[anupama.dutta, 2010-08-17 12:45:44]

http://codereview.appspot.com/1888046/diff/11001/12009
File config/container.js (right):

http://codereview.appspot.com/1888046/diff/11001/12009#newcode47
config/container.js:47: {"gadgets.container" : ["default", "accel"],
Move this accel related change to another CL.

http://codereview.appspot.com/1888046/diff/11001/12001
File java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12001#newcode49
java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java:49:
public HttpRequest getRequest() {
This change should not be needed. The main goal of this change is to propagate
the container information to all places where makeGadget needs to be called with
the container information (and not the default container). Here is what I found
out:

makeGadget(Uri) is called instead of makeGadget(HttpRequest) in several places.
But the ones that matter to us are:

a) CssResponseRewriter: All rewrite calls inside CssResponseRewriter can take
GadgetContext as input instead of Uri source, so that the makeGadget call inside
this rewriter has the right context. One of the rewrite methods is directly used
by both StyleTagExtractorVisitor and StyleTagProxyEmbeddedUrlsVisitor (you have
made changes in only the latter). We should change this call to take the
gadget.getContext() directly. Note that the original URL which was being
accessed as gadget.getSpec().getURL() can still be accessed as context.getURL()
wherever needed.

b) AccelHandler - We can change the getProxyUri call to take HttpRequest and
this can be passed along to both parseAndNormalize and finally to the makeGadget
call within getProxyUri itself.

d) DefaultAccelUriManager : parseAndNormalize can use HttpRequest (and not Uri)
and call makeGadget correctly.

We can make the changes for (a) in one patch and the changes for (b) and (c) in
another patch.

http://codereview.appspot.com/1888046/diff/11001/12008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12008#newcode91
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:91:
public List<String> rewrite(Reader content, HttpRequest request,
As suggested earlier, passing GadgetContext in all these places might be better.

http://codereview.appspot.com/1888046/diff/11001/12008#newcode126
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:126:
public List<String> rewrite(Element styleNode, HttpRequest request,
StyleTagExtractorVisitor also seems to be relying on this method - you should
change that use?

http://codereview.appspot.com/1888046/diff/11001/12007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12007#newcode242
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:242:
@Override
This should not be needed as mentioned before.

http://codereview.appspot.com/1888046/diff/11001/12006
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12006#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java:62:
elem, gadget.getContext().getRequest(),
We can pass the gadget.getContext() here as suggested earlier (we should do the
same change in StyleTagExtractorVisitor as well).

http://codereview.appspot.com/1888046/diff/11001/12003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12003#newcode78
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:78:
ProxyUriManager.ProxyUri proxyUri = getProxyUri(request.getUri());
As mentioned earlier, we can actually pass the "request" here directly, so that
parseAndNormalize can also be called with the request directly.
(Also, please move these changes to another patch).

http://codereview.appspot.com/1888046/diff/11001/12003#newcode185
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:185:
req.setContainer(AccelUriManager.CONTAINER);
Since the container has already been set in HtmlAccelServlet before
accelHandler.fetch is called, we should not need to set it again here.

http://codereview.appspot.com/1888046/diff/11001/12002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12002#newcode65
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:65:
req.setContainer(AccelUriManager.CONTAINER);
Please move these changes to another patch.

http://codereview.appspot.com/1888046/diff/11001/12004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12004#newcode42
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:42:
public Uri parseAndNormalize(Uri requestUri) throws GadgetException;
We can remove this method completely.
(Also, please move these changes to another patch.)

http://codereview.appspot.com/1888046/diff/11001/12005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12005#newcode51
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:51:
public Uri parseAndNormalize(Uri requestUri) throws GadgetException {
We should not need this method at all.
(Also, please move these changes to another patch.)

[henry.saputra, 2010-08-18 18:32:04]

http://codereview.appspot.com/1888046/diff/11001/12001
File java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12001#newcode49
java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java:49:
public HttpRequest getRequest() {
I dont think its good idea to return the HttpRequest object in the
GadgetContext.

This class should not be protocol specific and provide abstraction to specific
information during gadget request lifecycle.

[gagan.goku, 2010-08-19 05:28:23]

Thanks for the review Henry.

http://codereview.appspot.com/1888046/diff/11001/12001
File java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12001#newcode49
java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java:49:
public HttpRequest getRequest() {
On 2010/08/18 18:32:05, henry.saputra wrote:
> I dont think its good idea to return the HttpRequest object in the
> GadgetContext.
> 
> This class should not be protocol specific and provide abstraction to specific
> information during gadget request lifecycle.

Makes sense. Let me put some more thought into it.
In the meantime, lets review http://codereview.appspot.com/2004042/ uploaded by
Jesse. Its very similar to this cl and is already in a good shape :)

[gagan.goku, 2010-08-22 16:22:13]

Took care of comments. This is an alternate way of passing on container
information around (alternate to Jesse's cl:
http://codereview.appspot.com/2004042/). Please take a look at both.
To summarize, Jesse is passing along the container string, and i'm trying to get
away with passing the GadgetContext.
Sadly, this cl has exploded :(

http://codereview.appspot.com/1888046/diff/11001/12008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12008#newcode91
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:91:
public List<String> rewrite(Reader content, HttpRequest request,
On 2010/08/17 12:45:45, anupama.dutta wrote:
> As suggested earlier, passing GadgetContext in all these places might be
better.

Done.

http://codereview.appspot.com/1888046/diff/11001/12008#newcode126
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:126:
public List<String> rewrite(Element styleNode, HttpRequest request,
On 2010/08/17 12:45:45, anupama.dutta wrote:
> StyleTagExtractorVisitor also seems to be relying on this method - you should
> change that use?

Done.

http://codereview.appspot.com/1888046/diff/11001/12007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12007#newcode242
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:242:
@Override
On 2010/08/17 12:45:45, anupama.dutta wrote:
> This should not be needed as mentioned before.

Done.

http://codereview.appspot.com/1888046/diff/11001/12006
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12006#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java:62:
elem, gadget.getContext().getRequest(),
On 2010/08/17 12:45:45, anupama.dutta wrote:
> We can pass the gadget.getContext() here as suggested earlier (we should do
the
> same change in StyleTagExtractorVisitor as well).

Done.

[gagan.goku, 2010-08-22 17:32:46]

http://codereview.appspot.com/1888046/diff/11001/12009
File config/container.js (right):

http://codereview.appspot.com/1888046/diff/11001/12009#newcode47
config/container.js:47: {"gadgets.container" : ["default", "accel"],
On 2010/08/17 12:45:45, anupama.dutta wrote:
> Move this accel related change to another CL.

Done.

http://codereview.appspot.com/1888046/diff/11001/12003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12003#newcode78
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:78:
ProxyUriManager.ProxyUri proxyUri = getProxyUri(request.getUri());
On 2010/08/17 12:45:45, anupama.dutta wrote:
> As mentioned earlier, we can actually pass the "request" here directly, so
that
> parseAndNormalize can also be called with the request directly.
> (Also, please move these changes to another patch).

Done.

http://codereview.appspot.com/1888046/diff/11001/12003#newcode185
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:185:
req.setContainer(AccelUriManager.CONTAINER);
On 2010/08/17 12:45:45, anupama.dutta wrote:
> Since the container has already been set in HtmlAccelServlet before
> accelHandler.fetch is called, we should not need to set it again here.

Done.

http://codereview.appspot.com/1888046/diff/11001/12002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12002#newcode65
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:65:
req.setContainer(AccelUriManager.CONTAINER);
On 2010/08/17 12:45:45, anupama.dutta wrote:
> Please move these changes to another patch.

Done.

http://codereview.appspot.com/1888046/diff/11001/12004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12004#newcode42
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:42:
public Uri parseAndNormalize(Uri requestUri) throws GadgetException;
On 2010/08/17 12:45:45, anupama.dutta wrote:
> We can remove this method completely.
> (Also, please move these changes to another patch.)

Done.

http://codereview.appspot.com/1888046/diff/11001/12005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/1888046/diff/11001/12005#newcode51
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:51:
public Uri parseAndNormalize(Uri requestUri) throws GadgetException {
On 2010/08/17 12:45:45, anupama.dutta wrote:
> We should not need this method at all.
> (Also, please move these changes to another patch.)

Done.

[anupama.dutta, 2010-08-27 06:09:41]

Please move the changes to a new patch, so that the history of the old
(committed) changes can be ignored. Also, please update the description to give
more details regarding the CssSanitizer bug that is being fixed.

http://codereview.appspot.com/1888046/diff/86001/87009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitor.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87009#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitor.java:76:
List<String> extractedUrls = cssRewriter.rewrite(elem, contentBase,
Revert this change?

http://codereview.appspot.com/1888046/diff/86001/87008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87008#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java:63:
cssRewriter.rewrite(elem, contentBase,
Revert this change?

http://codereview.appspot.com/1888046/diff/86001/87004
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizerTest.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87004#newcode142
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizerTest.java:142:
"&container=mockContainer&nocache=0&debug=0&url=http%3A%2F%2Fwww.example.org%2Fimg.gif"
+
Good tests. I presume you have verified that without the current source code
change, these tests fail.

Would it be a good idea to have one test where the GadgetContext has the default
container and we get the default container correctly in the generated URL? Since
most shindig clients are possibly operating in the default container mode, it is
good to have one test to make sure this is still working as is.

http://codereview.appspot.com/1888046/diff/86001/87001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitorTest.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87001#newcode38
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitorTest.java:38:
import static org.easymock.EasyMock.*;
Revert this change.

http://codereview.appspot.com/1888046/diff/86001/87003
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87003#newcode147
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java:147:
Gadget gadget = DomWalker.makeGadget(new HttpRequest(Uri.parse("http://1.com/"))
Revert this change.

[gagan.goku, 2010-08-27 13:03:51]

Moved the codereview to http://codereview.appspot.com/2045041/.

Thanks for the thorough review.

http://codereview.appspot.com/1888046/diff/86001/87009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitor.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87009#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitor.java:76:
List<String> extractedUrls = cssRewriter.rewrite(elem, contentBase,
On 2010/08/27 06:09:42, anupama.dutta wrote:
> Revert this change?

Done.

http://codereview.appspot.com/1888046/diff/86001/87008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87008#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java:63:
cssRewriter.rewrite(elem, contentBase,
On 2010/08/27 06:09:42, anupama.dutta wrote:
> Revert this change?

Done.

http://codereview.appspot.com/1888046/diff/86001/87004
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizerTest.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87004#newcode142
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizerTest.java:142:
"&container=mockContainer&nocache=0&debug=0&url=http%3A%2F%2Fwww.example.org%2Fimg.gif"
+
On 2010/08/27 06:09:42, anupama.dutta wrote:
> Good tests. I presume you have verified that without the current source code
> change, these tests fail.
> 
> Would it be a good idea to have one test where the GadgetContext has the
default
> container and we get the default container correctly in the generated URL?
Since
> most shindig clients are possibly operating in the default container mode, it
is
> good to have one test to make sure this is still working as is.

Thanks. It feels good when you praise me :)
Added another test using default container as you suggested. Please take a look
at http://codereview.appspot.com/2045041/diff/2001/3002

http://codereview.appspot.com/1888046/diff/86001/87001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitorTest.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87001#newcode38
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitorTest.java:38:
import static org.easymock.EasyMock.*;
On 2010/08/27 06:09:42, anupama.dutta wrote:
> Revert this change.

Done.

http://codereview.appspot.com/1888046/diff/86001/87003
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java
(right):

http://codereview.appspot.com/1888046/diff/86001/87003#newcode147
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java:147:
Gadget gadget = DomWalker.makeGadget(new HttpRequest(Uri.parse("http://1.com/"))
On 2010/08/27 06:09:42, anupama.dutta wrote:
> Revert this change.

Done.

[gagan.goku, 2010-08-27 13:08:55]

Closing this issue now.