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: implement CDI 4.1 #40208

Merged
merged 17 commits into from
Apr 26, 2024
Merged

ArC: implement CDI 4.1 #40208

merged 17 commits into from
Apr 26, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Apr 23, 2024

No description provided.

@Ladicek Ladicek requested review from mkouba and manovotn April 23, 2024 09:38
@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 23, 2024

Each commit is best reviewed separately. They are all fairly small, except of the one that adds support for method invokers.

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/platform Issues related to definition and interaction with Quarkus Platform labels Apr 23, 2024
@quarkus-bot quarkus-bot bot added this to To do in Quarkus Documentation Apr 23, 2024
@gsmet
Copy link
Member

gsmet commented Apr 23, 2024

Not a review but nice work :).

@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 23, 2024 •
edited

🙈 The PR is closed and the preview is expired.

@manovotn manovotn mentioned this pull request Apr 24, 2024
Closed
@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 24, 2024

Rebased.

@quarkus-bot

This comment has been minimized.

Sign in to view
mkouba
mkouba approved these changes Apr 24, 2024
View reviewed changes
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.

@Ladicek do you plan to add support for Quarkus build items used to define method invokers?

Quarkus Documentation automation moved this from To do to Reviewer approved Apr 24, 2024
@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 24, 2024

I don't think that's necessary. Method invokers can be registered in the bean registration phase, for which a build item already exists. (Aside: maybe this should be better documented?) I have a few examples (@Scheduled, @ConsumeEvent, Reactive Messaging) that I hope to submit after this is merged and I didn't need any new build items.

@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor

mkouba commented Apr 24, 2024

I don't think that's necessary. Method invokers can be registered in the bean registration phase, for which a build item already exists. (Aside: maybe this should be better documented?) I have a few examples (@Scheduled, @ConsumeEvent, Reactive Messaging) that I hope to submit after this is merged and I didn't need any new build items.

With BeanRegistrar#getInvokerFactory() you mean? Yes, that's hidden very well :-). OTOH we also have SyntheticBeanBuildItem and BeanRegistrar.RegistrationContext#configure() but I'm not sure if a dedicated build item would bring any benefit here...

@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 24, 2024

With BeanRegistrar#getInvokerFactory() you mean? Yes, that's hidden very well :-)

Indeed that's what I meant :-) Yeah, I agree. I should probably write some documentation into the CDI integration guide...

@quarkus-bot

This comment has been minimized.

Sign in to view
@gastaldi gastaldi mentioned this pull request Apr 25, 2024
Closed
@quarkus-bot

This comment has been minimized.

Sign in to view
@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 26, 2024

Here's a PR to remove the test for @ManagedBean from the RESTEasy Reactive TCK: quarkusio/resteasy-reactive-testsuite#7 Will update this PR after that one is merged.

@Ladicek
ArC: update CDI to 4.1.0, Interceptors to 2.2.0 and Common Annotation…
2d8f7fd 
…s to 3.0.0
@Ladicek
ArC: implement the CreationalContexts TCK SPI
39c1aed
@Ladicek
ArC: implement the Contextuals TCK SPI
fe9a1f8
@Ladicek
ArC: prefer InvocationContext.getInterceptorBindings()
ae7c503
@Ladicek
ArC: implement BeanContainer.getContexts()
51d6521
@Ladicek
ArC: fix ambiguity resolution in case of producers with priority
26e6a16 
A producer may have priority as defined by CDI 4.1 (or obtained from
a stereotype, as permitted but not really specified in CDI 4.0),
but does not necessarily have to be an alternative. During ambiguity
resolution, if such non-alternative producer with priority is declared
on an alternative bean, we must first consider the priority declared
on the producer, even if the producer is not itself an alternative.

To do that, this commit changes the ambiguity resolution implementation
to use `BeanInfo.getPririty()` instead of `getAlternativePriority()`.
The difference is that `getAlternativePriority()` returns `null` when
the bean itself is not an alternative. Given that the method was
previously only used during ambiguity resolution, and only after
non-alternative beans (and producers on non-alternative beans)
were eliminated, this is correct.

In fact, this commit also marks `BeanInfo.getAlternativePriority()`
as deprecated for removal. For all practical purposes, `getPriority()`
is a better choice; it is usually meaningless to consider a bean
priority only when the bean is an alternative, and pretend that
the bean has no priority when it is not an alternative.
@Ladicek
ArC: support registering multiple custom contexts for single scope
68bb910
@Ladicek
ArC: implement BeanContainer.isMatchingBean() and isMatchingEvent()
71a5aea
@Ladicek
ArC: improve validation of built-in beans injection
c168bd4
@Ladicek
ArC: centralize code generation for wrapping InjectableReferenceProvi…
bc516b4 
…der with CurrentInjectionPointProvider
@Ladicek
ArC: fix CustomNormalScopeTest, we can use BeanContainer.getContexts(…
832a756 
…) now
@Ladicek
ArC: add support for method invokers
d93a237
@Ladicek
ArC: remove extra test/java17 directory, now that Java 17 is the minimum
5cd5952
@Ladicek
ArC: update documentation to CDI 4.1
ed93157
@Ladicek
ArC: fix running the CDI TCK
890ed6a
@Ladicek
Update TestNG to 7.8.0, a version used in the MicroProfile parent POM
3927cbd
@Ladicek
Update the RESTEasy Reactive TCK
944008f 
The new version doesn't contain the `@ManagedBean` annotation,
which was removed in Jakarta Annotations 3.0.
@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 26, 2024

Rebased and updated the RESTEasy Reactive TCK.

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 26, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 944008f.

✅ 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.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 26, 2024 •
edited by github-actions bot

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 944008f.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 Logs Raw logs 🔍
✔️ JVM Tests - JDK 21 Logs Raw logs 🔍
✖ JVM Tests - JDK 17 Windows Build ⚠️ Check → Logs Raw logs 🚧

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ Maven Tests - JDK 17 Windows

📦 integration-tests/maven

✖ io.quarkus.maven.it.CreateProjectMojoIT.testProjectGenerationFromScratch - History

  • Cannot delete directory: target\test-classes\projects\project-generation - java.lang.RuntimeException
java.lang.RuntimeException: Cannot delete directory: target\test-classes\projects\project-generation
	at io.quarkus.maven.it.MojoTestBase.initEmptyProject(MojoTestBase.java:72)
	at io.quarkus.maven.it.CreateProjectMojoIT.testProjectGenerationFromScratch(CreateProjectMojoIT.java:60)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: org.apache.commons.io.IOExceptionList: 1 exception(s): [org.apache.commons.io.IOIndexedException: IOException #0: Cannot delete file: target\test-classes\projects\project-generation\build-create-project-generation.log]
	at org.apache.commons.io.IOExceptionList.checkEmpty(IOExceptionList.java:50)

✖ io.quarkus.maven.it.CreateProjectMojoIT.testProjectGenerationFromScratch - History

  • Cannot delete directory: target\test-classes\projects\project-generation - java.lang.RuntimeException
java.lang.RuntimeException: Cannot delete directory: target\test-classes\projects\project-generation
	at io.quarkus.maven.it.MojoTestBase.initEmptyProject(MojoTestBase.java:72)
	at io.quarkus.maven.it.CreateProjectMojoIT.testProjectGenerationFromScratch(CreateProjectMojoIT.java:60)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: org.apache.commons.io.IOExceptionList: 1 exception(s): [org.apache.commons.io.IOIndexedException: IOException #0: Cannot delete file: target\test-classes\projects\project-generation\build-create-project-generation.log]
	at org.apache.commons.io.IOExceptionList.checkEmpty(IOExceptionList.java:50)

@gsmet
Copy link
Member

gsmet commented Apr 26, 2024

@mkouba @manovotn all good for you? CI looks happy. I let you merge if you are :).

@manovotn manovotn merged commit b77999a into quarkusio:main Apr 26, 2024
53 of 54 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Apr 26, 2024
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 26, 2024
@Ladicek Ladicek deleted the arc-cdi-4.1 branch April 29, 2024 06:34
@manovotn manovotn mentioned this pull request May 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkouba mkouba mkouba approved these changes

@manovotn manovotn manovotn 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 area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/platform Issues related to definition and interaction with Quarkus Platform triage/flaky-test
Projects
Quarkus Documentation
Done
Milestone
3.11.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Ladicek @gsmet @mkouba @gastaldi @manovotn