tapestry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Josh Canfield <joshcanfi...@gmail.com>
Subject Re: Injection calculation is difficult when building a service advisor
Date Thu, 03 Feb 2011 15:57:36 GMT
I'm not in front of my computer to test yet, but I believe that
@InjectService would have the same problem because its exactly the same code
except it gets the id from the annotation.

I believe I'm using the registry via the locator which is what implements
the exception you are referring to...

I'm heading into the office in an hour and I'll re-verify.

On Feb 3, 2011 7:21 AM, "Igor Drobiazko" <igor.drobiazko@gmail.com> wrote:
>
> 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