Configuration property to work with database schemas generated for Hibernate ORM 5.6 #31540

yrodiere · 2023-03-02T10:05:16Z

This is far from perfect but should addresses the biggest pain points.

I'll stress once again (hopefully it's clear in the docs): this is a best-effort feature. The goal is not to provide perfect compatibility for everything, but just to ease migration and early testing by allowing application developers to delay work on some of the most obvious incompatibilities when it comes to the database schema.

Note this only addresses what we could find in the migration guides; anything missing from migration guides hasn't been addressed.

The main problem remaining is enums mapped as ordinals (which is the default): their schema definition is different in Hibernate ORM 6 for pretty much every single dialect, resulting in schema validation. This will likely affect a large proportion of existing applications. Unfortunately we don't have a setting to revert to ORM 5 ("legacy") behavior for enums. Maybe we should add one in ORM 6.2?

The property can be used like this:

quarkus.hibernate-orm.database.orm-compatibility.version=5.6

If you disagree with the name, please provide constructive suggestions.

Regarding documentation, I'd personally stick to documenting the configuration property + referencing the property in the migration guide. I would not add a dedicated section in the Hibernate ORM Extension guide, because we want to remove this property eventually.

quarkus-bot · 2023-03-02T10:05:54Z

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

Sanne · 2023-03-02T12:37:12Z

Wow you did a lot of work already and looks very promising!

Let me start with high level questions, as I have to look better soon:

Do you have a phase-out plan for this property? I'm wondering how we'll handle when some of the subset flag you plan to apply will cease to work.

Also - should we nag users about its use? for example logging a warning?

Both aspects could be addressed by logging the individual flags you're setting as a warning;

WARN: Hibernate ORM is configured with `hibernate.id.db_structure_naming_strategy=legacy` because of `quarkus.hibernate-orm.database.orm-compatibility.version=5.6` - this affects schema compatibility, and the flag `hibernate.id.db_structure_naming_strategy` is not flagged as stable API.
WARN: Hibernate ORM is configured with `hibernate.type.preferred_instant_jdbc_type=TIMESTAMP` because of `quarkus.hibernate-orm.database.orm-compatibility.version=5.6` - this affects schema compatibility, and the flag `hibernate.type.preferred_instant_jdbc_type` is not flagged as stable API.
[...]

That would encourage people to get rid of them soonish, and also give them better awareness of what's going on behind the scenes; could be important if some of these incubating/deprecated propertie has to phase out or be adapted in some way.

yrodiere · 2023-03-02T12:46:57Z

Wow you did a lot of work already and looks very promising!

It helped that @gsmet went through the ORM migration guides and listed the most problematic breaking changes :)

Do you have a phase-out plan for this property?

In my mind we'd deprecate it soon (3.1? 3.2? Whatever makes sense for product needs.)

I'm wondering how we'll handle when some of the subset flag you plan to apply will cease to work.

Surely that won't happen before the next major of Hibernate ORM. I trust we don't want to introduce breaking changes in minors :)

Also - should we nag users about its use? for example logging a warning?

Both aspects could be addressed by logging the individual flags you're setting as a warning;
WARN: Hibernate ORM is configured with `hibernate.id.db_structure_naming_strategy=legacy` because of `quarkus.hibernate-orm.database.orm-compatibility.version=5.6` - this affects schema compatibility, and the flag `hibernate.id.db_structure_naming_strategy` is not flagged as stable API.
WARN: Hibernate ORM is configured with `hibernate.type.preferred_instant_jdbc_type=TIMESTAMP` because of `quarkus.hibernate-orm.database.orm-compatibility.version=5.6` - this affects schema compatibility, and the flag `hibernate.type.preferred_instant_jdbc_type` is not flagged as stable API.
[...]
That would encourage people to get rid of them soonish, and also give them better awareness of what's going on behind the scenes; could be important if some of these incubating/deprecated propertie has to phase out or be adapted in some way.

That's a very good idea. I'll add something along these lines.

Sanne · 2023-03-02T12:50:48Z

Surely that won't happen before the next major of Hibernate ORM. I trust we don't want to introduce breaking changes in minors :)

