Consistent Discovery of external Connections

[radcortez]

We have many extensions that require connections to external systems. Probably the most popular is JDBC and Datasources, but we have many other extensions that fall in the same category, like Redis, Mongo, Artemis, and many more. When we started, all of these extensions only supported a single (default) client, and we have been slowly adding support for multi-tenancy (which has been requested on many occasions from our users).

To summarize, these extensions conceptually have to do the following:

• Discover the clients
• Configure the clients
• Manage the clients

Unfortunately, all of these extensions do different things and behave differently to accomplish the previous steps. Since the JDBC extension is the most popular and one of the most heavily invested in, let's look at what's happening there:

JDBC Extension

The JDBC Extension is really straightforward. Each named client, including defaut, requires setting a build time configuration quarkus.datasource.[name].db-kind to discover the names at build time. From this list, we can easily generate the CDI Beans for DataSource, start DevServices, and enable or disable the DataSource at runtime.

Other Extensions

Other extensions do not mandate a build-time property to express the client name. In the case of the Redis and Mongo extensions, they check for specific injection annotations to derive the names. Additionally, Redis, queries runtime configuration names to discover additional clients.

The Artemis extension duplicates the runtime configuration in a build-time mapping to behave a bit like the JDBC extension, but this is far from ideal, because it relies on runtime configuration at build time.

To summarize:

• Other extensions do not have a required build time property to discover names
• They also discover names using runtime configuration at build time
• They all do it differently (Artemis with a mapping, Redis queries the names directly, Mongo does not query runtime)

There are also other inconsistencies:

• Same extensions offer ways to configure named devservices, and for others, the configuration is global
• Same extensions offer ways to activate / deactivate connections at runtime, and others don't
• Same extensions create and close clients in a CDI bean, and others do it in the Recorder

These may not seem like much, but they become increasingly annoying when you have to use each of these extensions. Also, other extensions that I haven't looked at, but fall into the same patterns, may include: ElasticSearch, Infinispan, Kafka, and probably more in the Quarkiverse.

Fundamentally, one big difference between JDBC and other extensions is that you can easily add more clients through configuration, while others may require a code change (by a new client injection). Consider:

quarkus.datasource.db-kind=postgresql
quarkus.datasource."named".db-kind=postgresql

@Inject
@Any
InjectableInstance<DataSource> dataSources;

In the JDBC extension, I can do this to get as many Datasources as required. Yes, the app still needs a rebuild to discover new clients, since the name is set at build time.

In others extension, take Redis for instance:

@Inject
@RedisClientName("client1")
InjectableInstance<Redis> redisClient1;

@Inject
@RedisClientName("client2")
InjectableInstance<Redis> redisClient2;

This is the only reliable way to do it. Remember, runtime configuration may not be available at build time, and even if it is just for the purpose of extracting the name, we need to drop those patterns because it is not reliable, and we cannot ask users to place such configuration at build time.

Proposal

Ideally, we should follow the same pattern that we already have implemented in the JDBC extension. Discover the names through configuration and use them across the board.

Obviously, this is not feasible, since it would be a big breaking change. We probably need something in the middle, a build-time configuration that we can use to define the client names, while still discovering names in injection points. The final list of names is a merge of both. We have been using *.enabled to enable / disable features at build time, so maybe this could be a good candidate.

We would also remove all lookups to the runtime config to discover additional clients, keeping only the runtime lookups to check if a DevServices is required (this is a whole other topic).

Additionally, ensure that we follow consistent configuration options:

• Way to activate / deactivate clients at runtime
• Named Devservices customization

Other things to consider

• Where should we create, track, and destroy clients? For instance, Redis does it in the recorder, while Mongo is in a CDI bean. It is not exactly the same because the order and time at which these operations execute may be meaningful. There may be other implications (see next).

• Should we offer specific APIs for consumers to create their clients? Yes, it can be done by doing the work yourself, but you are missing all the configuration and other things we do. As an example, Mongo Liquibase requires a separate client connection to run the schema updates, and of course, you want to create that client with the same configuration used by the application client (this is how it works currently). We have seen some users requesting the ability to create clients dynamically at runtime. While injection wouldn't work in these cases, it would make it less painful.

• Named clients across extensions use a custom annotation to inject by name: @DataSouce, @RedisClientName, @MongoClientName. I find it annoying that I have to search for which annotation to use. For instance, Artermins uses an annotation we added in SR Common, @Identifier. While we cannot cut support for the annotations we currently support, I think we could standardize the way we inject named "things".

