-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add R2DBC support #2545
Add R2DBC support #2545
Conversation
|
||
@Override | ||
public void cancel() { | ||
s.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be a case when
future just completed ->
void enter() { publisher.subscribe(this); } executed but subscription may be delivered later (See https://github.com/reactive-streams/reactive-streams-jvm/issues/486 for the clarification)
so the next cancel which is waiting on the synchronized block will end-up with NPE.
The simples solution for that is to add flag and synchronization between request cancel and onsubscriber to ensure we are not ending up with NPE if the subscription has not arrived yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
|
||
@Override | ||
public void onSubscribe(Subscription s) { | ||
this.s = s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we have to have a flag, e.g (canceled) and if so, immediately call s.cancel on the given one
@Override | ||
public void onSubscribe(Subscription s) { | ||
this.s = s; | ||
s.request(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mp911de does the connection factory absolutely guarantees a Publisher
with at most one onNext(Connection)
? if not, @bsideup it might be worth it to consider guarding against flux-like publishers by adding a boolean guard in onNext
(doesn't even have to be a volatile, since only onNext
would read/write it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does (checked with Mark), yes. See the design notes in the Javadoc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was unclear to me that comment was about the factory, since it mentions ConnectionPublisher
(the current class). I would instead phrase it as Publisher<Connection>
provided by R2DBC / the connectionFactory
is guaranteed to emit at most one item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connectionFactory
implements single element or failure semantics. Completion without an element or emitting multiple items violates the spec. It's a cold publisher that should allow multiple subscriptions.
newState.enter(); | ||
} | ||
|
||
abstract class SubscriptionState implements Subscription { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that pattern was a bit harder for me to follow because of the habit of seeing Subscription
as self-sufficient and thus aggressively guarded, but after review it looks correct. I feel it wouldn't be too hard to collapse the various SubscriptionState
into the StateMachineSubscription
itself and deal with int/enum based states for transitions, but eh as long as it is correct...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at it! FYI I just fixed a race reported by @OlegDokuka:
d3409df
Right now, Is it now possible to use the same container instance via URL from JDBC and R2DBC? The reuse of containers should be based on config options provided by both technologies. |
In JDBC, we use the JDBC URL string to identify the same container. r2dbc/r2dbc-spi#166 would be handy indeed.
No, not possible (since we can't "match" them). If one wants to reuse (not talking about the reusable containers feature, btw) the same DB with r2dbc and jdbc, they should create an instance themselves and use the programmatic way. |
Probably I miss some context. I understood you're already extracting bits from the JDBC URL. If those values match the ones from the R2DBC config, then it should be possible to match those.
Many utilities (Flyway, Liquibase) do not support R2DBC. When bootstrapping a database, the schema becomes essential. Launching a single container programmatically and wiring it up introduces additional burden on frameworks that aid integration testing and advertise URL-based Testcontainer integration. |
We don't, actually. We take the original string and use it as a key in the testcontainers-java/modules/jdbc/src/main/java/org/testcontainers/jdbc/ContainerDatabaseDriver.java Lines 78 to 81 in a09f4d5
|
What shall we do for docs? It feels to me like we should break apart https://www.testcontainers.org/modules/databases/ and have a new page for each of the JDBC and R2DBC general concepts, plus also add updates to the MySQL, MariaDB and PostgreSQL module docs pages, as they're the places people probably want to look... WDYT? |
@rnorth oh yes, I will add the docs (to this PR) once we agree on the API 👍 |
import org.testcontainers.r2dbc.R2DBCDatabaseContainer; | ||
import org.testcontainers.r2dbc.R2DBCDatabaseContainerProvider; | ||
|
||
@AutoService(R2DBCDatabaseContainerProvider.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah, TIL about @AutoService
. I like it.
For the sake of consistency, we should probably propagate this out to everywhere else where we've currently hand-written a service descriptor file. This could be a subsequent PR, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mostly wondering about some code reuse for the tests. But we can refactor this later of course.
|
||
import static org.junit.Assert.*; | ||
|
||
public class MariaDBR2DBCDatabaseContainerTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if we should prefer an approach similar to our jdbc-test
module, that runs the same test in a parameterized way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I was assuming something like this actually 😬
Still, we could maybe extract an abstract base test class or something similar in a later change, to reduced code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we can split it per-module and have some common base test classes to keep things neat. That's how I'd started with my draft splitting of jdbc-test
, and works nicely.
modules/r2dbc/src/main/resources/META-INF/services/io.r2dbc.spi.ConnectionFactoryProvider
Outdated
Show resolved
Hide resolved
# Conflicts: # modules/mariadb/build.gradle
Co-Authored-By: Richard North <rich.north@gmail.com>
Co-Authored-By: Richard North <rich.north@gmail.com>
Co-Authored-By: Richard North <rich.north@gmail.com>
This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉 Thanks for the contribution! |
This PR adds support for [R2DBC](https://r2dbc.io). ## URL support The URL support is similar to the JDBC's, but: 1. Does not reuse containers by URL - it is unclear yet how to "hash" `ConnectionFactoryOptions` 1. The image tag must be explicitly provided via `TC_IMAGE_TAG` r2dbc option 1. TODO: daemon mode 1. TODO: alias support (see #4) ## Example usage The user **must add the `org.testcontainers:r2dbc` module** - unlike JDBC, we use `compileOnly` dependency on R2DBC, since it is a 3rd party dependency. Once both the `r2dbc` and database's module are added, one can use: ```java String url = "r2dbc:tc:postgresql:///db?TC_IMAGE_TAG=10-alpine"; ConnectionFactory connectionFactory = ConnectionFactories.get(url); ``` ## Programmatic access For those who start containers manually and want to obtain `ConnectionFactoryOptions` for already started containers, we provide a helper static method: ```java try (PostgreSQLContainer<?> container = new PostgreSQLContainer<>()) { container.start(); ConnectionFactory connectionFactory = ConnectionFactories.get( PostgreSQLR2DBCDatabaseContainer.getOptions(container) ); } ```
This PR adds support for R2DBC.
URL support
The URL support is similar to the JDBC's, but:
ConnectionFactoryOptions
TC_IMAGE_TAG
r2dbc optionExample usage
The user must add the
org.testcontainers:r2dbc
module - unlike JDBC, we usecompileOnly
dependency on R2DBC, since it is a 3rd party dependency.Once both the
r2dbc
and database's module are added, one can use:Programmatic access
For those who start containers manually and want to obtain
ConnectionFactoryOptions
for already started containers, we can provide a helper static method:I decided to leave some generic way (e.g.
R2dbcContainers.getOptions(container);
) for future considerations because:instanceof
checks (or exposegetR2dbcDriverType()
-like method) onDatabaseContainer
postgres
taking precedence beforepostgis
and returning wrong factory/cc @mp911de