InterceptorBindingInheritanceTest relies on undefined behavior

[Ladicek]

Similarly to #464, two tests in InterceptorBindingInheritanceTest rely on recursive interception being prevented by the container, which is not defined properly anywhere and is unrelated to the stated purpose of the test (which is inheritance of interceptor binding annotations).

It is relatively straightforward to modify the test to not trigger recursive interception, which is what I propose we do.

[manovotn]

Good find, this really should have been part of #464 but I missed it.

[struberg]

The fix did introduce a new problem, thus I have to reopen it.

InterceptorBindingInheritanceTest relies storing information in a static field of the contextual instance, but asserts the information on the proxy instance.

Let me explain:

in e.g testInterceptorBindingDirectlyInheritedFromManagedBean you call assertTrue(Plant.inspectedBy(larch, squirrel));

The injected is the "Contextual Reference". Yes, our bean is @Dependent scoped, but we still have the whole interceptor stack. So we get an interceptor proxy for larch. inside the Plant.inspectBy you access the inspections List of the proxy.

But in the SquirrelInterceptor we are inside the interceptor stack. Thus we get the real "Contextual Instance". Which is the unproxied instance and thus the inspections is effectively a different instance!
You would need to keep this information e.g. in an injected @RequestScoped data holder if you want to portably store information and access it from inside the Interceptor.

edit
The very same problem also applies to org.jboss.cdi.tck.interceptors.tests.contract.invocationContext.InvocationContextTest#testGetTargetMethod which does a comparison of the proxy with the invocation target - which is the contextual instance as per the spec.

[struberg]

Maybe this problem does not bubble up in Weld as you create equals and hashcode method which equal the proxy with their respective contextual instances? If so then please point me to the section where this is defined please.

[Ladicek]

Contextual reference for a pseudo-scoped bean is the contextual instance, so better be careful with the wording. There's no client proxy involved.

However, I can see that this might be problematic if the CDI implementation in question implements interception using proxies (instead of subclasses). I vaguely remember thinking that this is not a valid implementation strategy at some point, but I no longer remember why. I might have easily been mistaken.

I'll take another look on the test; I guess we could actually use a static field (we currently don't) to sidestep all the possible troubles.

[struberg]

OWB does implement the proxy via subclassing. But how it does it is seemingly different to Weld. A static field would be perfectly fine for us - thanks Ladicek!

[Ladicek]

ArC and Weld implement interception without proxying -- we generate a subclass of the bean class where we override intercepted methods such that invocation of the intercepted method starts the interception chain. An instance of that class becomes the contextual instance. It seems to me that you implement interception through proxying, so you need to differentiate between a "proper" contextual instance and an "interception proxy" instance.

That is a little head-exploding situation :-), but the TCK obviously needs to support it. I'll try to submit a PR in a bit.

[Ladicek]

Here's a proposed fix for the CDI TCK 4.0 branch: https://github.com/Ladicek/cdi-tck/commits/proxy-based-interception-fix-4.0 It works fine with ArC and Weld, could you please verify if that works for you? Thanks!

[Ladicek]

I submitted a PR for the master branch (for CDI 4.1). I can also submit a fix for the 4.0 branch, or we can add exclusions.

[manovotn]

I submitted a PR for the master branch (for CDI 4.1). I can also submit a fix for the 4.0 branch, or we can add exclusions.

I don't mind the fix but I think we mostly agreed to do exclusions for these cases; a comment explaining why would be neat if you decide for the exclusion.

[Ladicek]

Yeah, since 4.1 is near, I think we should go for exclusions. I only uploaded a branch with a fix for 4.0 so that you could verify it. (The PR I did open is for 4.1.)

[manovotn]

Yeah, since 4.1 is near, I think we should go for exclusions. I only uploaded a branch with a fix for 4.0 so that you could verify it. (The PR I did open is for 4.1.)

Yeah, I forgot to note I tested that branch as well and it works fine too.

[struberg]

ArC and Weld implement interception without proxying -- we generate a subclass of the bean class where we override intercepted methods such that invocation of the intercepted method starts the interception chain. An instance of that class becomes the contextual instance. It seems to me that you implement interception through proxying, so you need to differentiate between a "proper" contextual instance and an "interception proxy" instance.

That is a little head-exploding situation :-), but the TCK obviously needs to support it. I'll try to submit a PR in a bit.

To go into detail a bit more because I feel there is potential for non portability ahead in the interceptor area:
Afair the EE umbrella spec did specify that interceptors only must get triggered when the bean gets invoked via a 'business method'. And somewhere else this is defined as an external call via the 'business interface'. And while for no-interface-views this is just the plain java class even there the 'business method' definition still applies. So essentially if you call an intercepted method of a contextual instance from within that very class, then the interceptor does not get triggered. Not sure how this is done in Weld if the contextual instance is the intercepted class itself?

[manovotn]

Not sure how this is done in Weld if the contextual instance is the intercepted class itself?

Yes, IIRC the interceptor subclass is the contextual instance.

So essentially if you call an intercepted method of a contextual instance from within that very class, then the interceptor does not get triggered.

The invocation needs to be via contextual reference for it to classify as bussiness method invocation (in your scenario). Therefore, in the case you described, if you make a direct method call, it won't trigger interception.

[manovotn]

We now have a fix for the main branch and an exclusion for 4.0, thanks for pointing this out @struberg.
Closing the issue.