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