I suspect you're right, but bear in mind some of these are flagged as "@Incubating" .. zero promises.

yrodiere · 2023-03-02T14:31:10Z

I fixed the test failure and pushed a new commit to add warnings as you suggested, @Sanne .

…y.version For reference, below are the failures when not setting the property (and ignoring schema validation). MariaDB: [ERROR] Failures: [ERROR] CompatibilityTest.instant:81 expected: 2018-01-01T10:58:30Z but was: 2018-01-01T11:58:30Z [ERROR] CompatibilityTest.offsetDateTime:91 expected: 2018-01-01T11:58:30+01:00 (java.time.OffsetDateTime) but was: 2018-01-01T11:58:30Z (java.time.OffsetDateTime) when comparing values using 'OffsetDateTime.timeLineOrder()' [ERROR] CompatibilityTest.persistUsingOldSchema:61 1 expectation failed. Expected status code is <200> but was <500>. [ERROR] CompatibilityTest.sequence:38 1 expectation failed. Expected status code is <200> but was <500>. [ERROR] CompatibilityTest.zonedDateTime:102 expected: 2018-01-01T11:58:30+01:00[Europe/Paris] (java.time.ZonedDateTime) but was: 2018-01-01T11:58:30Z[UTC] (java.time.ZonedDateTime) when comparing values using 'ChronoZonedDateTime.timeLineOrder()' PostgreSQL: Everything failing due to array support not liking binary columns, so I had to comment out the array/arrayList properties. See https://hibernate.zulipchat.com/#narrow/stream/132094-hibernate-orm-dev/topic/Array.20support Then we get that: [ERROR] Failures: [ERROR] CompatibilityTest.instant:79 expected: 2018-01-01T10:58:30Z but was: 2018-01-01T11:58:30Z [ERROR] CompatibilityTest.offsetDateTime:89 expected: 2018-01-01T11:58:30+01:00 (java.time.OffsetDateTime) but was: 2018-01-01T11:58:30Z (java.time.OffsetDateTime) when comparing values using 'OffsetDateTime.timeLineOrder()' [ERROR] CompatibilityTest.persistUsingOldSchema:59 1 expectation failed. Expected status code is <200> but was <500>. [ERROR] CompatibilityTest.sequence:36 1 expectation failed. Expected status code is <200> but was <500>. [ERROR] CompatibilityTest.zonedDateTime:100 expected: 2018-01-01T11:58:30+01:00[Europe/Paris] (java.time.ZonedDateTime) but was: 2018-01-01T11:58:30Z[UTC] (java.time.ZonedDateTime) when comparing values using 'ChronoZonedDateTime.timeLineOrder()'

Without the compatibility switch, we get the following failures. MariaDB: [ERROR] CompatibilityTest.sequence_defaultGenerator:38 1 expectation failed. Expected status code is <200> but was <500>. [ERROR] CompatibilityTest.sequence_genericGenerator_defaultAllocation:53 expected: 3L but was: -46L PostgreSQL: [ERROR] CompatibilityTest.sequence_defaultGenerator:38 1 expectation failed. Expected status code is <200> but was <500>. [ERROR] CompatibilityTest.sequence_genericGenerator_defaultAllocation:53 expected: 3L but was: -46L

Since ZoneOffsetDateTime serialization/deserialization in Hibernate ORM 5 gives different results based on the default JVM timezone, it is very important that we use the same TZ when generating data (which happened on my local machine) and running tests (which happens on CI). Here I'm forcing the default TZ when generating data too, so that other people can re-generate data even if their timezone is different.

yrodiere · 2023-03-03T12:41:50Z

I made the tests resilient to different default timezones (see last commit for an explanation), so that should fix CI.

@Sanne you can see the problems related to default sequence increment by commenting out the line that forces 5.6 compatibility in integration-tests/hibernate-orm-compatibility-5.6/postgresql/src/main/resources/application.properties and then running this:

./mvnw  -f integration-tests/hibernate-orm-compatibility-5.6/postgresql clean install -Dstart-containers -Dtest-containers

This will give you the error mentioned in the 4th commit's message:

[ERROR]   CompatibilityTest.sequence_genericGenerator_defaultAllocation:53
expected: 3L
 but was: -46L

yrodiere · 2023-03-03T15:03:35Z

@Sanne @gsmet IMO this is ready to merge, but if we want to merge this before Alpha5 I'll need an approval before wednesday (and preferably sooner if there are requests for changes).

So... WDYT? :)

yrodiere · 2023-03-03T15:05:31Z

Note I also started a migration guide from ORM 5 to 6 in Quarkus that I think addresses the main pain points (at least it addresses those @gsmet noticed). I suppose we can always dig deeper but I think it's a good start.

Sanne · 2023-03-03T15:05:51Z

I'll look now, sorry for the delay.

Sanne · 2023-03-03T15:11:37Z

...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java

@@ -382,17 +382,21 @@ public void configurationDescriptorBuilding(
        // First produce the PUs having a persistence.xml: these are not reactive, as we don't allow using a persistence.xml for them.
        for (PersistenceXmlDescriptorBuildItem persistenceXmlDescriptorBuildItem : persistenceXmlDescriptors) {
            ParsedPersistenceXmlDescriptor xmlDescriptor = persistenceXmlDescriptorBuildItem.getDescriptor();
+            Optional<JdbcDataSourceBuildItem> jdbcDataSource = jdbcDataSources.stream()
+                    .filter(i -> i.isDefault())
+                    .findFirst();


This confuses me a bit - why the first one?

No particular reason, findAny (if that exists) would work. I copy pasted that code from the method that handles non-persistence.xml config.

But don't we attempt to match the right datasource configuration for this PU ?

The code for persistence.xml handling has always assumed we were using the default datasource for these PUs:

https://github.com/yrodiere/quarkus/blob/8f171cdbcfa4c810f5ec10377240a5f3278607c0/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java#L396-L399

This new code is in line with that assumption.

If you think it's wrong and we should extract the datasource name from the persistence.xml (I guess it can be configured there?), I can open another ticket?

ok, existing code - no rush then. But yes it surprised me a bit - I guess I forgot we ignore the datasource section from a persistence.xml.
I guess ignoring it is fine, but to make things better we could try to spot if the user defined a datasource section and warn about it being ignored / or even fail.

Sanne

Great work

Sanne · 2023-03-03T15:37:58Z

ah I see now you added the test in native-image form as well; do you see value in it?

Otherwise I'd avoid it, saving some energy

Sanne · 2023-03-03T16:28:19Z

@Sanne you can see the problems related to default sequence increment by commenting out the line that forces 5.6 compatibility in integration-tests/hibernate-orm-compatibility-5.6/postgresql/src/main/resources/application.properties and then running this: [..]

Thanks yes that was useful. But did you notice that if you validate the schema Hibernate points out the problem:

Caused by: org.hibernate.tool.schema.spi.SchemaManagementException: Schema-validation: sequence [gengendefallocsize] defined inconsistent increment-size; found [1] but expecting [50]

That's really nice, and will help mitigate some confusion as well.

quarkus-bot · 2023-03-03T18:08:35Z

Failing Jobs - Building `47f418b`

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
⌛	JVM Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 17
✔️	JVM Tests - JDK 19
✔️	Maven Tests - JDK 11
⌛	Maven Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs

yrodiere · 2023-03-03T18:09:02Z

Otherwise I'd avoid it, saving some energy

Ok, will do.

But did you notice that if you validate the schema Hibernate points out the problem:

Right, that's nice. We should probably hunt down examples that use generation = none though, I think there's a few. And obviously that doesn't solve the problem of current values (which must be incremented to cope with the pooled optimizer's behavior). But at least there's a heads-up.

Sanne · 2023-03-03T18:27:19Z

BTW don't consider my comments at all blocking, feel free to merge and address the minor points another time. Leaving that up to you.

quarkus-bot bot added area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive area/persistence OBSOLETE, DO NOT USE labels Mar 2, 2023

This comment has been minimized.

Sign in to view

yrodiere force-pushed the i31475-schema-compat branch from 421f1c4 to 53de4ba Compare March 2, 2023 10:20

yrodiere requested a review from Sanne March 2, 2023 12:19

This comment has been minimized.

Sign in to view

yrodiere force-pushed the i31475-schema-compat branch from 53de4ba to a1051b1 Compare March 2, 2023 14:23

This comment has been minimized.

Sign in to view

yrodiere added 5 commits March 3, 2023 13:37

Add quarkus.hibernate-orm.database.orm-compatibility.version

af1abc7

Warn on startup when setting a database/orm compatibility version

8dc4c1d

yrodiere force-pushed the i31475-schema-compat branch from a1051b1 to 47f418b Compare March 3, 2023 12:37

quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Mar 3, 2023

Sanne reviewed Mar 3, 2023

View reviewed changes

Sanne approved these changes Mar 3, 2023

View reviewed changes

yrodiere merged commit 9285df2 into quarkusio:main Mar 6, 2023

quarkus-bot bot added the kind/enhancement New feature or request label Mar 6, 2023

quarkus-bot bot added this to the 3.0 - main milestone Mar 6, 2023

rsvoboda added the release/noteworthy-feature label Mar 6, 2023

yrodiere mentioned this pull request Mar 6, 2023

Polish tests and documentation for quarkus.hibernate-orm.database.orm-compatibility.version #31620

Merged

yrodiere deleted the i31475-schema-compat branch August 7, 2023 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration property to work with database schemas generated for Hibernate ORM 5.6 #31540

Configuration property to work with database schemas generated for Hibernate ORM 5.6 #31540

yrodiere commented Mar 2, 2023

quarkus-bot bot commented Mar 2, 2023

This comment has been minimized.

Sanne commented Mar 2, 2023

yrodiere commented Mar 2, 2023

Sanne commented Mar 2, 2023

This comment has been minimized.

yrodiere commented Mar 2, 2023 •

edited

Loading

This comment has been minimized.

yrodiere commented Mar 3, 2023 •

edited

Loading

yrodiere commented Mar 3, 2023

yrodiere commented Mar 3, 2023

Sanne commented Mar 3, 2023

Sanne Mar 3, 2023

yrodiere Mar 3, 2023

Sanne Mar 3, 2023

yrodiere Mar 6, 2023

Sanne Mar 6, 2023

Sanne left a comment

Sanne commented Mar 3, 2023

Sanne commented Mar 3, 2023

quarkus-bot bot commented Mar 3, 2023

yrodiere commented Mar 3, 2023

Sanne commented Mar 3, 2023

Configuration property to work with database schemas generated for Hibernate ORM 5.6 #31540

Configuration property to work with database schemas generated for Hibernate ORM 5.6 #31540

Conversation

yrodiere commented Mar 2, 2023

quarkus-bot bot commented Mar 2, 2023

This comment has been minimized.

Sanne commented Mar 2, 2023

yrodiere commented Mar 2, 2023

Sanne commented Mar 2, 2023

This comment has been minimized.

yrodiere commented Mar 2, 2023 • edited Loading

This comment has been minimized.

yrodiere commented Mar 3, 2023 • edited Loading

yrodiere commented Mar 3, 2023

yrodiere commented Mar 3, 2023

Sanne commented Mar 3, 2023

Sanne Mar 3, 2023

Choose a reason for hiding this comment

yrodiere Mar 3, 2023

Choose a reason for hiding this comment

Sanne Mar 3, 2023

Choose a reason for hiding this comment

yrodiere Mar 6, 2023

Choose a reason for hiding this comment

Sanne Mar 6, 2023

Choose a reason for hiding this comment

Sanne left a comment

Choose a reason for hiding this comment

Sanne commented Mar 3, 2023

Sanne commented Mar 3, 2023

quarkus-bot bot commented Mar 3, 2023

Failing Jobs - Building 47f418b

yrodiere commented Mar 3, 2023

Sanne commented Mar 3, 2023

yrodiere commented Mar 2, 2023 •

edited

Loading

yrodiere commented Mar 3, 2023 •

edited

Loading

Failing Jobs - Building `47f418b`