HHH-14744 : Refactor contextual information for SchemaManagementTool to be more easily extended by Hibernate Reactive #4106

gbadner · 2021-07-23T23:56:05Z

https://hibernate.atlassian.net/browse/HHH-14744

hibernate-github-bot · 2021-07-23T23:56:08Z

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

hibernate-core/src/main/java/org/hibernate/tool/schema/spi/SchemaManagementTool.java

gbadner · 2021-07-30T03:51:45Z

I rebased main and force-pushed.

…to be more easily extended by Hibernate Reactive HHH-14744 : Restore databases/pgsql/resources/hibernate.properties and gradle/databases.gradle

…to be more easily extended by Hibernate Reactive Move new methods out of SchemaManagementTool and into ExtractionTool

…to be more easily extended by Hibernate Reactive Add Javadoc and other minor changes to make it easier to review

gbadner · 2021-08-03T04:54:54Z

I added some Javadoc and some other minor changes to be more consistent with DatabaseMetaData methods.

I also force-pushed.

gbadner · 2021-08-03T04:56:16Z

@Sanne, I've added you as a reviewer in case @dreab8 is not available.

Sanne · 2021-08-03T11:01:03Z

Thanks @gbadner , this is larger than I expected and is changing some internal services which we use in Quarkus. I'll need to verify the feasibility of upgrading the Quarkus integration code as well.

gbadner · 2021-08-03T15:58:20Z

Thanks @gbadner , this is larger than I expected and is changing some internal services which we use in Quarkus. I'll need to verify the feasibility of upgrading the Quarkus integration code as well.

@Sanne, I can create default methods. I didn't do that because interfaces were marked @Incubating. I wasn't thinking of Quarkus though. I'll check out Quarkus code so I can keep it on my radar as a consumer in the future. I'm happy to look into how Quarkus uses the SchemaManagementTool currently, if it would save you some time. Please let me know if I should go ahead and do that.

Sanne · 2021-08-03T16:02:53Z

@gbadner don't worry for the SPI, it seems Quarkus is compiling just fine. But we're having a memory leak which has been preventing me to run all integration tests - sorry this will take a bit longer.

gbadner · 2021-08-03T16:06:05Z

@Sanne, that's fine. I made a couple of changes here that will affect the hibernate-reactive PR, so I'll take care of that now.

…to be more easily extended by Hibernate Reactive Correct specifications for row order of some ResultSets.

gbadner · 2021-08-04T00:01:07Z

I just pushed a minor change in Javadoc..

Sanne · 2021-08-04T10:53:34Z

It's looking great! I'm inclined to merge it already, but could you confirm why it seems you also wanted to change the PostgreSQL Dialect semantics, and also changed SchemaUpdateTest ?

gbadner · 2021-08-05T06:03:50Z

It's looking great! I'm inclined to merge it already, but could you confirm why it seems you also wanted to change the PostgreSQL Dialect semantics, and also changed SchemaUpdateTest ?

@Sanne, actually, it looks like the PostgreSQL10Dialect#buildIdentifierHelper can be removed now that HHH-13788 is fixed. I'll check that.

PostgreSQL10Dialect#getNameQualifierSupport was added because the DatabaseMetaData is not available, and ORM's default (NameQualifierSupport.BOTH) is incorrect.

PostgreSQL10Dialect#getSequenceInformationExtractor was added because the default (SequenceInformationExtractorLegacyImpl.INSTANCE) doesn't work for Hibernate Reactive. PostgreSQL stores the sequence metadata as strings. PostgreSQL's JDBC driver does the
conversion from String to Long automatically, but, unfortunately Vert.x driver does not do this conversion.

PostgreSQL10Dialect#getCurrentSchemaCommand was added because Connection#getSchema is not available in Hibernate Reactive.

It is probably possible to move these new methods into an earlier PostgreSQL dialect. The oldest version I know I have access to is PostgreSQL 8.4, so I could see about moving them into into PostgreSQL9Dialect.

