initial r2dbc mysql support #99

oshai · 2019-03-04T22:32:12Z

No description provided.

r2dbc-mysql/src/main/java/SimpleStatement.kt

codecov · 2019-03-04T22:38:06Z

Codecov Report

Merging #99 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #99   +/-   ##
=========================================
  Coverage     78.94%   78.94%           
  Complexity     1011     1011           
=========================================
  Files           258      258           
  Lines          3661     3661           
  Branches        487      487           
=========================================
  Hits           2890     2890           
  Misses          531      531           
  Partials        240      240

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dde3f6a...719cc7f. Read the comment docs.

build.gradle

mp911de

Great work and really excited to see this change happen. I wonder whether it would rather make sense to use Project Reactor as Reactor provides Mono.fromFuture(CompletableFuture) adapters and aligns, in general, better with Java 8 code.

As soon as there's support for parametrized statements, we should extend our r2dbc-client examples to include a MySQL example as well.

r2dbc-mysql/src/main/java/JasyncClientConnection.kt

r2dbc-mysql/src/main/java/SimpleStatement.kt

# Conflicts: # build.gradle

oshai · 2019-03-18T16:00:00Z

@mp911de I added prepared statement support. you're welcome to review it and if it looks ok I will merge it and release a version, so it can be tested externally.

mp911de

A great start, I left some comments on the PR.

r2dbc-mysql/src/main/java/JasyncMetadata.kt

mp911de · 2019-03-19T07:42:47Z

r2dbc-mysql/src/main/java/JasyncClientConnection.kt

