Support mapping volumes in Dev Service container based databases

[Sgitario]

This is a very useful feature that allows mapping volumes in the Dev Service containers.

For example: to keep the Postgres data folder in the localsystem and hence have the data persistently.

Fix #30595

[geoand]

I think this is a great feature!

But I'll defer reviewing to @yrodiere since it involves the Datasource code

[yrodiere]

Thanks.

The feature looks nice indeed, but there's some danger involved as Hibernate ORM will clear the database on startup by default. So if you mount a volume on your devservice, and don't take care to configure Hibernate ORM appropriately, you will lose data.

Granted, I don't expect any sensible person to mount valuable data there, but still, I think some more documentation is in order:

1. Clarify in the javadoc of that new configuration property that data contained in mounted volumes may be altered by the application in dev mode (and tests!), and may be completely wiped out.
2. Maybe add a subsection with an example in the databases-dev-services documentation, explaining in more details the risks (in particular with Hibernate ORM, but probably also Flyway, ...)? For ORM, essentially if you don't use Flyway/Liquibase and don't set quarkus.hibernate-orm.database.generation your devservice schema and data will be wiped out on startup; if you use Flyway/Liquibase or set quarkus.hibernate-orm.database.generation to none or validate you're safe.

[stuartwdouglas]

The code LGTM, but it would be great to see some docs with real world (e.g. postgres and mysql) examples of how you actually do this. I know you could figure it out by reading the relevant containers docs but a copy and paste example would be way more user friendly imho.

[Sgitario]

Thanks for your suggestions! I will be working on this.

[Sgitario]

PR updated with more documentation and a couple of examples. I verified each of these examples using the following tests:

public class DevServicesPostgresqlDatasourceWithFileVolumeTestCase {

    @RegisterExtension
    static QuarkusUnitTest test = new QuarkusUnitTest()
            .overrideConfigKey("quarkus.datasource.db-kind", "postgresql")
            .overrideConfigKey("quarkus.datasource.devservices.volumes.\"/home/jcarvaja/sources/tmp/data\"",
                    "/var/lib/postgresql/data");

    @Inject
    DataSource ds;

    // I executed this test to create my database schema and the data, and then I disabled to only execute the following one.
    @Disabled 
    @Test
    public void create() throws Exception {
        try (Connection con = ds.getConnection()) {
            con.createStatement()
                    .execute("CREATE TABLE PERSON (name VARCHAR(50) PRIMARY KEY)");
            con.createStatement()
                    .execute("INSERT INTO PERSON (name) VALUES ('JOHN')");
        }
    }

    // In the second round, this test passed!
    @Test
    public void testDatasource() throws Exception {
        String name = "";
        try (Connection con = ds.getConnection();
                CallableStatement cs = con.prepareCall("SELECT name FROM PERSON");
                ResultSet rs = cs.executeQuery()) {
            if (rs.next()) {
                name = rs.getString(1);
            }
        }
        assertEquals("JOHN", name, "The volume was not properly mounted");
    }
}

And for MySQL:

public class DevServicesMySqlDatasourceWithFileVolumeTestCase {

    @RegisterExtension
    static QuarkusUnitTest test = new QuarkusUnitTest()
            .overrideConfigKey("quarkus.datasource.db-kind", "mysql")
            .overrideConfigKey("quarkus.datasource.devservices.volumes.\"/home/jcarvaja/sources/tmp/mysqldata\"",
                    "/var/lib/mysql");

    @Inject
    DataSource ds;

    // I executed this test to create my database schema and the data, and then I disabled to only execute the following one.
    @Disabled 
    @Test
    public void create() throws Exception {
        try (Connection con = ds.getConnection()) {
            con.createStatement()
                    .execute("CREATE TABLE PERSON (name VARCHAR(50) PRIMARY KEY)");
            con.createStatement()
                    .execute("INSERT INTO PERSON (name) VALUES ('JOHN')");
        }
    }

