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

Change how actual DB version is checked #34889

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

marko-bekhta
Copy link
Contributor

This came up while looking into #34754

In that scenario, the actual DB version is 5.5, but because of the DialectVersions.Defaults, the version is set to 8.0 😨🥲, which I guess may lead to problems in runtime...
And checkActualDbVersion() call never got further than the first if statement. It seems safe to say that buildTimeDbVersion is whatever the driver version got initialized to...

With these changes, the app would first fail with:

Persistence unit '<default>' was configured to run with a database version of at least '8.0.0', but the actual version is '5.5.0'. Consider upgrading your database. Alternatively, rebuild your application with 'quarkus.datasource.db-version=5.5.0' (but this may disable some features and/or impact performance negatively).

and then if the db-version is configured:

WARN: HHH000511: The 5.5.0 version for [org.hibernate.dialect.MySQLDialect] is no longer supported, hence certain features may not work properly. The minimum supported version is 5.7.0. Check the community dialects project for available legacy versions.

ORM will warn the user about the unsupported version.

@quarkus-bot quarkus-bot bot added area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE labels Jul 20, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 20, 2023

/cc @Sanne (hibernate-orm), @gsmet (hibernate-orm), @yrodiere (hibernate-orm)

@yrodiere
Copy link
Member

In that scenario, the actual DB version is 5.5, but because of the DialectVersions.Defaults, the version is set to 8.0, which I guess may lead to problems in runtime...

That's right, and I agree that's problematic, but that's a pre-existing behavior, and in some cases it might be working just fine...

What you're suggesting basically boils down to strictly preventing users from connecting to DBs below the default (minimum supported) version.

That makes sense, but that's also a breaking change, one that I wanted to avoid when I originally added support for db-version (which, as you noticed, is optional).

So IMO we need to:

  1. Document the change in the migration guide.
  2. Maybe offer some way to bypass this check, and test it. Perhaps setting the db-version to 5.5 explicitly already allows users to bypass the check?

WDYT, @gsmet?

@yrodiere
Copy link
Member

BTW does anyone know how to add @marko-bekhta to an allow-list so that workflow runs are automatically allowed for his PRs? We can trust him :)

@quarkus-bot

This comment has been minimized.

@marko-bekhta
Copy link
Contributor Author

Maybe offer some way to bypass this check, and test it. Perhaps setting the db-version to 5.5 explicitly already allows users to bypass the check?

With

quarkus.datasource.db-version=5.5.0

added to the application properties, the app starts and ORM only logs the warning:

WARN: HHH000511: The 5.5.0 version for [org.hibernate.dialect.MySQLDialect] is no longer supported, hence certain features may not work properly. The minimum supported version is 5.7.0. Check the community dialects project for available legacy versions.

So the user can technically proceed on their own risk 😃

@yrodiere
Copy link
Member

yrodiere commented Aug 1, 2023

Ok, thanks. Well then I suppose that's fine to break this in 3.3 (probably not in 3.2 though, let's not backport this).

I suggest that you:

  • update the documentation in this PR for this feature; see https://quarkus.io/version/main/guides/hibernate-orm#hibernate-dialect-supported-databases, especially the part starting with "When a version is set explicitly" (it's no longer only when the version is set explicitly)
  • add something to the migration guide for 3.3, preferably mentioning the DB versions affected, as well as your workaround. I suppose a full workaround to revert to the previous behavior would be setting an old db-version explicitly (to bypass the version check) AND setting a new dialect with a hardcoded version explicitly (to get the behavior of the dialect for the newer version)

@gsmet
Copy link
Member

gsmet commented Sep 22, 2023

@marko-bekhta any chance you could finalize this? It doesn't look like a lot of work.

@yrodiere Marko will be added automatically to the allow list when more than 10 PRs of his have been merged. Maybe we should have a list in the config file though to bypass this check for people who are trusted from the get go.

@marko-bekhta
Copy link
Contributor Author

@gsmet yes sir 😃 thank you for the reminder. I'll try to make this ready for the next week.

added automatically to the allow list when more than 10 PRs of his have been merged

Noted 😃, need to add more useful things to Quarkus 😃 🙈

@marko-bekhta
Copy link
Contributor Author

I've updated the note in the documentation. As for the migration notes, I'm not sure I can add those, so here's the text I've came up with for it:

=== Hibernate ORM
The Hibernate ORM Quarkus extension changed the way it verifies the database version
to provide more useful feedback from Hibernate ORM itself. In particular, to let the user know if
the version of the database being used is no longer considered as https://in.relation.to/2023/02/15/hibernate-orm-62-db-version-support/[supported by Hibernate ORM].