+import java.util.function.Supplier
+import com.github.jasync.sql.db.Connection as JasyncConnection
+
+class JasyncClientConnection(private val jasyncConnection: JasyncConnection) : Connection, Supplier<JasyncConnection> {


Do you have support for MySQL's PING? Adding this functionality here would be a preparation for r2dbc/r2dbc-spi#54.

the connection itself don't have ping, but there is a connection factory that knows how to test the connection:

jasync-sql/db-async-common/src/main/java/com/github/jasync/sql/db/pool/ConnectionFactory.kt

Line 79 in dde3f6a

override fun test(item: T): CompletableFuture<T> {

The idea behind is to check for e.g. pooling implementations whether the connection works without actually executing SQL.

no, there is no ping support, but there is a 'validate' check that don't do any network action:

jasync-sql/db-async-common/src/main/java/com/github/jasync/sql/db/pool/ConnectionFactory.kt

Line 49 in dde3f6a

override fun validate(item: T): Try<T> {

mp911de · 2019-03-19T07:44:43Z

r2dbc-mysql/src/main/java/JasyncRow.kt

+    override fun <T> get(identifier: Any, requestedType: Class<T>): T? {
+
+
+        if (identifier is String) {


Initially dropping the requested type (Class<T>) is okay, but make sure to consider the requested type at some point in time as this is part of the SPI contract, otherwise, the calling side will run into ClassCastException.

I'm not sure I understand the spi. should the returned value be of type requestedType?

Exactly. Each database type maps to a specific Java type (e.g. TINYINT to Byte, SMALLINT to Short). You should be able to retrieve a TINYINT as Integer.class or as Long.class. The caller requests the type Integer.class and expects the get(Any, Class<T>) method to return that specific type.

This conversion makes only sense for convertible types (Numbers, Dates (LocalDateTime to LocalTime or LocalDate) and converting values to String).

For a start, if you don't want to implement an advanced Codec mechanism, it would make sense to consider the mentioned three conversions and if the value is not null, to requestedType.cast(…) the value.

In contrast, get(Any) returns the native type (i.e. what happens right now, that the driver determines the most appropriate type).

as you mentioned the driver has "builtin" types right now depending on the db type. I am not what you mean to do for a start? just a cast? didn't understand the different cases you mentioned.

The method should do at least a cast.

I would also suggest adding the following conversion rules:

If a value is a Number and the requested type is a number, too, then convert the value to T using Number.intValue(), Number.longValue() methods.

If the requested type is String, then call toString() on the value.

If a value is LocalDate, LocalDateTime, or LocalTime and the requested type is one of these date types, then convert the value into the requested value using the provided methods on LocalDate etc (e.g. LocalDateTime.atStartOfDay()).

Does this make sense?

yes, I added it, please review again to see we are aligned. Note that for DateTime the lib is using joda so I had to do an extra conversion.

Looks pretty decent now. I wasn't aware that you build on top of Joda. For R2DBC we want to stay within the Java 8 type realm for primitives. The native get(Any) method should ideally return Java 8 types for dates. Retrieving a Joda type would be still possible via get(Any, Class<T>) when requesting the Joda LocalDateTime.

I will change that. we have joda as a legacy from the original driver.

r2dbc-mysql/src/main/java/JaysncResult.kt

r2dbc-mysql/src/main/java/SimpleStatement.kt

r2dbc-mysql/src/main/java/JaysncResult.kt

mirromutth · 2019-03-19T11:43:09Z

r2dbc-mysql/build.gradle

+    compile project(':mysql-async')
+    compile "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlin_version"
+    compile "io.r2dbc:r2dbc-spi:1.0.0.BUILD-SNAPSHOT"
+    implementation "io.reactivex.rxjava2:rxjava:2.2.7"


Is it compatible or something else? Otherwise using reactor-core may be a better choice because of more powerful and concise API.

I don't have a strong opinion about it, just followed what @mp911de did in this gist: https://gist.github.com/mp911de/9ea13939e8fd9a6b4ef138419f085715

I used RxJava 2 in the vert.x example as vert.x had no dependency on Project Reactor.
My choice for a Java 8 project would be Project Reactor, because it properly supports CompletableFuture (Java 8 types) and it has a Context that can be used to bind a connection to, if someone wants to properly manage a transaction.

I changed it.

mp911de · 2019-03-19T13:45:05Z

Did a first integration test pass with Spring Data R2DBC and things look quite for simple queries using MySQL 5.6.43. The latest MySQL version fails to connect with UnsupportedAuthenticationMethodException: Unknown authentication method -> 'caching_sha2_password'.

MySQLConnectionFactory mycf = new MySQLConnectionFactory(
		new Configuration("root", "localhost", 3306, "my-secret-pw", "mysql"));

JasyncConnectionFactory cf = new JasyncConnectionFactory(mycf);

DatabaseClient client = DatabaseClient.builder().connectionFactory(cf)
		.dataAccessStrategy(new DefaultReactiveDataAccessStrategy(PostgresDialect.INSTANCE)).build();

client.execute()
		.sql("SELECT * FROM help_topic")
		.map((row, rowMetadata) -> row.get("example"))
		.all()
		.doOnNext(it -> {
			System.out.println("Record: " + it);
		})
		.as(StepVerifier::create)
		.expectNextCount(585)
		.verifyComplete();

For the JasyncConnectionFactory (if it will remain in a MySQL related package), it would be nicer to configure it with just Configuration, but that's pure cosmetics to me.

oshai · 2019-03-19T13:54:53Z

caching_sha2_password is default auth since 8.0.4, but it can be changed on the db server: https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_default_authentication_plugin

Regarding configuration, let's not change it now but think of a consistent api later.

So I will merge this PR and let you know what it's initial released version.

initial r2dbc support

6212765

oshai mentioned this pull request Mar 4, 2019

Consider exposing Postgres and MySQL drivers through R2DBC #98

Closed

oshai commented Mar 4, 2019

View reviewed changes

r2dbc-mysql/src/main/java/SimpleStatement.kt Outdated Show resolved Hide resolved

andy-yx-chen reviewed Mar 4, 2019

View reviewed changes

build.gradle Show resolved Hide resolved

mp911de reviewed Mar 5, 2019

View reviewed changes

r2dbc-mysql/src/main/java/JasyncClientConnection.kt Outdated Show resolved Hide resolved

r2dbc-mysql/src/main/java/SimpleStatement.kt Outdated Show resolved Hide resolved

initial r2dbc support - fixes from review

0537fdc

oshai mentioned this pull request Mar 7, 2019

would you like to incorporate the work with jasync-sql? mirromutth/r2dbc-mysql#1

Closed

oshai added 2 commits March 18, 2019 17:37

Merge branch 'master' into r2dbc

de1fac4

# Conflicts: # build.gradle

add support for prepares statements to r2dbc-mysql

dd760f8

mp911de reviewed Mar 19, 2019

View reviewed changes

oshai added 2 commits March 19, 2019 12:12

add support for not in order param binding

b45e6d9

add support for string binding

3edc151

mirromutth reviewed Mar 19, 2019

View reviewed changes

oshai added 3 commits March 19, 2019 14:16

add explicit type conversions

734c2c2

improve date conversions

25880d2

changed to use reactor-core

719cc7f

oshai merged commit 4d4a812 into master Mar 19, 2019

oshai deleted the r2dbc branch March 19, 2019 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial r2dbc mysql support #99

initial r2dbc mysql support #99

oshai commented Mar 4, 2019

codecov bot commented Mar 4, 2019 •

edited

Loading

mp911de left a comment

oshai commented Mar 18, 2019

mp911de left a comment

mp911de Mar 19, 2019

oshai Mar 19, 2019

mp911de Mar 19, 2019

oshai Mar 19, 2019

mp911de Mar 19, 2019

oshai Mar 19, 2019

mp911de Mar 19, 2019

oshai Mar 19, 2019

mp911de Mar 19, 2019

oshai Mar 19, 2019

mp911de Mar 19, 2019

oshai Mar 19, 2019

mirromutth Mar 19, 2019

oshai Mar 19, 2019

mp911de Mar 19, 2019

oshai Mar 19, 2019

mp911de commented Mar 19, 2019

oshai commented Mar 19, 2019

		override fun <T> get(identifier: Any, requestedType: Class<T>): T? {


		if (identifier is String) {

initial r2dbc mysql support #99

initial r2dbc mysql support #99

Conversation

oshai commented Mar 4, 2019

codecov bot commented Mar 4, 2019 • edited Loading

Codecov Report

mp911de left a comment

Choose a reason for hiding this comment

oshai commented Mar 18, 2019

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mp911de commented Mar 19, 2019

oshai commented Mar 19, 2019

codecov bot commented Mar 4, 2019 •

edited

Loading