HHH-15422 Pick up `CurrentTenantIdentifierResolver` and `MultiTenantConnectionProvider` from BeanContainer if not explicit set #8697

quaff · 2024-07-24T04:39:33Z

[Please describe here what your change is about]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

https://hibernate.atlassian.net/browse/HHH-15422

quaff · 2024-08-30T01:52:35Z

Could you review this also? @gavinking

gavinking · 2024-08-30T17:34:44Z

So we have not up until this point allowed ConnectionProviders to be beans, and so:

I wonder why it makes sense to do this for the special case of MultiTenantConnectionProvider if we don't do it in general. What's special about MultiTenantConnectionProvider? I assume the reason is that you want to @Inject into it. But inject what precisely? Tell me a story here. Given me an example.
We have been having troubles with chicken/egg problems with respect to the CDI integration we already have, and so I'm sorta disinclined to add new functionality to that until we feel like we have this stuff licked.

I dunno, WDYT @sebersole @scottmarlow @yrodiere?

yrodiere · 2024-09-03T10:06:40Z

I wonder why it makes sense to do this for the special case of MultiTenantConnectionProvider if we don't do it in general. What's special about MultiTenantConnectionProvider? I assume the reason is that you want to @Inject into it. But inject what precisely? Tell me a story here. Given me an example.

To others reading this: the conversation continued on the Jira, see https://hibernate.atlassian.net/browse/HHH-15422?focusedCommentId=116437

But IMO the main point here is that a MultiTenantConnectionProvider needs access to datasources, which are typically created by the framework, and thus are more easily retrieved through injection.

We have been having troubles with chicken/egg problems with respect to the CDI integration we already have

That's a good point, too much CDI reliance can be problematic in Jakarta application servers, or at least in WildFly.

I would add that relying on CDI in very early phases of bootstrap is problematic for Quarkus as well, if we want to move more of the bootstrap to build time. Though this specific PR seems fine in that regard.

and so I'm sorta disinclined to add new functionality to that until we feel like we have this stuff licked.

I kind of agree, but I don't think that's a healthy approach unless we have a clearly identified Jira issue and at least the beginning of a plan to solve that chicken/egg problem.
If we don't intend to solve that chicken/egg problem, then we probably need to record the criteria for further CDI integration somewhere (must be lazy, or must happen after a certain point in bootstrap, or ...).

gavinking

Please redo this to use the mechanism described here:

https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#entity-listeners

and also, redundantly, here:

https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#a2999

These passages of the JPA spec describe how to enable injection into a persistence-managed object without having to make that object into a CDI bean.

This is actually what we should always be doing in this sort of situation.

hibernate-core/src/main/java/org/hibernate/boot/internal/SessionFactoryOptionsBuilder.java

...-core/src/test/java/org/hibernate/orm/test/idgen/userdefined/UserDefinedGeneratorsTests.java

hibernate-core/src/test/java/org/hibernate/orm/test/idgen/userdefined/SimpleBeanContainer.java

...a/org/hibernate/engine/jdbc/connections/internal/MultiTenantConnectionProviderInitiator.java

gavinking · 2024-10-04T11:56:32Z

To be clear, because there's apparently a lot of historical confusion on this point.

There's three strong reasons why something might need to be a bean:

it's going to be injected into other things,
it's contextual, i.e., it has a scope, or
it needs to have interceptors to handle transaction management or whatever.

None of those motivations apply to the objects we're talking about here, and so they do not need to be beans.

beikov · 2024-10-04T11:59:20Z

If you think about a MultiTenantConnectionProvider which can delegate it's work to multiple datasource, it is highly desirable to make it a bean to inject datasources into it.
I just don't see a reason why we wouldn't want to make these things beans if we clearly know they could be.

gavinking · 2024-10-04T12:01:54Z

If you think about a MultiTenantConnectionProvider which can delegate it's work to multiple datasource, it is highly desirable to make it a bean to inject datasources into it.