• There is really no good way to enforce whatever comes out of this discussion. We can create a couple of shared configuration classes to lock down the structure a bit, but ultimately, we would need to code review all of these to ensure it is consistent across the board.

Related issues:

• Consistent naming scheme and semantics for enabled/active configuration properties #42244
• Multitenant datasources #11861
• Datasources and persistence units with multiple "alternatives" listed at build time and selected at runtime #43656
• Better detection of when a persistence unit is needed and better reporting of when a non-existing PU is referenced #51198
• MongoDB Devservices can't load with MongoDB Liquibase with named clients #50977
• Managing feature flags and runtime configuration without excessive build-time restrictions #50810

[geoand]

cc @yrodiere, @Ladicek

[yrodiere]

These may not seem like much, but they become increasingly annoying when you have to use each of these extensions. Also, other extensions that I haven't looked at, but fall into the same patterns, may include: ElasticSearch, Infinispan, Kafka, and probably more in the Quarkiverse.

Note Elasticsearch is currently being handled by @marko-bekhta . He's adding support for named clients.

Additionally, Redis, queries runtime configuration names to discover additional clients.

This is the kind of things we should heavily push back against IMO. It's fundamentally incompatible with AOT compilation in particular and moving work to build-time in general. We need a closed world.

We probably need something in the middle, a build-time configuration that we can use to define the client names, while still discovering names in injection points. The final list of names is a merge of both

This is very interesting. I've been thinking for a while that it's very confusing that e.g. you need to set db-kind for a named datasource for it to be discovered. If we could discover injection points on top of parsing configuration, that would make UX better

The devil is in the details though:

1. Sometimes clients/datasources don't get injected at all, e.g. if you're using datasources through Hibernate exclusively. We'd need to add some synthetic injection points and to take those into account.
2. There may be a chicken/egg problem in some cases where an extension checks for the existence of a datasource in order to decide what to do. E.g. Hibernate ORM would disable itself if it detects a blocking datasource doesn't exist, and let Hibernate Reactive do its thing. So we can't have Hibernate ORM's reliance on the default datasource trigger the creation of the default datasource...

We have been using *.enabled to enable / disable features at build time, so maybe this could be a good candidate.

Note there are problems with this, most notably the difference between "enable the extension" and "enable the datasource/client". See #42244.

Should we offer specific APIs for consumers to create their clients?

I would be in favor of improving our support for multi-tenancy and customization first. See e.g. #11861.

The problem with APIs to create clients is users generally call those APIs at runtime, and supporting that reliably would mean a lot more work and testing around native compilation, because more code would execute at (native) runtime.

Named clients across extensions use a custom annotation to inject by name: @DataSouce, @RedisClientName, @MongoClientName. I find it annoying [...]

+100, I would much prefer @Named everywhere. We can't drop the existing qualifiers without breaking the world though.

Way to activate / deactivate clients at runtime

See also #43656. The current solution we have for datasources is great because it exists, but UX isn't great and it's confusing people. We might want to come up with a better UX before generalizing the solution.

[Ladicek]

+100, I would much prefer @Named everywhere. We can't drop the existing qualifiers without breaking the world though.

I would very much prefer @Named to never be used (except of the "foreign identifier" use case). @Identifier works as expected and everyone should just use that.

Injections are identified by a combination of type and qualifiers, so if I inject RedisDataSource, I can just use @Identifier to specify which one, I don't need @RedisClientName. Moving is hard, but possible -- the bean may have both qualifiers for the transition period. If we want to unify, I think we could manage that. Here's how it would work for Redis:

1. Deprecate @RedisClientName for removal, but keep the qualifier on the beans. The deprecation notice should mention that @Identifier is the replacement.
2. At the same time, add @Identifier to all beans that currently have @RedisClientName.
3. Wait some time, say 6 months.
4. Remove @RedisClientName. By that, I mean both remove the qualifier from the beans and remove the class.
5. Profit!

[geoand]

@Identifier works as expected and everyone should just use that.

Works fine for me!

[yrodiere]

@Identifier works for me, though 6 months looks like an awfully optimistic amount of time. I wouldn't expect @PersistenceUnit to disappear before a very long time, given how many apps it would impact.

There's also the problem of annotations being used for multiple purposes, e.g. @PersistenceUnit is used both for configuration on packages to say "this package contains entity for this persistence unit" and as a qualifier. We'll have to think of how switching to @Identifier impacts UX. Though obviously the problem exists even if we went with @Named.

