tapestry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Igor Drobiazko <igor.drobia...@gmail.com>
Subject Re: Injection calculation is difficult when building a service advisor
Date Thu, 03 Feb 2011 15:20:59 GMT
Hi Josh,


On Thu, Feb 3, 2011 at 3:50 PM, Josh Canfield <joshcanfield@gmail.com>wrote:

> On Feb 3, 2011 12:12 AM, "Igor Drobiazko" <igor.drobiazko@gmail.com>
> wrote:
> >
> > I have strong objections against your suggestion as this change would
> > completely change the IoC experience.
> >
> I'm not sure I agree. There is no change in experience as far as I can
> tell.
>
> > Guessing service id can be very dangerous.
> There is no guessing, this is the default. I thought this was documented,
> perhaps its just a comment in the code.
>

This is not quite correct. When binding or building a service, you may
provide an id. If not provided, then Tapestry will take the default one.
This is documented. But resolving a service implementation is a completely
another story. In this case you are guessing as user didn't tell you which
implementation to inject. As of now you will get an exception because
Tapestry is not able to disambiguate and IMO it should stay this way.Now if
you change the code as you suggest, you might inject an implementation which
you shouldn't. The user would not see a message "Please disambiguate your
service" because you take the default id. This will definitely change the
current behaviour and break any existing app.

>
> > Just imagine a Tapestry beginner
> > who found a service interface in the javadoc. He injects the service into
> > his service/page/component and, if there are several implementations of
> the
> > interface, he will get a meaningful exception, saying that he must
> > disambiguate the implementation to be injeted. This is the point where
> the
>
> This is still true after my change.
>
It's not. Just take a look at the example which I provided in the last mail.
In this scenarion, the following code will cause an exception. After your
change it will not as, MyServiceRed will be injected.

public class Foo {
   @Inject
   private MyService myService;
}

>
> > user begns dig and read the documentation deeper. Now if you try to
> inject
> > by "default name", you could occasionally inject a wrong implementation.
> The
> > user will never notice that and will wonder why his app behaves strange.
> > Just as example.
> >
> > public static void bind(ServiceBinder binder) {
> >   binder.bind(MyService.class, MyServiceRed.class);
> >   binder.bind(MyService.class, MyServiceBlue.class).withId("Green");
> > }
> >
>
> This still works. As I mentioned this code passes all of the unit tests
> unchanged. It only shortcuts entrance into the master object provider.
>

Well, if you find the test which explicitely checks that disambiguation
exception is still thrown after your change, than you can be sure. If no
test fails, it might be a sign of missing test cases.

>
> > Having said that: -1 for this change.
> >
> > BTW I'm woundering why you need @InjectService annotation to inject your
> > MonitorAdvisor into the advise method. Do you have several
> implementations
> > of MonitorAdvisor? If not, just remove the annotations and it will work.
> :) if this were true then I wouldn't have added the annotations in the
> first
> place. I spent a long time playing with preventdecoration and scoured the
> codebase for examples.
>

It is true. Here is a code from a production application:

    @Advise(serviceInterface = AssetSource.class)
    public static void adviseAssetSource(MethodAdviceReceiver receiver,
TemplateClassifier classifier) {
        receiver.adviseAllMethods(new
DeviceAwareAssetSourceAdvisor(classifier));
    }


