MSSQL Schema update and validation #970

DavideD · 2021-09-22T15:29:11Z

Fixes #912
A test for the following pr: #967

I've rebased it to the latest changes and it's using the available HIbernate ORM (5.6.0.Beta2)

Waiting for this PR to be merged and released

DavideD · 2021-09-22T15:38:53Z

@gbadner, @blafond I was expecting this PR to fail but it seems to work fine. What am I missing?

gbadner · 2021-09-22T16:07:04Z

I'm not sure how it could be passing?!? I'm looking into it...

gbadner · 2021-09-22T16:40:54Z

@DavideD, take a look at the standard output for the SQL Server test:
hibernate-reactive-core/build/reports/tests/test/classes/org.hibernate.reactive.SchemaUpdateSqlServerTestBase$IndividuallySchemaUpdateSqlServerTestBase.html

The drop sequence statement fails:

?[33mWARN?[m ?[34mReactiveGenerationTarget?[m [vert.x-eventloop-thread-0] HR000021: DDL command failed [io.vertx.mssqlclient.MSSQLException: {number=166, state=1, severity=15, message=''DROP SEQUENCE' does not allow specifying the database name as a prefix to the object name.', serverName='8ec40056a401', lineNumber=1, additional=[io.vertx.mssqlclient.MSSQLException: {number=8180, state=1, severity=16, message='Statement(s) could not be prepared.', serverName='8ec40056a401', lineNumber=1}]}]

Later in the output, you'll see that the create sequence statement fails for the same reason.

Later in the test, you'll see that select next value for master.dbo.hibernate_sequence statements succeed.
(you can't actually search for that string in the output because of all the embedded "?[34m".)

Apparently, the hibernate_sequence is left over from a previous test, and that is what is being used.

Using a local ORM 5.6.0-SNAPSHOT, these messages are gone, and the drop/create sequence statements succeed.

DavideD · 2021-09-22T16:46:28Z

Thanks @gbadner, makes sense.

gbadner · 2021-09-22T17:12:18Z

Supersedes #967.

gbadner · 2021-09-22T17:13:59Z

We need to wait for hibernate/hibernate-orm#4235 to get merged and released before this PR can be merged.

gbadner · 2021-09-22T17:16:50Z

Using a local ORM 5.6.0-SNAPSHOT, these messages are gone, and the drop/create sequence statements succeed.

To clarify, after publishing the hibernate-core jar from hibernate/hibernate-orm#4235 (which is not merged yet) to a local repository, and setting gradle.properties to use that jar, the drop/create sequence statements succeed.

gbadner · 2021-09-22T17:38:38Z

Pushed a commit to fix inconsistent test arguments.

gbadner · 2021-09-22T18:32:06Z

@DavideD, is the DB2 test failure expected?

DavideD · 2021-09-22T18:40:01Z

I will restart the job... not sure why it failed

DavideD · 2021-09-22T19:26:22Z

I've added a commit that should make the tests fail consistently.
It will delete the sequence if it exists before running the creation of the schema.
@gbadner or @blafond could you check that it works when using the right ORM snapshot, please?

gbadner · 2021-09-22T20:25:34Z

@DavideD, I pulled your commit, confirmed the tests fail using 5.6.0.Beta2, and confirmed the tests pass using hibernate/hibernate-orm#4235.

FWIW, specifying the catalog when creating a sequence on ORM worked fine. Vert.x throws an exception when the catalog is specified.

SQL Server documentation of the create sequence syntax only allows the schema, not the catalog:
https://docs.microsoft.com/en-us/sql/t-sql/statements/create-sequence-transact-sql?view=sql-server-ver15&viewFallbackFrom=sql-server-ver12 . This is mentioned in ORM code that that was changed.

DavideD · 2021-09-23T09:12:03Z

Thanks @gbadner, I've updated the comment

hibernate-reactive-core/src/test/java/org/hibernate/reactive/SchemaUpdateSqlServerTestBase.java

blafond · 2021-09-23T15:34:08Z

tested successfully locally with default ORM 5.6.0.Beta2 as well as my local 5.6.0-SNAPSHOT

gbadner · 2021-09-23T16:31:49Z

@davide, I agree that what you did will make the test pass. I'm not sure if it will always be correct though. The problem is that ORM doesn't have a way to find out the catalog name (which is the database name) unless Hibernate Reactive provides it.

I think it would be fine to deal with the catalog name separately.

gbadner · 2021-09-23T16:36:18Z

@DavideD, I have some ideas about how the catalog name might be resolved. Should I go ahead and create the follow-on issue for catalog, or would like to do that?

DavideD · 2021-09-23T16:48:10Z

@DavideD, I have some ideas about how the catalog name might be resolved. Should I go ahead and create the follow-on issue for catalog, or would like to do that?

Yes, please. Create a separate issue. I've tried to play a bit with it but could figure out what to do.
It seems the property also causes issues with MySQL and MariaDB

DavideD · 2021-09-23T16:49:20Z

@davide, I agree that what you did will make the test pass. I'm not sure if it will always be correct though. The problem is that ORM doesn't have a way to find out the catalog name (which is the database name) unless Hibernate Reactive provides it.

Sure but I see this as a separate issue. If I'm not mistaken, everything will work for users that don't set the catalog name

gbadner · 2021-09-23T17:01:31Z

@DavideD, I'm really not sure if it will always work.

SQL Server's default catalog name is "master". I've read that it is possible to change it. I am not sure if/how that would impact what gets returned by information_schema views.

I have not figured out how to change it with podman, so I haven't tested it.

DavideD · 2021-09-23T17:02:48Z

We can just test with the default name and make sure that it works with that

gbadner · 2021-09-23T17:07:10Z

Your change will make the queries not take the catalog name into account at all. If it is possible for information_schema to have more than one catalog, then query results that ORM relies on might not be correct.

This may not be an issue; I don't know...

DavideD · 2021-09-23T17:08:41Z

What I mean is that the issue with the catalog is not just for schema validation/update. It's something that won't work in other cases as well. I would expect this test to pass with MSSQL: DavideD@5b979c2

gbadner · 2021-09-23T17:10:25Z

@DavideD, I think it really only matters for SchemaMigrator and SchemaValidator.

gbadner · 2021-09-23T17:14:11Z

Yes, the catalog name will be used if provided for SchemaExport, but it is not needed SchemaExport.

gbadner · 2021-09-23T17:15:33Z

I suppose it might be needed if the catalog is a non-default, but honestly, I don't know. This is not an issue for ORM because ORM can get the information it needs.

DavideD · 2021-09-24T10:02:02Z

I've refactored the code and added back the test with the default catalog name.
The latest changes in the PR test the following cases:

Property hibernate.default_catalog is not set and MSSQL uses the default catalog name -> Works
Property hibernate.default_catalog is set to master (the default catalog name for MSSQL) -> requires this fix for ORM

DavideD · 2021-09-29T07:22:46Z

hibernate-reactive-core/src/test/java/org/hibernate/reactive/SchemaUpdateSqlServerTestBase.java

+		private String anotherString;
+	}
+
+	public static class AOtherId implements Serializable {


@gbadner I've just noticed this class but I don't think we are using it. Is it necessary for the test or is this a leftover?

@DavideD, yes, it is needed. It is an IdClass used by AOther.

Ah yes, sorry

Remove SqlServerReactiveInformationExtractorImpl#getDatabaseSchemaColumnName

Change test arguments that are inconsistent with their function

…y string

DavideD · 2021-09-29T16:27:34Z

Merged!

gavinking · 2021-09-29T18:09:29Z

Excellent!

DavideD mentioned this pull request Sep 22, 2021

912 : SchemaMigrator/SchemaValidator support for MS SQL Server #967

Closed

gbadner added the waiting We are waiting for another PR or issue to be solved before merging this one label Sep 22, 2021

DavideD changed the title ~~912 schema update sql server 1~~ MSSQL Schema update and validation Sep 22, 2021

DavideD force-pushed the 912-schema-update-sql-server-1 branch 3 times, most recently from c33cbd2 to 3b80f9c Compare September 23, 2021 09:11

DavideD commented Sep 23, 2021

View reviewed changes

hibernate-reactive-core/src/test/java/org/hibernate/reactive/SchemaUpdateSqlServerTestBase.java Outdated Show resolved Hide resolved

DavideD force-pushed the 912-schema-update-sql-server-1 branch from 5a78810 to dca6ad6 Compare September 23, 2021 11:02

DavideD requested a review from gbadner September 23, 2021 14:01

DavideD mentioned this pull request Sep 28, 2021

SchemaMigrator/SchemaValidator tests for corner cases #977

Closed

DavideD commented Sep 29, 2021

View reviewed changes

DavideD added 2 commits September 29, 2021 16:58

[hibernate#979] Upgrade to ORM 5.6.0.CR1

75eb0f4

Disable SQL log in MultiLineImportsTest

02d446a

DavideD force-pushed the 912-schema-update-sql-server-1 branch from 3e3ea79 to 79b1938 Compare September 29, 2021 16:05

blafond and others added 7 commits September 29, 2021 17:07

[hibernate#912] SchemaMigrator/SchemaValidator support for MS SQLServer

2abe5c1

[hibernate#912] SchemaMigrator/SchemaValidator support for MS SQL Server

de02438

[hibernate#912] SchemaMigrator/SchemaValidator support for MS SQL Server

617777b

Remove SqlServerReactiveInformationExtractorImpl#getDatabaseSchemaColumnName

[hibernate#912] SchemaMigrator/SchemaValidator support for MS SQL Server

326de68

Change test arguments that are inconsistent with their function

[hibernate#912] Make the tests work consistently

e958399

[hibernate#912] Fix NullPointerException when Catalog name is an empt…

24fc353

…y string

[hibernate#912] Test using the default catalog name for MSSQL

c5f48c0

DavideD force-pushed the 912-schema-update-sql-server-1 branch from 79b1938 to c5f48c0 Compare September 29, 2021 16:08

DavideD marked this pull request as ready for review September 29, 2021 16:08

DavideD merged commit c5f48c0 into hibernate:main Sep 29, 2021

MSSQL Schema update and validation #970

MSSQL Schema update and validation #970

Uh oh!

Conversation

DavideD commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavideD commented Sep 22, 2021

Uh oh!

gbadner commented Sep 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

DavideD commented Sep 22, 2021

Uh oh!

gbadner commented Sep 22, 2021

Uh oh!

gbadner commented Sep 22, 2021

Uh oh!

gbadner commented Sep 22, 2021

Uh oh!

gbadner commented Sep 22, 2021

Uh oh!

gbadner commented Sep 22, 2021

Uh oh!

DavideD commented Sep 22, 2021

Uh oh!

DavideD commented Sep 22, 2021

Uh oh!

gbadner commented Sep 22, 2021

Uh oh!

DavideD commented Sep 23, 2021

Uh oh!

Uh oh!

blafond commented Sep 23, 2021

Uh oh!

gbadner commented Sep 23, 2021

Uh oh!

gbadner commented Sep 23, 2021

Uh oh!

DavideD commented Sep 23, 2021

Uh oh!

DavideD commented Sep 23, 2021

Uh oh!

gbadner commented Sep 23, 2021

Uh oh!

DavideD commented Sep 23, 2021

Uh oh!

gbadner commented Sep 23, 2021

Uh oh!

DavideD commented Sep 23, 2021

Uh oh!

gbadner commented Sep 23, 2021

Uh oh!

gbadner commented Sep 23, 2021

Uh oh!

gbadner commented Sep 23, 2021

Uh oh!

DavideD commented Sep 24, 2021

Uh oh!

DavideD Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

gbadner Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

DavideD Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

DavideD commented Sep 29, 2021

Uh oh!

gavinking commented Sep 29, 2021

Uh oh!

Uh oh!

DavideD commented Sep 22, 2021 •

edited

Loading

gbadner commented Sep 22, 2021 •

edited

Loading