Ok, so you want to use the ClientContext (or a thread local) internally in Features.isEnabled(String), right? I'm fine with that.
Now, it still does not solve the problem at hand, or I'm misunderstanding something :) Let's have a look at the flow a) a request comes in b) global filter sets the context to the current request object c) resourceresolver.resolve is called - this results in features to be tested d) request filter sets the context to the current sling request including the servlet resolver The features called during c) don't have access to the resource resolver - or do you create a context in that case containing the resource resolver as you're in the implementation anyway? Carsten 2014-01-29 Felix Meschberger <[email protected]> > Hi > > Am 29.01.2014 um 14:38 schrieb Carsten Ziegeler <[email protected]>: > > > This doesn't solve the problem - see previous mails as the resource > > resolver is not available when it should be. > > If we drop the "eager evaluation" requirement, it helps. > > While at the same time, application use easier: > > Today, the application does: > > Features f = getFeaturesService(); > ClientContext cc = f.getCurrentClientContext(); > if (cc.isEnabled(featureName)) { > ... > } > > Compare this to: > > > Features f = getFeaturesService(); > > if (f.isEnabled(featureName)) { > > ... > > } > > BTW: We need a JSP taglib for this :-) > > > > I like the ClientContext approach as this gives you context information, > > especially which features are enabled - which e.g. might be interesting > for > > debugging etc. > > Why can't you iterate over existing features and ask each feature in turn > whether it is enabled or not for the debugging case ? I am not really at > ease adding functionality just-in-case. > > We can still provide a Features.getEnabledFeatures method. But this would > be evaluated on demand, IMHO. > > Our primary use case is real-life applications. > > Regards > Felix > > > > > Carsten > > > > > > 2014-01-29 Felix Meschberger <[email protected]> > > > >> Hi > >> > >> I think we have an API problem ;-) > >> > >> One reason for eager resolution would probably be to have a > >> ClientContext.getEnabledFeatures() method. I somehow fail to understand > why > >> we have this method in the first place. > >> > >> So if we remove getEnabledFeatures() from ClientContext we are left with > >> isEnabled(String). A single method makes the a dubious case for an > >> interface and we might rather have this on the Features service itself. > >> > >> This drops ClientContext from the public API. > >> > >> Then, adding Features.isEnabled(String), we need to have contextual > >> information there. But this could be managed internally by the Features > >> service. > >> > >> The filters would then just call (a new method) > >> setRequest(HttpServletRequest) and dropRequest(HttpServletRequest) which > >> just causes the internal contextual information to be updated. > >> > >> Results of calling Features.isEnabled(String) could be cached in the > >> internal contextual information and be dropped on calls to setRequest > and > >> dropRequest > >> > >> WDYT ? > >> > >> Regards > >> Felix > >> > >> Am 29.01.2014 um 13:40 schrieb Carsten Ziegeler <[email protected]>: > >> > >>> I think we should avoid the double evaluation - especially as this > might > >>> result in interesting situations, a feature is enabled in the global > >> filter > >>> by disabled in the per request filter. > >>> Apart from enabling a feature based on request information, features > >> might > >>> be enabled based on user information - which can only get once the > >> resource > >>> resolver is enabled - so it makes sense to have this in the > >>> ExecutionContext. > >>> > >>> I guess lazy evaluation is the only way to solve this - the other would > >> be > >>> to not use filters and add the code to the sling main servlet, > something > >>> which I would try avoid :) > >>> > >>> If you want to, I can give the lazy evaluation a try? > >>> > >>> Carsten > >>> > >>> > >>> 2014-01-29 Felix Meschberger <[email protected]> > >>> > >>>> Hi > >>>> > >>>> Thanks for reporting. This is actually a consequence of me removing > the > >>>> global ClientContext filter yesterday. I have fixed it now in trunk > (as > >>>> well as another bug causing negative features to not be evaluated > >>>> correctly). > >>>> > >>>> Why is the global filter needed ? Because the initial resource > >> resolution > >>>> takes place *before* the Sling Filters are called. Thus, the only way > to > >>>> setup the "global" per-request client context properly before resource > >>>> resolver kicks in is to have the global filter set. > >>>> > >>>> Hence we have two filters again. > >>>> > >>>> So maybe, we should start thinking about whether eager evaluation of > the > >>>> flags might become too expensive or how we can actually improve in > other > >>>> ways ? > >>>> > >>>> Regards > >>>> Felix > >>>> > >>>> Am 29.01.2014 um 11:03 schrieb Simone Tripodi < > [email protected] > >>> : > >>>> > >>>>> Hi all mates, > >>>>> > >>>>> I am using the new FeatureFlags APIs to implement my own Flags rules > >> but > >>>> I > >>>>> noticed that Features are never evaluated, I mean that I cannot track > >>>>> any Feature#isEnabled(ExecutionContext) method invocation. > >>>>> > >>>>> It is enough for a resource having a sling:features property with a > >> valid > >>>>> service name to be disabled -and dropping it re-enables the Resource- > >> it > >>>>> doesn't matter which boolean value the > >>>>> method Feature#isEnabled(ExecutionContext) returns, it is not hit > >> neither > >>>>> evaluated. > >>>>> > >>>>> How can I provide you a valid IT to demonstrate it? > >>>>> > >>>>> TIA, all the best! > >>>>> -Simo > >>>>> > >>>>> http://people.apache.org/~simonetripodi/ > >>>>> http://twitter.com/simonetripodi > >>>> > >>>> > >>> > >>> > >>> -- > >>> Carsten Ziegeler > >>> [email protected] > >> > >> > > > > > > -- > > Carsten Ziegeler > > [email protected] > > -- Carsten Ziegeler [email protected]