I'm not sure if you've simply not read my comments, or if you've not understood them. But the whole point is that:

Things do not need to be beans in order to have beans injected into them.

Please go and read those sections of the JPA spec which explain how to do it. (We even already have an implementation of that in Hibernate, by the way.)

I just don't see a reason why we wouldn't want to make these things beans if we clearly know they could be.

Because that ties the lifecycle of Hibernate with the lifecycle of CDI and creates problems.

gavinking · 2024-10-04T19:10:38Z

So it turns out that my objections to this pull request actually boil down to only the following, which occurs in two places:

@Override
public boolean useJpaCompliantCreation() {
	return false;
}

If we change those to return true;, I think my objections are addressed, and I'm fine with doing this.

(Note that this is consistent with every other implementation of BeanContainer.LifecycleOptions internal to Hibernate.)

quaff · 2024-10-08T02:25:26Z

So it turns out that my objections to this pull request actually boil down to only the following, which occurs in two places:
@Override
public boolean useJpaCompliantCreation() {
	return false;
}
If we change those to return true;, I think my objections are addressed, and I'm fine with doing this.

(Note that this is consistent with every other implementation of BeanContainer.LifecycleOptions internal to Hibernate.)

Quoted from JpaCompliantLifecycleStrategy.java

 * In particular, {@code @Singleton}-scoped or {@code @ApplicationScoped} beans are instantiated
 * directly by this strategy, even if there is already an instance in the CDI context.
 * This means singletons are not really singletons, but this seems to be the behavior required by
 * the JPA 2.2 spec.

It means bean in context are not reused, it will create two instances, I don't like this behavior.

gavinking · 2024-10-08T06:38:56Z

bean in context are not reused

Because they would not be beans. That's the point.

it will create two instances, it will create two instances

It will create one instance per persistence unit, which is completely fine.

quaff · 2024-10-08T06:44:32Z

it will create two instances

It will create one instance per persistence unit, which is completely fine.

Take spring for example, I mean one instance for spring container, one instance for Hibernate, I need it be real singleton.

gavinking · 2024-10-08T06:52:45Z

it will create two instances

You need what to be a singleton?

quaff · 2024-10-08T06:58:49Z

it will create two instances

You need what to be a singleton?

The proposed CurrentTenantIdentifierResolver and MultiTenantConnectionProvider.

yrodiere · 2024-10-08T08:04:23Z

it will create two instances

You need what to be a singleton?

The proposed CurrentTenantIdentifierResolver and MultiTenantConnectionProvider.

This is what we were discussing yesterday, Gavin. Most people will just expect a CDI integration to involve actual CDI beans and to comply with scopes, and frankly I agree with them: it's just KISS. I know you disagree, at least in general, and I know there are technical reasons to not do that for e.g. ID generators and connection providers.

That being said, putting aside other component types, I thought you agreed it was reasonable to do that for the tenant identifier resolver?

EDIT: That being said, @quaff, regarding MultiTenantConnectionProvider I'm not sure we can reasonably reuse a singleton bean, as that class can implement Configurable and get injected with PU-specific config.

gavinking · 2024-10-08T08:06:01Z

So as I was saying to Yoann on Zulip, I can see that CurrentTenantIdentifierResolver might legitimately be a contextual bean.

I absolutely struggle to see how this could ever possibly be legit for any sort of connection provider. Connection providers have persistence-unit-specific configuration.

And any contextual state which they might need can be injected into them.

quaff · 2024-10-08T08:39:27Z

as that class can implement Configurable and get injected with PU-specific config.

OK, I get the point, I changed useJpaCompliantCreation() to return true, please review.

gavinking · 2024-10-08T09:18:36Z

Most people will just expect a CDI integration to involve actual CDI beans and to comply with scopes

I mean, just I don't see that.