This change should only affect a supported database subset and only for older versions:

- MariaDB older than `10.6`
- MySQL older than `8`
- Oracle Database older than `12`
- Microsoft SQL Server older than `13 (2016)`

If the database cannot be upgraded at the time, it is still possible to use it after explicitly setting
the database version and, in some cases of very old databases that only have a community version of the dialect, by setting the dialect.
Refer to https://docs.jboss.org/hibernate/orm/6.3/userguide/html_single/Hibernate_User_Guide.html#database-dialect[database dialects] for more information.

I took the DBs and versions from the DialectVersions.Defaults.

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

LGTM, though you might want to check whether there's dead code in RecordedConfig now (is the db version still needed there?).

Now we just need to think about adding this change to the migration guide for Quarkus 3.5.

@yrodiere
Copy link
Member

yrodiere commented Sep 25, 2023

I've updated the note in the documentation. As for the migration notes, I'm not sure I can add those, so here's the text I've came up with for it:

=== Hibernate ORM
The Hibernate ORM Quarkus extension changed the way it verifies the database version
to provide more useful feedback from Hibernate ORM itself. In particular, to let the user know if
the version of the database being used is no longer considered as https://in.relation.to/2023/02/15/hibernate-orm-62-db-version-support/[supported by Hibernate ORM].

This change should only affect a supported database subset and only for older versions:

- MariaDB older than `10.6`
- MySQL older than `8`
- Oracle Database older than `12`
- Microsoft SQL Server older than `13 (2016)`

If the database cannot be upgraded at the time, it is still possible to use it after explicitly setting
the database version and, in some cases of very old databases that only have a community version of the dialect, by setting the dialect.
Refer to https://docs.jboss.org/hibernate/orm/6.3/userguide/html_single/Hibernate_User_Guide.html#database-dialect[database dialects] for more information.

I took the DBs and versions from the DialectVersions.Defaults.

If that's fine with you, I'll add this slightly modified version to the migration guide:

=== Hibernate ORM

==== Database version now gets verified on startup even if relying on defaults

