quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.77k stars 2.68k forks source link

Flyway documentation should mention required database modules #40834

Closed FroMage closed 4 months ago

FroMage commented 5 months ago

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?

quarkus-bot[bot] commented 5 months ago

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

FroMage commented 5 months ago

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.

aloubyansky commented 5 months ago

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

aloubyansky commented 5 months ago

Edited the above comment to fix autocomplete rubbish

gsmet commented 5 months ago

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

Patch welcome?

gsmet commented 5 months ago

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

gastaldi commented 5 months ago

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

geoand commented 5 months ago

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

alyelalwany commented 5 months ago

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

gsmet commented 5 months ago

@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:

If you feel otherwise, suggestions are welcome!

FroMage commented 5 months ago

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.

geoand commented 5 months ago

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

joaodsimoes commented 4 months ago

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.

geoand commented 4 months ago

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

FroMage commented 4 months ago

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

geoand commented 4 months ago

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

geoand commented 4 months ago

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.