ArC: add InjectableInstance.getActive() and listActive()

[Ladicek]

Related to #43765

[Ladicek]

I know @mkouba wasn't exactly a fan, so I can be persuaded to get rid of most of this :-)

[github-actions]

🙈 The PR is closed and the preview is expired.

[mkouba]

I know @mkouba wasn't exactly a fan, so I can be persuaded to get rid of most of this :-)

My point is that @All List<> is useful for end users. I'm not so sure about @Active which is quite complicated to understand and can only be used to filter synthetic beans 🤷.

[manovotn]

I know @mkouba wasn't exactly a fan, so I can be persuaded to get rid of most of this :-)

To be fair, I am unsure of this as well.
I see inactive beans mostly as a compromise we have to do to enable certain functionality for integrators and I think (hope?) ordinary users won't have to deal with it at all.
With that in mind, I don't see much use for this feature; not at the moment anyway.
To be clear, I am not against it, just not really convinced we need it now :)

[Ladicek]

Understood. @yrodiere is this something you would use? If not, I can close this, or turn back to the alternative described in #43765.

[yrodiere]

Thanks for working on this!

I see inactive beans mostly as a compromise we have to do to enable certain functionality for integrators and I think (hope?) ordinary users won't have to deal with it at all.

Yes, integrators need to determine if a bean is active or not, but those active/inactive beans are ultimately something that ordinary users need to manipulate as well.

And so, some users will need to determine whether a bean is active or not. See #41810 (comment)

Essentially if your app defines two datasouces, and only one of them will be active at any time, you need to be able to retrieve the active one.

So yes, you can hardcode it:

public class MyProducer {
    @Inject
    @DataSource("pg")
    InjectableInstance<AgroalDataSource> pgDataSourceBean; 

    @Inject
    @DataSource("oracle")
    InjectableInstance<AgroalDataSource> oracleDataSourceBean;

    @Produces 
    @ApplicationScoped
    public AgroalDataSource dataSource() {
        if (pgDataSourceBean.getHandle().getBean().isActive().result()) { 
            return pgDataSourceBean.get();
        } else if (oracleDataSourceBean.getHandle().getBean().isActive().result()) { 
            return oracleDataSourceBean.get();
        } else {
            throw new RuntimeException("No active datasource!");
        }
    }
}

But isn't it nice to just do this?

public class MyProducer {
    @Inject
    @Active
    AgroalDataSource activeDataSource;

    @Produces 
    @ApplicationScoped
    public AgroalDataSource dataSource() {
        return activeDataSource;
    }
}

Or rather, if I understand this patch correctly:

public class MyProducer {
    @Inject
    @Active
    List<AgroalDataSource> activeDataSources;

    @Produces 
    @ApplicationScoped
    public AgroalDataSource dataSource() {
        if (activeDataSources.size() != 1) {
            throw new IllegalStateException("Number of active datasources should be 1, was " + activeDataSources.size() + " instead");
        }
        return activeDataSources.get(0);
    }
}

Understood. @yrodiere is this something you would use? If not, I can close this, or turn back to the alternative described in #43765.

If my snippet above works, then yes it would be useful.

[Ladicek]

I don't think we can do

@Inject
@Active
AgroalDataSource activeDataSource;

but the last example of your comment is exactly right.

[mkouba]

And so, some users will need to determine whether a bean is active or not. See #41810 (comment)

In my opinion, for those users an InjectableInstance#listActive() method would be enough:

default List<T> listActive() {
    return handlesStream().filter(h -> h.getBean().isActive()).map(InstanceHandle::get).toList();
}

and then:

public class MyProducer {
    @Any
    InjectableInstance<AgroalDataSource> dataSources;

    @Produces 
    @ApplicationScoped
    public AgroalDataSource dataSource() {
        List<AgroalDataSource> list = dataSources.listActive();
        if (list.size() != 1) {
            throw new IllegalStateException("Number of active datasources should be 1, was " + activeDataSources.size() + " instead");
        }
        return list.get(0);
    }
}

I mean is it really worth it? Compare the diff of this PR with the default method above...

[manovotn]

The above examples won't actually work - if you use an injection point with type DataSource and qualifier @Any and make that a requirement to create a producer method which returns DataSource (or any subclass) and arbitrary qualifier, you create a cycle between those two.

[yrodiere]

I mean is it really worth it? Compare the diff of this PR with the default method above...

I don't think it's fair to compare LOC in applications with LOC in libraries. I'm certain most applicaitons using Hibernate ORM have a smaller LOC count than Hibernate ORM itself, that doesn't mean Hibernate ORM is not worth it.

EDIT: I misunderstood, you were comparing two implementations. Well, see below :)

Anyway, ultimately it's up to you. You ask if it's useful, I tell you it is and show an example. If you think a different solution is superior and should be preferred for that particular use case, that's certainly your right.

The above examples won't actually work - if you use an injection point with type DataSource and qualifier @Any and make that a requirement to create a producer method which returns DataSource (or any subclass) and arbitrary qualifier, you create a cycle between those two.

Damn. @mkouba's example too? So only having one injection point per datasource would ever work?

[yrodiere]

The above examples won't actually work - if you use an injection point with type DataSource and qualifier @Any and make that a requirement to create a producer method which returns DataSource (or any subclass) and arbitrary qualifier, you create a cycle between those two.

Damn. @mkouba's example too? So only having one injection point per datasource would ever work?

Confirmed, neither solution works: https://github.com/yrodiere/quarkus/commits/datasource-arc-inactive-beans-active-qualifier/

I get this in both cases:

java.lang.IllegalStateException: Number of active datasources should be 1, was 2 instead
	at io.quarkus.agroal.test.MultipleDataSourcesAsAlternativesWithActiveDS1Test$MyProducer.dataSource(MultipleDataSourcesAsAlternativesWithActiveDS1Test.java:86)

[manovotn]

The above examples won't actually work - if you use an injection point with type DataSource and qualifier @Any and make that a requirement to create a producer method which returns DataSource (or any subclass) and arbitrary qualifier, you create a cycle between those two.

Damn. @mkouba's example too? So only having one injection point per datasource would ever work?

Confirmed, neither solution works: https://github.com/yrodiere/quarkus/commits/datasource-arc-inactive-beans-active-qualifier/

Yea, Ladislav's current approach with @Active List<> uses @Any underneath and Martin example does it directly.

Your example with:

@Inject
@Active
AgroalDataSource activeDataSource;

Currently won't work and would require some very special handling of the @Active annotation (assuming it is a qualifier) that would contradict CDI's definition of how qualifiers (except @Named) work.

IIUIC, the use case is that there can be multiple data sources but only one can be active - and your producers are basically just wrappers of existing beans enforcing that. I don't like the fact that users need to be aware of this and on top of that write a wrapper to have an injectable object. So I was thinking if we should maybe introduce a different abstraction such as:

@Inject
ActiveBean<DataSource> activeDataSource;

Where users could inject this directly and the ActiveBeans interface would have a simple get() method that returns a single matching bean for that type and qualifiers or throws an exception if there is none or there are multiple.
This might have different downsides, I am just thinking out loud :)

[Ladicek]

Yeah I'm afraid I missed that problem. I'm not sure what's the best way to cater to that use case. It seems to me that the ActiveBean<...> abstraction @manovotn proposes would have the same issue. I'll try to think about it.

[manovotn]

Yeah I'm afraid I missed that problem. I'm not sure what's the best way to cater to that use case. It seems to me that the ActiveBean<...> abstraction @manovotn proposes would have the same issue. I'll try to think about it.

I should have emphasized - what I propose assumes you no longer create the producer method at all.
You just inject this abstraction and operate on it directly - activeDataSource.get().doSomeWork().
I am trying to think of a way to remove the need for user to create another wrapper to begin with.

[Ladicek]

Ah, understood. Yes, that would work.

[Ladicek]

Technically, the ActiveBean<> thing would be very close to current @Active List<>. It might include more than one active bean (by implementing Iterable), and it might have a simple method to require exactly one instance (get()? single()?).

[yrodiere]

Technically, the ActiveBean<> thing would be very close to current @Active List<>. It might include more than one active bean (by implementing Iterable), and it might have a simple method to require exactly one instance (get()? single()?).

To clarify, you're considering ActiveBean<> because simply allowing @Active DataSource datasource is hard to implement, right?

I was wondering because, assuming @Active DataSource datasource (without List<>) was supported, most users wouldn't even need the @Produces wrapper.

[Ladicek]

@Active DataSource datasource is hard to implement if the bean is normal scoped, yes, and impossible if it is pseudo-scoped. There needs to be some level of indirection that figures out what bean is currently active. It could be the client proxy, but that goes directly against the current design.

[Ladicek]

I quickly put together an implementation of Active<>, which is IIUC what @manovotn proposed (except I called it Active instead of ActiveBean): https://github.com/Ladicek/quarkus/commits/arc-active-builtin-bean/ The API surface seems quite a bit smaller than this PR, which should be good.

[mkouba]

Anyway, ultimately it's up to you. You ask if it's useful, I tell you it is and show an example. If you think a different solution is superior and should be preferred for that particular use case, that's certainly your right.

@yrodiere I'm not asking if it's useful - we agreed on that while discussing #43765 ;-). I'm just saying that it's a "niche" feature. And the users that would understand this feauture would not need a new sophisticated API.

I quickly put together an implementation of Active<>, which is IIUC what @manovotn proposed (except I called it Active instead of ActiveBean): https://github.com/Ladicek/quarkus/commits/arc-active-builtin-bean/ The API surface seems quite a bit smaller than this PR, which should be good.

@Ladicek IIUC Active is just a variant of InjectableInstance. Why not just add InjectableInstance#listActive() and InjectableInstance#getActive() and call it a day?

[Ladicek]

@Ladicek IIUC Active is just a variant of InjectableInstance. Why not just add InjectableInstance#listActive() and InjectableInstance#getActive() and call it a day?

That is indeed correct, although not exactly friendly, I would say.

It seems to me that it is way too soon to know how exactly users are going to use this feature and so too soon to know how we can improve it. I'll add the 2 methods you mention for now and we can revisit later.

[mkouba]

It seems to me that it is way too soon to know how exactly users are going to use this feature and so too soon to know how we can improve it. I'll add the 2 methods you mention for now and we can revisit later.

That sounds like a plan. We can always improve the UX later based on the feedback from users.

[Ladicek]

Done.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 3a1d883.

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

Warning

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

[manovotn]

I agree this is a good course of action for now.
Thank you @Ladicek!

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 3a1d883.

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

You can consult the Develocity build scans.