Hey, already done. I'll try to check in this morning. On Sep 2, 2011 10:19 PM, "Mark Goodrich" <[email protected]> wrote: > 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]
_________________________________________ 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]