Servlets support CDI injection. Do you expect to be able to inject a servlet into something else?
Interceptors support CDI injection. Do you expect to be able to inject an interceptor into something else?
etc.

I don't think you do expect that, so why would you expect it here?

From where would the assumption that one can inject a MultiTenantConnectionProvider into an arbitrary CDI bean arise? It's defined as a Hibernate Service, not as a CDI bean!

I thought you agreed it was reasonable to do that for the tenant identifier resolver?

Right, correct, I think there's a reasonable argument that CurrentTenantIdentifierResolver is a special case here, since:

it's much more likely to be dealing with contextual state (associated with a request or session),
it's not a Hibernate Service,
it's not likely to depend on any Hibernate Services or other PU-specific state, and
it doesn't have any defined instantiation/configuration semantics, and so Hibernate doesn't need to be involved in instantiating it.

Therefore, by making CurrentTenantIdentifierResolver we're providing some convenience, without tying together the lifecycle of the bean container with the instantiation of Hibernate.

I'm much more concerned about MultiTenantConnectionProvider here.

gavinking · 2024-10-08T09:32:16Z

as that class can implement Configurable and get injected with PU-specific config.

OK, I get the point, I changed useJpaCompliantCreation() to return true, please review.

Yeah, that looks fine now.

gavinking · 2024-10-08T09:58:11Z

I think there's a reasonable argument that CurrentTenantIdentifierResolver is a special case here

Indeed, you could choose to view the CurrentTenantIdentifierResolver as precisely a kind of "bridge" between the world where contextual information about the request/session is available, and the world of PU-specific state held by Services.

And that explains why it's a special case.

yrodiere · 2024-10-08T10:15:50Z

I mean, just I don't see that.

Servlets support CDI injection. Do you expect to be able to inject a servlet into something else?

Interceptors support CDI injection. Do you expect to be able to inject an interceptor into something else?

etc.

I don't think you do expect that, so why would you expect it here?

I do not expect to need to inject servlets, interceptors, or most pluggable Hibernate components into my application -- with the exception of tenant identifier resolvers and some others.

I do however expect that if servlets, interceptors, or pluggable Hibernate components are involved in CDI, then:

Trying to inject them into my application will either work, using a CDI scope as intuitive as possible, or somehow notify me (exception?) that such component just cannot be a CDI bean.
Putting @ApplicationScoped or @RequestScoped on them, however nonsensical, will either be taken into account, or somehow notify me (exception?) that it's invalid.

These expectations may not match current implementations, or even the specs, but I think they are reasonable from an UX point of view, and they do comply with the principle of least surprise.
They may not be achievable completely (since part of the behavior would have to be implemented in the CDI container), but I think we should at least try where possible.

...rc/main/java/org/hibernate/engine/jdbc/connections/internal/ConnectionProviderInitiator.java

…ectionProvider from BeanContainer if not explicit set

…it set

gavinking

@quaff, thanks, but please see my further comments.

gavinking · 2024-10-17T07:35:59Z

hibernate-core/src/main/java/org/hibernate/boot/internal/SessionFactoryOptionsBuilder.java

+			final BeanContainer beanContainer = Helper.allowExtensionsInCdi( serviceRegistry ) ? serviceRegistry.requireService( ManagedBeanRegistry.class ).getBeanContainer() : null;
+			if (beanContainer != null) {
+				this.currentTenantIdentifierResolver = beanContainer.getBean(
+						CurrentTenantIdentifierResolver.class,
+						new BeanContainer.LifecycleOptions() {
+							@Override
+							public boolean canUseCachedReferences() {
+								return true;
+							}
+
+							@Override
+							public boolean useJpaCompliantCreation() {
+								return false;
+							}
+						},
+						new BeanInstanceProducer() {
+
+							@Override
+							public <B> B produceBeanInstance(Class<B> beanType) {
+								return null;
+							}
+
+							@Override
+							public <B> B produceBeanInstance(String name, Class<B> beanType) {
+								return null;
+							}
+
+						}
+				).getBeanInstance();
+			}


