quarkus icon indicating copy to clipboard operation
quarkus copied to clipboard

Flyway documentation should mention required database modules

Open FroMage opened this issue 1 year ago • 14 comments

Trying to use Flyway, I imported quarkus-flyway and then it complained about:

Caused by: org.flywaydb.core.api.FlywayException: Unsupported Database: PostgreSQL 16.3
	at org.flywaydb.core.internal.database.DatabaseTypeRegister.getDatabaseTypeForConnection(DatabaseTypeRegister.java:105)
	at org.flywaydb.core.internal.jdbc.JdbcConnectionFactory.<init>(JdbcConnectionFactory.java:73)
	at org.flywaydb.core.FlywayExecutor.execute(FlywayExecutor.java:134)
	at org.flywaydb.core.Flyway.migrate(Flyway.java:147)
	at io.quarkus.flyway.runtime.FlywayRecorder.doStartActions(FlywayRecorder.java:136)
	at io.quarkus.deployment.steps.FlywayProcessor$startActions2099152139.deploy_0(Unknown Source)
	at io.quarkus.deployment.steps.FlywayProcessor$startActions2099152139.deploy(Unknown Source)

The documentation at https://quarkus.io/guides/flyway is not clear, because it does mention that other DBs need special extra modules, but it doesn't say that about Postgres. Perhaps it's new, but now it's required, so we should document that:

<dependency>
    <groupId>org.flywaydb</groupId>
    <artifactId>flyway-database-postgresql</artifactId>
</dependency>

Is now required. Also, I'm a bit suspicious about the fact that some modules start with flyway-database and others just flyway- I don't know if that's really the case, we should check.

And finally, but VERY IMPORTANT, we should fix that stupid exception listed above, because it's NOT helpful. I installed other versions of Postgres, I looked online to find which version was supported, and got really mixed results from Flyway documentation, one page saying only 16, another saying all of them.

It was not that my DB version was unsupported, the real error was that the extra module was missing. If we can't automatically add those modules at build time (if flyway + postgres -> extra dependency) (ask @aloubyansky) then perhaps we should throw a build time error with helpful text for how to resolve it by adding the required module?

FroMage avatar May 24 '24 15:05 FroMage

/cc @cristhiank (flyway), @gastaldi (flyway), @geoand (flyway), @gsmet (flyway)

quarkus-bot[bot] avatar May 24 '24 15:05 quarkus-bot[bot]

Also, I'll add that the documentation page should probably not list the configuration options before the rest of the page. I was convinced I didn't need to scroll down after the config options (very long), but there's a lot of content down there.

FroMage avatar May 24 '24 15:05 FroMage

If "extra dependency" is also an extension then it can be done.

aloubyansky avatar May 24 '24 15:05 aloubyansky

Edited the above comment to fix autocomplete rubbish

aloubyansky avatar May 24 '24 15:05 aloubyansky

Yes, PostgreSQL is a separate module since Flyway 10 and Quarkus 3.10.

Patch welcome?

gsmet avatar May 24 '24 15:05 gsmet

And no they are not extensions, just extra Flyway dependencies.

gsmet avatar May 24 '24 15:05 gsmet

And finally, but VERY IMPORTANT, we should fix that stupid exception listed above, because it's NOT helpful. I installed other versions of Postgres, I looked online to find which version was supported, and got really mixed results from Flyway documentation, one page saying only 16, another saying all of them.

IMHO this should be opened against https://github.com/flyway/flyway, as it's a Flyway feature

gastaldi avatar May 24 '24 15:05 gastaldi

Unless I am missing something, we can make this work OOTB, by introducing a conditional dependency that will get triggered when the Flyway and Postgres extensions are present

geoand avatar May 24 '24 17:05 geoand

Do you need someone to contribute on this ? I am interested in helping out

alyelalwany avatar May 28 '24 20:05 alyelalwany

@alyelalwany sure feel free to have a look, the more the merrier :).

From what I know, conditional dependencies are only working for extensions so for now the best we could do is to improve the documentation and make sure that this section https://github.com/quarkusio/quarkus/blob/main/docs/src/main/asciidoc/flyway.adoc?plain=1#L29-L82 is made much better.

We need to make sure PostgreSQL, DB2 and Derby are listed as needing additional dependencies.

But I also wonder if we should do it in two steps:

  • have the Maven/Gradle example with PostgreSQL
  • have a bullet list of all the databases that require an additional Flyway dependency

If you feel otherwise, suggestions are welcome!

gsmet avatar May 28 '24 21:05 gsmet

And finally, but VERY IMPORTANT, we should fix that stupid exception listed above, because it's NOT helpful. I installed other versions of Postgres, I looked online to find which version was supported, and got really mixed results from Flyway documentation, one page saying only 16, another saying all of them.

