quarkusio / quarkus

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

Datasource Devservice: Skip init script, if testcontainers.reuse.enable=true #36987

Open fsomme2s opened 12 months ago

fsomme2s commented 12 months ago

Describe the bug

When testcontainers.reuse.enable=true, the database container is reused between Quarkus hot reloads (a really awesome feature btw). But also the init script referenced in quarkus.datasource.devservices.init-script-path seems to be executed again.

I would count that as a bug - but that might be debateable.

My reasoning is: The script is meant to initialize the database. So if I keep the database up, it does not need to be initialized again. If your init script does contain actions that are not repeatable, there will be errors on hot reload and your app has to be restarted manually (and sometimes the still running container needs to be killed manually).

If not counted as bug, it should at least be configurable. Maybe some kinds of init scripts should run on each hot reload and some don't?

Current workaround is to make your init scripts idempotent (which is often easy, but in case of creating users in postgres a simple "if not exists" is not available for example).

Expected behavior

Don't execute the init script on quarkus hot reload, when testcontainers.reuse.enable=true is set.

Actual behavior

Script gets executed again on every hot reload.

How to Reproduce?

Steps to reproduce:

  1. Create a Quarkus Project with SQL Datasource, for example with "quarkus-jdbc-postgresql" dependency

    <dependency>
      <groupId>io.quarkus</groupId>
      <artifactId>quarkus-jdbc-postgresql</artifactId>
    </dependency>
  2. Configure DevService with init script and enable container reuse

application.properties:

quarkus.datasource.devservices.init-script-path=db/init/init-users.sql

~/.testcontainers.properties:

testcontainers.reuse.enable=true

  1. Do something non-idempotent in the init script, that will produce an error on second execution, for example:

src/main/resources/db/init/init-users.sql: create user app_user with password 'local';

  1. Start the app (everything should be fine for first start)
  2. Change a line in your code and trigger something for the hot reload feature (for example a rest api call)

=> Now your init script will throw an error

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

quarkus-bot[bot] commented 12 months ago

/cc @geoand (devservices), @stuartwdouglas (devservices)

gsmet commented 12 months ago

testcontainers.reuse.enable=true is totally outside of our control so I'm not sure how we could apply this.

Maybe we could have an option to run the init script only when starting dev mode but I'm not sure this will work very well in practice as any changes to your model will make your schema outdated.

/cc @yrodiere

yrodiere commented 12 months ago

So first, there are certainly users out that who want init scripts to be re-executed, because the script drops the whole content of the database before re-creating it, and they want this behavior on app restart.

So, this would need to be configurable, and it's debatable whether it should be the default (I'd say no, but... who knows).

testcontainers.reuse.enable=true is totally outside of our control so I'm not sure how we could apply this.

There is a protected method in GenericContainer that we can override to know if the container was reused or not:

    protected void containerIsStarted(InspectContainerResponse containerInfo, boolean reused) {

If other *Container classes expose that, there's a way. That's a big "if" though.


On a related note, test containers are only reused if testcontainers.reuse.enable=true and if we call .withReuse(true) on the *Container instance.

If container reuse works right now, I suppose we must be calling .withReuse(true) indiscriminately, which is agruably a bad idea since users might be setting testcontainers.reuse.enable=true because they want reuse for one application, but not for another.

Maybe we should just make "reuse" a devservice option, and make it opt-in? That way we could possibly detect reuse and skip init scripts. We could even offer more than two options: reuse=false, reuse=as-is (no init scripts whatsoever), reuse=reinit (reuse the container, but execute init scripts).

yrodiere commented 10 months ago

There is a protected method in GenericContainer that we can override to know if the container was reused or not:

    protected void containerIsStarted(InspectContainerResponse containerInfo, boolean reused) {

FWIW there's this code in JdbcDatabaseContainer (the base class for PostgreSQLContainer):

    protected void containerIsStarted(InspectContainerResponse containerInfo) {
        this.logger().info("Container is started (JDBC URL: {})", this.getJdbcUrl());
        this.runInitScriptIfRequired();
    }

So... we should already skip init scripts. There's something more to this issue.

yrodiere commented 10 months ago

There is a protected method in GenericContainer that we can override to know if the container was reused or not:

    protected void containerIsStarted(InspectContainerResponse containerInfo, boolean reused) {

FWIW there's this code in JdbcDatabaseContainer (the base class for PostgreSQLContainer):

    protected void containerIsStarted(InspectContainerResponse containerInfo) {
        this.logger().info("Container is started (JDBC URL: {})", this.getJdbcUrl());
        this.runInitScriptIfRequired();
    }

So... we should already skip init scripts. There's something more to this issue.

:facepalm: It's not the same method, the one implemented in PostgreSQLContainer doesn't have a reused parameter. That explains the behavior, I guess.

On a related note, I created this to document the reuse feature: #37884 . Doesn't fix this issue though.