Comment 1 Denis Zhdanov 2008-11-25 11:32:33 EST

Created attachment 118667 [details]
test-case source and output

Comment 2 Denis Zhdanov 2008-11-25 11:35:27 EST

Correction: AopService class from the test-case shouldn't implement any interface, i.e. the following row

public class AopService implements AopInterface<Number> {

should be changed to 

public class AopService

Comment 3 Andrew Clement 2008-11-27 17:22:49 EST

I'm afraid annotation style if() does not work the same as code style if().  We don't support the embedding of arbitrary bits of Java in a string as it would be too difficult to compile.  And so, as per the "if() pointcut expressions" section in:

http://www.eclipse.org/aspectj/doc/released/adk15notebook/ataspectj-pcadvice.html

You have to supply an empty if() and the code for the if in a method body.  For example, this refactoring of your code works fine:

@Aspect
public class X {

   @Pointcut("execution(* AopService.*(..)) && if()")
   public static boolean foo() {
     return System.currentTimeMillis()>1;
   }

   @Around("foo()")
    public Object advice(ProceedingJoinPoint joinPoint) throws Throwable {...}
}

Because the pointcut is in a string, javac will not see anything wrong with the pointcut and so it fails at loadtime indicating the bad pointcut (it could be more clear with its message though).

If using ajc to compile the annotation style aspect, it will fail but (again) the message is hopeless:

C:\temp\foo\X.java:16 [error] Syntax error on token "execution(* AopService.*(..)) && if(java.lang.System.currentTimeMillis() > 1)", ")" expected
@Around("execution(* AopService.*(..)) && if(java.lang.System.currentTimeMillis() > 1)")

So - I'll leave this bug open to cover improving the messages in both cases.  But it is working as designed.

Comment 4 Denis Zhdanov 2008-11-28 03:00:38 EST

Thank you very much for clarification. It was my fault that I didn't find exact explanation of 'if' pointcut usage during annotation-based configuration.

I tested the mentioned approach and have one more question now. Suppose we change the aspect class and starter class as follows:

__________________________________________________________________________
TestAspect.java
__________________________________________________________________________

package com.spring.aop;

import org.aspectj.lang.annotation.*;

@Aspect
public class TestAspect {

    @Pointcut("execution(* com.spring.aop..*(java.lang.Number)) && args(i) && if()")
     public static boolean someCallWithIfTest(Number i) {
        return i.longValue() > 5;
     }

    @Before("someCallWithIfTest(i)")
    public void beforeAdviceWithRuntimeTest(Number i) {
        System.out.println("xxxxxxxxxxxxx: TestAspect.beforeAdviceWithRuntimeTest() called");
    }
}


__________________________________________________________________________
AAA.java
__________________________________________________________________________

package com;

import com.spring.aop.AopService;

public class AAA {
    public static void main(String[] args) throws Exception {
        AopService aopService = new AopService();
        aopService.service(1);
        aopService.service(10);
    }
}

I expect aspect to be applied only to the second aopService.service() execution then. However, I got the following:

[AppClassLoader@13f5d07] info register aspect com.spring.aop.TestAspect
[AppClassLoader@13f5d07] weaveinfo Join point 'method-execution(void com.spring.aop.AopService.service(java.lang.Number))' in Type 'com.spring.aop.AopS
ce' (AopService.java:6) advised by before advice from 'com.spring.aop.TestAspect' (TestAspect.java) [with runtime test]
[AppClassLoader@13f5d07] weaveinfo Join point 'method-execution(boolean com.spring.aop.TestAspect.someCallWithIfTest(java.lang.Number))' in Type 'com.s
g.aop.TestAspect' (TestAspect.java:10) advised by before advice from 'com.spring.aop.TestAspect' (TestAspect.java) [with runtime test]
Exception in thread "main" java.lang.StackOverflowError
        at com.spring.aop.TestAspect.someCallWithIfTest(TestAspect.java)
        at com.spring.aop.TestAspect.someCallWithIfTest(TestAspect.java:10)
        at com.spring.aop.TestAspect.someCallWithIfTest(TestAspect.java:10)
        at com.spring.aop.TestAspect.someCallWithIfTest(TestAspect.java:10)
        .....

I dumped generated classes and decompiled them. TestAspect looks wierd:

__________________________________________________________________________
package com.spring.aop;

import java.io.PrintStream;
import org.aspectj.lang.NoAspectBoundException;

public class TestAspect
{

