On Tue, Nov 9, 2010 at 1:28 AM, Paul Lindner <[email protected]> wrote:
> Thanks Sam. > > Any suggestions on the best way to support arbitrary ordered plugin modules > using multibindings? So far proposed solutions include: > > * Use separate bindings for separate "phases" > * Add a getPhase() method that returns an integer, sort the Multibinding > Set based on this. > * implement Comparable interface and have each class compare against the > list of dependent classes. Sort. > > Thanks! > The last one sounds the least appealing to me.. each class would have to know about each other class. The middle one is OK, but then an arbitrary plugin could still insert itself in the wrong place. Separate bindings sounds nice, because core/internal stuff can bind using core/internal binding annotations and prevent external stuff from being inserted. sam > > > > On Mon, Nov 8, 2010 at 8:46 PM, Sam Berlin <[email protected]> wrote: > >> It's unlikely that we'll add an OrderedSet or UniqueList into Guice >> itself. Guava may have something like this (or may consider adding it), but >> we've been hesitant to put a full Guava dependency in Guice since Guice is >> intended to be a small/lightweight library, and Guava is fairly big. >> >> sam >> >> >> On Mon, Nov 8, 2010 at 6:12 PM, Paul Lindner <[email protected]> wrote: >> >>> +sberlin >>> >>> On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <[email protected]> wrote: >>> >>>> If I were the Guice folks I would have proposed creating an OrderedSet, >>>> or >>>> perhaps UniquifiedList (if that's the point of Set here) interface in >>>> com.google.inject :) >>>> >>>> On Mon, Nov 8, 2010 at 3:03 PM, Henry Saputra <[email protected] >>>> >wrote: >>>> >>>> > Well, the concern is the definition of Set itself which is/should not >>>> > guarantee sequential/ordering of the contents. >>>> > >>>> > The default implementation does return an OrderedSet (via Guice module >>>> > based on the individual call to bind it) but the contract API change >>>> > from List to Set which does not guarantee ordering. >>>> > >>>> > >>>> > - Henry >>>> > >>>> > On Mon, Nov 8, 2010 at 2:52 PM, Paul Lindner <[email protected]> >>>> wrote: >>>> > > The nice part about the patch is that MultiBinding results in an >>>> Ordered >>>> > Set >>>> > > based on the order of the individual bind statements. >>>> > > >>>> > > So this means a third party module can only add a rewriter at the >>>> > beginning >>>> > > of the pipeline (by using a Guice Module that loads before the >>>> > > RewriterModule) or by adding a rewriter at the end of the pipeline >>>> (by >>>> > > adding a Guice Module that runs after the Rewrite Module) >>>> > > >>>> > > I suspect that one could also rejigger the order of the rewriters >>>> using >>>> > > Modules.override(), but that remains to be seen. >>>> > > >>>> > > On Mon, Nov 8, 2010 at 2:41 PM, John Hjelmstad <[email protected]> >>>> wrote: >>>> > > >>>> > >> Hi Paul: >>>> > >> >>>> > >> Fundamental problem here is that rewriter ordering is important: a >>>> Set >>>> > >> obviously doesn't offer that guarantee. >>>> > >> >>>> > >> I'd recommend we rejigger this implementation so that the >>>> Set-provided >>>> > >> rewriters come as an addendum to those provided as "core" by >>>> Shindig. If >>>> > >> relative ordering of *those* Rewriters is important, we can have >>>> > pre-core >>>> > >> and post-core Sets. >>>> > >> >>>> > >> Apologies for not having seen this in JIRA; my mailbox has gotten >>>> so >>>> > >> overstuffed of late that some of the messages get lost in the >>>> aether. >>>> > >> >>>> > >> --j >>>> > >> >>>> > >> On Thu, Nov 4, 2010 at 3:58 PM, <[email protected]> wrote: >>>> > >> >>>> > >> > Author: lindner >>>> > >> > Date: Thu Nov 4 22:58:54 2010 >>>> > >> > New Revision: 1031332 >>>> > >> > >>>> > >> > URL: http://svn.apache.org/viewvc?rev=1031332&view=rev >>>> > >> > Log: >>>> > >> > SHINDIG-1456 | Patch from Kai Feng Zhang | Allow modules to add >>>> new >>>> > >> > rewriters >>>> > >> > >>>> > >> > Modified: >>>> > >> > >>>> > >> > >>>> > >> >>>> > >>>> >>>> shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java >>>> > >> > >>>> > >> > >>>> > >> >>>> > >>>> >>>> shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java >>>> > >> > >>>> > >> > >>>> > >> >>>> > >>>> >>>> shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java >>>> > >> > >>>> > >> > Modified: >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java >>>> > >> > URL: >>>> > >> > >>>> > >> >>>> > >>>> http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java?rev=1031332&r1=1031331&r2=1031332&view=diff >>>> > >> > >>>> > >> > >>>> > >> >>>> > >>>> ============================================================================== >>>> > >> > --- >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java >>>> > >> > (original) >>>> > >> > +++ >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java >>>> > >> > Thu Nov 4 22:58:54 2010 >>>> > >> > @@ -19,13 +19,13 @@ >>>> > >> > >>>> > >> > package org.apache.shindig.gadgets.render; >>>> > >> > >>>> > >> > -import com.google.inject.Inject; >>>> > >> > -import com.google.inject.name.Named; >>>> > >> > +import java.util.Set; >>>> > >> > >>>> > >> > import org.apache.shindig.gadgets.GadgetContext; >>>> > >> > import org.apache.shindig.gadgets.rewrite.GadgetRewriter; >>>> > >> > >>>> > >> > -import java.util.List; >>>> > >> > +import com.google.inject.Inject; >>>> > >> > +import com.google.inject.name.Named; >>>> > >> > >>>> > >> > /** >>>> > >> > * Class to provide list of rewriters according to gadget >>>> request. >>>> > >> > @@ -34,15 +34,14 @@ import java.util.List; >>>> > >> > * @since 2.0.0 >>>> > >> > */ >>>> > >> > public class GadgetRewritersProvider { >>>> > >> > - private final List<GadgetRewriter> renderRewriters; >>>> > >> > + private final Set<GadgetRewriter> renderRewriters; >>>> > >> > >>>> > >> > @Inject >>>> > >> > - public GadgetRewritersProvider( >>>> > >> > - @Named("shindig.rewriters.gadget") List<GadgetRewriter> >>>> > >> > renderRewriters) { >>>> > >> > + public >>>> GadgetRewritersProvider(@Named("shindig.rewriters.gadget") >>>> > >> > Set<GadgetRewriter> renderRewriters) { >>>> > >> > this.renderRewriters = renderRewriters; >>>> > >> > } >>>> > >> > >>>> > >> > - public List<GadgetRewriter> getRewriters(GadgetContext >>>> context) { >>>> > >> > + public Set<GadgetRewriter> getRewriters(GadgetContext context) >>>> { >>>> > >> > return renderRewriters; >>>> > >> > } >>>> > >> > } >>>> > >> > >>>> > >> > Modified: >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java >>>> > >> > URL: >>>> > >> > >>>> > >> >>>> > >>>> http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java?rev=1031332&r1=1031331&r2=1031332&view=diff >>>> > >> > >>>> > >> > >>>> > >> >>>> > >>>> ============================================================================== >>>> > >> > --- >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java >>>> > >> > (original) >>>> > >> > +++ >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java >>>> > >> > Thu Nov 4 22:58:54 2010 >>>> > >> > @@ -18,12 +18,8 @@ >>>> > >> > */ >>>> > >> > package org.apache.shindig.gadgets.rewrite; >>>> > >> > >>>> > >> > -import com.google.common.collect.ImmutableList; >>>> > >> > -import com.google.inject.AbstractModule; >>>> > >> > -import com.google.inject.Provides; >>>> > >> > -import com.google.inject.Singleton; >>>> > >> > -import com.google.inject.name.Named; >>>> > >> > -import com.google.inject.name.Names; >>>> > >> > +import java.util.List; >>>> > >> > + >>>> > >> > import org.apache.shindig.gadgets.parse.GadgetHtmlParser; >>>> > >> > import org.apache.shindig.gadgets.render.CajaResponseRewriter; >>>> > >> > import >>>> > org.apache.shindig.gadgets.render.OpenSocialI18NGadgetRewriter; >>>> > >> > @@ -33,7 +29,13 @@ import org.apache.shindig.gadgets.render >>>> > >> > import >>>> org.apache.shindig.gadgets.rewrite.image.BasicImageRewriter; >>>> > >> > import org.apache.shindig.gadgets.servlet.CajaContentRewriter; >>>> > >> > >>>> > >> > -import java.util.List; >>>> > >> > +import com.google.common.collect.ImmutableList; >>>> > >> > +import com.google.inject.AbstractModule; >>>> > >> > +import com.google.inject.Provides; >>>> > >> > +import com.google.inject.Singleton; >>>> > >> > +import com.google.inject.multibindings.Multibinder; >>>> > >> > +import com.google.inject.name.Named; >>>> > >> > +import com.google.inject.name.Names; >>>> > >> > >>>> > >> > /** >>>> > >> > * Guice bindings for the rewrite package. >>>> > >> > @@ -45,25 +47,23 @@ public class RewriteModule extends Abstr >>>> > >> > bind(ResponseRewriterRegistry.class) >>>> > >> > >>>> > >> > >>>> > >> >>>> > >>>> .annotatedWith(Names.named("shindig.accelerate.response.rewriter.registry")) >>>> > >> > .to(AccelResponseRewriterRegistry.class); >>>> > >> > + >>>> > >> > + configureRewriters(); >>>> > >> > + >>>> > >> > } >>>> > >> > >>>> > >> > - @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) { >>>> > >> > - return ImmutableList.of(pipelineRewriter, templateRewriter, >>>> > >> > - absolutePathRewriter, styleTagExtractorRewriter, >>>> > >> > styleAdjacencyRewriter, proxyingRewriter, >>>> > >> > - cajaRewriter, sanitizedRewriter, renderingRewriter, >>>> > >> i18nRewriter); >>>> > >> > + private void configureRewriters() { >>>> > >> > + Multibinder<GadgetRewriter> multibinder = >>>> > >> > Multibinder.newSetBinder(binder(), GadgetRewriter.class, >>>> > >> > Names.named("shindig.rewriters.gadget")); >>>> > >> > + >>>> multibinder.addBinding().to(PipelineDataGadgetRewriter.class); >>>> > >> > + multibinder.addBinding().to(TemplateRewriter.class); >>>> > >> > + >>>> multibinder.addBinding().to(AbsolutePathReferenceRewriter.class); >>>> > >> > + >>>> > multibinder.addBinding().to(StyleTagExtractorContentRewriter.class); >>>> > >> > + >>>> multibinder.addBinding().to(StyleAdjacencyContentRewriter.class); >>>> > >> > + multibinder.addBinding().to(ProxyingContentRewriter.class); >>>> > >> > + multibinder.addBinding().to(CajaContentRewriter.class); >>>> > >> > + multibinder.addBinding().to(SanitizingGadgetRewriter.class); >>>> > >> > + multibinder.addBinding().to(RenderingGadgetRewriter.class); >>>> > >> > + >>>> multibinder.addBinding().to(OpenSocialI18NGadgetRewriter.class); >>>> > >> > } >>>> > >> > >>>> > >> > @Provides >>>> > >> > >>>> > >> > Modified: >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java >>>> > >> > URL: >>>> > >> > >>>> > >> >>>> > >>>> http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java?rev=1031332&r1=1031331&r2=1031332&view=diff >>>> > >> > >>>> > >> > >>>> > >> >>>> > >>>> ============================================================================== >>>> > >> > --- >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java >>>> > >> > (original) >>>> > >> > +++ >>>> > >> > >>>> > >> >>>> > >>>> shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java >>>> > >> > Thu Nov 4 22:58:54 2010 >>>> > >> > @@ -42,6 +42,7 @@ import java.util.Collection; >>>> > >> > import java.util.concurrent.Callable; >>>> > >> > >>>> > >> > import com.google.common.collect.ImmutableList; >>>> > >> > +import com.google.common.collect.ImmutableSet; >>>> > >> > >>>> > >> > /** >>>> > >> > * Tests for HtmlRenderer >>>> > >> > @@ -84,7 +85,7 @@ public class HtmlRendererTest { >>>> > >> > @Before >>>> > >> > public void setUp() throws Exception { >>>> > >> > renderer = new HtmlRenderer(preloaderService, proxyRenderer, >>>> > >> > - new >>>> GadgetRewritersProvider(ImmutableList.of((GadgetRewriter) >>>> > >> > captureRewriter)), >>>> > >> > + new >>>> GadgetRewritersProvider(ImmutableSet.of((GadgetRewriter) >>>> > >> > captureRewriter)), >>>> > >> > null); >>>> > >> > >>>> > >> > } >>>> > >> > >>>> > >> > >>>> > >> > >>>> > >> >>>> > > >>>> > > >>>> > > >>>> > > -- >>>> > > Paul Lindner -- [email protected] -- linkedin.com/in/plindner >>>> > > >>>> > >>>> > >>>> > >>>> > -- >>>> > Thanks, >>>> > Henry >>>> > >>>> >>> >>> >>> >>> -- >>> Paul Lindner -- [email protected] -- linkedin.com/in/plindner >>> >> >> > > > -- > Paul Lindner -- [email protected] -- linkedin.com/in/plindner >
