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 >