    public TestAspect()
    {
    }

    public static boolean someCallWithIfTest(Number i)
    {
        Number number = i;
        if(someCallWithIfTest(number))
            aspectOf().beforeAdviceWithRuntimeTest(number);
        return i.longValue() > 5L;
    }

    public void beforeAdviceWithRuntimeTest(Number i)
    {
        System.out.println("xxxxxxxxxxxxx: TestAspect.beforeAdviceWithRuntimeTest() called");
    }

    public static TestAspect aspectOf()
    {
        if(ajc$perSingletonInstance == null)
            throw new NoAspectBoundException("com.spring.aop.TestAspect", ajc$initFailureCause);
        else
            return ajc$perSingletonInstance;
    }

    public static boolean hasAspect()
    {
        return ajc$perSingletonInstance != null;
    }

    private static void ajc$postClinit()
    {
        ajc$perSingletonInstance = new TestAspect();
    }

    private static Throwable ajc$initFailureCause; /* synthetic field */
    public static final TestAspect ajc$perSingletonInstance; /* synthetic field */

    static
    {
        try
        {
            ajc$postClinit();
        }
        catch(Throwable throwable)
        {
            ajc$initFailureCause = throwable;
        }
    }
}
__________________________________________________________________________

I.e. join point condition is injected to the pointcut method ('if' and its body), hence, the method calls itself recursively . If I change aspect as follows, everything works as expected. However, the reference you mentioned says that 'Extra implicit  arguments of type JoinPoint, JoinPoint.StaticPart and JoinPoint.EnclosingStaticPart can also be used', i.e. that attributes are not mandatory for the 'if' pointcut. Can you advice on that?

package com.spring.aop;

import org.aspectj.lang.annotation.*;
import org.aspectj.lang.JoinPoint;

@Aspect
public class TestAspect {

    @Pointcut("execution(* com.spring.aop..*(java.lang.Number)) && args(i) && if()")
     public static boolean someCallWithIfTest(Number i, JoinPoint jp, JoinPoint.EnclosingStaticPart esjp) {
        return i.longValue() > 5;
     }

    @Before("someCallWithIfTest(i, jp, enc)")
    public void beforeAdviceWithRuntimeTest(Number i, JoinPoint jp, JoinPoint.EnclosingStaticPart enc) {
        System.out.println("xxxxxxxxxxxxx: TestAspect.beforeAdviceWithRuntimeTest() called");
    }
}

Comment 5 Andrew Clement 2008-11-28 12:25:08 EST

I believe it is simply that your pointcut method matches the signature you specified for execution(* com.spring.aop..*(java.lang.Number)) - hence it gets advised.  By adding the extra parameters (that have no real use to you) you simply ensure it no longer matches.

I would write your pointcut so you are not advising elements of your aspect, since that really doesn't seem to be what you want to do:

 @Pointcut("execution(* com.spring.aop..*(java.lang.Number)) && args(i) &&
if() && !within(com.spring.aop.TestAspect)")

Comment 6 Denis Zhdanov 2008-11-28 13:20:01 EST

Such a silly mistake :(

Thank you very much.

Comment 7 Andrew Clement 2008-12-05 13:42:47 EST

improved message:

"in annotation style, if(...) pointcuts cannot contain code. Use if() and put the code in the annotated method",

Comment 8 Denis Zhdanov 2008-12-05 15:20:09 EST

Excellent, thanks!