Dave-- Do you want me to try to pick this one up? There doesn't seem to be much objection to using a hibernate interceptor, so I could prototype this solution next week.
-Mark -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Mark Goodrich Sent: Thursday, September 01, 2011 9:52 AM To: [email protected] Subject: Re: [OPENMRS-DEV] sync and namephonetics, and openmrs handlers One key point is that we would be using a hibernate interceptor in Name Phonetics for both sync purposes and standard purposes. (That is, the Hibernate interceptor would completely replace the current AOP interceptor--we could maintain both, but this seems ugly). However, I still agree that this seems like the best solution (or at least the best solution that doesn't require a change to core). Mark -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Michael Seaton Sent: Thursday, September 01, 2011 9:23 AM To: [email protected] Subject: Re: [OPENMRS-DEV] sync and namephonetics, and openmrs handlers If it works, using a hibernate interceptor seems to me to be the best of the available options. Although I understand the desire to keep us from "baking in hibernate", I think for all practical purposes, that ship has sailed. And particularly since sync uses hibernate interceptors and bypasses the API, it seems like a logical extension to use the same mechanism within modules for sync purposes. If one day we wish to remove this hibernate dependency, we can always revisit this question and put in an alternative solution... The advantages that I see with this approach: * It's more reliable than AOP (eg. AOP only works if only those method calls it is listening for are the ones that can save Person Names) * It may reduce the AOP overhead * It requires no dependencies at all between sync and namephonetics to work * It does not force us to make namephonetics extend OpenmrsObject * It reduces the payload of sync transmissions as namephonetics would not be synched, but generated locally * It will work even if namephonetics is not installed on all servers within a sync network Any other opinions? Is anything I have put above incorrect? Thanks, Mike On 09/01/2011 04:40 AM, Dave Thomas wrote: > I like Ben's idea of adding the 'intendedTarget' to the Handler > interface in core even if we don't use it in this case. This creates > a nice mechanism for modules to register handlers intended for each > other's services, which will be useful i think in the long run. > > This however, creates a 'must update' situation for both rwanda and > haiti, which is difficult. And this handler fix would need to be > applied to all branches 1.6 onwards. > > But true, we could move the PersonName logic to a hibernate > interceptor, now that we've gone down that road. Although we'd have > to be explicit about this being a post-processing event to avoid the > famous hibernate cascade error, but maybe this is a trivial concern. > > Opinions on baking hibernate into namephonetics? > > d > > On 8/31/11, Friedman, Roger (CDC/CGH/DGHA) (CTR)<[email protected]> wrote: >> Are we baking in Hibernate now? >> >> -----Original Message----- >> From: [email protected] [mailto:[email protected]] On Behalf Of Mark Goodrich >> Sent: Wednesday, August 31, 2011 5:33 PM >> To: [email protected] >> Subject: Re: [OPENMRS-DEV] sync and namephonetics, and openmrs handlers >> >> Just talked with Mike about this. I'm not that familiar with Hibernate >> interceptors, but is there any reason we can't change Name Phonetic so that >> instead of AOP'ing around savePatient it uses a Hibernate interceptor to >> intercept all saves of PersonName at the hibernate level? >> >> Mark >> >> -----Original Message----- >> From: [email protected] [mailto:[email protected]] On Behalf Of Dave Thomas >> Sent: Friday, August 26, 2011 7:57 AM >> To: [email protected] >> Subject: Re: [OPENMRS-DEV] sync and namephonetics, and openmrs handlers >> >> Thanks ben. I'd still like to do this without adding anything to core >> in the short term, because i don't have the bandwidth to update the >> war file on our servers before our 1.8 migration in a few months. At >> that point, however, it would be nice to have the key property to use >> in 1.8. >> >> I also don't know how widely used namephonetics is -- if PIH is the >> only consumer, then putting a stack trace in the logs at startup for >> the next 2 months before implementing a proper solution bothers me a >> lot less. I'm going to wait for Mark G. to get back from vacation >> first, before doing anything anyway. >> >> d >> >> On Fri, Aug 26, 2011 at 12:06 PM, Ben Wolfe<[email protected]> wrote: >>> Well, its mostly independent. Sync still knows about name phonetics in >>> this case, which is not ideal or scalable. >>> >>> You should be able to use the generic handler interface and method on the >>> Context to get all handlers for the NamePhonetics class. The only question >>> is how to get a specific class of handler so you're sure the handler you >>> found is the one specified for sync. The simplest solution would be to >>> add >>> another option to the @Handler annotation for a "key" or something else. >>> It'd just be a string that is known between SyncService and NamePhonetics >>> to >>> know whether it has the right handler or not. >>> >>> Ben >>> >>> On Wed, Aug 24, 2011 at 5:11 PM, Dave Thomas<[email protected]> wrote: >>>> Hi. AOP'ing around SyncService from NamePhonetics seems to work, >>>> although if you're using NamePhonetics without sync, you get a >>>> ClassNotFoundException stack trace in the logs during module start, >>>> which is a little sloppy. Also, I'm not totally sure what the rules >>>> are: against all versions of OpenMRS past 1.6.x, is this still how >>>> exceptions while loading an AOP advice class from a module will be >>>> handled (i.e. other AOP advice classes from the module will still be >>>> loaded even if the first AOP errors during loading)? Also, there's >>>> no danger of NamePhonetics being loaded before Sync causing this to >>>> fail, right? This is why the context is built twice at startup? >>>> >>>> -------------- >>>> >>>> Here's short-term option 2: >>>> >>>> in NamePhonetics, you add this handler: >>>> >>>> >>>> @Handler(supports = PersonName.class) >>>> public class PersonNamePhoneticsSaveHandler implements >>>> RequiredDataHandler<PersonName>{ >>>> >>>> public void handle(PersonName personName, User currentUser, Date >>>> currentDate, String other) { >>>> >>>> NamePhoneticsUtil.savePhoneticsForPersonName(personName); >>>> >>>> } >>>> } >>>> >>>> >>>> and in sync, you add the following postProcessor to SyncService: >>>> >>>> >>>> public class NamePhoneticsSaveOrUpdateAfterAdvice implements >>>> AfterReturningAdvice { >>>> >>>> >>>> private static final String NAME_PHONETICS_HANDLER_CLASS = >>>> "org.openmrs.module.namephonetics.advice.PersonNamePhoneticsSaveHandler"; >>>> private RequiredDataHandler<PersonName> handler = null; >>>> >>>> >>>> @SuppressWarnings("unchecked") >>>> public synchronized void afterReturning(Object returnVal, Method >>>> method, Object[] arg2, Object arg3) throws Throwable { >>>> String methodName = method.getName(); >>>> if (methodName.equals("saveOrUpdate")) { >>>> //check for valid main argument >>>> if (arg2 == null || arg2.length == 0) >>>> return; >>>> Object mainArgument = arg2[0]; >>>> if (mainArgument == null) >>>> return; >>>> if (mainArgument instanceof PersonName){ >>>> Class<?> handlerClass = null; >>>> try { >>>> handlerClass = >>>> Class.forName(NAME_PHONETICS_HANDLER_CLASS, false, >>>> OpenmrsClassLoader.getInstance()); >>>> } catch (Exception ex){ >>>> handler = null; >>>> //pass on doing anything else... >>>> } >>>> if (handlerClass != null){ >>>> if (handler == null) >>>> handler = >>>> (RequiredDataHandler<PersonName>) >>>> HandlerUtil.getPreferredHandler(handlerClass, PersonName.class); >>>> handler.handle((PersonName) >>>> mainArgument, null, null, null); >>>> } >>>> } >>>> } >>>> } >>>> } >>>> >>>> >>>> This approach creates true module independence with no changes to >>>> core, and no problems just running sync, or just running >>>> namephonetics. >>>> >>>> The sort-of-lame thing about this is that the Sync post-processor >>>> looks for a handler that's an implementation of RequiredDataHandler, >>>> which was the most useful handler interface i could find in core. It >>>> might be useful to create a new core handler interface with the single >>>> argument handle method: handle(OpenmrsObject o). Maybe >>>> GenericOpenmrsObjectHandler? This interface could also require Ben's >>>> toBeUsedBy(String className) method (where in this case this method >>>> would return org.openmrs.module.sync). >>>> >>>> Another lesser sort-of-lame thing is having such a specific item in >>>> Sync at all -- this was the best i could come up with without pushing >>>> something new into 1.6.x core, which i don't want to do because i >>>> don't have the bandwidth for upgrading the war file on all of our >>>> remote servers. >>>> >>>> Thoughts? I have both of these implemented, and i can apply either one. >>>> >>>> I really like the new handler interface idea, but it depends on an >>>> upgrade to core, which then introduces core version dependencies to >>>> both NamePhonetics and Sync. >>>> >>>> d >>>> >>>> On 8/24/11, Ben Wolfe<[email protected]> wrote: >>>>> The quickest solution would be to AOP around the sync save, check to >>>>> see >>>>> if >>>>> its a personname, and update accordingly. >>>>> >>>>> Ben >>>>> >>>>> On Wed, Aug 24, 2011 at 2:40 AM, Darius Jazayeri >>>>> <[email protected]>wrote: >>>>> >>>>>> I agree that this seems like something that's more appropriate to >>>>>> calculate >>>>>> on the child after importing a name. >>>>>> >>>>>> The problem is that we do our post-service-call actions using AOP >>>>>> rather >>>>>> than having a proper event-notification mechanism. (Dave, I assume >>>>>> that >>>>>> namephonetics does AOP-after savePerson, savePersonName, etc.) >>>>>> >>>>>> Ideally sync would be able to call a core method like this: >>>>>> save(importedObject); // uses something internal to sync. >>>>>> publishEvent(importedObject, "saved"); >>>>>> >>>>>> Another solution (also requiring core changes) would be if there was a >>>>>> generic save method would would delegate to the appropriate service >>>>>> method. >>>>>> So that sync doesn't have to do its own hibernateSession.save(object) >>>>>> which >>>>>> bypasses the service layer. >>>>>> >>>>>> I know that Rafal implemented something like this in the Metadata >>>>>> Sharing >>>>>> Module--it actually scans everything that implements OpenmrsService on >>>>>> the >>>>>> classpath, and finds save(OpenmrsObject) methods. >>>>>> >>>>>> -Darius >>>>>> >>>>>> On Mon, Aug 22, 2011 at 12:41 AM, Ben Wolfe<[email protected]> wrote: >>>>>> >>>>>>> Am I remembering right that NamePhonetics has a table listing out the >>>>>>> manipulations of the names? That it doesn't do that calculation on >>>>>>> the >>>>>>> fly? It does sound a lot like conceptword in that you don't need to >>>>>>> send >>>>>>> those back an forth, instead have them re-calculated on the receiving >>>>>>> server. >>>>>>> >>>>>>> We do have the @Handler annotation in trunk. But its usually used as >>>>>>> an >>>>>>> @Handler(supports=NamePhonetics.class) PLUS extended a special class. >>>>>>> This >>>>>>> class would have to be in the sync module. Its possible that you >>>>>>> extend >>>>>>> a >>>>>>> class from sync and then use the aware_of_module tag in config.xml. >>>>>>> This >>>>>>> is >>>>>>> a loose module requirement that Daniel did >>>>>>> https://tickets.openmrs.org/browse/TRUNK-1587 (but its 1.9+ :-/) >>>>>>> >>>>>>> The generic Handler class you're proposing would just have a >>>>>>> handle(T) >>>>>>> method? What happens when there are two or more classes defined as: >>>>>>> @Handler(supports=NamePhonetics.class) >>>>>>> public class NamePhoneticsHandler extends Handler >>>>>>> ? >>>>>>> >>>>>>> How is sync supposed to know which one to use? Or which one is for >>>>>>> sync >>>>>>> and which one is for some other handler? >>>>>>> >>>>>>> Answering my own question after pondering for a few minutes: >>>>>>> add another method called "whoAmIIntendedToBeUsedBy()" that returns a >>>>>>> String key that the sync module can use to distinguish different >>>>>>> handler >>>>>>> classes. (Obviously the name of the method needs some work...) >>>>>>> >>>>>>> Ben >>>>>>> >>>>>>> >>>>>>> On Sat, Aug 20, 2011 at 12:10 PM, Dave Thomas<[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi. At PIH, we have a few implementations using both sync and >>>>>>>> namephonetics. Right now, namephonetics objects aren't propagating >>>>>>>> through sync which is something we want to fix. >>>>>>>> >>>>>>>> Option 1 is to have NamePhonetics extend OpenmrsObject, and let sync >>>>>>>> propogate these. The downsides are 1) extra stuff in the payloads, >>>>>>>> and 2) I have a feeling there would need to be explicit handling of >>>>>>>> NamePhonetics objects during ingest, because PersonName objects are >>>>>>>> explicitly marked as getting processed last (because of the famous >>>>>>>> TreeSet re-ordering bug). This, i think, would mean that there >>>>>>>> would >>>>>>>> have to be a new category of 'things to process last' in the ingest >>>>>>>> routine, which in sync we could maybe define safely as anything >>>>>>>> associated with a module (org.openmrs.module*)? Not sure about this >>>>>>>> though. It is definitely true that we don't want to build any >>>>>>>> module >>>>>>>> dependencies between Sync and NamePhonetics. >>>>>>>> >>>>>>>> Option 2 is to treat NamePhonetics like ConceptWords in that they're >>>>>>>> calculated locally. NamePhonetics currently AOPs around savePerson >>>>>>>> to >>>>>>>> do this on a local system, but the saving of PersonNames during sync >>>>>>>> ingest uses the SyncService.saveOrUpdate method. Here's what I was >>>>>>>> thinking -- what if SyncService had a postInterceptor modeled on >>>>>>>> RequiredDataAdvice? This postInterceptor class would check for >>>>>>>> registered handlers for incoming sync items. The only architectural >>>>>>>> piece that would be needed to do this and avoid building in module >>>>>>>> dependencies between Sync and NamePhonetics would be a truly default >>>>>>>> Handler interface in OpenMRS core. NamePhonetics would then >>>>>>>> register >>>>>>>> a PersonName handler. Sync ingest would then look for a handler in >>>>>>>> the saveOrUpdate postProcessor, and if found, apply it. If not >>>>>>>> found >>>>>>>> (for sync implementations not using namephonetics), no problem. >>>>>>>> >>>>>>>> Option 2 seems like overkill, maybe, although i like the idea of >>>>>>>> keeping sync packages as simple and small as possible... thoughts on >>>>>>>> this would be appreciated. >>>>>>>> >>>>>>>> Also, generally, would it be useful to have a truly generic handler >>>>>>>> interface in core (or actually, maybe generic pre-processor and >>>>>>>> post-processor handler interfaces)? This would be kind of a cool >>>>>>>> way >>>>>>>> for modules to register handlers against each other, without >>>>>>>> building >>>>>>>> in module dependencies. >>>>>>>> >>>>>>>> d >>>>>>>> >>>>>>>> _________________________________________ >>>>>>>> >>>>>>>> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail >>>>>>>> to >>>>>>>> [email protected] with "SIGNOFF openmrs-devel-l" in the >>>>>>>> body >>>>>>>> (not the subject) of your e-mail. >>>>>>>> >>>>>>>> [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] >>>>>>>> >>>>>>> ------------------------------ >>>>>>> Click here to >>>>>>> >>>>>>> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from >>>>>>> OpenMRS Developers' mailing list >>>>>> >>>>>> ------------------------------ >>>>>> Click here to >>>>>> >>>>>> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from >>>>>> OpenMRS Developers' mailing list >>>>> _________________________________________ >>>>> >>>>> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to >>>>> [email protected] with "SIGNOFF openmrs-devel-l" in the body >>>>> (not >>>>> the subject) of your e-mail. >>>>> >>>>> [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] >>>>> >>>> _________________________________________ >>>> >>>> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to >>>> [email protected] with "SIGNOFF openmrs-devel-l" in the body >>>> (not >>>> the subject) of your e-mail. >>>> >>>> [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] >>> ________________________________ >>> Click here to unsubscribe from OpenMRS Developers' mailing list >> _________________________________________ >> >> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to >> [email protected] with "SIGNOFF openmrs-devel-l" in the body (not >> the subject) of your e-mail. >> >> [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] >> >> _________________________________________ >> >> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to >> [email protected] with "SIGNOFF openmrs-devel-l" in the body (not >> the subject) of your e-mail. >> >> [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] >> >> _________________________________________ >> >> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to >> [email protected] with "SIGNOFF openmrs-devel-l" in the body (not >> the subject) of your e-mail. >> >> [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] >> > _________________________________________ > > To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to > [email protected] with "SIGNOFF openmrs-devel-l" in the body (not > the subject) of your e-mail. > > [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] _________________________________________ To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to [email protected] with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] _________________________________________ To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to [email protected] with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l] _________________________________________ To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to [email protected] with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]
