Enable shindig to run on non-ROOT context path

Please see details in the jira:
https://issues.apache.org/jira/browse/SHINDIG-1525

[Li Xu, 2011-05-02 21:12:23]

update the patch with correct svn base and comments from Henry

[Han Nguyen, 2011-05-05 14:48:34]

Hi guys,
If there's no other objections nor concerns, I'll commit this patch by the end
of the week. Thanks!

On 2011/05/02 21:12:23, leegd28 wrote:
> update the patch with correct svn base and comments from Henry

[henry.saputra, 2011-05-05 15:32:30]

http://codereview.appspot.com/4430047/diff/3001/config/container.js
File config/container.js (right):

http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
Not sure why you need to duplicate the property here? This is already defined in
the shindig.properties file

http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):

http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';
I would prefer to have this set as part of the common container bootstrap. Maybe
as part of gadget metadata response?

http://codereview.appspot.com/4430047/diff/3001/java/common/src/main/java/org...
File
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java
(right):

http://codereview.appspot.com/4430047/diff/3001/java/common/src/main/java/org...
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java:67:
binder.bindConstant().annotatedWith(Names.named("shindig.contextroot")).to(contextRoot);
May want to check if system properties already have this set similar to
shindig.root property.

[Li Xu, 2011-05-05 17:47:25]

As it's part of servletContext, there's no need to manually set up
system.properties?

[Li Xu, 2011-05-05 18:24:36]

thanks for the review. please check the comments below.

http://codereview.appspot.com/4430047/diff/3001/config/container.js
File config/container.js (right):

http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
yes, I moved it from shindig.properties into container.js.
so Context Root can be resolved here. 
I also noticed a duplicate with "gadgets.uri.oauth.callbackTemplate", do you
have suggestions to consolidate these two? thanks.

http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):

http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';

the value is only available in the jsp. does common container has a place to
pass in this value? I set it to window.__CONTEXT_ROOT for now to make it
consistent with other global variables in init.js.

The other alternative sounds good. How about this...
1. add a configuration in container.js
"container" : {
    "relayPath": "${CONTEXT_ROOT}/gadgets/files/container/rpc_relay.html",
    "contextroot":"${CONTEXT_ROOT}"
  }
2. then it's returned and resolved as part of "/gadgets/ifr?..."
then gadgets.config.init will initialize window.__CONTEXT_ROOT in
gadget-holder.js

function init(config) {
if (config.container){
var rpath = config["container"]["relayPath"];
		osapi.container.GadgetHolder.prototype.relayPath_ = rpath;
window.__CONTEXT_ROOT = config["container"]["contextroot"];
	}
};

http://codereview.appspot.com/4430047/diff/3001/java/common/src/main/java/org...
File
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java
(right):

http://codereview.appspot.com/4430047/diff/3001/java/common/src/main/java/org...
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java:67:
binder.bindConstant().annotatedWith(Names.named("shindig.contextroot")).to(contextRoot);
As it is part of ServletContext, there's no need to set up from
System.properties?

[henry.saputra, 2011-05-06 06:13:08]

http://codereview.appspot.com/4430047/diff/3001/config/container.js
File config/container.js (right):

http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
I am not too familiar with the param but you might want to check comment in the
GadgetOAuthCallbackGenerator class.

AFAIK, the properties of Shindig are shared across all container, so thats why
shindig.signing.global-callback-url is put in the shindig.properties file.
 

On 2011/05/05 18:24:36, leegd28 wrote:
> yes, I moved it from shindig.properties into container.js.
> so Context Root can be resolved here. 
> I also noticed a duplicate with "gadgets.uri.oauth.callbackTemplate", do you
> have suggestions to consolidate these two? thanks.
>

http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):

http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';
I like this approach better

