Fix DatabaseFactory db loading when Database implementation is specified (fix for Issues 5371 & 5396 )

[dhsmith1001]

Impact

• Bug fix (non-breaking change which fixes expected existing functionality)
• Enhancement/New feature (adds functionality without impacting existing logic)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

When a specific Database implementation is specified (e.g. with --databaseClass using CLI), the DatabaseFactory loads only that Database instance in its implementedDatabases HashMap. This causes a failure when code such as DatabaseList.validateDefinitions() (line 88) tries to verify the database shortnames in the dbms attribute of a changeSet or createProcedure element.

This PR changes the behavior of DatabaseFactory so that it loads all Database instances when a databaseClass is provided, but also keeps track of the class specified. It will provide the specified database for the relevant shortName, but will also still return the appropriate Database instance for shortNames other than that of the specified database.

Things to be aware of

• A new private member variable was added to DatabaseFactory: specifiedDbClass
• The getDatabase() method was changed to return the specifiedDbClass instance when its shortName is passed as the parameter

Things to worry about

Additional Context

[filipelautert]

Thanks for the fix @dhsmith1001 ! I'm moving it ahead for test review.

[MalloD12]

@dhsmith1001 code changes look good to me, although I was going to propose creating a separate method as I thought the updated method was used by an integration, but looking into the details of the reported issue it was reproduced using CLI so let's leave it like that for now and I'll discuss it with the team next week.

I would like to see some integration tests here in this PR. I thought it would be a good idea to use one of the changelogs (TestChangelogOld.xml) you provided and run an update command setting the databaseClass property and one without setting that up. Could you do that for us? If you need any help I'll be here starting Tuesday to guide you.

Thanks,
Daniel.

[dhsmith1001]

@dhsmith1001 code changes look good to me, although I was going to propose creating a separate method as I thought the updated method was used by an integration, but looking into the details of the reported issue it was reproduced using CLI so let's leave it like that for now and I'll discuss it with the team next week.

I would like to see some integration tests here in this PR. I thought it would be a good idea to use one of the changelogs (TestChangelogOld.xml) you provided and run an update command setting the databaseClass property and one without setting that up. Could you do that for us? If you need any help I'll be here starting Tuesday to guide you.

Thanks, Daniel.

@MalloD12 I'm happy to add an integration test. What kind of test you have in mind? I see several possibilities in liquibase-integration-tests, e.g. a new method in LiquibaseCommandLineTest or a new test class under src/test/groovy/liquibase/command/core/. Is there a specific test that would make a good example?

[MalloD12]

@dhsmith1001 code changes look good to me, although I was going to propose creating a separate method as I thought the updated method was used by an integration, but looking into the details of the reported issue it was reproduced using CLI so let's leave it like that for now and I'll discuss it with the team next week.
I would like to see some integration tests here in this PR. I thought it would be a good idea to use one of the changelogs (TestChangelogOld.xml) you provided and run an update command setting the databaseClass property and one without setting that up. Could you do that for us? If you need any help I'll be here starting Tuesday to guide you.
Thanks, Daniel.

@MalloD12 I'm happy to add an integration test. What kind of test you have in mind? I see several possibilities in liquibase-integration-tests, e.g. a new method in LiquibaseCommandLineTest or a new test class under src/test/groovy/liquibase/command/core/. Is there a specific test that would make a good example?

Hi @dhsmith1001,

Yeap, I think LiquibaseCommandLineTest test suite would be a good place. What we can do is if you want you can create the same test class under the Groovy directory and then we can migrate the other test in the Java class, I can do that if you prefer.

Thanks,
Daniel.

[dhsmith1001]

@MalloD12 I added a test to LiquibaseCommandLineTest.java. I opted not to convert it to Groovy as that's not my strength. Hope this is OK.

[MalloD12]

Approved.

Thanks @dhsmith1001 for this PR and for taking the time to add a test to it. I have added another based on the one you added, but without specifying a databaseClass. Also, I had to change the database used in these tests, because HSQL driver used was not compatible with Java8, so I decided to use H2 instead. Other than that code changes looks good, build and test checks are successfully executed.

Thanks,
Daniel.

[dhsmith1001]

Thanks @MalloD12! Any estimate on when 4.25.2 will be released?

[MalloD12]

Thanks @MalloD12! Any estimate on when 4.25.2 will be released?

We are aiming to have our next release around the end of January or the beginning of February.

Thanks,
Daniel.