R2dbc support

[lmyslinski]

I've recently decided to try R2DBC and noticed that TestContainers is currently only compatible with JDBC. I've decided to add the support for it, however there are lots of impactful decisions to be made so I'd like some feedback on the assumptions I've made. I've currently implemented a very basic support for Postgresql containers. Before I go any further I'd like to discuss the following topics:

• Packaging. So far it seems possible to reuse GenericContainer, so I've decided to work with that and created a new package equivalent to the org.testcontainers.jdbc. The existing container implementations are tightly coupled with JDBC, so it would be best to have a separate class for each R2DBC container. The container implementations would be prefixed with R2dbc and reside in the packages next to original JDBC versions.

• JDBC URI. As @mp911de has mentioned here and also here it would be desirable to not reimplement DataSource discovery based on string parsing. For the time being, I suggest a following approach:

  • A database container can only be created explicitly in a test
  • A container should expose an R2dbc object so that a test client can easily get access to the DB without having to create it on their own. We could also expose just the ConnectionFactory, but so far I see no benefit in this.
  • A container should also expose all the connection params (as a POJO or getters for specific properties) so that if there is a requirement to create a JDBC connection to the DB, they can do so.

When ConnectionFactory discovery is merged maybe we could use that.

• Code reuse. Current implementation of R2dbcDatabaseContainer is mostly copied from JdbcDatabaseContainer, and all concrete implementation would contain lots of duplicate code as well. To solve this we could either:
  • create another abstraction layer between R2dbcDatabaseContainer and GenericDatabaseContainer
  • pull as much code up into GenericDatabaseContainer as possible add R2dbc compatibility at that level (seems unlikely)
  • maybe create some helper classes for the common code

Looking forward to your opinions, I'm happy to be devastated in a review

[mp911de]

Awesome news. You might want to wait until the configuration/discoverability PR (r2dbc/r2dbc-spi#30) is merged. This will be an improvement to generic client bootstrap.

[lmyslinski]

Thanks, definitely will.

[kiview]

Hi @Humblehound
thanks for your PR and great to see, that your first GitHub PR is a Testcontainers PR 🙂

I have no experience with R2DBC yet, so I'd need to look into it first, but the feature itself is probably very desirable.

Right now I'm wondering, if we couldn't support JDBC and R2DBC in the same classes, just a different getter?

[lmyslinski]

Thanks :)

So that would totally be possible - I thought initially that it wouldn't work for the containers to use both JDBC and R2DBC at the same time and we would have to make the existing methods for getting DataSource throw an exception and immediately left the idea... but this is actually much simpler and should work with minimal impact on the existing design. The containers would use the existing infrastructure for setup/teardown etc and then just expose an R2DBC instance. Let me try that approach :)

[lmyslinski]

This proved to be much simpler than I expected - I've reverted previous commits and just exposed a ConnectionFactory from R2DBC. I can see only two issues left:

• I needed to add the milestone repository to each module for the project to compile
• The naming for all DB containers and packages includes jdbc which is slightly misleading

If you are ok with both I'd add missing documentation

[mp911de]

Connection Factory Discovery has now landed in R2DBC. This way, using code is not required to touch driver classes but rather can declare options that are used to determine an appropriate driver:

ConnectionFactoryOptions options = builder()
    .option(DRIVER, "mssql") // alternative: postgresql
    .option(HOST, "…")
    .option(PORT, …)
    .option(USER, "…")
    .option(PASSWORD, "…")
    .option(DATABASE, "…")
    .build();

ConnectionFactory connectionFactory = ConnectionFactories.get(options);

Mono<Connection> connectionMono = factory.create();

[lmyslinski]

While this syntax is easier to use for the end user, here I think that using specific implementations would be better. The ConnectionFactory needs to be returned for each driver:

            @Override
	    public ConnectionFactory getR2dbcConnectionFactory() {
	        return new PostgresqlConnectionFactory(PostgresqlConnectionConfiguration.builder()
	            .host(getContainerIpAddress())
	            .database(databaseName)
	            .password(password)
	            .username(username)
	            .port(getFirstMappedPort())
	            .build());
	    }

If we were to use ConnectionFactoryOptions we would be getting rid of type safety. Also we need to include the dependency on specific driver implementation in each DB module anyway.

[nebhale]

@Humblehound I’m not sure you lose any type safety with ConnectionFactoryOptions. The option values are all strongly typed (String, Integer, etc.) and in your example, you’re returning the interface type ConnectionFactory rather than PostgresqlConnectionFactory.

[lmyslinski]

@nebhale Indeed, but here I just don't see a benefit in passing a string containing a driver name to the ConnectionFactoryOptions so that it can determine what driver to use, when we know exactly what driver we need to use in each case. This would've been very useful if we needed to determine the driver in the generic container, but because we need to include a dependency on a specific driver for each supported DB module, we might as wall explicitly specify which driver to use. It just looks to me as a less complex solution as a whole. Unless I'm missing something here, like I mentioned I'm not very familiar with R2dbc source code.

[lmyslinski]

But of course, it's your project so I'm happy to refactor to ConnectionFactoryOptions

[nebhale]

No, no I didn't want to be strongly one way or the other 😄 I just wanted to clarify the lack of type safety. What I think @mp911de was getting at is that using ConnectionFactories would allow you to write a single implementation for all of the configuration (shared constants for host, port, user, password, and database) and then the only set driver on a per-type basis. If that level of sharing isn't desired then the driver-specific types are perfectly adequate.

[bsideup]

@Humblehound @nebhale I think the best end goal we could achieve is supporting R2DBC URI (like we do with JDBC) and implementing a fake, proxying R2DBC driver inside Testcontainers.

I don't think it makes sense to add R2DBC connection factory getter to the containers (because it is trivial to do on the user's side).

Also, R2DBC is not available in Maven Central yet and we can't depend on it directly, only with Gradle's compileOnly linking.

[nebhale]

Also, R2DBC is not available in Maven Central yet and we can't depend on it directly, only with Gradle's compileOnly linking.

Already discussing internally.

[kiview]

@bsideup

I don't think it makes sense to add R2DBC connection factory getter to the containers (because it is trivial to do on the user's side).

We do provide a getJdbcUrl() method which is also constructed in a trivial way, so the idea of having a getR2dbcConnectionFactory() method isn't that far fetched.

However, I'm not to keen to having ConnectionFactory or ConnectionFactoryOptions in our public API if it's not really necessary.
Are there some other (more generic) ways to construct ConnectionFactoryOptions, like e.g. a map?

[lmyslinski]

I've refactored the code into a single module with support for postgres and mssql.

[lmyslinski]

I believe I've resolved all the conflicts and done all requested changes. I've also added a sample doc section. Anything else left to improve?

[lmyslinski]

@bsideup @kiview I'd appreciate some feedback / action, so that this PR doesn't hang here forever. Thanks alot

[bsideup]

FYI r2dbc/r2dbc-spi#37 was merged and now we can finally continue with the R2DBC support 🎉

[mp911de]

R2DBC 0.8.0.M8 is released and so URL formats are stable.

R2DBC Javadoc: https://r2dbc.io/spec/0.8.0.M8/api/

[rnorth]

@mp911de, @kiview and I met and discussed a couple of weeks ago. Sorry for the late writeup, but what we discussed was a plan to:

• With the new milestone release of R2DBC, introduce a String getR2dbcUrl() method for the existing database container classes. As a simple string getter, we'd have no binary dependency on R2DBC, so this would be straightforward to add to our existing modules. (@bsideup I know you have some thoughts on this - let's discuss)

• Later, we'll do a separate piece of work to implement a proxying driver implementation, similar to our ContainerDatabaseDriver class and conforming to a R2DBC SPI.

The latter would have a binary dependency on R2DBC and thus belongs in a new module.

As such, I think this particular PR can be trimmed a lot or replaced, unfortunately @Humblehound. Thank you for sparking the conversations, and sorry it's taken a long time so far.

[lmyslinski]

No problem, I'm glad to have the problem resolved one way or another.

[daggerok]

Hello, so what about testing r2dbc spring-boot apps with postgres and testcontainers?
is there some progress?
Anybody, please let me some link to that kind of repo with worked setup.

Thanks