Out of curiosity, what's the reason for staying away from @Named? Wouldn't it "just work" to use both @Named and @Identifier?

[geoand]

Out of curiosity, what's the reason for staying away from @nAmed

I don't know the CDI details, but I am pretty sure that it would fail if you had two Redis and two Mongo clients for example where you tried to use the same name

[Ladicek]

@Identifier works for me, though 6 months looks like an awfully optimistic amount of time. I wouldn't expect @PersistenceUnit to disappear before a very long time, given how many apps it would impact.

I'm sure the @PersistenceUnit annotation is a wholly different beast to, say, @RedisClientName, and would have to be treated differently. Perhaps moving to @Identifier doesn't even make sense in this case.

Out of curiosity, what's the reason for staying away from @Named? Wouldn't it "just work" to use both @Named and @Identifier?

The problem with @Named is that its presence on its own doesn't prevent the addition of @Default. If the only qualifier a bean has is @Named, the bean also has @Default and so it can be injected unqualified.

I'll illustrate: if you have

@Named("foobar")
@ApplicationScoped
public class FooBar {
}

then you can inject it using @Inject FooBar.

In this particular case, when we'd like to use a qualifier to distinguish "named clients" from the "default client", using @Named means that @Inject MyClient results in an unresolvable ambiguity whenever a default client and at least one named client exists, because there would be multiple MyClient beans with the @Default qualifier.

In short: avoid @Named like a plague.

[radcortez]

1. Sometimes clients/datasources don't get injected at all, e.g. if you're using datasources through Hibernate exclusively. We'd need to add some synthetic injection points and to take those into account.

But because db-kind is mandatory, you already know that you are going to require that datasource, even without an injection point, right?

2. There may be a chicken/egg problem in some cases where an extension checks for the existence of a datasource in order to decide what to do. E.g. Hibernate ORM would disable itself if it detects a blocking datasource doesn't exist, and let Hibernate Reactive do its thing. So we can't have Hibernate ORM's reliance on the default datasource trigger the creation of the default datasource...

So the issue here is if there is no injection point, you don't know which synthetic bean to keep for the creation? In that case, couldn't we keep both beans and defer to runtime, which one is going to be used by looking into the runtime configuration?

[yrodiere]

1. Sometimes clients/datasources don't get injected at all, e.g. if you're using datasources through Hibernate exclusively. We'd need to add some synthetic injection points and to take those into account.

But because db-kind is mandatory, you already know that you are going to require that datasource, even without an injection point, right?

db-kind is optional when you only have one "flavor" of datasource extension in your app, say only postgresql. So it is weird that we require it in the case of named datasources, for the sole purpose of knowing that the user wants this named datasource to exist.

2. There may be a chicken/egg problem in some cases where an extension checks for the existence of a datasource in order to decide what to do. E.g. Hibernate ORM would disable itself if it detects a blocking datasource doesn't exist, and let Hibernate Reactive do its thing. So we can't have Hibernate ORM's reliance on the default datasource trigger the creation of the default datasource...

So the issue here is if there is no injection point, you don't know which synthetic bean to keep for the creation? In that case, couldn't we keep both beans and defer to runtime, which one is going to be used by looking into the runtime configuration?

You're essentially suggesting to deactivate the ORM/Reactive PU depending on whether the datasource is active at runtime. That's possible but last time I tried to introduce that I had problems.
I don't remember exactly what they were but I think it's mostly around Panache: if we do what you suggest, and someone uses Panache, and they forget to configure the datasource, we will just boot happily, deactivating both datasource and Hibernate ORM, and fail at runtime, on the first use of Panache, instead of detecting the problem on startup. That's because when using Panache, users don't need to inject Hibernate-related CDI components in their app, so we assume they don't use it, so deactivation is fine.

I suppose the solution to that would be to somehow mark the Hibernate PU as used as soon as people are using Panache in that PU.

Anyway, this is probably solvable. But not straightforward, especially for people who'll go through the changes in their app.

[radcortez]

db-kind is optional when you only have one "flavor" of datasource extension in your app, say only postgresql. So it is weird that we require it in the case of named datasources, for the sole purpose of knowing that the user wants this named datasource to exist.

Ok, I thought it was always mandatory. So, can we claim that the complete list of data sources can be inferred from:

• datasource injection points
• build time named clients configuration references
• hibernate injection points
• build time named clients configuration references

