Using AOP for non services

Development
,
1

AOP within OpenMRS can only work for services, this in config.xml would fail since there’s no service for UserContext for-example;

<advice>
	<point>org.openmrs.api.context.UserContext</point>
	<class>org.openmrs.module.msfcore.aop.LoginAdvice</class>
</advice>

trying to add an aop proxy factory bean configuration to moduleApplicationContext.xml such as below doesn’t register the advise.

<!-- AOP before advice that calls the LoginAdvice methods -->
<bean id="userContext" class="org.openmrs.api.context.UserContext"/>
<bean id="loginInterceptor" class="org.openmrs.module.msfcore.aop.LoginAdvice"/>
<bean id="userContextProxy" class="org.springframework.aop.framework.ProxyFactoryBean">
    <property name="target" ref="userContext" />
    <property name="interceptorNames">
        <list>
            <value>loginInterceptor</value>
        </list>
    </property>
</bean>

Should we not support the second option for non services?

Suggestion: We have a use-case to log user activity such as login, i suggest either supporting AOP to extend to non services, otherwise to support this use-case, i suggest.

  • Add UserActivityService#authenticationAttempt(User) to referenceapplication module such as ApplicationEventService
  • Fire the authenticationAttempt event after Context.authenticate for either a successful or failed login
  • Create an advise in my module wrapped around UserActivityService#authenticationAttempt so as to log authentication outcomes.

what do you folks think, would this be helpful to OpenMRS?

CC: @dkayiwa, @darius, @mseaton, @burke, @mksd, @wyclif, @ssmusoke

2

That’s not how you register AOP advice from a module, please see this page for how to do it.

With that said, I think it’s not exactly correct to wrap the advice around UserContext, one it’s not a spring bean and secondly it’s just a holder of the details about the authenticated user and does no actions, instead you should wrap the advice around the Context object specifically around the authenticate method in your case.

3

i have been using advice tag in the module’s config and it works fine as seen at:

is that what u mean is not how you register them?

4

Then I guess you’re right, looks like adding advice via the config.xml file and Context.addAdvice only supports services.

As I said to make it work you need to wrap the AOP around the Context class and not UserContext because I don’t think you can ‘springfy’ the UserContext objects given that they are thread locals that are created outside of spring.

5

since Context is not a service as well, it results into the same problem as using UserContext

6

I’m aware of that but you can wrap AOP around pretty much any spring bean the ‘spring way’

7

Just don’t do it around UserContext

1 Like
8

i think that’s what i was trying out using ProxyFactoryBean, which other way do you know of? am trying out annotations ATM

9

I doubt if you’re getting what am trying to say, what am saying is that what you’re doing from your original post should work, just change the target you are wrapping the AOP around to be the bean with id context, and that should work.

10

Of course you’d need to get rid of the advice tag in config.xml file to avoid the NPE

1 Like
11

then how would openmrs know that my advice points to Context without config.xml

12

The OpenMRS way is meant to simplify things for module developers by trying to hide the complexities, the point is that spring will still pick it up anyways.

13

The configuration below in the snippet you provided with a small change of the target bean are what instruct spring to set up the AOP around the context class

<bean id="userContextProxy" class="org.springframework.aop.framework.ProxyFactoryBean">
    <property name="target" ref="context" />
    <property name="interceptorNames">
        <list>
            <value>loginInterceptor</value>
        </list>
    </property>
</bean>
14

just trying out exactly that ATM

15

I’d expect spring to wrap the AOP around the context class but it doesn’t, I guess it’s because the bean is already defined in core and you can’t do it from a module. I think the only option you’re left with is to use aspectj integration with spring, spring’s documentation describes how you can do it.

16

exactly, i had already discovered and now trying aspectj annotations

17

with the right dependencies and aop:aspectj-autoproxy set well in my moduleApplicationContext.xml either with or without include,

<bean name="loginAdvice" class="org.openmrs.module.msfcore.aop.LoginAdvice"/>
<aop:aspectj-autoproxy>
 	<aop:include name="loginAdvice"/>
</aop:aspectj-autoproxy>

i annotated my advice looking like;

    package org.openmrs.module.msfcore.aop;

    import org.aspectj.lang.annotation.AfterReturning;
    import org.aspectj.lang.annotation.Aspect;
    import org.openmrs.User;
    import org.openmrs.api.context.Context;
    import org.openmrs.module.msfcore.api.MSFCoreService;
    import org.openmrs.module.msfcore.audit.MSFCoreLog;
    import org.openmrs.module.msfcore.audit.MSFCoreLog.Event;

    @Aspect
    public class LoginAdvice {

      @AfterReturning(pointcut = "execution(* org.openmrs.api.context.Context.authenticate(..))", returning = "user")
      public void loginAuthenticator(Object user) {
        /*
         * depends on org.openmrs.api.db.ContextDAO#authenticate
         * retaining its structure
         */
        MSFCoreLog userLoginLog = new MSFCoreLog(Event.LOGIN, "failed user login", Context.getAuthenticatedUser());
        if (user != null) {// successful login
          userLoginLog.setUser((User) user);
          userLoginLog.setDetail("successful user login from: " + ((User) user).getUsername());
        }
        Context.getService(MSFCoreService.class).saveMSFCoreLog(userLoginLog);
      }
    }

But still have issues, aspectj conflicts with org.springframework.transaction.interceptor.TransactionProxyFactoryBean i think, see:

Removing aop:aspectj-autoproxy resolves the above log implying the failure happens as soon as the tag is added

18

Now that you have this resolved, do you mind updating this documentation? https://wiki.openmrs.org/display/docs/OpenMRS+AOP

19

i meant resolves the attached log but that means not being able to wrap non service classes with aop

20

Those errors look deceptive, I see no relationship between the AllUserApps bean and AOP advice you’re setting up, I think it would be more useful to say exactly what you have done, you’re saying you added the necessary dependencies, I think you need to say exactly what dependencies you added.

1 Like