>
> > Also the constructor of MonitorAdvisorImpl looks strange to me. You don't
> > need @InjectService annotation at all.
>
> I agree, I shouldn't need @InjectService, but I do. If you look at the
> error
> provided by the recursion exception (when I don't use @InjectService) you
> see why I have to use the annotations. The annotations are evaluated before
> the masterobjectprovider when calculating injection parameters. I've just
> moved the default service id logic up a level to get the same behavior.
>
> Howard suggested using @Local, but I don't think I should need anything. I
> did notice that the only similar code was the LoggingAdvisor which is
> created in one module but only used in a @Match(*) in a demo module.
>
> Josh
>
> >
> > On Thu, Feb 3, 2011 at 2:48 AM, Josh Canfield <joshcanfield@gmail.com
> >wrote:
> >
> > > I started writing a long winded description of my problem, but let's
> > > start with the gist and then get into the details and finally a
> > > solution.
> > >
> > > The gist:
> > > I struggled for too long to figure out how to build a service advisor
> > > that uses services, and is a service. Not due to recursion into my own
> > > advisor, but due to recursion generated by the MasterObjectProvider
> > > when it attempts to construct it's various ObjectProvider instances.
> > >
> > > The details:
> > >
> > > One form of the error:
> > > [ERROR] Registry [ 1] Resolving object of type
> > > org.apache.tapestry5.services.ApplicationGlobals using
> > > MasterObjectProvider
> > > [ERROR] Registry [ 2] Realizing service AssetObjectProvider
> > > [ERROR] Registry [ 3] Invoking
> > >
> > >
>
> org.apache.tapestry5.monitor.MonitorModule.adviseForMonitoredServices(MethodAdviceReceiver,
> > > MonitorAdvisor) (at MonitorModule.java:52)
> > > [ERROR] Registry [ 4] Reloading class
> > > org.apache.tapestry5.internal.monitor.MonitorAdvisorImpl.
> > > [ERROR] Registry [ 5] Determining injection value for parameter #2
> > > (org.apache.tapestry5.monitor.MonitorNamingStrategy)
> > > [ERROR] Registry [ 6] Resolving object of type
> > > org.apache.tapestry5.monitor.MonitorNamingStrategy using
> > > MasterObjectProvider
> > > [ERROR] Registry [ 7] Realizing service AssetObjectProvider <---
> > > AHHH!!!! I don't need an AssetObjectProvider
> > >
> > > I can work around the problem by using 'InjectService("MyService")
> > > MyService' because by default the service id maps to the simple name
> > > of the service interface and the MasterObjectProvider isn't consulted
> > > about getting my Service. But I'm hopefully not alone in thinking this
> > > is silly.
> > >
> > > I am building service advice based on an annotation, I haven't seen
> > > anything in the system that puts all these together using the current
> > > advice (as opposed to decoration), but if you have I'd love to see it.
> > >
> > > This is what I have that works with the existing system.
> > >
> > >    @Match("*")
> > >    public static void adviceForMonitoredServices(
> > >            MethodAdviceReceiver receiver,
> > >            @InjectService("MonitorAdvisor") MonitorAdvisor
> monitorAdvisor)
> > > {
> > >        monitorAdvisor.monitor(receiver);
> > >    }
> > >
> > >  public MonitorAdvisorImpl(
> > >            Logger logger,
> > >            @InjectService("MonitorNamingStrategy")
> > > MonitorNamingStrategy monitorNamingStrategy,
> > >            @InjectService("MonitorJmxObjectNamingStrategy")
> > > MonitorJmxObjectNamingStrategy jmxObjectNamingStrategy,
> > >            @InjectService("MBeanSupport") MBeanSupport mBeanSupport) {
> > >
> > > The solution:
> > >
> > > If I modify the InternalUtils.calculateInjection method so that it
> > > checks for the simple name of the service before delegating to the
> > > master object provider then I don't have to use the @InjectService
> > > annotation anymore.
> > >
> > >        // by default services are registered by their simple name,
> > > see if we can get to it that way
> > >        // although, if there is a marker annotation then you can't
> > > load the service this way.
> > >        if ( provider.getAnnotation(Marker.class) != null) {
> > >            try
> > >            {
> > >                return
> > > locator.getService(injectionType.getSimpleName(), injectionType);
> > >            }
> > >            catch (RuntimeException e)
> > >            {
> > >                // ignored, it will fall through to the master object
> > > provider
> > >            }
> > >        }
> > >
> > > I've run the ioc,core, spring and hibernate unit tests and they pass.
> > > The fact that the simple name is used as the default id is pervasive
> > > so I don't imagine using it here breaks any encapsulation.
> > >
> > > What use cases have I missed that will cause this code to break
> > > something not covered in the unit tests?
> > >
> > > Thanks,
> > > Josh
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
> > > For additional commands, e-mail: dev-help@tapestry.apache.org
> > >
> > >
> >
> >
> > --
> > Best regards,
> >
> > Igor Drobiazko
> > http://tapestry5.de
>



-- 
Best regards,

Igor Drobiazko
http://tapestry5.de

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message