On 2011/05/05 18:24:36, leegd28 wrote:
> 
> the value is only available in the jsp. does common container has a place to
> pass in this value? I set it to window.__CONTEXT_ROOT for now to make it
> consistent with other global variables in init.js.
> 
> The other alternative sounds good. How about this...
> 1. add a configuration in container.js
> "container" : {
>     "relayPath": "${CONTEXT_ROOT}/gadgets/files/container/rpc_relay.html",
>     "contextroot":"${CONTEXT_ROOT}"
>   }
> 2. then it's returned and resolved as part of "/gadgets/ifr?..."
> then gadgets.config.init will initialize window.__CONTEXT_ROOT in
> gadget-holder.js
> 
> function init(config) {
> if (config.container){
> var rpath = config["container"]["relayPath"];
> 		osapi.container.GadgetHolder.prototype.relayPath_ = rpath;
> window.__CONTEXT_ROOT = config["container"]["contextroot"];
> 	}
> };

[Li Xu, 2011-05-06 17:56:43]

http://codereview.appspot.com/4430047/diff/3001/config/container.js
File config/container.js (right):

http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
I did a search and only found "shindig.signing.global-callback-url" is used by
OAuthModule. Insteadof getting it by using Guice injection ( from
shindig.properties) it's now picked up from containerConfig ( as it's provided
by default container). Since there's no other use of that property, we're ok
here?

if there's some usecase that may break, Would you please give me some details?
like how to see the failure via testing... thanks.

[Li Xu, 2011-05-09 15:10:26]

http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):

http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';
Sorry, I need to revise the approach after trying it.
It's too late to pass context root from a <script> tag loading.
context needs to be set as part of href of <script> tags anyway.

I think the code that requires context root on client side can be divided into 2
parts. The set is sample container implementation (index.jsp,
assember.js,viewcontroller.js) and the other set is the core common container
implementation (like init.js).

For the usage in sample container, I think it's flexible to the sample
container, it can choose any variable to pass the value around.

For core common container implementation, I added a default a value. If
window.__CONTEXT_ROOT is not set by sample container bootstrap (like index.jsp),
it will use "" as default.
this will be added to initializeConfig() as part of init.js
    if (!window.__CONTEXT_ROOT) window.__CONTEXT_ROOT = "";

Please let me know what you think. Thanks for your help, it has been there for a
couple of weeks. appreciate your help to help move it forward.

[Li Xu, 2011-05-09 18:03:15]

set window__CONTEXT_ROOT default to ROOT

[henry.saputra, 2011-05-10 06:56:39]

LGTM with proposed change to keep shindig.signing.global-callback-url in
shindig.properties file.

- Henry

http://codereview.appspot.com/4430047/diff/22001/config/container.js
File config/container.js (right):

http://codereview.appspot.com/4430047/diff/22001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
I dont think we need to move this to container.js file. This property could
should be in shindig.properties because its not container specific but service
provider specific.

[Li Xu, 2011-05-11 02:49:56]

move "shindig.signing.global-callback-url" back to shindig.properties

[Li Xu, 2011-05-11 16:02:29]

change approach  a little to create binding in PropertiesModule so test cases
won't break

[Li Xu, 2011-05-12 13:42:21]

Henry, would you please review the latest change? thanks.

[henry.saputra, 2011-05-12 23:25:27]

Some comments about keeping with html than jsp.

- Henry

http://codereview.appspot.com/4430047/diff/32001/content/samplecontainer/exam...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):

http://codereview.appspot.com/4430047/diff/32001/content/samplecontainer/exam...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';
I dont think we should change the html with jsp file. The container usually have
different context than the gadget server.

I would think instead of using window.__CONTEXT_ROOT probably inject the context
root when constructing the osapi.container.Container and provide function to
change it.

The demo page could have field to add context root that could be set before ask
the common container code to render gadget.

Thoughts?

[Li Xu, 2011-05-13 14:49:05]

I agree container and gadget apis could have different context. However jsp
would be the better solution in this case.
When container page is loaded,  common container css and js are loaded too.use
jsp it would be flexible to resolve a different context path for the url to load
common container css and js.

use inputbox could be problematic... that means common container and css
couldn't be loaded upon page loading, seperate loading can only be done via
xhr(quite a change from the current sample container, need to rewrite ) or an
additional full page load with jsp support.

