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 fixes for spec compatibility, round 7 #32885

Merged
merged 9 commits into from
Apr 27, 2023
Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Apr 25, 2023

Related to #28558

  • remove superfluous CDI TCK exclusions
  • fix merging @AroundConstruct interceptor bindings
  • validate non-@Repeatable interceptor bindings for member values conflicts
    • we allow @Repeatable bindings to have different member values, as repeatability is their entire point
    • this has a bit of code duplication :-/
  • fix class-level interceptor bindings search to always look at stereotypes
  • fix searching for bindings on an interceptor declaration to also look for inherited bindings
  • fix interceptor method "nesting" for @PostConstruct/@PreDestroy interceptors
  • fix bean creation in presence of @AroundConstruct interceptors
  • fix wrapping exceptions thrown by @AroundConstruct and @PostConstruct interceptors
  • fix generated Bean.destroy() in case a user directly passes in a client proxy

@Ladicek Ladicek requested review from mkouba and manovotn April 25, 2023 15:57
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Apr 25, 2023
@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. Thanks!

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice amount of TCK fixes, thanks!

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Ladicek added 9 commits April 27, 2023 12:04
Some of these tests were fixed in CDI TCK 4.0.8, some were
fixed by the bean discovery in strict mode improvements,
some were fixed by the improvements to interceptors.
Interceptor bindings have to be merged correctly. That is, a class-level
binding of some type is ignored if a method-level binding of the same type
is also present. This used to be the case for `@AroundInvoke` interceptors,
but not for `@AroundConstruct` interceptors. This commit fixes that.
…ereotypes

Previously, the class-level interceptor bindings search only looked
at stereotypes when searching for interceptor bindings of business
method interceptors (`@AroundInvoke`).

With this commit, the class-level interceptor binding search works
identically for business method interceptors and lifecycle callbacks
(`@AroundConstruct`, `@PostConstruct`, `@PreDestroy`).
…terceptors

Previously, `@PostConstruct`/`@PreDestroy` methods declared on the target
class were not invoked from within the `@PostConstruct`/`@PreDestroy`
methods declared on an interceptor class. Instead, they were invoked
_after_ the interceptor chain finished, effectively creating two chains
of lifecycle callback interceptors.

This didn't allow an "outer" lifecycle callback method, when declared
on an interceptor class, to catch and handle an exception thrown by an
"inner" lifecycle callback method, when declared on the target class.

This commit fixes the implementation of `@PostConstruct`/`@PreDestroy`
interceptors by reifying the "inner" chain of lifecycle callback methods,
declared on the target class, into a `Runnable` and passing that to the
"outer" chain of lifecycle callback methods, declared on the interceptor
class, to be invoked at the very end of the chain. This effectively
merges the two chains into one, as prescribed by the specification.

Note that the "inner" chain is reified into a `Runnable` only when
an "outer" chain actually exists. It often doesn't, in which case,
allocating a `Runnable` only to call it is a waste of resources.
`@AroundConstruct` interceptors may modify the constructor arguments
using `InvocationContext.setParameters()`. ArC used to ignore this
modification and call the bean constructor using the original
arguments, obtained before calling the interceptors. This commit
fixes that and uses the arguments array from `InvocationContext`
to call the bean constructor.

To do that, the forwarding function passed along the interceptor
chain is no longer a `Supplier<Object>` (which used to call the bean
constructor with original arguments), but a `Function<Object[], Object>`
(which is passed the arguments stored in the `InvocationContext` and
uses them to call the bean constructor).
…truct interceptors

ArC used to always wrap exceptions thrown by `@AroundConstruct` and
`@PostConstruct` interceptors into `RuntimeException`, even if these
exceptions were themselves `RuntimeException`s. This commit fixes that
and only wraps checked exceptions.
… client proxy

ArC callers of `Bean.destroy()` never pass in a client proxy, but
there may be direct callers too, even though this API is low-level.
This commit unwraps the client proxy if it was passed to `destroy()`
before proceeding, which avoids a `ClassCastException` in case
the bean has a `@PreDestroy` interceptor (and hence the contextual
instance is a generated subclass).
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 27, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 27, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@Ladicek Ladicek merged commit eab2f0a into quarkusio:main Apr 27, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 27, 2023
@Ladicek Ladicek deleted the arc-fixes branch April 27, 2023 15:40
@quarkus-bot quarkus-bot bot added this to the 3.1 - main milestone Apr 27, 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants