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 - introduce the concept of synthetic injection points #31178

Merged
merged 4 commits into from
Feb 18, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Feb 15, 2023

  • a synthetic injection point can be registered by a synthetic bean via BeanConfiguratorBase#addInjectionPoint()
  • this injection point is validated at build time and considered when
    detecting unused beans
  • at runtime an injected reference is accessible through the SyntheticCreationalContext#getInjectedReference() methods

TODO

  • update docs

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/liquibase labels Feb 15, 2023
@mkouba mkouba requested review from Ladicek and manovotn February 15, 2023 09:48
@mkouba
Copy link
Contributor Author

mkouba commented Feb 15, 2023

CC @geoand @gsmet

@geoand
Copy link
Contributor

geoand commented Feb 15, 2023

As I don't have any context about this, I would like to know what the use case you have in mind is

@mkouba
Copy link
Contributor Author

mkouba commented Feb 15, 2023

As I don't have any context about this, I would like to know what the use case you have in mind is

@geoand Take a look at the second commit (Liquibase extension).

@geoand
Copy link
Contributor

geoand commented Feb 15, 2023

I see, thanks.

Seems like a nice improvement.

@mkouba mkouba force-pushed the arc-synthetic-injection-points branch from 4d01556 to 33bf2b1 Compare February 15, 2023 10:08
@mkouba mkouba force-pushed the arc-synthetic-injection-points branch from 33bf2b1 to bb5acbe Compare February 15, 2023 10:27
@mkouba mkouba force-pushed the arc-synthetic-injection-points branch from bb5acbe to bcd4940 Compare February 15, 2023 10:41
@mkouba mkouba force-pushed the arc-synthetic-injection-points branch 2 times, most recently from dabd239 to b8dcf27 Compare February 15, 2023 12:07
@mkouba
Copy link
Contributor Author

mkouba commented Feb 15, 2023

This pull request is also related to #3699. The difference is that synthetic injection points are validated at build time and considered when detecting unused beans. Of course, it's possible to define a synthetic equivalent of @Inject Instance<Object>.

@quarkus-bot quarkus-bot bot added the area/persistence OBSOLETE, DO NOT USE label Feb 16, 2023
@mkouba
Copy link
Contributor Author

mkouba commented Feb 16, 2023

I found another use case for synthetic injection point while fixing a test failure - the Hibernate ORM extension.

@yrodiere FYI

@Ladicek
Copy link
Contributor

Ladicek commented Feb 16, 2023

I realized yesterday this is fairly close to what we allow in CDI Build Compatible Extensions (except this is quite a bit better), and that made me think about these interesting test cases that I see are not covered by the tests in this PR:

  • synthetic injection point of type InjectionPoint
    • if the synthetic bean is @Dependent
    • if the synthetic bean is not @Dependent (should fail)
  • synthetic injection point of type Instance<Object>
    • should disable unused bean removal IIUC, but that's probably sufficiently tested elsewhere
    • lookup of InjectionPoint from that Instance<Object>
      • if the synthetic bean is @Dependent
      • if the synthetic bean is not @Dependent (should fail)
    • lookup of @Dependent bean from the Instance<Object> (should be destroyed when the synthetic bean is destroyed IIUC)
  • synthetic injection point that resolves to a @Dependent bean -- test when is the @Dependent bean destroyed

Also, do we want something like this in the synthetic bean disposal function? IIUC, this PR only allows this in the creation function.

@mkouba
Copy link
Contributor Author

mkouba commented Feb 16, 2023

I realized yesterday this is fairly close to what we allow in CDI Build Compatible Extensions (except this is quite a bit better), and that made me think about these interesting test cases that I see are not covered by the tests in this PR:

* synthetic injection point of type `InjectionPoint`

Well, this is something we should probably test.

  * if the synthetic bean is `@Dependent`
  * if the synthetic bean is _not_ `@Dependent` (should fail)

* synthetic injection point of type `Instance<Object>`
  
  * should disable unused bean removal IIUC, but that's probably sufficiently tested elsewhere

This should be tested in the io.quarkus.arc.test.buildextension.beans.SyntheticInjectionPointInstanceTest.

  * lookup of `InjectionPoint` from that `Instance<Object>`

This is rather exotic use case ;-)

    * if the synthetic bean is `@Dependent`
    * if the synthetic bean is _not_ `@Dependent` (should fail)
  * lookup of `@Dependent` bean from the `Instance<Object>` (should be destroyed when the synthetic bean is destroyed IIUC)

This one is interesting.

* synthetic injection point that resolves to a `@Dependent` bean -- test when is the `@Dependent` bean destroyed

A test for synthetic @Dependent bean would be nice.

Also, do we want something like this in the synthetic bean disposal function? IIUC, this PR only allows this in the creation function.

I don't think we want it now. But we could add the support later.

@quarkus-bot

This comment has been minimized.

@mkouba
Copy link
Contributor Author

mkouba commented Feb 16, 2023

I found another use case for synthetic injection point while fixing a test failure - the Hibernate ORM extension.

@yrodiere FYI

Hm, there's a problem with the Hibernate Reactive extension - it obviously depends on ORM but some of the dependencies of Session/EntityManager synthetic beans are not registered in the HibernateOrmProcessor.registerBeans(). The tests fail with Unsatisfied dependency for type io.quarkus.hibernate.orm.runtime.TransactionSessions - so it's probably because the Capability.TRANSACTIONS is not present.

I wonder if it actually makes sense to register the beans for Session/EntityManager when using Hibernate Reactive?

@Sanne @DavideD @yrodiere @gsmet WDYT?

@mkouba mkouba force-pushed the arc-synthetic-injection-points branch from c9dc056 to 89effe4 Compare February 16, 2023 15:58
@quarkus-bot quarkus-bot bot added the area/hibernate-reactive Hibernate Reactive label Feb 16, 2023
@mkouba
Copy link
Contributor Author

mkouba commented Feb 16, 2023

I found another use case for synthetic injection point while fixing a test failure - the Hibernate ORM extension.
@yrodiere FYI

Hm, there's a problem with the Hibernate Reactive extension - it obviously depends on ORM but some of the dependencies of Session/EntityManager synthetic beans are not registered in the HibernateOrmProcessor.registerBeans(). The tests fail with Unsatisfied dependency for type io.quarkus.hibernate.orm.runtime.TransactionSessions - so it's probably because the Capability.TRANSACTIONS is not present.

I wonder if it actually makes sense to register the beans for Session/EntityManager when using Hibernate Reactive?

@Sanne @DavideD @yrodiere @gsmet WDYT?

I've modified the HibernateOrmProcessor to only register a bean for Session/EntityManager if JTA is present.

@mkouba mkouba requested review from Ladicek and manovotn February 16, 2023 16:05
@Sanne
Copy link
Member

Sanne commented Feb 16, 2023

I wonder if it actually makes sense to register the beans for Session/EntityManager when using Hibernate Reactive?

No, I don't think people should use the blocking APIs on Hibernate Reactive.

FWIW the fact that the Hibernate Reactive extension is depending on the Hibernate ORM extension was IMO a mistake; it's a fact that the Hibernate Reactive project depends on Hibernate ORM core, but it shouldn't pull in its extension as that expresses a different intent for the user.

@quarkus-bot

This comment has been minimized.

@mkouba mkouba force-pushed the arc-synthetic-injection-points branch from 89effe4 to d556b2d Compare February 17, 2023 07:03
@quarkus-bot quarkus-bot bot added the area/hibernate-search Hibernate Search label Feb 17, 2023
Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor comments, feel free to ignore :-)

- a synthetic injection point can be registered by a synthetic bean
- this injection point is validated at build time and considered when
detecting unused beans
- at runtime an injected reference is accessible through the
SyntheticCreationalContext
- register Session/EntityManager bean only if JTA is present
@mkouba mkouba force-pushed the arc-synthetic-injection-points branch from d556b2d to 81168bc Compare February 17, 2023 08:20
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 17, 2023
@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 17, 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.

@mkouba mkouba merged commit d596a3c into quarkusio:main Feb 18, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 18, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 18, 2023
@yrodiere
Copy link
Member

FWIW the fact that the Hibernate Reactive extension is depending on the Hibernate ORM extension was IMO a mistake; it's a fact that the Hibernate Reactive project depends on Hibernate ORM core, but it shouldn't pull in its extension as that expresses a different intent for the user.

And FTR @mkouba we have an issue about this: #28629

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) area/documentation area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive area/hibernate-search Hibernate Search area/liquibase area/persistence OBSOLETE, DO NOT USE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants