Health check on io.quarkus.agroal.runtime.UnconfiguredDataSource always reports healthy

jtama commented 8 months ago

Describe the bug

An application using only agroal extension can start even if no datasource has been configured.

This is because https://github.com/quarkusio/quarkus/blob/main/extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSources.java#L147C9-L153C1

An the same application healthCheck wil lreport healthy as UnconfiguredDatasource doesn't overwrite AgroalDatasource.isHealthy which defaults to true.

That means the error is only discovered once someone tries to use the datasource, which is far too late.

Expected behavior

I would expect the application not to start or at least report as not ready.

Actual behavior

The application runs without any errors.

How to Reproduce?

Simply create an application using only agroal datasource (no orm), start (not in dev mode) the application without configuring anything

Output of `uname -a` or `ver`

No response

Output of `java -version`

No response

Quarkus version or git rev

main

Build tool (ie. output of `mvnw --version` or `gradlew --version`)

No response

Additional information

I can provide a MR but I am quite sure what the rigth way would be as even if the behaviour is expected on some cases (as per Datasources comment) it's not always the case.

quarkus-bot[bot] commented 8 months ago

/cc @Sanne (agroal), @barreiro (agroal), @jmartisk (health), @xstefank (health), @yrodiere (agroal)

yrodiere commented 8 months ago

Thanks for reporting.

I'm not sure how we could solve this since an UnconfiguredDataSource may exist without being used, in which case it's perfectly fine and should be considered healthy.

IMO this whole UnconfiguredDataSource stuff should just be removed and we just shouldn't create the bean if there's no configuration. But I may be missing something...

jtama commented 8 months ago

I'm all for removing this UnconfiguredDataSource, which would prevent the application from starting. I'm just not sure about the impacts on other extensions.

I can provide a PR if you'd like.

yrodiere commented 8 months ago

I can provide a PR if you'd like.

That would be appreciated, but I can't guarantee it will get merged since the git history suggests this was needed for Dev Services.

Maybe the best way to find out would be for you to send a PR removing UnconfiguredDataSource, and see which tests fail? From there we might be able to find an alternative solution (e.g. only return UnconfiguredDataSource in dev mode, fail immediately otherwise).

gsmet commented 8 months ago

Maybe the best way to find out would be for you to send a PR removing UnconfiguredDataSource, and see which tests fail?

Ideally, a draft PR to avoid overloading our CI and have it run in a fork. I will make the PR a draft one.

yrodiere commented 6 months ago

See https://groups.google.com/g/quarkus-dev/c/enMgpOrb61o/m/cRKwiWmGAgAJ for the discussion to change the current behavior of allowing a datasource to be "unconfigured".

yrodiere commented 3 weeks ago

See https://groups.google.com/g/quarkus-dev/c/enMgpOrb61o/m/cRKwiWmGAgAJ for the discussion to change the current behavior of allowing a datasource to be "unconfigured".

Outcome of that conversation, for future reference:

We will add checks that datasources are injected (statically) in user beans through @Inject, or used by other extensions (e.g. Hibernate ORM/Reactive).
If a datasource is active and unconfigured (~= missing URL):
- if we know it's statically injected somewhere, we'd fail on startup
- if we don't know of any static injection, we'd deactivate the datasource automatically. That would disable healthchecks for that datasource, and would throw an exception on any attempt to retrieve the datasource

Here are hints regarding how to detect static injection:

BeanDiscoveryFinishedBuildItem#getInjectionPoints() if you're interested in "class-based" injection points or SynthesisFinishedBuildItem#getInjectionPoints() if you also need synthetic injection points as well.

And for both build items there's also the

getBeanResolver().resolveBeans() method that can help with type-safe

resolution rules.

yrodiere commented 1 week ago

Opened #41466 to try to generalize the solution.

quarkusio / quarkus