Please extract out a function for this, the constructor is already waaaaaay too long.

@gavinking I suggest to introduce new util methods for org.hibernate.resource.beans.internal.Helper and refactor all BeanContainer access to use those methods in another PR, WDYT?

@SuppressWarnings( "unchecked" ) public static <T> T getBean(ServiceRegistry serviceRegistry, Class<?> beanType, boolean canUseCachedReferences, boolean useJpaCompliantCreation, T fallback) { final BeanContainer beanContainer = allowExtensionsInCdi( serviceRegistry ) ? serviceRegistry.requireService( ManagedBeanRegistry.class ).getBeanContainer() : null; if ( beanContainer == null ) { return null; } return (T) beanContainer.getBean( beanType, new BeanContainer.LifecycleOptions() { @Override public boolean canUseCachedReferences() { return canUseCachedReferences; } @Override public boolean useJpaCompliantCreation() { return useJpaCompliantCreation; } }, new BeanInstanceProducer() { @Override public <B> B produceBeanInstance(Class<B> beanType) { return (B) fallback; } @Override public <B> B produceBeanInstance(String name, Class<B> beanType) { throw new UnsupportedOperationException("The method shouldn't be called"); } } ).getBeanInstance(); }

I suggest to introduce new util methods for org.hibernate.resource.beans.internal.Helper and refactor all BeanContainer access to use those methods

That sounds fine to me, but please do it now, so that it doesn't get forgotten. Thanks.

I created https://hibernate.atlassian.net/browse/HHH-18737 just now.

OK, great, thanks 🙏

gavinking · 2024-10-17T07:36:32Z

...rc/main/java/org/hibernate/engine/jdbc/connections/internal/ConnectionProviderInitiator.java

+				return beanContainer.getBean(
+						ConnectionProvider.class,
+						new BeanContainer.LifecycleOptions() {
+							@Override
+							public boolean canUseCachedReferences() {
+								return true;
+							}
+
+							@Override
+							public boolean useJpaCompliantCreation() {
+								return true;
+							}
+						},
+						new BeanInstanceProducer() {
+
+							@Override
+							public <B> B produceBeanInstance(Class<B> beanType) {
+								return (B) noAppropriateConnectionProvider();
+							}
+
+							@Override
+							public <B> B produceBeanInstance(String name, Class<B> beanType) {
+								return (B) noAppropriateConnectionProvider();
+							}
+
+						}
+				).getBeanInstance();
+			}
+			else {
+				return noAppropriateConnectionProvider();
+			}
+
 		}


Similarly, please extract a function.

gavinking · 2024-10-17T07:38:47Z

...a/org/hibernate/engine/jdbc/connections/internal/MultiTenantConnectionProviderInitiator.java

+			final BeanContainer beanContainer = Helper.allowExtensionsInCdi( registry ) ? registry.requireService( ManagedBeanRegistry.class ).getBeanContainer() : null;
+			if (beanContainer != null) {
+				return beanContainer.getBean(
+						MultiTenantConnectionProvider.class,
+						new BeanContainer.LifecycleOptions() {
+							@Override
+							public boolean canUseCachedReferences() {
+								return true;
+							}
+
+							@Override
+							public boolean useJpaCompliantCreation() {
+								return true;
+							}
+						},
+						new BeanInstanceProducer() {
+
+							@Override
+							public <B> B produceBeanInstance(Class<B> beanType) {
+								return null;
+							}
+
+							@Override
+							public <B> B produceBeanInstance(String name, Class<B> beanType) {
+								return null;
+							}
+
+						}
+				).getBeanInstance();
+			}


Similarly, extract a function here, please.

quaff force-pushed the HHH-15422 branch from 323ae64 to d43e8eb Compare July 24, 2024 06:49

