Invalid logic of matching joinpoints for complex pointcuts with logical operators

[andreyvk]

Hi,

I've stumbled upon a behavior which I cant seem to figure out. Consider the following pointcut expression which applies to any controller in **\common\** namespace except for LoginController:

@Before("execution(public **\common\controllers\*Controller->handle_*(*)) && !within(app\common\controllers\LoginController)")

Then when I change it to:

@Before("execution(public **\base\controllers\*Controller->handle_*(*)) && !within(app\base\controllers\JscssController)")

!within stops working in the sense that the JscssController is never excluded - i.e. beforeMethodExecution always fires. What am I doing wrong here?

[andreyvk]

I figured that this problem was because method called was inherited from the superclass. In details it's actually more tricky than that:

BaseController has 2 methods handle_GetDefault and handle_Default. The first one calls the latter (in general all HTTP methods boil down to handle_Default)

JscssController overrides handle_Default method only, which supposedly caused the problem.

Is there a way to make this setup work somehow?

[lisachenko]

Hello!
I'm checking your example now, wait a little bit for my answer.

[lisachenko]

Ok, so my test case:

BaseController.php

class BaseController
{
    public function handle_GetDefault()
    {
        echo __CLASS__.':'.__FUNCTION__, PHP_EOL;
        $this->handle_Default();
    }

    public function handle_Default()
    {
        echo __CLASS__.':'.__FUNCTION__, PHP_EOL;
    }
}

JscssController.php:

class JscssController extends BaseController
{
    public function handle_Default()
    {
        echo __CLASS__.':'.__FUNCTION__, PHP_EOL;
    }
}

Pointcut is modified a bit: @Before("execution(public **\*Controller->handle_*(*)) && !within(**\JscssController)")

And then test case:

$ctrl = new \Demo\Example\BaseController();
$ctrl->handle_GetDefault();
echo PHP_EOL;

$ctrl = new \Demo\Example\JscssController();
$ctrl->handle_GetDefault();
die;

Output is following:

Before: execution(Demo\Example\BaseController->handle_GetDefault())
Demo\Example\BaseController__AopProxied:handle_GetDefault
Before: execution(Demo\Example\BaseController->handle_Default())
Demo\Example\BaseController__AopProxied:handle_Default

Before: execution(Demo\Example\JscssController->handle_GetDefault())
Before: execution(Demo\Example\JscssController->handle_GetDefault())
Demo\Example\BaseController__AopProxied:handle_GetDefault
Demo\Example\JscssController__AopProxied:handle_Default

First part is perfect, because it's what we want when declare execution(public **\*Controller->handle_*(*)) (don't pay an attention to the __AopProxied suffix - it's ok), but the second part is interesting.

First thing that we see is two lines "Before". This wasn't expected, but there is a reason, method handle_GetDefault is declared in the BaseController, so it's matched two times (in the parent and one more time in the JscssController). Both interceptors receive one "$this" which is JscssController, so there are two similar lines.

Then we see, that original method BaseController->handle_GetDefault() was called and it's make a call to the overridden JscssController->handle_Default() method. This method is explicitly declared in the JscssController class and excluded from the matching due to the pointcut !within(**\JscssController).

Now I'm thinking is there a bug or not. According to the class inheritance and definied pointcut this is and expected behaviour for now. But it's impossible to ignore this methods in children classes. So, maybe new syntax should be developed or keyword, like this or target as in AspectJ.

[lisachenko]

Alternatively, you could use a workaround for this and add a check into the advice body.

if ($invocation->getThis() instanceof JscssController) {
    return; // just do nothing for this case
}

[andreyvk]

Hi,

First of all, terribly sorry for late reply!

The suggestion of using code to filter out controller - I'm already doing that. It just would be nice to use expressions only. :)

Now to the test case.... To me it looks like a bug, because although handle_Default() method is overridden by the JscssController, it was still executed. The pointcut expression says otherwise though.

However, the fact that a child BaseController->handle_GetDefault() method was included there as well - to me that's an expected behavior, since method was never overridden by the JscssController. In this case, having scoping keywords may definitely help improving pointcut expression's flexibility. What do you think?

[lisachenko]

Hi!

Don't worry about late reply ) I'm also busy and can spend only limited time on my project, so I should excuse instead.

But yesterday I revised the whole logic of matching joinpoints for complex pointcuts and surprisingly discovered that it can be absolutely incorrect 😃 So, yes, I can confirm that for complex pointcut expressions matching can be incorrect due to two cases:

• Incorrect grammar parsing due to the logic operator precedence. Need to work on grammar more carefully. Maybe you know someone who can help me with LALR grammar definition? Otherwise, I can try to do this myself, but it's complex task to describe grammar with operator precendence properly.
• Incorrect optimization of joinpoints mathcing for complex expressions. It's a weird one, that can require significant changes in the core for correct matching. Example: execution(public Some\ConcreteClass->method(*)) || access(public **->*) will match every method/property in every class. Reason of bug here is mixing different types and incorrect optimization for class and method mathcing (pseudocode): if ($class in [Some\ConcreteClass, **] && $method in ['method', '*']) which equals to true for every method.

[andreyvk]

I see, but I cant say that it's absolutely incorrect. Let's say it would cover most of the cases that ppl usually encounter :)

I'm ashamed to say that I cannot suggest anyone worthy to help with the grammar (including myself). Of course you most probably have looked at existing solutions like rvanvelzen/lime? Could they be of any help?

[lisachenko]

Incorrect grammar parsing due to the logic operator precedence.

This part is fixed by #220

Incorrect optimization of joinpoints mathcing for complex expressions. It's a weird one, that can require significant changes in the core for correct matching.

This still present, I will look into it soon.

[lisachenko]

There is a progress on this: see #274, this feature allows to control matching of inherited methods. And now I'm trying to fix logic for "OR" pointcuts.