Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArC interceptors - implement inheritance rules properly #11942

Closed
mkouba opened this issue Sep 7, 2020 · 9 comments
Closed

ArC interceptors - implement inheritance rules properly #11942

mkouba opened this issue Sep 7, 2020 · 9 comments
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request

Comments

@mkouba
Copy link
Contributor

mkouba commented Sep 7, 2020

The Interceptors spec states:

If an interceptor class itself has superclasses, the interceptor methods defined by the interceptor class’s
superclasses are invoked before the interceptor method defined by the interceptor class
, most general
superclass first.

(5.2.2 Invocation Order of Interceptors with Superclasses)

And also:

Around-invoke methods may be defined on interceptor classes and/or the target class and/or super-
classes of the target class or the interceptor classes. However, only one around-invoke method may be
defined on a given class.

(2.5 Business Method Interceptors)

@mkouba mkouba added the kind/enhancement New feature or request label Sep 7, 2020
@quarkusbot quarkusbot added the area/arc Issue related to ARC (dependency injection) label Sep 7, 2020
@quarkusbot
Copy link

/cc @manovotn

mkouba added a commit to stuartwdouglas/quarkus that referenced this issue Sep 7, 2020
- add basic support for interceptor's inheritance
- other unsupported use cases are described here:
quarkusio#11942
@manovotn
Copy link
Contributor

I have composed a test to see what we actually support; it's taken from interceptors TCK and works with a hierarchy of interceptors as well as hierarchy of interceptors declared on a target class.
Test can be seen here and is based on this TCK test.

According to the specification, all of that should work. However, ArC currently:

  • Supports overriding methods in interceptor class hierarchy - overriden methods are correctly ignored and only the most specific version of the method is picked
  • Doesn't support interceptor methods declared directly on target classes at all
  • Doesn't support hierarchy of (not overriden) interceptor method - only the most specific one will be picked up and invoked

@manovotn
Copy link
Contributor

manovotn commented Sep 3, 2021

An update:

Here is a CDI discussion and a nice table depicting which interception types are expected to work in CDI Lite which is pretty much the model we will want to follow.

Doesn't support interceptor methods declared directly on target classes at all

This will likely not be supported in Lite and I think Arc shouldn't support it either. At least not for the time being.

Doesn't support hierarchy of (not overriden) interceptor method - only the most specific one will be picked up and invoked

This is something we will have to look into because the TCKs will test it anyway.

@dufoli
Copy link
Contributor

dufoli commented Sep 3, 2021

@dufoli I will take this one

@manovotn
Copy link
Contributor

manovotn commented Sep 3, 2021

Assigned to you.
We are basically interested in interception where the interceptor class has a hierarchy of @AroundInvoke methods that are not overriden by subclasses. You can get inspiration for tests in my test branch and feel free to make use of it but you will need to adjust it and take out any bits that work with interceptors declared on bean classes.

@dufoli
Copy link
Contributor

dufoli commented Sep 8, 2021

questions:

  • is it only for aroundInvoke ? that we call parent class of interceptor if it contain methods ?
  • I add add index of method in AroundInvokeInvocationContext because I can not touch to interceptor. Inherited class is not an interceptor so I pass this index to interceptor.intercept through the context because I can not change the signature of method) come from interface. => I am not very happy of this index argument. Can I store it in contextData ? or add it to AbstractInvocationContext. Else I can switch my mind and make the super class as an interceptor by genering interceptorInfo. wdyt ?

@manovotn
Copy link
Contributor

manovotn commented Sep 8, 2021

is it only for aroundInvoke ? that we call parent class of interceptor if it contain methods ?

That's very good question indeed.
I'd say that according to spec, you can have the same for any interceptor method declared on interceptor class. See also https://jakarta.ee/specifications/interceptors/2.0/interceptors-spec-2.0.html#interceptor_ordering_rules
Though I am not sure I ever saw that used in practice...
Thoughts @Ladicek?

I add add index of method in AroundInvokeInvocationContext because ....

Didn't go too deep into the code as I am preoccupied with other things right now but contextData is user facing information so that might not be the best idea as users can potentially see and change this information. Adding it to AbstractInvocationContext sounds more sensible.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 9, 2021

Agree that the spec is pretty clear in that all kinds of interceptor methods are inherited [from the superclasses of the interceptor class].

luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    - add basic support for interceptor's inheritance
    - other unsupported use cases are described here:
    quarkusio/quarkus#11942
@manovotn
Copy link
Contributor

manovotn commented May 2, 2023

I think this issue is no longer needed due to changes @mkouba did in #32730, right?

@mkouba mkouba closed this as completed May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants