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

Use Arc features in datasource extensions for eager startup and active/inactive #41929

Merged
merged 10 commits into from
Oct 18, 2024

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Jul 16, 2024

Based on #41810 , which must be merged first. => Done

Best reviewed commit by commit.

Effects of this PR:

  1. Fixes Health check on io.quarkus.agroal.runtime.UnconfiguredDataSource always reports healthy #36666: see changes in expected behavior in *DatasourceHealthCheckTest.java for Agroal.
  2. Fixes Reactive datasource should not have a default for quarkus.datasource.reactive.url #43517: see changes in expected behavior in *DatasourceHealthCheckTest.java and *ConfigUrlMissing*Test.java for reactive SQL client extensions.
  3. Improves error messages when an application attempts to statically inject or dynamically access a Datasource bean that is deactivated and/or has no URL set.
  4. Introduces a few (arguably beneficial) breaking changes, see below.

Content to be added to the migration guide:

== Datasources

[[datasource-no-default-url]]
=== No default URL for Reactive SQL clients

In previous versions of Quarkus, when dev services were disabled, or when running in production mode, `quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url`  would be implicitly set to an undocumented default targeting `localhost` and a DB-specific port.

Starting with this version of Quarkus, when dev services are disabled, or when running in production mode, `quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` no longer has a default: if the property is not set, the corresponding datasource will be deactivated, leading to the behavior described in <<datasource-inactive-fail-fast>>.

If your application needs to target localhost for one of its datasources, set `quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` explicitly, for example in `application.properties`.

[[datasource-no-health-check-if-no-url]]
=== Datasources without a URL no longer contribute a health check

In previous versions of Quarkus, when dev services were disabled, or when running in production mode, and when `quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` was not set, a health check would be contributed for the corresponding datasource regardless, resulting in unreliable health checks that would either always succeed (JDBC datasource) or (almost) always fail (reactive datasources).

Starting with this version of Quarkus, datasources without a URL will no longer contribute a health check.

If your application needs a health check for a datasource, make sure to set `quarkus.datasource.jdbc.url`/`quarkus.datasource."datasource-name".jdbc.url`/`quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` explicitly.

NOTE: Quarkus will still attempt to detect misconfiguration on application startup, but will report it differently, potentially earlier: see <<datasource-inactive-fail-fast>>.

[[datasource-inactive-fail-fast]]
=== Datasource usage fails fast if datasource is deactivated or has no URL set

In previous versions of Quarkus, when dev services were disabled, or when running in production mode, and when the datasource was deactivated (`quarkus.datasource.active=false`) or had no URL set, the application would be allowed to start successfully, but could fail much later upon first access to the datasource.

Starting with this version of Quarkus, datasources that are deactivated or do not have a URL will lead to a startup failure if Quarkus can detect they are used:

* Static CDI injection points involving the datasource (`@Inject DataSource ds` or `@Inject Pool pool`) will cause application startup to fail with an explicit, actionable message.
* Dynamic retrieval of the datasource (e.g. through `CDI.getBeanContainer()`/`Arc.instance()`, or by injecting an `Instance<DataSource>`) will not be detected on startup, but will cause an exception to be thrown with an explicit, actionable message.

The <<flyway-inactive-fail-fast,Flyway>> and <<liquibase-inactive-fail-fast,Liquibase>> extensions implement a similar behavior for their `Flyway`/`LiquibaseFactory` CDI beans.

If your application needs to inject a `DataSource`/`Flyway`/`LiquibaseFactory` bean for a datasource that can potentially be deactivated or have no URL, you may encounter a startup failure similar to this:

```
io.quarkus.arc.InactiveBeanException: Bean is not active: SYNTHETIC bean [class=io.agroal.api.AgroalDataSource, id=sqqLi56D50iCdXmOjyjPSAxbLu0]
Reason: Datasource '<default>' was deactivated automatically because its URL is not set. To activate the datasource, set configuration property 'quarkus.datasource.jdbc.url'. Refer to https://quarkus.io/guides/datasource for guidance.
To avoid this exception while keeping the bean inactive:
	- Configure all extensions consuming this bean as inactive as well, if they allow it, e.g. 'quarkus.someextension.active=false'
	- Make sure that custom code only accesses this bean if it is active
	- Inject the bean with 'Instance<io.agroal.api.AgroalDataSource>' instead of 'io.agroal.api.AgroalDataSource'
This bean is injected into:
	-io.quarkus.agroal.test.ConfigUrlMissingDefaultDatasourceStaticInjectionTest$MyBean#ds
	at io.agroal.api.AgroalDataSource_sqqLi56D50iCdXmOjyjPSAxbLu0_Synthetic_Bean.doCreate(Unknown Source)
	[...]
```

To avoid this failure, follow the instructions in the exception message.
In particular, consider injecting the problematic bean as an `InjectableInstance` (for example `@Inject InjectableInstance<DataSource> ds`) instead.
You can check whether that bean is active using `.getHandle().getBean().isActive().result()` on the `InjectableInstance`, and retrieve the bean instance using `InjectableInstance#get()`.
See https://quarkus.io/guides/datasource#datasource-active for an example of such a setup for `DataSource` beans -- the idea would be similar for a `Flyway`/`LiquibaseFactory` beans.

== Flyway

[[flyway-inactive-inactive-fail-fast]]
=== Flyway usage fails fast if datasource is deactivated or has no URL set

Starting with this version of Quarkus, datasources that are deactivated or have no URL set will lead to a startup failure if Quarkus can detect a `Flyway` bean is used for that datasource.

See the <<datasource-inactive-fail-fast,datasources,similar migration guide entry for datasources>> for more information.

== Liquibase

[[liquibase-inactive-if-no-url]]
=== Liquibase usage fails fast if datasource is deactivated or has no URL set

Starting with this version of Quarkus, datasources that are deactivated or have no URL set will lead to a startup failure if Quarkus can detect a `LiquibaseFactory` bean is used for that datasource.

See the <<datasource-inactive-fail-fast,datasources,similar migration guide entry for datasources>> for more information.

Copy link

quarkus-bot bot commented Jul 16, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added area/agroal area/arc Issue related to ARC (dependency injection) area/flyway area/hibernate-orm Hibernate ORM area/liquibase area/persistence OBSOLETE, DO NOT USE labels Jul 16, 2024
Copy link

quarkus-bot bot commented Jul 16, 2024

/cc @gsmet (hibernate-orm)

@yrodiere yrodiere marked this pull request as draft July 16, 2024 14:57
@yrodiere
Copy link
Member Author

Actually I'll keep this as a draft... the base commit makes changes in Arc, which probably means the rebuild will be massive, even if I only need to test things related to datasources :/

This comment has been minimized.

@yrodiere yrodiere force-pushed the datasource-arc-inactive-beans branch 2 times, most recently from 8fe1856 to 5ffad3c Compare July 19, 2024 16:13
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/hibernate-reactive Hibernate Reactive area/reactive-sql-clients labels Jul 19, 2024
@yrodiere yrodiere force-pushed the datasource-arc-inactive-beans branch 4 times, most recently from 705e007 to 645c876 Compare July 26, 2024 14:14
@yrodiere yrodiere force-pushed the datasource-arc-inactive-beans branch 2 times, most recently from 41fe36b to efac1c7 Compare July 30, 2024 16:22
@yrodiere
Copy link
Member Author

yrodiere commented Aug 1, 2024

Hey @Ladicek, FYI I made some progress here regarding integration with your work in #41810, but didn't find the time to finish before my PTO. Sorry... I'll resume my work when I'm back at the end of August.

I'd personally not bother merging #41810 until then, but feel free if you're in a rush and are okay with what you're seeing here so far. I guess we can always iterate later on.

See you!

@Ladicek
Copy link
Contributor

Ladicek commented Aug 2, 2024

@yrodiere Thanks, and no worries -- I have intentionally made #41810 a draft and there's no need to rush it in. I'll wait for more of your feedback, and maybe change the API a little bit before making it ready, we'll see. Enjoy your PTO!

So that we correctly detect empty URLs.
Because it duplicates:

* ConfigUrlMissingDefaultDatasourceStaticInjectionTest
* ConfigUrlMissingDefaultDatasourceDynamicInjectionTest
* ConfigUrlMissingNamedDatasourceStaticInjectionTest
* ConfigUrlMissingNamedDatasourceDynamicInjectionTest
Consequently:

* Skip the health check for datasources without a URL
* Fail on startup if datasources without URL are (statically)
  injected into user bean
* Fail on first access if datasources without URL are retrieved
  dynamically through CDI
…jected into user beans

By reimplementing Flyway's/Liquibase's inactive/active handling and eager
startup through Arc's native features, which is better integrated and gives
us this behavior.
For consistency with datasource documentation.
@yrodiere yrodiere force-pushed the datasource-arc-inactive-beans branch from 00a940d to e5e300d Compare October 17, 2024 12:02
@yrodiere
Copy link
Member Author

yrodiere commented Oct 17, 2024

I rebased on main and added documentation for the getActive() solution implemented by @Ladicek in #43803 .

Note the Hibernate ORM documentation is a bit awkward at the moment, but it's just because we haven't fixed #43594 yet.

Will merge in the next few days unless someone objects.

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Oct 17, 2024

This comment has been minimized.

This comment has been minimized.

@yrodiere yrodiere force-pushed the datasource-arc-inactive-beans branch from e5e300d to 093b837 Compare October 17, 2024 15:39
Copy link

quarkus-bot bot commented Oct 17, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 093b837.

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

Copy link

quarkus-bot bot commented Oct 17, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 093b837.

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


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/mongodb-panache

io.quarkus.it.mongodb.panache.ReflectionFreeSerializationTest.testReactiveBookEntity - History

  • com.fasterxml.jackson.databind.exc.MismatchedInputException: No content to map due to end-of-input at [Source: REDACTED (\StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1]-java.lang.RuntimeException`
java.lang.RuntimeException: 
com.fasterxml.jackson.databind.exc.MismatchedInputException: No content to map due to end-of-input
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1]
	at io.restassured.internal.path.json.mapping.JsonPathJackson2ObjectDeserializer.deserialize(JsonPathJackson2ObjectDeserializer.java:30)
	at io.restassured.path.json.mapping.JsonPathObjectDeserializer$deserialize.call(Unknown Source)
	at io.restassured.internal.mapping.Jackson2Mapper.deserialize(Jackson2Mapper.groovy:58)
	at io.restassured.mapper.ObjectMapper$deserialize.call(Unknown Source)
	at io.restassured.internal.mapping.ObjectMapping.parseWithJackson2(ObjectMapping.groovy:254)

@yrodiere
Copy link
Member Author

yrodiere commented Oct 18, 2024

@geoand or @gastaldi I will need one of you to approve; I'm afraid Marko's and Thomas' approvals are non-binding :)

EDIT: Please!

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Rubber-stamped!

@yrodiere yrodiere merged commit d368f3b into quarkusio:main Oct 18, 2024
49 checks passed
@quarkus-bot quarkus-bot bot added kind/bugfix kind/enhancement New feature or request labels Oct 18, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 18, 2024
@yrodiere
Copy link
Member Author

Merged! Thanks a lot to everyone involved :)

@yrodiere
Copy link
Member Author

Also, I updated the 3.17 migration guide: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.17#datasources

@geoand
Copy link
Contributor

geoand commented Oct 18, 2024

👍🏽

private final RuntimeValue<DataSourcesRuntimeConfig> runtimeConfig;
private final RuntimeValue<DataSourcesReactiveRuntimeConfig> reactiveRuntimeConfig;

@Inject
Copy link
Member

Choose a reason for hiding this comment

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

interesting, does jakarta.inject.Inject; has any effect on records at all? I didn't know.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't really. If the annotation wasn't present, it would work exactly the same.

If anything, this is more of a documentation that injection happens. But it's way out of any specification, so perhaps it shouldn't be here after all 🤷

Copy link
Member

Choose a reason for hiding this comment

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

copy that, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
7 participants