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! 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
