quarkus-liquibase-mongodb enhancements

[mazenkhalil]

This PR introduce the following enhancements to quarkus-liquibase-mongodb extension:

• Utilize the existing MongoClient for running liquibase migration scripts instead of opening a separate connection
• Allow named MongoClient to be used with liquibase
• Allow configurable database, changeLog & changeLogParameters for programatic access using LiquibaseMongodbFactory

[quarkus-bot]

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

• title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

[gsmet]

Thanks for this PR!

I will let @loicmathieu review it and provide you with feedback on the actual code.

I have a few comments on the form though: I think the formatting is probably incorrect. Also we don't want formatting commits so you need to squash the formatting commits to have a proper semantic history.

[loicmathieu]

@mazenkhalil thanks for this PR.

Currently, the connection is created by Liquibase because there is no public API to give Liquibase a Mongo Client, you hack a way to do this by creating new MongoLiquibaseDatabase and MongoLiquibaseConnection, I don't know the internal of Liquibase so I'm not very compfortable with the choice. By the way, I'm not sure it worth it as Liquibase will open a single connection, do the migration, then close it. Re-using our Mongo Client will not brings a big advantage here.

Moreover, the client is now retrieved from Arc at Liquibase creation, this will create it eagerly and would change current behavior. This may be an issue.

I agree that supporting multiple Mongo client should be done. But I'm more reluctant for the change that reuse the Mongo Client.

[mazenkhalil]

@loicmathieu Well, MongoLiquibaseDatabase and MongoLiquibaseConnection are the actual implementation for Database and Connection from Liquibase core. The only functionality there is creating MongoClient and MongoDatabase objects.

The reason why I am proposing this PR is to solve the following:

• With the current implementation, if you enable cleanAtStart, validateAtStart and migrateAtStart, 3 DB connections get opened and none get closed (ref. LiquibaseMongodbRecorder#doStartActions).
• Using the programatic method utilizing LiquibaseMongodbFactory on startup, do the job then close the connection took somewhere like 25 seconds for the app to start compares to 5 seconds without closing the connection.
• Currently there is no direct API to run migration scripts against multiple DB's. Only one DB and one changeLog file supported. The only workaround here is to access Liquibase API directly which I don't think is a clean way for implementations.

I can modify the implementation in a way to inject MongoClient when the migration actually starts, this way we can prevent the eager initialization. What do you think?

[loicmathieu]

• With the current implementation, if you enable cleanAtStart, validateAtStart and migrateAtStart, 3 DB connections get opened and none get closed (ref. LiquibaseMongodbRecorder#doStartActions).

Maybe this should be reported as a bug in the Liquibase MongoDB repo.

• Using the programatic method utilizing LiquibaseMongodbFactory on startup, do the job then close the connection took somewhere like 25 seconds for the app to start compares to 5 seconds without closing the connection.

You means millisecondes not seconds right ? Anyway it's not very good yes.

• Currently there is no direct API to run migration scripts against multiple DB's. Only one DB and one changeLog file supported. The only workaround here is to access Liquibase API directly which I don't think is a clean way for implementations.

I think we add the created Liquibase object inside the CDI container but I agree multiple DBs should be handled natively.

I can modify the implementation in a way to inject MongoClient when the migration actually starts, this way we can prevent the eager initialization. What do you think?

Yes, it would be great.

[mazenkhalil]

• With the current implementation, if you enable cleanAtStart, validateAtStart and migrateAtStart, 3 DB connections get opened and none get closed (ref. LiquibaseMongodbRecorder#doStartActions).

Maybe this should be reported as a bug in the Liquibase MongoDB repo.

The problem is from the recorder itself where 3 Liquibase instances get created (New connection/Liquibase instance) and none get closed.

• Using the programatic method utilizing LiquibaseMongodbFactory on startup, do the job then close the connection took somewhere like 25 seconds for the app to start compares to 5 seconds without closing the connection.

You means millisecondes not seconds right ? Anyway it's not very good yes.

My bad. It was actually taking seconds as I was running multiple migration scripts against multiple databases sequentially. For some reason most of the time was coming from closing the connection.

[mazenkhalil]

I've made the required changes and squashed the commits.

[gsmet]

I added a few comments but really I'm not the right person to review this stuff, it's really @loicmathieu 's ballpark.

[gsmet]

Do we want to be able to target a specific client or should we support migrations of potentially all the MongoDB clients?

[mazenkhalil]

Basically we should have the ability to decide which mongo client to use for running liquibase (The proposed solution here).

An extent to this would be implementing named liquibase configurations where you can create multiple liquibase configurations each operates on specific mongo client as needed (Same as you can create named mongo clients to connect against different databases). This would allow us to run multiple migration scripts against different mongo clients through configurations.

I was exploring the codebase of mongoclient to see how named client were implemented, but I don't feel confident enough to propose same concept for liquibase within this PR before fully understanding how build steps work in quarkus.

[loicmathieu]

I agree we should provide liquibase migration for all MongoDB client as its JDBC counterpart support migration for all named datasources.

[loicmathieu]

I think the behavious changed. Before we can use liquibase with a database name inside the connection string (we had a regex that check that if the database is not in the connection string it must be specified in the config), now we mandate a database inside the config.

[loicmathieu]

I agree we should provide liquibase migration for all MongoDB client as its JDBC counterpart support migration for all named datasources.

[mazenkhalil]

I think the behavious changed. Before we can use liquibase with a database name inside the connection string (we had a regex that check that if the database is not in the connection string it must be specified in the config), now we mandate a database inside the config.

You are right. What do you think about proposing a new property under liquibase for that? I think it is cleaner, and we can ensure backward compatibility as follows:

1. Liquibase configuration (e.g. quarkus.liquibase.database)
2. If quarkus.liquibase.database is not defined, check mongoclient database property (quarkus.mongodb.database)
3. If quarkus.mongodb.database is not defined, check the connection string to get the DB name
4. If no DB name defined within the connection string, throw an error.

[loicmathieu]

@mazenkhalil I'm not sure we need two properties, one for the MongoDB client/panache extension and one for the Liquibase MongoDB extension. What's the use case for different propoperties ?

[mazenkhalil]

@mazenkhalil I'm not sure we need two properties, one for the MongoDB client/panache extension and one for the Liquibase MongoDB extension. What's the use case for different propoperties ?

This would allow running two different migration scripts against different databases utilizing same MongoClient.

[loicmathieu]

This would allow running two different migration scripts against different databases utilizing same MongoClient.

For this we would need the ability to have multiple liquibase configuration. Maybe the best is to make these enhancements steps by steps : first allow to reuse the client and use a named client, then allow multiple liquibase.

[mazenkhalil]

For this we would need the ability to have multiple liquibase configuration. Maybe the best is to make these enhancements steps by steps : first allow to reuse the client and use a named client, then allow multiple liquibase.

Agree. I've added back the regex to parse the connection string if no database defined in the config; but I changed the order. Previously the config property was used if no database defined in the connection string, now we fallback to the connection string when no database config property is defined (I think it is more proper this way as we use mongo client now and its properties should have the priority).

I will introduce another PR soon to allow multi liquibase migrations.

[gsmet]

@mazenkhalil I extracted the part where you avoid to create 3 LiquibaseFactory in #30778 as I think we should merge this ASAP (and backport it).

As for the rest, I really think we should try to focus on an approach similar to what is done in the standard Liquibase extension. Would you be willing to give it a try?

[mazenkhalil]

@mazenkhalil I extracted the part where you avoid to create 3 LiquibaseFactory in #30778 as I think we should merge this ASAP (and backport it).

As for the rest, I really think we should try to focus on an approach similar to what is done in the standard Liquibase extension. Would you be willing to give it a try?

I will look into it later this week.

[gsmet]

Closing for lack of activity and conflicts.

I think part of the issues have been addressed (like the fact that connections are closed) but maybe not all of them.