    // In the second round, this test passed!
    @Test
    public void testDatasource() throws Exception {
        String name = "";
        try (Connection con = ds.getConnection();
                CallableStatement cs = con.prepareCall("SELECT name FROM PERSON");
                ResultSet rs = cs.executeQuery()) {
            if (rs.next()) {
                name = rs.getString(1);
            }
        }
        assertEquals("JOHN", name, "The volume was not properly mounted");
    }
}

Note that to make Postgresql worked, I had to modify the log waiting logic because of one issue in Testcontainers side.

[yrodiere]

Thanks! A few suggestions below, mostly about wording.

[yrodiere]

Maybe add some code to throw an exception if that happens?

[Sgitario]

There are other properties that are container-based and that do not have this exception, so I would not over-complicate the logic here. We can always add it if it's really necessary.

[yrodiere]

Note you could probably take advantage of QuarkusDevModeTest to test that data is persisted upon restart... Though I don't know how to force a restart, except by modifying a file like in io.quarkus.hibernate.orm.HibernateHotReloadTestCase#testImportSqlWithContinuousTesting.

[Sgitario]

This test is not mapping the db data, just using a volume where there is a script.
Maybe we could add another test case to verify this use case using QuarkusDevModeTest (actually, I have these tests in another repository because I wanted to be sure about it), but I don't think it's really necessary to be covered here.

[stuartwdouglas]

The container is not restarted on hot reload, so testing by modifying the file does not really prove anything with regards to how container volumes are handled.

[yrodiere]

Right. We'd need to modify application.properties and change very specific configuration properties to trigger a restart:

quarkus/extensions/datasource/deployment/src/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java

Lines 75 to 101 in 5e78877

	boolean restartRequired = false;
	if (!restartRequired) {
	for (Map.Entry<String, String> entry : cachedProperties.entrySet()) {
	if (!Objects.equals(entry.getValue(),
	trim(ConfigProvider.getConfig().getOptionalValue(entry.getKey(), String.class).orElse(null)))) {
	restartRequired = true;
	break;
	}
	}
	}
	if (!restartRequired) {
	//devservices properties may have been added
	for (var name : ConfigProvider.getConfig().getPropertyNames()) {
	if (name.startsWith("quarkus.datasource.") && name.contains(".devservices.")
	&& !cachedProperties.containsKey(name) && !EXCLUDED_PROPERTIES.contains(name)) {
	restartRequired = true;
	break;
	}
	}
	}
	if (!restartRequired) {
	for (RunningDevService database : databases) {
	devServicesResultBuildItemBuildProducer.produce(database.toBuildItem());
	}
	// keep the previous behaviour of producing DevServicesDatasourceResultBuildItem only when the devservices first starts.
	return null;
	}

[Sgitario]

I think this is out of the scope of this pull request.

[yrodiere]

I think testing the features we document in a PR ("is handy [...] to preserve database data and reuse it after an application restart") is very much in scope of that PR.
But I'll grant you this opinion isn't widely shared.

[github-actions]

🙈 The PR is closed and the preview is expired.

[Sgitario]

I think the failures are unrelated to these changes.
Would you mind having another round to review this PR? @yrodiere @stuartwdouglas

[yrodiere]

I think the feature is ready, and it's nice. Thanks for that.

I also think the tests you ran manually (and posted somewhere in a comment) should be automated and included in this PR, because otherwise they'll never make it into the code base and then we won't notice regressions.

That being said, feel free to merge if you don't want to work on those tests.

[quarkus-bot]

Failing Jobs - Building 00625d0

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 Windows	Build	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 19

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: integration-tests/mongodb-panache 

📦 integration-tests/mongodb-panache

✖ io.quarkus.it.mongodb.panache.reactive.ReactiveMongodbPanacheResourceTest.testMoreRepositoryFunctionalities line 351 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

[Sgitario]

I think the feature is ready, and it's nice. Thanks for that.

I also think the tests you ran manually (and posted somewhere in a comment) should be automated and included in this PR, because otherwise they'll never make it into the code base and then we won't notice regressions.

That being said, feel free to merge if you don't want to work on those tests.

I feel that the example only exposes a very handy use case and yes, ideally it should be covered by tests, but taking into account the complexity, let's merge it as it is and we can add tests if required in the future.

The test failures are unrelated. Merging