Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flyway documentation should mention required database modules #40834

Closed
FroMage opened this issue May 24, 2024 · 17 comments · Fixed by #41560
Closed

Flyway documentation should mention required database modules #40834

FroMage opened this issue May 24, 2024 · 17 comments · Fixed by #41560
Labels
area/flyway kind/bug Something isn't working
Milestone

Comments

@FroMage
Copy link
Member

FroMage commented May 24, 2024

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 FroMage added the kind/bug Something isn't working label May 24, 2024
Copy link

quarkus-bot bot commented May 24, 2024

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

@FroMage
Copy link
Member Author

FroMage commented May 24, 2024

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
Copy link
Member

aloubyansky commented May 24, 2024

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

@aloubyansky
Copy link
Member

Edited the above comment to fix autocomplete rubbish

@gsmet
Copy link
Member

gsmet commented May 24, 2024

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

Patch welcome?

@gsmet
Copy link
Member

gsmet commented May 24, 2024

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

@gastaldi
Copy link
Contributor

gastaldi commented May 24, 2024

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
Copy link
Contributor

geoand commented May 24, 2024

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
Copy link

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

@gsmet
Copy link
Member

gsmet commented May 28, 2024

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

@FroMage
Copy link
Member Author

FroMage commented May 29, 2024

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
Copy link
Contributor

geoand commented Jun 6, 2024

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

@joaodsimoes
Copy link
Contributor

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
Copy link
Contributor

geoand commented Jun 29, 2024

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

@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 1, 2024
@FroMage
Copy link
Member Author

FroMage commented Jul 1, 2024

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

@geoand
Copy link
Contributor

geoand commented Jul 1, 2024

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

@geoand
Copy link
Contributor

geoand commented Jul 1, 2024

#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flyway kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants