Panache: add support for interceptors in static methods

[FroMage]

ATM interceptors are only applied to beans, and not static methods.

Similar to what I've done for panache-mocking, we should be able to inject interceptor code in the static methods.

If you have a bean called PersonRepository ArC will generate a class called PersonRepository_Subclass which contains all intercepted methods:

public java.util.List findOrderedTransactional(){
 Object[] objectArray = new Object[0]; // probably parameters?
 if(metadata == null)
  return super.findOrderedTransactional();
 Function<InvocationContext> target = ctx -> {
  ctx.getParameters(); //ignored
  return findOrderedTransactional$$superaccessor1();
 };
 Object v1 = metadata.get("m1");
 Method targetMethod = ((SubclassMethodMetadata)v1).method;
 List chain = ((SubclassMethodMetadata)v1).chain;
 Set bindings = ((SubclassMethodMetadata)v1).bindings;
 return (List)InvocationContexts.performAroundInvoke(this, targetMethod, target, objectArray, chain, bindings);
}

public List findOrderedTransactional$$superaccessor1(){
 return super.findOrderedTransactional();
}

I should be able to inject similar code in the static methods to get them intercepted.

[FroMage]

WDYT @mkouba, should this work?

[mkouba]

Well, static methods are not intercepted in CDI. In fact, the interceptors spec explicitly forbids this scenario: "An around-invoke method must not be declared as abstract, final or static." (2.5 Business Method Interceptors). So no, it should not work.

[FroMage]

Well, in the general case, sure, you don't want to run interceptors on bean static methods, because you can't without instrumentation.

But here we're talking about entities, they're not beans, and we most certainly do want to allow people to run interceptors for those cases. I'm not sure the CDI spec constraint applies here.

[mkouba]

Ok, my point was that interceptors are in general not intended to work with static methods. I think that we could make it work but there are few challenges:

1. Interceptors are not stateless - they are dependent objects of the bean instance they intercept. We could either create new interceptors for each invocation of a static method or hold the interceptor instances in a dedicated "store".
2. The same applies to metadata.
3. Some interceptors may rely on parts of the API that would not be available for static methods, a typical example is javax.interceptor.InvocationContext.getTarget() - right now there is always a target instance.
4. Maybe more...

[mkouba]

I can try to prepare a POC to see how difficult it would be...

[emmanuelbernard]

@mkouba +1 for the PoC. I am split brained on that one. An entity is not a bean therefore should not intercept. But we do have a specific use case here which makes sense since Panache entities are half entities, half component that would be a CDI bean in another life.

So maybe that facility is dedicated to Panache Entities only and not a generally exposed feature to ArC beans.

[mkouba]

So maybe that facility is dedicated to Panache Entities only and not a generally exposed feature to ArC beans.

I think that if we decide to implement this it should be possible to use it anywhere. On the other hand, it seems that the implementation would require bytecode transformation of call sites (due to the nature of @AroundInvoke interceptors) and so it could be quite expensive. We could restrict the set of analyzed classes or something like that.

From the CDI POV - only method-level interceptor bindings should be considered to retain backwards compatibility. For panache entities we could use an annotation transformer to add bindings declared on the class to all static methods.

BTW CDI observers and producers could be static. So I believe that the original restriction from the interceptor spec originates from a technical limitation.

[emmanuelbernard]

Ah, call side bytecode enhancement might be a problem. On the other hand that's what we do for entity access already @FroMage
Which makes me think, we could also check the query validity of this call then someEntity.find("name", "Emmanuel"); for the same price.

[mkouba]

On the other hand that's what we do for entity access already...

Yes, we do replace field access with getter/setter invocations. The performance drop could be a problem since we have to produce a lot of BytecodeTransformerBuildItems but we could try optimize this further and as I wrote we could restrict the set of analyzed/transformed classes.

So this is the first working version of POC:
https://github.com/mkouba/quarkus/tree/intercept-static-methods

It's not optimized, only public static methods are supported, etc. I also did not verify the panache use case yet. Still, it seems to be doable ;-).

CC @manovotn

[mkouba]

Hmm, so the current approach would not work with panache entities because it does not analyze the inherited static methods :-(. I need to think it over...

[mkouba]

For the record - we'll need to fix this: https://github.com/quarkusio/quarkus/blob/master/extensions/narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java#L86

[mkouba]

So I've got a first version that also works for panache entities annotated with @Transactional (or any other binding):
https://github.com/mkouba/quarkus/blob/intercept-static-methods/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/Beer.java#L8

And the test:
https://github.com/mkouba/quarkus/blob/intercept-static-methods/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/TransactionalPanacheEntityBaseTest.java#L17-L18

Now I'll try to polish/optimize the code and then I'll send a DRAFT PR...

[FroMage]

Thanks a lot @mkouba I need to look at this. My idea was that the existing Panache transformer would generate the interception bytecode itself in the static methods, the same way we generate mocking interception bytecode for the same static methods. But I'll check your code first.

[FroMage]

For the record - we'll need to fix this: https://github.com/quarkusio/quarkus/blob/master/extensions/narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java#L86

We can probably return a bogus instance for all the use-cases that want to access the class. Is there no way to access the target class without going through the instance?

[FroMage]

Oh, I see, it looks like you're switching the callers of the static methods to use an intercepted static method.

My idea was more to inject the interception code into the static method itself, similar to the _Subclass you generate:

public static java.util.List findOrderedTransactional(){
 Object[] objectArray = new Object[0]; // probably parameters?
 if(metadata == null)
  return findOrderedTransactional$$real();
 Function<InvocationContext> target = ctx -> {
  ctx.getParameters(); //ignored
  return findOrderedTransactional$$real();
 };
 Object v1 = metadata.get("m1");
 Method targetMethod = ((SubclassMethodMetadata)v1).method;
 List chain = ((SubclassMethodMetadata)v1).chain;
 Set bindings = ((SubclassMethodMetadata)v1).bindings;
 return (List)InvocationContexts.performAroundInvoke(this, targetMethod, target, objectArray, chain, bindings);
}

public static List findOrderedTransactional$$real(){
// original method code goes here
        return find("ORDER BY name").list();
}

That would allow us to not scan static method call use-sites. OTOH, now that @stuartwdouglas has added support for limiting the scanning to jars that depend on Panache, perhaps it's cheap enough.

[FroMage]

FTR, this is how we inject mock interceptors for each static methods in Panache: https://github.com/quarkusio/quarkus/blob/master/extensions/panache/panache-mock/src/main/java/io/quarkus/panache/mock/impl/PanacheMockMethodCustomizer.java#L21

But this sort of interceptor can only either handle the entire call, or delegate to the original method: there's no "around invoke" concept like in CDI, which pretty much requires moving the original code into a second method like I showed. I could make an extension point like this that would allow moving stuff to another method.

OTOH this would be specific to Panache and would end up in user stack traces because we're moving the real code around, whereas your solution changes the call sites by adding one layer, so stack traces are more correct when something happens in the user code.

So, probably your solution is more user-friendly.

[mkouba]

We can probably return a bogus instance for all the use-cases that want to access the class. Is there no way to access the target class without going through the instance?

Well, we could try to return a "fake" instance but that could bring more problems than benefits. Interceptors could inspect InvocationContext.getMethod() to detect static methods if needed.

Oh, I see, it looks like you're switching the callers of the static methods to use an intercepted static method.

Yes.

My idea was that the existing Panache transformer would generate the interception bytecode itself in the static methods...

Yes, that's the other possible solution. I chose the transformation of call sites because this would allow us to leave the static methods untouched which is easier for debugging. Anyway, the snippet in your comment would require call site transformation as well because the client calls MyEntity.find() and not MyEntity.findOrderedTransactional(), or? Maybe I'm missing something though.

[FroMage]

Anyway, the snippet in your comment would require call site transformation as well because the client calls MyEntity.find() and not MyEntity.findOrderedTransactional(), or?

Well, I generate the bridge method for find and I instrument user methods for findOrderedTransactional so in both cases I inject the mocking interception code.

[mkouba]

Well, I generate the bridge method for find and...

I see.

So panache adds a synthetic method to the entity class to override some static methods declared on PanacheEntityBase (such as count()). I wanted to make the static method interception a general feature, not a panache-specific stuff, so I decided to go the other way and treat the generated methods as if they're regulat static methods. Maybe it was a wrong decision. I'll prepare the DRAFT so that we can decide which way to go...

[stuartwdouglas]

I am very much -100 on call site transformation, this should be done by modifying the method itself. I only just finished optimising the call site modification we do for panache but this is not generally applicable, so that work would likely go to waste.

[mkouba]

this should be done by modifying the method itself

So I'm -1 for modifying the original static methods. IIUIC this would break the debugger, i.e. users would not be able to debug the static methods they create. Also due to the nature of @AroundInvoke interceptors we would probably have to make a copy of the original method (that would be called from a function when the last InvocationContex#proceed() is invoked) and then replace the body of the original method...

@stuartwdouglas Is your only concern the performance?

[stuartwdouglas]

If you enhance the call sites then users won't be able to debug the calling method.

Except that really they still can, you just make sure the LineNumberTableAttribute is still pointing at the correct code and breakpoints will work fine. There may be some weirdness if you step into the very first like (as in the debugger will start to run the interceptors), but you get this whenever you step into an intercepted class. Even if you have to copy the method the debugger will still work fine, as long as the line numbers match up.

[mkouba]

If you enhance the call sites then users won't be able to debug the calling method.

Well, they will be lost for a while (if they step into) but they will get to the original method eventually (in few steps)...

you just make sure the LineNumberTableAttribute..

Modyfing line number attributes? That sounds cumbersome...

Maybe we should get some numbers to estimate the cost of the call site transformation. Anyway, your BytecodeTransformerBuildItem.requireConstPoolEntry optimization seems to be applicable (and is actually applied in my branch).

[stuartwdouglas]

The big problem is not even the requireConstPoolEntry optimisation. The big problem is that it would negate the annotation processor based marker file approach I used for panache.

The idea is that this annotation processor creates a marker files, that can be used to tell which jars on the class path have panache as a dependency. We know that only modules with panache as a dependency can access panache entities, so we can limit the number of jars we transform.

If potentially any static method can want call site transformation the we have to parse the whole class path on every dev mode reload. Basically one option involves modify a single class, the other option involves scanning every single class on the classpath, so for me it is an obvious choice.

In terms of the LineNumberTableAttribute if you rename the method then you don't have to do anything, ASM will just copy it for you and it will not need modification. If you insert bytecode into the start of the method then you just need to increment every entry by the number of bytes you have inserted.

[FroMage]

OK, so @stuartwdouglas essentially says this should not be general-case, but only for Panache static methods.

Now, about the debug issues:

• inserting bytecode in the method requires shifting debug info as Stuart said. I don't do that for the mock interceptor, but I should I guess..
• we can't insert bytecode for ArC interceptors because of the "around" semantics, so yeah we have to move the method to another method name, which keeps debug info, but will show up as a different name in the stack. Is that an issue?

[mkouba]

The big problem is that it would negate the annotation processor based marker file approach I used for panache.

Yeah, I do understand this optimization. So you're basically saying that the call site transformation is fordbidden and we only keep it for panache. Shoudn't we try to optimize our transformation approach then so that it's usable for other extensions as well?

If you insert bytecode into the start of the method then you just need to increment every entry by the number of bytes you have inserted.

We would not insert something at the start of the method but replace the whole body...

[stuartwdouglas]

No, general case is fine, but not for call site instrumentation. Just modify the class being called so you don't have to potentially modify the whole class path.

[stuartwdouglas]

If you insert bytecode into the start of the method then you just need to increment every entry by the number of bytes you have inserted.

We would not insert something at the start of the method but replace the whole body...

But you would copy the original method right (at least that is what you mentioned above). If so you don't need to do anything, as long as you leave the method attributes alone, the line number table will be copied across and debug will work as normal.

[mkouba]

But you would copy the original method

Yes, Foo.someStaticMethod() -> Foo.someStaticMethod_original() and modify the original Foo.someStaticMethod() to call the interceptor chain and pass a function that invokes Foo.someStaticMethod_original().

[FroMage]

No, general case is fine, but not for call site instrumentation. Just modify the class being called so you don't have to potentially modify the whole class path.

Oh, you want ArC to modify the original classes rather than deal with generating new classes (which is the only thing it currently does). Interesting. Since I don't think it currently does that, I wonder how much work that would involve, and I wonder how it would integrate with Panache also doing other enhancements. We'd have Hibernate, Panache and ArC modifying the same class, so we have to make sure transformers happen in the right order.

But you would copy the original method right (at least that is what you mentioned above). If so you don't need to do anything, as long as you leave the method attributes alone, the line number table will be copied across and debug will work as normal.

Yes but the method name would be different once the user steps into it. Probably not a big deal.

[stuartwdouglas]

In that case then debugging will just work. It will look pretty much exactly the same debugging any other intercepted window, the only difference is that the stack trace will have the _original prefix in it.

[mkouba]

and I wonder how it would integrate with Panache also doing other enhancements. We'd have Hibernate, Panache and ArC modifying the same class, so we have to make sure transformers happen in the right order.

That's exactly what I'm afraid of...

[stuartwdouglas]

No we won't, because the methods that the user can apply an interceptor to are just normal static methods. The user can't modify the generated static methods, as there is nothing to add annotations to.

[mkouba]

No we won't, because the methods that the user can apply an interceptor to are just normal static methods. The user can't modify the generated static methods, as there is nothing to add annotations to.

I don't understand? What are we not going to do?

[FroMage]

No we won't, because the methods that the user can apply an interceptor to are just normal static methods

In the case of Panache, that's not strictly true, though it depends on the semantics we ascribe to static methods "inheritance". If the user adds @Transactional to an entity class, they probably expect it to apply also to the PanacheEntity static methods that they invoke on their entity, even without having overriden them. They call them with MyEntity.find() and so to them there's a quasi inheritance-like behaviour they expect.

We certainly do inject the mock-interception bytecode for both user-defined static methods and the ones that are "inherited" where we generate the bridge methods in Panache.

So there is going to be an interaction.

Now, I certainly would not advise users make the entire entity @Transactional because that is most likely wrong. So perhaps the same is true of all interceptors and we would declare that they cannot apply to generated bridge methods, only to user-defined ones. That could get us around this.

But even for those we do transform them to inject the mock-interceptor. I don't think Hibernate transforms those, though. So perhaps interaction is not that complex.

[mkouba]

So perhaps the same is true of all interceptors and we would declare that they cannot apply to generated bridge methods, only to user-defined ones. That could get us around this.

So if a user wants a static method from the PanacheEntityBase to be intercepted? What would be the preferred way? Add e.g. @Transactional MyEntity.deleteAll() and delegate to PanacheEntityBase.deleteAll()? In that case, you would have to transform the MyEntity.deleteAll() method or?

[FroMage]

So if a user wants a static method from the PanacheEntityBase to be intercepted? What would be the preferred way? Add e.g. @transactional MyEntity.deleteAll() and delegate to PanacheEntityBase.deleteAll()? In that case, you would have to transform the MyEntity.deleteAll() method or?

That sounds reasonable, no? At least, we can start with this approach, and extend support for class-declared interceptors later if people come up with a good use-case? Or do you think it's too restrictive?

[FroMage]

bytecode transformers should have some priority so that we can sort them
i.e. ArC transformer should come after panache transformer

ATM we do define an ordering by using markers. They are applied in reverse order I think, so I think Panache declares its transformer after Hibernate declares them, which means Panache transformers run first. That's from memory, though, perhaps I'm wrong.

[mkouba]

I've updated my POC branch to use the other approach discussed above. It seems to work well for "regular" static methods. I still need to verify the Panache use case... afterwards I'll send a draft PR with more detailed description.

[FroMage]

OK great, make a PR indeed because I have comments to add there, but it's looking really good!

[mkouba]

FYI: #9657

UPDATE: This PR got merged.

[FroMage]

Yeah, why is this not closed?