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.

Sign up for GitHub

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

Jump to bottom

ArC fixes for spec compatibility, round 10 #33523

Merged
merged 4 commits into from
May 29, 2023
Merged

ArC fixes for spec compatibility, round 10 #33523

merged 4 commits into from
May 29, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented May 22, 2023 •
edited
Loading

Related to #28558

  • fix InstanceHandle.close() to behave as specified in strict mode
    • there's a contention between what we want in ArC and what CDI specifies, so this needs future work
  • fix inheritance of interceptor binding annotations
    • 2 fixes actually:
      • fix inheritance of non-@Inherited class-level interceptor bindings, but only in strict mode, because Quarkus expects security annotations to be inherited even though they are not @Inherited
      • fix inheritance of interceptor bindings on methods, as method-level annotations are never inherited
    • ideally, the fix for class-level interceptor bindings would apply unconditionally, but I'm leaving that for the future
  • defer unproxyability errors to runtime in strict mode
    • the CDI spec demands that if a bean isn't injected anywhere, unproxyability shouldn't be a deployment problem, but an exception at runtime
    • this is not necessarily what we want in ArC, so again, this needs future work
  • upgrade Arquillian to 1.7.0.Final

With these changes, the only remaining TCK exclusions are tests that have been challenged. With an upcoming release of CDI TCK 4.0.10, we'll pass the entire CDI Lite TCK (with exactly 1 exclusion for a challenged test) 🎉

@Ladicek Ladicek requested review from mkouba and manovotn May 22, 2023 12:26
@Ladicek Ladicek mentioned this pull request May 22, 2023
Closed
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file labels May 22, 2023
manovotn
manovotn approved these changes May 22, 2023
View reviewed changes
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.

I am not exactly happy about the recursive interception and deferred unproxyability error, but I suppose it's as good as it gets now (given that it's in strict mode only).
We need to solve this on spec/tck lvl and then we can remove these workarounds.

@quarkus-bot

This comment has been minimized.

Sign in to view
@Ladicek
Copy link
Contributor Author

Ladicek commented May 23, 2023

Ouch. I didn't see this test failing locally, because I still have the commit that fixes inheriting non-@Inherited interceptor bindings. I'll think about what we can do here...

@Ladicek
Copy link
Contributor Author

Ladicek commented May 23, 2023

I added the fix for inheritance of non-@Inherited class-level interceptor bindings that we previously backed off -- but only in strict mode. This needs follow-up work in my opinion -- we should just not inherit non-@Inherited annotations, and add annotation transformations for the security annotations or something like that.

@quarkus-bot

This comment has been minimized.

Sign in to view
@manovotn
Copy link
Contributor

I am not exactly happy about the recursive interception and deferred unproxyability error, but I suppose it's as good as it gets now (given that it's in strict mode only). We need to solve this on spec/tck lvl and then we can remove these workarounds.

For future references, I've created a TCK challenge for the recursive interception test and a CDI issue for deferring unproxyable bean error.

@Ladicek
Copy link
Contributor Author

Ladicek commented May 26, 2023

Rebased and removed the commit that prevents recursive interception.

@Ladicek Ladicek mentioned this pull request May 26, 2023
Merged
@quarkus-bot

This comment has been minimized.

Sign in to view
@Ladicek Ladicek mentioned this pull request May 29, 2023
Closed
@Ladicek
ArC: fix InstanceHandle.close() to behave as specified in strict mode
b50f56d 
This means that it delegates to `destroy()` not only for `@Dependent`
beans, but for all beans.

To avoid breaking existing users, this is only done in the strict mode.
@Ladicek Ladicek mentioned this pull request May 29, 2023
Closed
@Ladicek
ArC: fix inheritance of interceptor binding annotations
9c95114 
Class-level interceptor bindings declared on a superclass should
only be taken into account when they are `@Inherited`. ArC however
inherits all interceptor bindings from superclasses, even if
they are _not_ declared `@Inherited`. This commit fixes that in
the strict mode, to not affect Quarkus security (which assumes
that security annotations are inherited, even if they are not).

Method-level annotations are never inherited. This means that the lowest
override of a method determines whether method-level interceptor binding
annotations are present. ArC used to behave differently. If a method
on a class had no interceptor binding annotations but overrode a method
from a superclass that _did_ have interceptor binding annotations,
the method-level annotations from the superclass were inherited.
This commit fixes that.
@Ladicek
ArC: defer unproxyability errors to runtime in strict mode
778abc8 
The CDI specification demands that if an injection point resolves
to a bean, unproxyability of that bean should be treated as
a deployment problem. ArC used to treat all unproxyable beans
uniformly, even those that are not injected anywhere. This commit
changes that: in strict mode, only unproxyable beans that are
actually injected somewhere trigger a deployment-time error.
Unproxyable beans that are not injected anywhere do not cause
a deployment-time error and instead, the `Bean` class (its
`get(CreationalContext)` method, specifically) is generated
in a way that `UnproxyableResolutionException` is thrown when
obtaining a contextual reference, as expected by the CDI spec.
@Ladicek
ArC: upgrade Arquillian to 1.7.0.Final
22c5798
@Ladicek Ladicek mentioned this pull request May 29, 2023
Closed
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 29, 2023
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot
Copy link

quarkus-bot bot commented May 29, 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 3212b8a into quarkusio:main May 29, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 29, 2023
@Ladicek Ladicek deleted the arc-fixes branch May 29, 2023 17:06
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone May 29, 2023
@Ladicek Ladicek mentioned this pull request Sep 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RuanNunes RuanNunes RuanNunes approved these changes

@manovotn manovotn manovotn approved these changes

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
3.2.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@Ladicek @manovotn @mkouba @RuanNunes