SchemaMigrator/SchemaValidator tests for corner cases #977

blafond · 2021-09-28T15:07:18Z

Addresses #936

Test cases for corner cases of schema update, migration and validation

This is a draft and I've only checked out some of the tests in #936 and a number of them are targeted specifically to H2.

DavideD · 2021-09-28T16:21:28Z

Is this for all databases?

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

Shouldn't you rebase this on top of this other PR? #970

DavideD · 2021-09-28T16:26:30Z

hibernate-reactive-core/src/test/java/org/hibernate/reactive/schema/NumericValidationTest.java

+
+		if ( DatabaseConfiguration.dbType() != DatabaseConfiguration.DBType.POSTGRESQL ) {
+			return;
+		}


If you do things this way, this test will result in a success even if, in reality, you want to skip it.
That's way I've created the rule. It will also call the before/after methods.

Right. Some of these tests are not DB specific in their purpose, but DB specific in query implementation. So options are still to find a cleaner (efficient) way to make that happen in one test class, or break them apart into multiple classes. I'm good with either.

Break it in separate classes, please. Maybe inner classes extending an abstract super class.

The rule I've created is not that powerful and it only works for classes. if the only difference is the query, having an abstract super class with a mehod getQuery (and maybe getResult) will avoid a lot of duplicated work,

I think we could now switch to JUnit 5 and it will make the management of different test sper database easier. But that's a separate issue for when we have time.

sure.. I'll work toward abstraction and getQuery() & getResult().

@DavideD I added a schema package since there may be more tests. Any issue with doing that at this time?

@DavideD I added a schema package since there may be more tests. Any issue with doing that at this time?

No, that's fine

Just keep in mind that we already have several test classes in the project and you should move them in the sme package if you want to create a new one

DavideD · 2021-09-28T16:28:30Z

hibernate-reactive-core/src/test/java/org/hibernate/reactive/schema/NumericValidationTest.java

+	protected Configuration constructConfiguration(String hbm2DdlOption) {
+		Configuration configuration = constructConfiguration();
+		configuration.setProperty( Settings.HBM2DDL_AUTO, hbm2DdlOption );
+		configuration.setProperty( Settings.DEFAULT_CATALOG, "hreact" );


Is setting the catalog part of the corner case? Because I don't think it's going to work for all databases

good point. Will tweak that test.

I know that there was an issue where not setting the catalog will cause a NullPointerException. If you rebase on top of this pr, that issue should be solved.

It is the dialect's implementation of #getNameQualifierSupport that determines what may be required.

If NameQualifierSupport#CATALOG is returned, then Settings.DEFAULT_CATALOG may need to be defined.

If NameQualifierSupport#SCHEMA is returned, then Settings.DEFAULT_SCHEMA may need to be defined.

If NameQualifierSupport#BOTH is returned, then both Settings.DEFAULT_CATALOG and Settings.DEFAULT_SCHEMA may need to be defined.

I think it depends on the particular database whether it is needed and whether GROUPED strategy is required. I'm not sure what happens if, for example #getNameQualifierSupport returns NameQualifierSupport#CATALOG and Settings.DEFAULT_SCHEMA is provided. ORM may simply ignore the setting.

@dreab8, may be able to shed more light on this.

If I remember correctly if the db does not support schema and we provide a schema in the mapping of a table we have errors not sure about the Settings, but I may be wrong...We need to test this behaviour.

DavideD · 2021-09-28T18:12:54Z

By the way, the reason I use the # and then the issue number in the commit message is that it's clickable when looking at the history on github. You can see an example here: https://github.com/hibernate/hibernate-reactive/commits/main

DavideD · 2021-09-28T18:14:15Z

If you only use the issue number (for example, [936] SchemaMigrator/SchemaValidator tests for corner cases) 936 is not linkable anymore

DavideD · 2021-09-29T07:16:38Z

...eactive-core/src/test/java/org/hibernate/reactive/schema/SchemaUpdateProceedOnErrorTest.java

+	}
+
+	@Test
+	public void testHaltOnError(TestContext context) {


What's the purpose of this test?

By default, an error found while executing SchemaMigrator should not terminate the bootstrap process. This is controlled by hibernate.hbm2ddl.halt_on_error (default is false)

This test will try to create a table named From, which is a reserved word for most databases. It should cause warnings to get logged, but the SessionFactory should still be created. Obviously, there will be problems if an application tries to query or insert data related to the failed DDL.

The test just checks that no exception is thrown.

I think we need to set hibernate.hbm2ddl.halt_on_error to false and see that nothing fails, and then set it to true and check that we have the failures.

DavideD · 2021-09-29T07:30:10Z

...active-core/src/test/java/org/hibernate/reactive/schema/SchemaUpdateSchemaNameMySQLTest.java

+
+import io.vertx.ext.unit.TestContext;
+
+public abstract class SchemaUpdateSchemaNameMySQLTest extends BaseReactiveTest {


We have this class already in the project: https://github.com/hibernate/hibernate-reactive/blob/main/hibernate-reactive-core/src/test/java/org/hibernate/reactive/SchemaUpdateMySqlTestBase.java

Do we need a separate class for this additional tests or we can add them to the existing class?

It's possible to add the tests to current HR MySQL test but I may need to bring over the the ORM version of the entities which don't have PK/FK defined. The tests will fails when loading the ASimpleNext metadata during build session factory.

I can do that but I think it muddies the scope of the SchemaUpdateMySqlTestBase.java

Well OK, it would be nice to have all the common configuration in one place but we can do it later.

I think it's worth having this in a separate test because it really is very much a corner case.

Notice that the entities map schema = "test".

For MySQL, #getNameQualifierSupport returns NameQualifierSupport.CATALOG.

In other words, the mapping really is incorrect; it should map catalog="test".

Internally, MySQL DatabaseMetaData method take the catalog value and uses it like it is the schema. That's mentioned here

I don't remember, but I think that this weird behavior was introduced at some point, and old mappings got broken.

This is a test that the legacy mapping will still work.

@blafond Can you add some javadoc for this class, please?

From what I understand, we are testing:

Combination of catalog + schema names

Two different classes with with the same entity name and table name

I also don't see anything specific to MySQL in this test class. I think it will work for MSSQL if we change the name of the catalog to master during the build and probably for PostgreSQL without additional changes. @blafond could you also check if it works for the other dbs?

DavideD · 2021-09-29T07:39:05Z

In the project we have several test classes for testing schema update/validation for each database (see SchemaUpdate*TestBase).
If I'm not mistaken, it seems you could add the test methods to those classes (But you will need to create a new one for Db2).

blafond · 2021-09-29T21:38:57Z

@gbadner @DavideD NumericValidationTest.javatests validation for a specific entity BigDecimal datatype. Since ORM test version does not specify DB type in the tests, so I'm assuming we want to set the test(s) up to do the same in HR.

The ORM test only tests if validation doesn't fail. My HR version actually executes a query to find the specific column and type and verify the DB's expected type.

Is this necessary? In HR, I don't think we've been writing tests that don't query the data or schema metadata.

DavideD · 2021-09-30T08:50:32Z

@gbadner @DavideD NumericValidationTest.javatests validation for a specific entity BigDecimal datatype. Since ORM test version does not specify DB type in the tests, so I'm assuming we want to set the test(s) up to do the same in HR.

Ideally, every test should run on every database. The only time we don't do that is when a feature is not supported by all databases or when we need to run native queries.

For this particular type of tests, I don't think it's possible because you will probably need to check that the column in the table exists and it's of the expected type. And I think you can only do that with native queries.

The ORM test only tests if validation doesn't fail. My HR version actually executes a query to find the specific column and type and verify the DB's expected type.

Sounds good, but to test that the validation is working we would need the following use case:

Validation fails when a table is missing (with and without inheritance)
Validation fails when a column is missing
Validation doesn't fail if everything is alright

An example of how to test for case 1 is in SchemaUpdateSqlServerTestBase#testVaslidationFails.
We could move those tests in a separate class and run it for all databases

For case 2, I would run a native query that drops the column before running the test. You will need a native query for this case but because it's just a query,
you can keep track of them in a constant.

Case 3 is what you have done already

Let me know if this makes sense

DavideD · 2021-09-30T09:52:52Z

I've just realized that we should also check that the validation doesn't fail (even if a column is missing) when Settings.HBM2DDL_AUTO is not set or set to none

It's already defined in the Closable interface

DavideD · 2021-10-21T16:43:42Z

Superseeded by #1017

[936] SchemaMigrator/SchemaValidator tests for corner cases

3ca0035

blafond requested a review from gbadner September 28, 2021 15:09

DavideD reviewed Sep 28, 2021

View reviewed changes

blafond changed the title ~~[936] SchemaMigrator/SchemaValidator tests for corner cases~~ [#936] SchemaMigrator/SchemaValidator tests for corner cases Sep 28, 2021

DavideD reviewed Sep 29, 2021

View reviewed changes

DavideD changed the title ~~[#936] SchemaMigrator/SchemaValidator tests for corner cases~~ SchemaMigrator/SchemaValidator tests for corner cases Sep 29, 2021

DavideD linked an issue Sep 29, 2021 that may be closed by this pull request

SchemaMigrator/SchemaValidator - add tests for corner cases #936

Closed

DavideD reviewed Sep 29, 2021

View reviewed changes

Attempt at abstract test class for schema update/validation

e9f3457

blafond force-pushed the 936-schema-corner-test-cases branch from 914c128 to e9f3457 Compare September 30, 2021 19:54

DavideD added a commit to DavideD/hibernate-reactive that referenced this pull request Oct 14, 2021

[hibernate#977] Remove duplicated close method in the Session

4ff2745

It's already defined in the Closable interface

DavideD closed this Oct 21, 2021

DavideD mentioned this pull request Oct 21, 2021

Schema Update & Validation corner cases #1017

Closed


		import io.vertx.ext.unit.TestContext;

		public abstract class SchemaUpdateSchemaNameMySQLTest extends BaseReactiveTest {

SchemaMigrator/SchemaValidator tests for corner cases #977

SchemaMigrator/SchemaValidator tests for corner cases #977

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DavideD commented Sep 28, 2021

Uh oh!

DavideD commented Sep 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavideD Sep 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavideD Sep 28, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

DavideD commented Sep 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavideD commented Sep 29, 2021

Uh oh!

blafond commented Sep 29, 2021

Uh oh!

DavideD commented Sep 30, 2021

Uh oh!

DavideD commented Sep 30, 2021

Uh oh!

DavideD commented Oct 21, 2021

Uh oh!

Uh oh!

blafond commented Sep 28, 2021 •

edited

Loading

DavideD Sep 29, 2021 •

edited

Loading

DavideD Sep 28, 2021 •

edited

Loading

DavideD commented Sep 28, 2021 •

edited

Loading