?

I suppose the solution to that would be to somehow mark the Hibernate PU as used as soon as people are using Panache in that PU.

I guess that we could validate this directly in Panache?

[yrodiere]

• datasource injection points

• build time named clients configuration references

• hibernate injection points

• build time named clients configuration references

If the last one is meant to be "build time named persistence units", and if by "named" you mean "default and named", then yes I think that's sufficient information.

Though like I said above, if we do that we'll likely need some changes in the Hibernate extensions.

I suppose the solution to that would be to somehow mark the Hibernate PU as used as soon as people are using Panache in that PU.

I guess that we could validate this directly in Panache?

I suppose we could have Panache, on startup, retrieve all PUs it relies on so that if one is unavailable we'll throw an exception. The error message wouldn't be great, but it'd be a start.

Essentially we'd remove this code in Hibernate ORM (because the PU is now necessarily inactive if the datasource is inactive):

quarkus/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/FastBootHibernatePersistenceProvider.java

Lines 468 to 473 in c7f02f0

	try {
	// ClientProxy.unwrap is necessary to trigger exceptions on inactive datasources
	dataSource = ClientProxy.unwrap(AgroalDataSourceUtil.dataSourceInstance(dataSourceName).get());
	} catch (RuntimeException e) {
	throw PersistenceUnitUtil.unableToFindDataSource(persistenceUnitName, dataSourceName, e);
	}

... and we're introduce similar code in Panache, except it'd be retrieving a PU instead of a datasource.

[FroMage]

db-kind is optional when you only have one "flavor" of datasource extension in your app, say only postgresql

Right, I was thinking the same when reading the discussion description.

[radcortez]

We have been using *.enabled to enable / disable features at build time, so maybe this could be a good candidate.

Note there are problems with this, most notably the difference between "enable the extension" and "enable the datasource/client". See #42244.

Certainly. I think we are in agreement that we need to offer a way to enable / disable clients at build time. If we consider something like:

quarkus.hibernate-orm.enabled - Extension level build-time - The extension doesn't do anything if disabled
quarkus.hibernate-orm[."persistence-unit-name"].enabled - Persistence Unit level build-time - The extension client doesn't do anything if disabled
quarkus.hibernate-orm[."persistence-unit-name"].active - Pesistente Unit level runtime - The extension client may be deactivated if there are no mandatory injection points and the unit was enabled at build time

So enabled at the root config of the extension and in selected features in the tree, will either disable the entire thing or disable selected features. The active, same thing, but for runtime.

Obviously, we need to align this behaviour across the board.

[yrodiere]

quarkus.hibernate-orm.enabled - Extension level build-time - The extension doesn't do anything if disabled
quarkus.hibernate-orm[."persistence-unit-name"].enabled - Persistence Unit level build-time - The extension client doesn't do anything if disabled

If you do that, there is no way to disable just the default PU and not the named ones.

Because yes, we decided at some point it was a good idea to allow having both a default PU and named PU -- same for datasources :(

[radcortez]

Well, it is possible, but you are not going to like it :)

You can actually use the key that you set in @WithUnnamedKey to refer directly to the Map entry in the configuration Map, so this would work: quarkus.hibernate-orm.<default>.enabled.

But I do agree it may be confusing.

[yrodiere]

Exactly. Which is why I'd rather we moved the "extension disabling" to something separate.
Quoting from #42244:

• And possibly, to avoid ambiguity between disabling an extension and e.g. disabling just the default persistence unit[/datasource/client], a built-in feature in Quarkus (no need to implement anything in extensions) to disable extensions that are in the classpath. This could be either quarkus.myextension.enable-extension, or more likely some more arcane property like quarkus.extensions."extension-id".enabled.

[radcortez]

Ok, so I think we are all in agreement to support @Identifier instead of specific annotations. @PersistenceUnit is a different case, because it is coming from the JPA spec. For all other cases where we added our own annotations like @RedisClientName, @MongoClientName, etc., let's move to @Identifier and deprecate the old annotations.

[Ladicek]

I think I agree.

[yrodiere]

@PersistenceUnit is a different case

Yes.

because it is coming from the JPA spec

Nope. We're talking about io.quarkus.hibernate.orm.PersistenceUnit.

[radcortez]

Ah, nvm, I didn't notice we had a io.quarkus.hibernate.orm.PersistenceUnit. I thought we were talking about jakarta.persistence.PersistenceUnit :)