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" <[email protected]> wrote: > > Hi Josh, > > > On Thu, Feb 3, 2011 at 3:50 PM, Josh Canfield <[email protected] >wrote: > > > On Feb 3, 2011 12:12 AM, "Igor Drobiazko" <[email protected]> > > 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 <[email protected] > > >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: [email protected] > > > > For additional commands, e-mail: [email protected] > > > > > > > > > > > > > > > > > -- > > > Best regards, > > > > > > Igor Drobiazko > > > http://tapestry5.de > > > > > > -- > Best regards, > > Igor Drobiazko > http://tapestry5.de