yeah, context root can be  passed into osapi.container.Container. However that
doesn't help.
Context root path for APIs(js features) needs to be resolved in init.js.
There're couple of other global variables defined there eg
window.__CONTAINER_URI,window._API_URI.
Thinking about rename it to window._API_CONTEXT_ROOT, so it will be inline with
other global variable. what do you think?

[henry.saputra, 2011-05-13 23:46:26]

One suggestion is to keep sample index.html and modify the conf/container.js
core.io section to:

// Only configuration for required features will be used.
// See individual feature.xml files for configuration details.
"gadgets.features" : {
  "core.io" : {
    // Note: /proxy is an open proxy. Be careful how you expose this!
    // Note: Here // is replaced with the current protocol http/https
    "proxyUrl" :
"//%host%/gadgets/proxy?container=%container%%rewriteMime%&refresh=%refresh%&url=%url%%rewriteMime%&gadget=%gadget%%rawurl%",
    "jsonProxyUrl" : "//%host%/gadgets/makeRequest"
  },

Then modify the init.js to remove the code that setss core.io config:

Index: features/src/main/javascript/features/container/init.js
===================================================================
--- features/src/main/javascript/features/container/init.js	(revision 1102897)
+++ features/src/main/javascript/features/container/init.js	(working copy)
@@ -28,13 +28,6 @@
     gadgets.config.init({
       'rpc': {
         'parentRelayUrl': ''
-      },
-      'core.io': {
-        'jsonProxyUrl': 'http://%host%/gadgets/makeRequest',
-        'proxyUrl': 'http://%host%/gadgets/proxy' +
-            '?refresh=%refresh%' +
-            '&container=%container%%rewriteMime%' +
-            '&gadget=%gadget%/%rawurl%'
       }
     });
   }

With new JS serving framework, the config will be pushed to client for container
by ConfigInjectionProcessor so I think common container init.js does not have to
set the config for core.io.

Including Michael Hermanto for his comment.

- Henry

[henry.saputra, 2011-05-14 19:33:16]

Forgot to copy the ${CONTEXT_ROOT}:

"gadgets.features" : {
  "core.io" : {
   ... 

    "proxyUrl" :
"//%host%${CONTEXT_ROOT}/gadgets/proxycontainer=%container%%rewriteMime%&refresh=%refresh%&url=%url%%rewriteMime%&gadget=%gadget%%rawurl%",
  
  ...

- Henry 

On 2011/05/13 23:46:26, henry.saputra wrote:
> One suggestion is to keep sample index.html and modify the conf/container.js
> core.io section to:
> 
> // Only configuration for required features will be used.
> // See individual feature.xml files for configuration details.
> "gadgets.features" : {
>   "core.io" : {
>     // Note: /proxy is an open proxy. Be careful how you expose this!
>     // Note: Here // is replaced with the current protocol http/https
>     "proxyUrl" :
>
"//%host%/gadgets/proxy?container=%container%%rewriteMime%&refresh=%refresh%&url=%url%%rewriteMime%&gadget=%gadget%%rawurl%",
>     "jsonProxyUrl" : "//%host%/gadgets/makeRequest"
>   },
> 
> Then modify the init.js to remove the code that setss core.io config:
> 
> Index: features/src/main/javascript/features/container/init.js
> ===================================================================
> --- features/src/main/javascript/features/container/init.js	(revision 1102897)
> +++ features/src/main/javascript/features/container/init.js	(working copy)
> @@ -28,13 +28,6 @@
>      gadgets.config.init({
>        'rpc': {
>          'parentRelayUrl': ''
> -      },
> -      'core.io': {
> -        'jsonProxyUrl': 'http://%host%/gadgets/makeRequest',
> -        'proxyUrl': 'http://%host%/gadgets/proxy' +
> -            '?refresh=%refresh%' +
> -            '&container=%container%%rewriteMime%' +
> -            '&gadget=%gadget%/%rawurl%'
>        }
>      });
>    }
> 
> With new JS serving framework, the config will be pushed to client for
container
> by ConfigInjectionProcessor so I think common container init.js does not have
to
> set the config for core.io.
> 
> Including Michael Hermanto for his comment.
> 
> - Henry

[Li Xu, 2011-05-16 21:35:23]

Thanks. I investigated a little here today. You're right, gadget.config.init()
in init.js is not necessary. Even it's invoked, it's always overwritten by
what's defined in container.js since that's injected by
ConfigInjectionProcessor.
Possible to remove gadgets.config.init from init.js? -- I know this probably
better to addressed as a seperate issue.

I will update the patch to remove the changes in init.js.

Also don't need to define window.__Context_Root on client side anymore.

Regarding jsp vs html...it will be helpful if you could please explain a little
why html is preferred. 
If it's static html, the src(to load container) needs to be dynamically
generated and set. We need to guarantee the order here.
Using script element with jsp would have  guaranteed order.



<script type="text/javascript"
		src="/gadgets/js/container:rpc:pubsub-2.js?c=1&debug=1&container=default"></script>
<script type="text/javascript" src="./assembler.js"></script>

[henry.saputra, 2011-05-16 23:32:09]

I think example files should not be in jsp just because it shows that you dont
need Java stack to actually build container with common container.

And yes the order of the script tag need to be guaranteed.

- Henry

On 2011/05/16 21:35:23, Li Xu wrote:
> Thanks. I investigated a little here today. You're right, gadget.config.init()
> in init.js is not necessary. Even it's invoked, it's always overwritten by
> what's defined in container.js since that's injected by
> ConfigInjectionProcessor.
> Possible to remove gadgets.config.init from init.js? -- I know this probably
> better to addressed as a seperate issue.
> 
> I will update the patch to remove the changes in init.js.
> 
> Also don't need to define window.__Context_Root on client side anymore.
> 
> Regarding jsp vs html...it will be helpful if you could please explain a
little
> why html is preferred. 
> If it's static html, the src(to load container) needs to be dynamically
> generated and set. We need to guarantee the order here.
> Using script element with jsp would have  guaranteed order.
> 
> 
> 
> <script type="text/javascript"
>
		src="/gadgets/js/container:rpc:pubsub-2.js?c=1&debug=1&container=default"></script>
> <script type="text/javascript" src="./assembler.js"></script>

[Li Xu, 2011-05-17 20:16:31]

change jsp to html, also added support to samplecontainer.html

[henry.saputra, 2011-05-18 00:53:16]

Several more comments but LGTM in general.

To make sure, we may want to remove the config initialization for gadgets.io in
the common container init.js.

Sorry for long review.

- Henry

http://codereview.appspot.com/4430047/diff/36002/config/container.js
File config/container.js (right):

http://codereview.appspot.com/4430047/diff/36002/config/container.js#newcode147
config/container.js:147: "proxyUrl" :
"//%host%${CONTEXT_ROOT}/gadgets/proxy?container=default&refresh=%refresh%&url=%url%%rewriteMime%",
Need to change the proxyUrl to:

 "proxyUrl" :
"//%host%${CONTEXT_ROOT}/gadgets/proxy?container=%container%%rewriteMime%&refresh
     =%refresh%&url=%url%%rewriteMime%&gadget=%gadget%/%rawurl%"

Modify the "container" param and add "gadget" param. The gadgets.io.getProxyUrl
will make the substitution correctly.

http://codereview.appspot.com/4430047/diff/36002/content/samplecontainer/exam...
File content/samplecontainer/examples/commoncontainer/index.html (right):

http://codereview.appspot.com/4430047/diff/36002/content/samplecontainer/exam...
content/samplecontainer/examples/commoncontainer/index.html:25: <script
src="../../../gadgets/js/container:rpc:pubsub-2.js?c=1&debug=1&container=default"></script>
Not sure why you needs to change the URLs for the css and script

http://codereview.appspot.com/4430047/diff/36002/features/src/main/javascript...
File features/src/main/javascript/features/shindig.uri/uri.js (right):

http://codereview.appspot.com/4430047/diff/36002/features/src/main/javascript...
features/src/main/javascript/features/shindig.uri/uri.js:230: setPath:
function(path) { if (path && path != null ) { path_ = (path[0] === '/' ? '' :
'/') + path; } return bundle; },
I assume you want to check for undefined, may want to do it like:

if (typeof path !== 'undefined' && path != null)

http://codereview.appspot.com/4430047/diff/36002/java/common/src/main/java/or...
File java/common/src/main/java/org/apache/shindig/common/PropertiesModule.java
(right):

http://codereview.appspot.com/4430047/diff/36002/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/common/PropertiesModule.java:111:
String contextRoot = System.getProperty("shindig.contextroot", "");
Can it just call getContextRoot() method?

http://codereview.appspot.com/4430047/diff/36002/java/common/src/main/java/or...
File
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java
(right):

http://codereview.appspot.com/4430047/diff/36002/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java:57:
contextRoot = context.getContextPath();
This probably better done in the setSystemProperties() to check if
shindig.contextRoot has been set in the "system.properties" contextParam in
web.xml.

Also make sure to use 2 spaces for indentation.

[Li Xu, 2011-05-19 14:56:17]

updated with comments

[Li Xu, 2011-05-19 15:02:25]

Comments is addressed. 
Regarding the change in init.js and value for "proxyUrl", I prefer to do a
followup since all the current sample pages works.  Will post to the mailing
list for discussion first.

If there's no other objection, could anyone please help committing? Han is not
available now. thanks.

http://codereview.appspot.com/4430047/diff/36002/config/container.js
File config/container.js (right):

http://codereview.appspot.com/4430047/diff/36002/config/container.js#newcode147
config/container.js:147: "proxyUrl" :
"//%host%${CONTEXT_ROOT}/gadgets/proxy?container=default&refresh=%refresh%&url=%url%%rewriteMime%",
doesn't work, "proxyUrl" in current patch works. 
Might be a bug in common container. suggest to look at this seperately.

http://codereview.appspot.com/4430047/diff/36002/content/samplecontainer/exam...
File content/samplecontainer/examples/commoncontainer/index.html (right):

http://codereview.appspot.com/4430047/diff/36002/content/samplecontainer/exam...
content/samplecontainer/examples/commoncontainer/index.html:25: <script
src="../../../gadgets/js/container:rpc:pubsub-2.js?c=1&debug=1&container=default"></script>
had to change it so it could work out of box for any context root path.

http://codereview.appspot.com/4430047/diff/36002/features/src/main/javascript...
File features/src/main/javascript/features/shindig.uri/uri.js (right):

http://codereview.appspot.com/4430047/diff/36002/features/src/main/javascript...
features/src/main/javascript/features/shindig.uri/uri.js:230: setPath:
function(path) { if (path && path != null ) { path_ = (path[0] === '/' ? '' :
'/') + path; } return bundle; },
On 2011/05/18 00:53:16, henry.saputra wrote:
> I assume you want to check for undefined, may want to do it like:
> 
> if (typeof path !== 'undefined' && path != null)

Done.

http://codereview.appspot.com/4430047/diff/36002/java/common/src/main/java/or...
File java/common/src/main/java/org/apache/shindig/common/PropertiesModule.java
(right):

http://codereview.appspot.com/4430047/diff/36002/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/common/PropertiesModule.java:111:
String contextRoot = System.getProperty("shindig.contextroot", "");
On 2011/05/18 00:53:16, henry.saputra wrote:
> Can it just call getContextRoot() method?

Done.

http://codereview.appspot.com/4430047/diff/36002/java/common/src/main/java/or...
File
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java
(right):

http://codereview.appspot.com/4430047/diff/36002/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java:57:
contextRoot = context.getContextPath();
Done, moved to setSystemProperties. But web.xml change is not required since it
can be detected.