----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1632/ -----------------------------------------------------------
(Updated 2011-08-26 15:34:00.521113) Review request for shindig. Changes ------- I'm attaching an updated patch which addresses the open issues I identified when I posted the original (sans a JIRA ticket and updated unit tests -- if we decide to move forward with this patch I'll be glad to take care of those too). The first issue was figuring out how to deal with tokens for preloaded gadgets. The current common container maintains tokens for them, but those tokens don't include a real/valid moduleId (they're all hard coded to some static value). The second issue was figuring out how to get the token that is included in the original iframe URL to include a moduleId. My speculation was that the token in the iframe URL was based on the result of the metadata call and that proved to indeed be the case. It turns out I was able to work out a common solution for both of these issues. In the GadgetSite.render function there is a call to GadgetSite.updateSecurityToken_ -- that call used to check the cache for a token, and if it found one, it would set it in the loadingGadgetHolder_ -- if it didn't it would just give up. My fix was to not give up if the token was not found in the cache, and instead go ahead and kick off an OSAPI call to fetch it and then set it in the loadingGadgetHolder_ -- then further down the line in the rendering process this token gets used instead of the one that came back in the original metadata response. So now no matter if your navigating a preloaded gadget or a non-preloaded gadget, your guaranteed to get token with a valid moduleId into the iframeUrl. Note however that this means that we now have the possibility of firing off an asynchronous request during the rendering process which introduced a small pile of new callback functions needing to be passed around, but this is the best I could come up with without completely tearing things apart. Also note that currently this means you incur at least one XHR for every navigateGadget call to fetch the token (unless of course you've already pushed valid tokens into the container, in which case the tokens would come out of cache). To get around this we need to update the preloadGadget(s) functions to take a moduleId for each gadget as well -- then if the moduleId (or siteId) is knows at the time we preload the gadgets we can make one call for the gadget metadata in bulk (as we do currently) but then we could make another call to fetch the tokens in bulk as well. Then during navigateGadget all tokens and metadata would be coming from cache. I've included a TODO in the preloadGadgets function about this but haven't implemented it yet. Summary ------- Common container currently doesn't include the siteId (moduleId) in any of it's security token processing/handling (all security tokens in common container currently get minted with a moduleId of 0). This patch is a first (rough) cut at getting moduleId's into security tokens. I am posting it somewhat prematurely to solicit feedback before I invest any more time in finishing up the last bits. Here are the things that I know are still left to do: -- Update unit tests on both the JS and Java side -- currently I've been building and deploying with skipTests=true... -- Figure out a strategy for dealing with preloaded gadgets. The current auth-refresh process maintains tokens for preloaded gadgets, however the preoad JS functions just take a gadgetUrl so there is no concept of a siteId (moduleId) for them at this time. -- Figure out how to get the token that is included in the original iframe to include a moduleId. I think the token in the iframe likely comes back in the metadata request (although I haven't looked yet to verify), which means that the call to the metadata service would likely need to include the moduleId as well. I'd greatly appreciate any comments people have on the patch and strategies for dealing with the outstanding issues noted above. Thanks! Diffs (updated) ----- http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1148651 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1151535 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1150288 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1157893 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1158700 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_site_test.js 1086837 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1146846 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1098824 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1098824 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1125606 Diff: https://reviews.apache.org/r/1632/diff Testing ------- Thanks, Jesse