quaff force-pushed the HHH-15422 branch from a75672b to 80572d0 Compare August 30, 2024 01:33

gavinking requested changes Oct 4, 2024

View reviewed changes

beikov requested changes Oct 4, 2024

View reviewed changes

quaff force-pushed the HHH-15422 branch from 80572d0 to d24d5ff Compare October 8, 2024 02:38

quaff force-pushed the HHH-15422 branch from d24d5ff to 7582932 Compare October 8, 2024 08:37

yrodiere requested review from beikov and gavinking October 8, 2024 09:52

gavinking approved these changes Oct 8, 2024

View reviewed changes

quaff marked this pull request as draft October 9, 2024 01:04

quaff marked this pull request as ready for review October 9, 2024 02:21

beikov approved these changes Oct 16, 2024

View reviewed changes

gavinking requested changes Oct 16, 2024

View reviewed changes

...rc/main/java/org/hibernate/engine/jdbc/connections/internal/ConnectionProviderInitiator.java Outdated Show resolved Hide resolved

quaff added 3 commits October 17, 2024 08:32

HHH-15422 Pick up CurrentTenantIdentifierResolver and MultiTenantConn…

7f73b84

…ectionProvider from BeanContainer if not explicit set

HHH-15422 Update corresponding documentation

08c6892

HHH-15422 Pick up ConnectionProvider from BeanContainer if not explic…

477faa1

…it set

quaff force-pushed the HHH-15422 branch from 942353e to 477faa1 Compare October 17, 2024 00:32

gavinking requested changes Oct 17, 2024

View reviewed changes

beikov merged commit 8b5bc44 into hibernate:main Oct 17, 2024
19 of 20 checks passed

Uh oh!

HHH-15422 Pick up CurrentTenantIdentifierResolver and MultiTenantConnectionProvider from BeanContainer if not explicit set #8697

HHH-15422 Pick up CurrentTenantIdentifierResolver and MultiTenantConnectionProvider from BeanContainer if not explicit set #8697

Uh oh!

Conversation

quaff commented Jul 24, 2024 • edited by hibernate-github-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quaff commented Aug 30, 2024

Uh oh!

gavinking commented Aug 30, 2024

Uh oh!

yrodiere commented Sep 3, 2024

Uh oh!

gavinking left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gavinking commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beikov commented Oct 4, 2024

Uh oh!

gavinking commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavinking commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quaff commented Oct 8, 2024

Uh oh!

gavinking commented Oct 8, 2024

Uh oh!

quaff commented Oct 8, 2024

Uh oh!

gavinking commented Oct 8, 2024

Uh oh!

quaff commented Oct 8, 2024

Uh oh!

yrodiere commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavinking commented Oct 8, 2024

Uh oh!

quaff commented Oct 8, 2024

Uh oh!

gavinking commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavinking commented Oct 8, 2024

Uh oh!

gavinking commented Oct 8, 2024

Uh oh!

yrodiere commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gavinking left a comment

Choose a reason for hiding this comment

Uh oh!

gavinking Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

quaff Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

gavinking Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

quaff Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

gavinking Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

HHH-15422 Pick up `CurrentTenantIdentifierResolver` and `MultiTenantConnectionProvider` from BeanContainer if not explicit set #8697

HHH-15422 Pick up `CurrentTenantIdentifierResolver` and `MultiTenantConnectionProvider` from BeanContainer if not explicit set #8697

quaff commented Jul 24, 2024 •

edited by hibernate-github-bot bot

Loading

gavinking left a comment •

edited

Loading

gavinking commented Oct 4, 2024 •

edited

Loading

gavinking commented Oct 4, 2024 •

edited

Loading

gavinking commented Oct 4, 2024 •

edited

Loading

yrodiere commented Oct 8, 2024 •

edited

Loading

gavinking commented Oct 8, 2024 •

edited

Loading

yrodiere commented Oct 8, 2024 •

edited

Loading