IMHO this should be opened against https://github.com/flyway/flyway, as it's a Flyway feature

Sure, but we can catch that exception and add extra info on top of it, until it's fixed upstream. That's the helpful thing to do.

On the other hand, if we go with throwing a build exception if the required extension is mising (not the best option, the best option is to automatically add the dependency, but sometimes we have to compromise), then handling this exception is not necessary.

FroMage avatar May 29 '24 12:05 FroMage

@alyelalwany hey, are you still planning on looking into this one?

geoand avatar Jun 06 '24 17:06 geoand

I just want to reiterate @FroMage's initial point that the documentation should be updated, I was following the official Quarkus' Using Flyway guide and by following exactly what is documented for Postgres you would get the above error. In my case, since I'm using the dev-services provided postgres instance, I got the following:

 Failed to start quarkus: io.quarkus.dev.appstate.ApplicationStartException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.dev.appstate.ApplicationStateNotification.waitForApplicationStart(ApplicationStateNotification.java:58)
	at io.quarkus.runner.bootstrap.StartupActionImpl.runMainClass(StartupActionImpl.java:132)
	at io.quarkus.deployment.dev.IsolatedDevModeMain.restartApp(IsolatedDevModeMain.java:193)
	at io.quarkus.deployment.dev.IsolatedDevModeMain.restartCallback(IsolatedDevModeMain.java:174)
	at io.quarkus.deployment.dev.RuntimeUpdatesProcessor.doScan(RuntimeUpdatesProcessor.java:548)
	at io.quarkus.deployment.dev.RuntimeUpdatesProcessor.doScan(RuntimeUpdatesProcessor.java:448)
	at io.quarkus.vertx.http.runtime.devmode.VertxHttpHotReplacementSetup$6.call(VertxHttpHotReplacementSetup.java:161)
	at io.quarkus.vertx.http.runtime.devmode.VertxHttpHotReplacementSetup$6.call(VertxHttpHotReplacementSetup.java:148)
	at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$0(ContextImpl.java:178)
	at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:279)
	at io.vertx.core.impl.ContextImpl.lambda$internalExecuteBlocking$2(ContextImpl.java:210)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1495)
	at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:11)
	at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:11)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at io.quarkus.runtime.ApplicationLifecycleManager.run(ApplicationLifecycleManager.java:111)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:71)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:44)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:124)
	at io.quarkus.runner.GeneratedMain.main(Unknown Source)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.runner.bootstrap.StartupActionImpl$1.run(StartupActionImpl.java:113)
	... 1 more
Caused by: org.flywaydb.core.api.FlywayException: Unsupported Database: PostgreSQL 14.12
	at org.flywaydb.core.internal.database.DatabaseTypeRegister.lambda$getDatabaseTypeForConnection$7(DatabaseTypeRegister.java:122)
	at java.base/java.util.Optional.orElseThrow(Optional.java:403)
	at org.flywaydb.core.internal.database.DatabaseTypeRegister.getDatabaseTypeForConnection(DatabaseTypeRegister.java:122)
	at org.flywaydb.core.internal.jdbc.JdbcConnectionFactory.<init>(JdbcConnectionFactory.java:77)
	at org.flywaydb.core.FlywayExecutor.execute(FlywayExecutor.java:138)
	at org.flywaydb.core.Flyway.clean(Flyway.java:273)
	at io.quarkus.flyway.runtime.FlywayRecorder.doStartActions(FlywayRecorder.java:123)
	at io.quarkus.deployment.steps.FlywayProcessor$startActions2099152139.deploy_0(Unknown Source)
	at io.quarkus.deployment.steps.FlywayProcessor$startActions2099152139.deploy(Unknown Source)
	... 11 more

Only after digging around a bit and finding this issue on flyway's Github did I manage to get it working.

joaodsimoes avatar Jun 28 '24 19:06 joaodsimoes

I'll look at fixing this next week so users won't have to include the dependency

geoand avatar Jun 29 '24 05:06 geoand

@geoand do we want to close this issue and open a new one, or keep this one open?

FroMage avatar Jul 01 '24 09:07 FroMage

~~I'll open a new one describing what I am thinking~~ #41580

geoand avatar Jul 01 '24 09:07 geoand

#41583 shows how Quarkus can allow users to use quarkus-flyway and quarkus-jdbc-postgresql without having to add org.flywaydb:flyway-database-postgresql.

If folks want to take up the task for other JDBC drivers, it should be straightforward by following what I did in said PR.

Shout out to @aloubyansky who introduced the conditional extension feature.

geoand avatar Jul 01 '24 12:07 geoand