The Hibernate ORM extension for Quarkus now verifies that the database version
it connects to at runtime is at least as high as the one configured at build time,
even when that configuration is not explicit
(i.e. when relying on the defaults that target Quarkus' minimum supported DB versions.

This change was made to make application developers aware
they use a version of the database that is no longer considered as https://in.relation.to/2023/02/15/hibernate-orm-62-db-version-support/[supported by Hibernate ORM] or Quarkus:
in that case, Quarkus will refuse to start with an exception.
 
This change should only affect a supported database subset and only for older versions:

- MariaDB older than `10.6`
- MySQL older than `8`
- Oracle Database older than `12`
- Microsoft SQL Server older than `13 (2016)`

If the database cannot be upgraded to a supported version, it is still possible to use it (although some feature might not work).
To continue using an older, unsupported version of a database:

* https://quarkus.io/guides/hibernate-orm#hibernate-dialect-supported-databases[Set the `db-version` explicitly]
* If necessary, https://quarkus.io/guides/hibernate-orm#hibernate-dialect-other-databases[set the dialect explicitly].
   In some cases, very old databases versions require using a legacy dialect found in the `hibernate-community-dialects` Maven artifact. See https://github.com/hibernate/hibernate-orm/blob/6.2/dialects.adoc#community-dialects[here] for more information.

@github-actions
Copy link

github-actions bot commented Sep 25, 2023

🎊 PR Preview f0d4d20 has been successfully built and deployed to https://quarkus-pr-main-34889-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gsmet gsmet force-pushed the fix/check-actual-db-version branch from 1f77223 to 01b96bc Compare June 7, 2024 17:04
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@yrodiere
Copy link
Member

Oh my... We added the notes to the migration guide but never merged the PR...
@marko-bekhta could you please have a look at the test failure when you have some time?
Let's try to get this merged in the next version, then I'll adapt the various migration guides...

@marko-bekhta
Copy link
Contributor Author

The minimum Derby version was bumped to 10.15.2 in ORM: https://hibernate.atlassian.net/browse/HHH-15203
and in Quarkus - Derby is still at 10.14.2 😕

Can we bump the version in Quarkus?

As for the Kafka test... all ok locally 😕🤷🏻‍♂️

@yrodiere
Copy link
Member

Can we bump the version in Quarkus?

+1, will do

yrodiere added a commit to yrodiere/quarkus that referenced this pull request Jun 10, 2024
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14:
https://hibernate.atlassian.net/browse/HHH-15203

Right now Quarkus and Hibernate ORM will start even if using Derby
10.14, but:

* there will be a warning on startup
* some Hibernate ORM features may not work correctly

Also, we're adding stricter checks in
quarkusio#34889,
so startup will start failing if people are still on an older version of
Derby.
@yrodiere
Copy link
Member

Can we bump the version in Quarkus?

+1, will do

#41084

yrodiere added a commit to yrodiere/quarkus that referenced this pull request Jun 10, 2024
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14:
https://hibernate.atlassian.net/browse/HHH-15203

Right now Quarkus and Hibernate ORM will start even if using Derby
10.14, but:

* there will be a warning on startup
* some Hibernate ORM features may not work correctly

Also, we're adding stricter checks in
quarkusio#34889,
so startup will start failing if people are still on an older version of
Derby.
yrodiere added a commit to yrodiere/quarkus that referenced this pull request Jun 10, 2024
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14:
https://hibernate.atlassian.net/browse/HHH-15203

Right now Quarkus and Hibernate ORM will start even if using Derby
10.14, but:

* there will be a warning on startup
* some Hibernate ORM features may not work correctly

Also, we're adding stricter checks in
quarkusio#34889,
so startup will start failing if people are still on an older version of
Derby.
yrodiere added a commit to yrodiere/quarkus that referenced this pull request Jun 10, 2024
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14:
https://hibernate.atlassian.net/browse/HHH-15203

Right now Quarkus and Hibernate ORM will start even if using Derby
10.14, but:

* there will be a warning on startup
* some Hibernate ORM features may not work correctly

Also, we're adding stricter checks in
quarkusio#34889,
so startup will start failing if people are still on an older version of
Derby.
@yrodiere yrodiere force-pushed the fix/check-actual-db-version branch from 01b96bc to 693bfdb Compare June 11, 2024 09:17
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

Rebased on top of #41084 (which was merged to main).
LGTM, let's see if CI agrees.

Reminder: we'll need to adjust migration guides once this gets in, see #34889 (comment)

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 11, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 693bfdb.

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

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

@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 11, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 11, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 693bfdb.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
JVM Tests - JDK 17 Build Failures Logs Raw logs 🔍
✔️ JVM Tests - JDK 21 Logs Raw logs 🔍

You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/jpa-mssql 

📦 integration-tests/jpa-mssql

Failed to execute goal io.fabric8:docker-maven-plugin:0.44.0:start (docker-start) on project quarkus-integration-test-jpa-mssql: I/O Error


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@gsmet gsmet merged commit 79c5ac6 into quarkusio:main Jun 11, 2024
44 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 11, 2024
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone Jun 11, 2024
@gsmet
Copy link
Member

gsmet commented Jun 11, 2024

I will let you add an item to the migration guide here: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.12 .

@yrodiere
Copy link
Member

I will let you add an item to the migration guide here: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.12 .

Done, and I also removed the section that was mistakenly added to the 3.5 migration guide.

@yrodiere
Copy link
Member

Oh my... We added the notes to the migration guide but never merged the PR... @marko-bekhta could you please have a look at the test failure when you have some time? Let's try to get this merged in the next version, then I'll adapt the various migration guides...

@michelle-purcell @rolfedh See ^. There was a section in the migration guide for Quarkus 3.5 that looked a bit like this and that essentially said database version checks were stricter now.
That was wrong, as the corresponding change hadn't been merged into Quarkus. If you incorporated this in RHBQ migration notes, you may want to remove it.

@rolfedh
Copy link
Contributor

rolfedh commented Jun 12, 2024

Thanks for this update, @yrodiere.
I've created https://issues.redhat.com/browse/QDOCS-727 to handle this task.
Please review this Jira and confirm the changes it proposes.

holly-cummins pushed a commit to holly-cummins/quarkus that referenced this pull request Jul 31, 2024
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14:
https://hibernate.atlassian.net/browse/HHH-15203

Right now Quarkus and Hibernate ORM will start even if using Derby
10.14, but:

* there will be a warning on startup
* some Hibernate ORM features may not work correctly

Also, we're adding stricter checks in
quarkusio#34889,
so startup will start failing if people are still on an older version of
Derby.
danielsoro pushed a commit to danielsoro/quarkus that referenced this pull request Sep 20, 2024
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14:
https://hibernate.atlassian.net/browse/HHH-15203

Right now Quarkus and Hibernate ORM will start even if using Derby
10.14, but:

* there will be a warning on startup
* some Hibernate ORM features may not work correctly

Also, we're adding stricter checks in
quarkusio#34889,
so startup will start failing if people are still on an older version of
Derby.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants