MethodInvocation.getArguments return value for 0 arguments

[sergeypk]

Hi,

I'm trying to debug this problem: http://www.jboss.com/index.html?module=bb&op=viewtopic&t=104450&postdays=0&postorder=asc&start=10.

The exception is thrown from this aspect:

public Object handleCreateTextMessage(Invocation invocation) throws Throwable
 {
 JBossTextMessage jbm = new JBossTextMessage(0);

 MethodInvocation mi = (MethodInvocation)invocation;

 if (mi.getArguments() != null)
 {
 jbm.setText((String)mi.getArguments()[0]);
 }

 return new TextMessageProxy(jbm);
 }

What should MethodInvocation.getArguments() return when there are no arguments? Can it ever return null? Do all AOP versions behave the same way or has this changed?

Thanks,
Sergey

[clebert.suconic]

this returns null or aop1.x if I call someMethod();

if (mi.getArguments() != null)
 {
 jbm.setText((String)mi.getArguments()[0]);
 }

While it returns something different with aop 2.0

Notice that I have an overloaded method.. where I can do either someMethod() or someMethod(String).

Our aop binding is set on:

<bind pointcut="execution(* org.jboss.jms.client.delegate.ClientSessionDelegate->createTextMessage(..))">
 <advice name="handleCreateTextMessage" aspect="org.jboss.jms.client.container.SessionAspect"/>
 </bind>

[starksm64]

I would just make the check more robust:

 if (mi.getArguments() != null && mi.getArguments().length() == 1)
 {
 jbm.setText(mi.getArguments()[0].toString());
 }

[flavia.rainone]

Hi!

I changed that in version 2.0, and I added a javadoc documenting the rules related to getArguments():

/**
 * Returns a non-null array containing all method arguments.
 * <p>
 * The returned array can be changed by the advice or interceptor accordingly. All
 * changes are reflected on joinpoint execution, and are noticed as well by
 * other advices and interceptors that are executed after the current one.
 * <br>
 * However, changes to this array are limited to the scope of current advice
 * execution, and must be performed before execution of {@link #invokeNext()},
 * {@link #invokeNext(Interceptor[])}, or {@link #invokeTarget()} method.
 * Otherwise, inconsistency on joinpoint argument values may be noticed.
 *
 * @return the method arguments
 */
 public Object[] getArguments()

The main cause of this was the use of annotated arguments on before/after advices (new feature of JBoss AOP 2.0). A before/after advice can receive the arguments array as a parameter:

public void beforeAdvice(@Args Object[] arguments)
{
 // do something before joinpoint execution
}

Kabir and I decided that an advice that receives arguments would be called to intercept all types of joinpoint, even though arguments array would be null during a field read joinpoint.

Given that, and considering I couldn't find a documentation on getArguments method returning a null value (please, Kabir, let me know if there is one), I thought that the arguments array should be a non-null value during the interception of other joinpoint types (hence, for methods with no arguments, the arguments array should be an empty array).

Since the intention is to make @Args annotated parameters consistent with [InvocationType].getArguments() and [InvocationType].setArguments() methods, I changed getArguments method to return always a non-null value, as you can see on the javadoc comments (notice that these methods are available only on Invocation classes that are equivalent to joinpoints with a valid arguments list, which excludes the FieldReadInvocation class).

So, this is the reason for which I changed that. I apologize for causing any trouble.

Now, I think we need to decide whether we keep things this way, or we make arguments array null on methods and constructors with an empty argument list.

Any thoughts? Kabir? Clebert?

[kabirkhan]

If it is an easy change, I think we should put it back the way it was.

[clebert.suconic]

I will make the code more robust as Scott sugested...

We were just discussing if this is right or wrong.

[flavia.rainone]

It is relatively easy.

We just need to be careful because @Args array used on before advices are currently the same as the one inside the invocation. And, the arguments array contained in the invocation is also used on after advices.

So, either we add a special case, by not doing that before/around/after interaction when there are zero arguments, or we turn the empty arguments array into null for before and after advices.

[kabirkhan]

I think in the case of no arguments null should be fine for around as well as before/after/throwing/finally?

I don't see how interaction between b/a/t/f and around makes any difference when there are no arguments?

[flavia.rainone]

I also think that it would be fine if we apply no arguments null to b/a/t/f.

If we don't apply the same policy, it would make a difference. Look at this:

public void before(@Args Object arguments)
{
...
}

public Object around(Invocation invocation) throws Throwable
{
...
}

Suppose both advices are applied to a POJO->method() execution joinpoint:

public class POJO
{
 public void method()
 {
 ...
 }
}

If we apply different policies to before and around, i.e.:
- keep null @Args only for FieldRead (current policy), which means "before" advice would receive a non-null array in the example above
- Invocation needs to contain a null array because the arguments length is 0

We would need to change JoinPointGenerator because it currently copies arguments array used on before advice to the invocation that will be passed as parameter to "around" advice, and this would break the null args invocation policy.

This is just an example. But it illustrates how interaction between b/a/t/f and around makes a difference when there are no arguments, if we use different null policies.

Nonetheless, I can also implement different policies on both, if that is the case.

[kabirkhan]

OK, in the context of null vs non-null I get your point.

Although I do actually prefer the empty array, as we discovered now for messaging people have aspects relying on the null behaviour.

We should definitely use the same on both, so I think we should use null on both for backwards compatiblity reasons.

[flavia.rainone]

A Jira issue has been created to fix this bug:
http://jira.jboss.org/jira/browse/JBAOP-379

[flavia.rainone]

While I was working on JBAOP-379, I found out that call arguments array have never been null for empty arguments list. This means that CallInvocation.getArguments returns an empty array in that case.

So, changing this would be equivalent to changing method and constructor execution arguments to being empty instead of null. We would be changing the semantics of the getArguments method anyway.

On the other hand, we would prefer to be more consistent and stick with only one of the possibilities:
- null arguments array when there are no arguments
- empty arguments array when there are no arguments

Given that, I want to know how do you find the idea of keeping 2.0 as is (i.e., all arguments array are non-null).

[kabirkhan]

Despite what I have said previously on this thread, I prefer the empty array, and think we should go with that.

[clebert.suconic]

For JBossMessaging, we have adapted our code to test also the size of the array...

but I just think changing semantics on the API is kind of dangerous. I don't know if other users will have problems with that.

As you guys know your user base, take the decision thinking how other users would accept this change. Anyway I would clearly document this change on the release notes and documentation if you go that path.