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]
