Thank you guys for reviewing this, and thank you Paul for applying it into code base.
On Fri, Nov 5, 2010 at 4:46 AM, Henry Saputra <[email protected]>wrote: > According to > > http://google-guice.googlecode.com/svn/trunk/latest-javadoc/com/google/inject/multibindings/Multibinder.html > > the ordering is consistent with the binding ordering/sequence: "The > set's iteration order is consistent with the binding order" > > so I guess the ordering issue shouldnt be a problem. > > - Henry > > On Thu, Nov 4, 2010 at 1:09 AM, Kai Feng Zhang <[email protected]> wrote: > > hi Paul and Gagandeep, > > > > Seems we have no other options. Could I propose to code review my patch? > > https://issues.apache.org/jira/browse/SHINDIG-1456 > > > > If the new rewriter to be appended is simple enough ,it's does not matter > on > > the rewriters ordering issue I think. Otherwise we need to provide > > Comparable implementation for rewriters. > > > > > > On Tue, Nov 2, 2010 at 12:51 PM, Gagandeep singh <[email protected] > >wrote: > > > >> Hi Kai > >> > >> Thanks for finding this. Sad that the Modules.override approach had a > bug > >> :( > >> > >> On Tue, Nov 2, 2010 at 7:46 AM, Kai Feng Zhang <[email protected]> > wrote: > >> > >>> Seems this is a known issue, I found this: > >>> http://code.google.com/p/google-guice/issues/detail?id=263 > >>> > >>> I am not sure if this fix has gone into Guice 2.0 which is in classpath > of > >>> Shindig. If it's not, then I think I still need to propose my patch, > which > >>> will provide capability of extending/appending rewriters for Shindig > gadget > >>> renderer. > >>> > >> Would it be possible to go through this codereview< > http://codereview.appspot.com/2058042/diff/123001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java>( > maybe patch it in your client and play with it ) to see if it solves your > >> problem ? > >> > >> > >>> https://issues.apache.org/jira/browse/SHINDIG-1456 > >>> > >>> Any comments? Thanks. > >>> > >>> <http://code.google.com/p/google-guice/issues/detail?id=263> > >>> Thanks, > >>> > >>> Kevin Zhang (凯峰) > >>> Gtalk: [email protected] > >>> Blog: http://www.zhangkf.com > >>> Twitter: http://twitter.com/zhangkf > >>> > >>> > >>> On Mon, Nov 1, 2010 at 11:50 AM, Kai Feng Zhang <[email protected] > >wrote: > >>> > >>>> Hi Gagandeep, > >>>> > >>>> Thank you for the detailed sample code. > >>>> > >>>> I tried with your module override code, but seems it does not work, > there > >>>> are binding exception if I replace the DefaultGuiceModule with my new > >>>> CustomGuiceModule in web.xml. > >>>> > >>>> ============ Exception stack ================= > >>>> 2010-11-1 11:19:53 org.apache.catalina.core.StandardContext > listenerStart > >>>> com.google.inject.CreationException: Guice creation errors: > >>>> > >>>> 1) A binding to java.util.Set<java.lang.String> annotated with > >>>> > @com.google.inject.name.Named(value=org.apache.shindig.features-extended) > >>>> was already configured at > >>>> > org.apache.shindig.gadgets.DefaultGuiceModule.registerFeatureHandlers(DefaultGuiceModule.java:124). > >>>> at > >>>> > org.apache.shindig.extras.ShindigExtrasGuiceModule.configureExtraFeatures(ShindigExtrasGuiceModule.java:41) > >>>> > >>>> 2) A binding to java.util.Set<java.lang.Object> annotated with > >>>> @com.google.inject.name.Named(value=org.apache.shindig.handlers) was > already > >>>> configured at > >>>> > org.apache.shindig.gadgets.DefaultGuiceModule.registerGadgetHandlers(DefaultGuiceModule.java:105). > >>>> at > >>>> > org.apache.shindig.social.core.config.SocialApiGuiceModule.configure(SocialApiGuiceModule.java:76) > >>>> > >>>> 2 errors > >>>> at > >>>> > com.google.inject.internal.Errors.throwCreationExceptionIfErrorsExist(Errors.java:354) > >>>> at > >>>> > com.google.inject.InjectorBuilder.initializeStatically(InjectorBuilder.java:152) > >>>> at com.google.inject.InjectorBuilder.build(InjectorBuilder.java:105) > >>>> at com.google.inject.Guice.createInjector(Guice.java:92) > >>>> at > >>>> > org.apache.shindig.common.servlet.GuiceServletContextListener.contextInitialized(GuiceServletContextListener.java:73) > >>>> at > >>>> > org.apache.catalina.core.StandardContext.listenerStart(StandardContext.java:3972) > >>>> at > >>>> > org.apache.catalina.core.StandardContext.start(StandardContext.java:4467) > >>>> at > >>>> org.apache.catalina.core.ContainerBase.start(ContainerBase.java:1045) > >>>> at org.apache.catalina.core.StandardHost.start(StandardHost.java:785) > >>>> at > >>>> org.apache.catalina.core.ContainerBase.start(ContainerBase.java:1045) > >>>> at > org.apache.catalina.core.StandardEngine.start(StandardEngine.java:443) > >>>> at > >>>> > org.apache.catalina.core.StandardService.start(StandardService.java:519) > >>>> at > org.apache.catalina.core.StandardServer.start(StandardServer.java:710) > >>>> at org.apache.catalina.startup.Catalina.start(Catalina.java:581) > >>>> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > >>>> at > >>>> > sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) > >>>> at > >>>> > sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) > >>>> at java.lang.reflect.Method.invoke(Method.java:592) > >>>> at org.apache.catalina.startup.Bootstrap.start(Bootstrap.java:289) > >>>> at org.apache.catalina.startup.Bootstrap.main(Bootstrap.java:414) > >>>> 2010-11-1 11:19:53 org.apache.catalina.core.StandardContext start > >>>> > >>>> ==================CustomGuiceModule======================= > >>>> > >>>> protected void configure() { > >>>> install(Modules.override(new DefaultGuiceModule()).with(new > >>>> AbstractModule() { > >>>> > >>>> @Override > >>>> protected void configure() { > >>>> // TODO Auto-generated method stub > >>>> } > >>>> > >>>> @Provides > >>>> @Singleton > >>>> @Named("shindig.rewriters.gadget") > >>>> protected List<GadgetRewriter> provideGadgetRewriters( > >>>> PipelineDataGadgetRewriter pipelineRewriter, > >>>> TemplateRewriter templateRewriter, > >>>> AbsolutePathReferenceRewriter absolutePathRewriter, > >>>> StyleTagExtractorContentRewriter styleTagExtractorRewriter, > >>>> StyleAdjacencyContentRewriter styleAdjacencyRewriter, > >>>> ProxyingContentRewriter proxyingRewriter, > >>>> CajaContentRewriter cajaRewriter, > >>>> SanitizingGadgetRewriter sanitizedRewriter, > >>>> RenderingGadgetRewriter renderingRewriter, > >>>> OpenSocialI18NGadgetRewriter i18nRewriter, > >>>> CustomGadgetRewriter customGadgetRewriter) { > >>>> return ImmutableList.of(pipelineRewriter, templateRewriter, > >>>> absolutePathRewriter, styleTagExtractorRewriter, > >>>> styleAdjacencyRewriter, proxyingRewriter, > >>>> cajaRewriter, sanitizedRewriter, renderingRewriter, > >>>> i18nRewriter, customGadgetRewriter); > >>>> } > >>>> > >>>> })); > >>>> > >>>> } > >>>> > >>>> ================web.xml===================== > >>>> <context-param> > >>>> <param-name>guice-modules</param-name> > >>>> <param-value> > >>>> org.apache.shindig.common.PropertiesModule: > >>>> org.apache.shindig.extras.CustomGuiceModule: > >>>> org.apache.shindig.social.core.config.SocialApiGuiceModule: > >>>> org.apache.shindig.social.sample.SampleModule: > >>>> org.apache.shindig.gadgets.oauth.OAuthModule: > >>>> org.apache.shindig.common.cache.ehcache.EhCacheModule: > >>>> org.apache.shindig.sample.shiro.ShiroGuiceModule: > >>>> org.apache.shindig.sample.container.SampleContainerGuiceModule: > >>>> org.apache.shindig.extras.ShindigExtrasGuiceModule: > >>>> org.apache.shindig.extras.as.ActivityStreamsGuiceModule > >>>> </param-value> > >>>> </context-param> > >>>> > >>>> ================================== > >>>> From the exception, it says 2 binding are already configured in > >>>> DefaultGuiceModule, then they should not be configured again in other > >>>> modules, but these 2 binding are multibinding. I am curious why this > would > >>>> happen. > >>>> > >>>> If this kind of override module way works, it does be helpful for my > >>>> case, since I even do not need to change any code of existing Shindig > >>>> code(any modules). Thanks. > >>>> > >>>> > >>>> Thanks, > >>>> > >>>> Kevin Zhang (凯峰) > >>>> Gtalk: [email protected] > >>>> Blog: http://www.zhangkf.com > >>>> Twitter: http://twitter.com/zhangkf > >>>> > >>>> > >>>> On Mon, Nov 1, 2010 at 12:31 AM, Gagandeep Singh < > [email protected]>wrote: > >>>> > >>>>> Hi Kai > >>>>> > >>>>> > >>>>> On Sun, Oct 31, 2010 at 5:38 PM, Kai Feng Zhang <[email protected] > >wrote: > >>>>> > >>>>>> Hey Gagandeep, > >>>>>> > >>>>>> Thank you, it seems your solution is cool thing with more > flexibility. > >>>>>> > >>>>>> What I am asking is to *append* a rewriter to current rewriters list > >>>>>> from DefaultGuiceModule, b/c I think the existing rewriters are good > enough > >>>>>> for my case. And as I know, DefaultGuiceModule does a lot of things > >>>>>> (installing different modules etc), but not only install > RewriterModule for > >>>>>> binding rewiters. > >>>>>> > >>>>> > >>>>> Your module (say MyGuiceModule) would look like: > >>>>> class MyGuiceModule extends AbstractModule { > >>>>> public void configure() { > >>>>> install(Modules.override(new DefaultGuiceModule()).with(new > >>>>> MySpecialModule())); > >>>>> } > >>>>> } > >>>>> > >>>>> This will provide everything that DefaultGuiceModule already does. In > >>>>> addition, it will provide the bindings you specifically provided in > >>>>> MySpecialModule and override these when they conflict with ones > provided > >>>>> by DefaultGuiceModule. > >>>>> Take a look at Modules.override< > http://google-guice.googlecode.com/svn/trunk/javadoc/com/google/inject/util/Modules.html#override(com.google.inject.Module...) > >documentation. > >>>>> > >>>>> As far as appending a rewriter to existing list is concerned, can't > >>>>> help you much there. > >>>>> This sample code: > >>>>> class T { > >>>>> public final String name; > >>>>> public T(String name) { > >>>>> this.name = name; > >>>>> } > >>>>> } > >>>>> > >>>>> class A extends AbstractModule { > >>>>> @Override > >>>>> protected void configure() { > >>>>> > bind(T.class).annotatedWith(Names.named("hello")).toInstance(new > >>>>> T("hello")); > >>>>> } > >>>>> } > >>>>> class B extends AbstractModule { > >>>>> @Override > >>>>> protected void configure() { > >>>>> } > >>>>> > >>>>> @Provides @Named("hello") @Inject > >>>>> public T provideBuffalo(@Named("hello") T helloT) { > >>>>> return new T("buffalo"); > >>>>> } > >>>>> } > >>>>> > >>>>> Injector injector = Guice.createInjector(Modules.override(new > >>>>> A()).with(new B())); > >>>>> T h = injector.getInstance(Key.get(T.class, > Names.named("hello"))); > >>>>> assertEquals("buffalo", h.name); > >>>>> > >>>>> throws StackOverflowException, because guice is trying to access the > >>>>> same @Named("hello")that its trying to provide. > >>>>> So you'l have to enumerate the rewriters you need to append your > >>>>> rewriter to again in your module. > >>>>> > >>>>> > >>>>>> So I think replace DefaultGuiceModule with custom module class in > >>>>>> web.xml maybe not enough, you still need to do other same work as > >>>>>> DefaultGuiceModule in your module.Please note that RewriterModule > installed > >>>>>> in DefaultGuiceModule is not configured in web.xml. > >>>>>> > >>>>>> Any comments? Thanks a lot. > >>>>>> > >>>>>> Thanks, > >>>>>> > >>>>>> Kevin Zhang (凯峰) > >>>>>> Gtalk: [email protected] > >>>>>> Blog: http://www.zhangkf.com > >>>>>> Twitter: http://twitter.com/zhangkf > >>>>>> > >>>>>> > >>>>>> On Sat, Oct 30, 2010 at 10:28 AM, Gagandeep singh < > >>>>>> [email protected]> wrote: > >>>>>> > >>>>>>> Hi Kai, Paul > >>>>>>> > >>>>>>> You are both right :) The order of rewriters matters. > >>>>>>> > >>>>>>> I have a patch< > >>>>>>> > http://codereview.appspot.com/2058042/diff/123001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > >>>>>>> > > >>>>>>> > >>>>>>> (which > >>>>>>> JohnH has been reviewing and providing ideas for) which does > something > >>>>>>> similar. The idea is, we create a map binder from container + flow > id > >>>>>>> -> * > >>>>>>> provider* of list of rewriters. > >>>>>>> And we provide the list of rewriters similarly to how we providing > >>>>>>> currently. Since we add provider of rewriter list, we can provide a > >>>>>>> different list without having to add it to the map binder > separately > >>>>>>> (map > >>>>>>> binder will cry if you add same key twice). > >>>>>>> Any new binding can be added to map binder in the custom module. > >>>>>>> > >>>>>>> The way ppl can override this is, in your new module, you again > >>>>>>> provide a > >>>>>>> new list of rewriters. And then you override the DefaultGuiceModule > >>>>>>> with > >>>>>>> your module. > >>>>>>> > >>>>>>> Say you were providing a default list of GadgetRewriters in > >>>>>>> DefaultGuiceModule: > >>>>>>> @Named("gadget.rewriters") List<GadgetRewriter> > provideDef(Rewriter1 > >>>>>>> r1) { > >>>>>>> } > >>>>>>> > >>>>>>> and you want to change this list of provide Rewriter2 as well. You > >>>>>>> create > >>>>>>> your module called MyModule and provide: > >>>>>>> @Named("gadget.rewriters") List<GadgetRewriter> > provideDef(Rewriter1 > >>>>>>> r1, > >>>>>>> Rewriter2 r2) { > >>>>>>> } > >>>>>>> > >>>>>>> And then early on in the code, you override default guice module > with > >>>>>>> your > >>>>>>> module: > >>>>>>> Modules.override(new GuiceModule()).with(new MyModule()); > >>>>>>> > >>>>>>> I think your facing the problem because maybe web.xml does not > allow > >>>>>>> you to > >>>>>>> override a module with a new one. And when 2 modules provide same > >>>>>>> binding, > >>>>>>> guice dies. > >>>>>>> A quick solution for your case would be to change web.xml to > provide > >>>>>>> your > >>>>>>> module in place of DefaultGuiceModule, and change your module to > >>>>>>> install DefaultGuiceModule after overriding it with your rewriter > >>>>>>> list. > >>>>>>> Something like: > >>>>>>> > >>>>>>> web.xml: > >>>>>>> <context-param> > >>>>>>> <param-name>guice-modules</param-name> > >>>>>>> <param-value> > >>>>>>> org.apache.shindig.common.PropertiesModule: > >>>>>>> org.apache.shindig.gadgets.*MyGuiceModule*: > >>>>>>> org.apache.shindig.social.core.config.SocialApiGuiceModule: > >>>>>>> ...... > >>>>>>> > >>>>>>> MyGuiceModule.java: > >>>>>>> protected void configure() { > >>>>>>> Modules.override(new DefaultGuiceModule()).with(new > AbstractModule() > >>>>>>> { > >>>>>>> protected void configure() { > >>>>>>> } > >>>>>>> @Provides @Named("gadget.rewriters") List<GadgetRewriter> > >>>>>>> .... > >>>>>>> } > >>>>>>> } > >>>>>>> > >>>>>>> I hope im making sense. > >>>>>>> > >>>>>>> On Fri, Oct 29, 2010 at 5:52 PM, Paul Lindner <[email protected]> > >>>>>>> wrote: > >>>>>>> > >>>>>>> > I think this is a good idea, however I'm concerned that the > ordering > >>>>>>> of > >>>>>>> > Rewriters may be important. > >>>>>>> > > >>>>>>> > I'm guessing that we could make a SortedSet if we had a > Comparable<> > >>>>>>> > implementation of all rewriters. > >>>>>>> > > >>>>>>> > On Fri, Oct 29, 2010 at 1:23 AM, Kai Feng Zhang < > [email protected] > >>>>>>> > > >>>>>>> > wrote: > >>>>>>> > > >>>>>>> > > I found a possible solution to extend Shindig rewriter > capability, > >>>>>>> using > >>>>>>> > > multibinding in Guice. > >>>>>>> > > > >>>>>>> > > Please see: > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > >>>>>>> > http://google-guice.googlecode.com/svn/trunk/latest-javadoc/com/google/inject/multibindings/Multibinder.html > >>>>>>> > > > >>>>>>> > > I created a jira for this, see > >>>>>>> > > https://issues.apache.org/jira/browse/SHINDIG-1456 > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > On Fri, Oct 29, 2010 at 3:02 PM, Kai Feng Zhang < > >>>>>>> [email protected]> > >>>>>>> > > wrote: > >>>>>>> > > > >>>>>>> > > > Hi, > >>>>>>> > > > > >>>>>>> > > > I'd like to add a custom rewriter into Shindig, with > requirement > >>>>>>> of no > >>>>>>> > > need > >>>>>>> > > > to change Shindig rendering gadget server side code directly. > I > >>>>>>> want to > >>>>>>> > > add > >>>>>>> > > > it as a new feature in extras, so when this feature is > required > >>>>>>> by > >>>>>>> > > gadget, > >>>>>>> > > > the custom rewriter will do some special work when rendering > >>>>>>> gadget at > >>>>>>> > > > server side. > >>>>>>> > > > > >>>>>>> > > > But I checked Shindig code, RewriteModule @Provides all > >>>>>>> predefined > >>>>>>> > > > rewriters with @Named("shindig.rewriters.gadget") , and > >>>>>>> > > > then GadgetRewritersProvider will provide all rewriters as > per > >>>>>>> the same > >>>>>>> > > > @Name when rendering gadget. > >>>>>>> > > > > >>>>>>> > > > If I create a new Module in shindig extras, and @Provides > custom > >>>>>>> > rewriter > >>>>>>> > > > with same @Name, and startup server, there will be error > saying > >>>>>>> the > >>>>>>> > same > >>>>>>> > > > @Name is configured already by RewriteModule. > >>>>>>> > > > > >>>>>>> > > > Can anyone please help if there is some way to append such a > >>>>>>> rewriter > >>>>>>> > > > without touching any existing Shindig gadget rendering code? > Or > >>>>>>> that is > >>>>>>> > > the > >>>>>>> > > > only way to extend a new rewriter for gadget rendering? > >>>>>>> > > > > >>>>>>> > > > Thanks a lot! > >>>>>>> > > > > >>>>>>> > > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> > -- > >>>>>>> > Paul Lindner -- [email protected] -- linkedin.com/in/plindner > >>>>>>> > > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > > > > > -- > Thanks, > Henry >
