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]