gbadner · 2021-08-05T06:09:55Z

I added @GeneratedValue to the IDs in SchemaUpdateTest just so that I could step through JDBC code that processes the sequences in the debugger so I could better understand why SequenceInformationExtractorLegacyImpl wasn't working with Hibernate Reactive.

It will be helpful to have at least one sequence ID in this test when we start adding SchemaMigrator/SchemaValidator support for more DBs.

I could change the test so that there are both generated and non-generated IDs.

…agementTool to be more easily extended by Hibernate Reactive Remove PostgreSQL10Dialect#buildIdentifierHelper since it is no longer needed.

gbadner · 2021-08-05T06:44:32Z

I just pushed a commit that removes PostgreSQL10Dialect#buildIdentifierHelper, since it's no longer needed.

gbadner · 2021-08-10T00:18:26Z

...org/hibernate/tool/schema/extract/internal/InformationExtractorJdbcDatabaseMetaDataImpl.java

-					schemaFilter,
-					tableName.getTableName().getText(),
-					false,        // DO NOT limit to just unique
-					true        // DO require up-to-date results


I just noticed that true for the approximate argument to `DatabaseMetaData#getIndexInfo says:

approximate - when true, result is allowed to reflect approximate or out of data values; when false, results are requested to be accurate

ORM was passing true here, meaning that approximate data is OK, but the comment is inconsistent saying that up-to-date results are required.

Which is correct?

FWIW, when DatabaseMetaData is not used to get this information, there does not seem to be a way to request up-to-date information.

For example, MySQL documentation says that information_schema_stats_expiry=0 should be set in order to get up-to-date statistics. This is probably not what is wanted on a production database.

@dreab8, any idea what the intention is here? Is this just a documentation bug?

This should not delay merging this issue. I just wanted to raise it since I saw the inconsistency.

not sure but I would say we want to use up-to-date info but better to ask @sebersole for a confirmation

FYI, I don't see a way for Hibernate to force this for MySQL without setting a system variable.

See https://dev.mysql.com/doc/mysql-infoschema-excerpt/8.0/en/information-schema-statistics-table.html.

…agementTool to be more easily extended by Hibernate Reactive Move methods from PostgreSQL10Dialect into PostgreSQL81Dialect; remove @GeneratedValue from SchemaUpdateTest

gbadner · 2021-08-13T02:03:10Z

@Sanne, I've added a commit that moves a couple of methods from PostgreSQL10Dialect into PostgreSQL81Dialect. I verified those methods would work in that dialect by looking at the JDBC source code for PostgreSQL 8.1: https://github.com/pgjdbc/pgjdbc/tree/REL8_1_STABLE.

I also restored SchemaUpdateTest by removing @GeneratedValue annotations.

I will ask about updating io.vertx.sqlclient.Tuple#getLong to do the conversion if the value is a String (rather than throwing ClassCastException. That way, PostgreSQL10Dialect#getSequenceInformationExtractor and SequenceInformationExtractorPostgresSQLDatabaseImpl could be removed.

Is there anything else that needs to be done before merging this?

…to be more easily extended by Hibernate Reactive Remove SequenceInformationExtractorPostgresSQLDatabaseImpl and PostgreSQL10Dialect#getSequenceInformationExtractor

gbadner · 2021-08-13T02:44:08Z

I was able to fix this in ResultSetAdapter, so I've removed SequenceInformationExtractorPostgresSQLDatabaseImpl and PostgreSQL10Dialect#getSequenceInformationExtractor in a new commit.

gavinking · 2021-08-13T08:51:54Z

moves a couple of methods from PostgreSQL10Dialect into PostgreSQL81Dialect.

We should reproduce those changes in H6 I guess.

Actually, eventually we're going to have to replicate all this work across.

gbadner · 2021-08-16T16:52:26Z

@Sanne, can this be merged?

…to be more easily extended by Hibernate Reactive Add MySQLDialect#getNameQualifierSupport

gbadner · 2021-08-16T17:52:40Z

I've added a commit that is required for MySQL. I checked that the setting returned by MySQLDialect#getNameQualifierSupport is correct in MySQL 5.0. MySQLDialect is deprecated, and the oldest subclass is MySQL5Dialect, so this should be fine.

FYI, #buildIdentifierHelper was also added to MySQLDialect.

…to be more easily extended by Hibernate Reactive 1. Add CockroachDB192Dialect#getNameQualifierSupport and #buildIdentifierHelper. 2. Change semantics for AbstractInformationExtractorImpl#processIndexInfoResultSet to be more friendly to subclasses.

gbadner · 2021-08-17T01:28:16Z

I just added a commit that:

Adds CockroachDB192Dialect#getNameQualifierSupport and #buildIdentifierHelper.
Changes semantics for AbstractInformationExtractorImpl#processIndexInfoResultSet to be more friendly to subclasses.

Sanne

Thanks @gbadner !

Sanne · 2021-08-25T13:25:49Z

I'll merge this soon but would prefer to branch first.

Sanne · 2021-08-25T14:31:38Z

Merging this in ORM 5.6 (current main)

yodous · 2021-12-14T13:24:31Z

hibernate-core/src/main/java/org/hibernate/dialect/MySQLDialect.java

@@ -566,6 +567,11 @@ public JDBCException convert(SQLException sqlException, String message, String s
 		};
 	}

+	@Override
+	public NameQualifierSupport getNameQualifierSupport() {
+		return NameQualifierSupport.CATALOG;


Copying comment from initial commit:

Shouldn't it be NameQualifierSupport.SCHEMA and not CATALOG (since MySQL use the concept of schema and not the catalogs).
Whenever there would be an Entity class annotated with @Table(schema = "tech", name = "point"), the class QualifiedObjectNameFormatterStandardImpl would format the name into point and not the tech.point (since it does not define catalog)

gbadner marked this pull request as draft July 23, 2021 23:56

gbadner changed the title ~~HHH-14744 : Refactor contextual information for SchemaManagementTool …~~ HHH-14744 : Refactor contextual information for SchemaManagementTool to be more easily extended by Hibernate Reactive Jul 23, 2021

gbadner force-pushed the HHH-14744 branch from 22feea4 to c1c14f4 Compare July 24, 2021 00:08

gbadner requested review from gavinking and dreab8 July 24, 2021 00:09

gbadner mentioned this pull request Jul 24, 2021

[536] [913] [910] SchemaMigrator/SchemaValidator support for PostgreSQL and CockroachDB hibernate/hibernate-reactive#903

Merged

dreab8 reviewed Jul 26, 2021

View reviewed changes

hibernate-core/src/main/java/org/hibernate/tool/schema/spi/SchemaManagementTool.java Outdated Show resolved Hide resolved

gbadner requested a review from dreab8 July 28, 2021 20:05

gbadner force-pushed the HHH-14744 branch from e4390a0 to 1f75f7c Compare July 30, 2021 03:50

gbadner added 4 commits August 2, 2021 21:51

HHH-14744 : Refactor contextual information for SchemaManagementTool …

8065436

…to be more easily extended by Hibernate Reactive HHH-14744 : Restore databases/pgsql/resources/hibernate.properties and gradle/databases.gradle

HHH-14744 : Checkstyle fixes

87b5b8d

HHH-14744 : Refactor contextual information for SchemaManagementTool …

5b8d67b

…to be more easily extended by Hibernate Reactive Move new methods out of SchemaManagementTool and into ExtractionTool

HHH-14744 : Refactor contextual information for SchemaManagementTool …

e80712c

…to be more easily extended by Hibernate Reactive Add Javadoc and other minor changes to make it easier to review

gbadner force-pushed the HHH-14744 branch from 1f75f7c to e80712c Compare August 3, 2021 04:53

gbadner requested review from Sanne and removed request for gavinking August 3, 2021 04:53

gbadner marked this pull request as ready for review August 3, 2021 04:55

HHH-14744 : Refactor contextual information for SchemaManagementTool …

a64d010

…to be more easily extended by Hibernate Reactive Correct specifications for row order of some ResultSets.

HHH-14744 : HHH-14744 : Refactor contextual information for SchemaMan…

b63d6e8

…agementTool to be more easily extended by Hibernate Reactive Remove PostgreSQL10Dialect#buildIdentifierHelper since it is no longer needed.

gbadner commented Aug 10, 2021

View reviewed changes

HHH-14744 : HHH-14744 : Refactor contextual information for SchemaMan…

7f316ae

…agementTool to be more easily extended by Hibernate Reactive Move methods from PostgreSQL10Dialect into PostgreSQL81Dialect; remove @GeneratedValue from SchemaUpdateTest

HHH-14744 : Refactor contextual information for SchemaManagementTool …

4180195

…to be more easily extended by Hibernate Reactive Remove SequenceInformationExtractorPostgresSQLDatabaseImpl and PostgreSQL10Dialect#getSequenceInformationExtractor

HHH-14744 : Refactor contextual information for SchemaManagementTool …

1083534

…to be more easily extended by Hibernate Reactive Add MySQLDialect#getNameQualifierSupport

gbadner mentioned this pull request Aug 16, 2021

[910] SchemaMigrator/SchemaValidator support for MySQL and MariaDB hibernate/hibernate-reactive#940

Closed

Sanne approved these changes Aug 25, 2021

View reviewed changes

Sanne merged commit a70b994 into hibernate:main Aug 25, 2021

Sanne mentioned this pull request Aug 27, 2021

Upgrade to Hibernate ORM 5.6.0.Beta1 hibernate/hibernate-reactive#944

Merged

yodous reviewed Dec 14, 2021

View reviewed changes

HHH-14744 : Refactor contextual information for SchemaManagementTool to be more easily extended by Hibernate Reactive #4106

HHH-14744 : Refactor contextual information for SchemaManagementTool to be more easily extended by Hibernate Reactive #4106

Uh oh!

Conversation

gbadner commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hibernate-github-bot bot commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gbadner commented Jul 30, 2021

Uh oh!

gbadner commented Aug 3, 2021

Uh oh!

gbadner commented Aug 3, 2021

Uh oh!

Sanne commented Aug 3, 2021

Uh oh!

gbadner commented Aug 3, 2021

Uh oh!

Sanne commented Aug 3, 2021

Uh oh!

gbadner commented Aug 3, 2021

Uh oh!

gbadner commented Aug 4, 2021

Uh oh!

Sanne commented Aug 4, 2021

Uh oh!

gbadner commented Aug 5, 2021

Uh oh!

gbadner commented Aug 5, 2021

Uh oh!

gbadner commented Aug 5, 2021

Uh oh!

gbadner Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbadner Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

gbadner Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

dreab8 Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

gbadner Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

gbadner commented Aug 13, 2021

Uh oh!

gbadner commented Aug 13, 2021

Uh oh!

gavinking commented Aug 13, 2021

Uh oh!

gbadner commented Aug 16, 2021

Uh oh!

gbadner commented Aug 16, 2021

Uh oh!

gbadner commented Aug 17, 2021

Uh oh!

Sanne left a comment

Choose a reason for hiding this comment

Uh oh!

Sanne commented Aug 25, 2021

Uh oh!

Sanne commented Aug 25, 2021

Uh oh!

yodous Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gbadner commented Jul 23, 2021 •

edited

Loading

hibernate-github-bot bot commented Jul 23, 2021 •

edited

Loading

gbadner Aug 10, 2021 •

edited

Loading

yodous Dec 14, 2021 •

edited

Loading