Backport the MS SQL Client to Vert.x 3.9.x

[cescoffier]

Just adapt the code from the 4.x branch (master) to work with 3.9.x.

Motivation:

There is a need to get the MS SQL client to work against 3.9. This PR is just adapting the code to follow the Vert.x 3.9 conventions.

[gavinking]

Oh, that's great!

[BillyYccc]

LGTM

[aguibert]

LGTM, thanks!

Can you also add this stage to the .travis.yml file though so we have test coverage right away and make sure that all tests are passing?

    - stage: test
      name: "MSSQL 2017-CU12"
      jdk: openjdk8
      script: mvn -q clean verify -B --projects vertx-sql-client,vertx-mssql-client

[pmlopes]

As a personal note, I am not currently in favor of this back-port because it is lacking some important features. These features will almost for sure let users down and we will get more maintenance burden on the backport, than focusing on getting the current master and 4.0.0 out of the door.

• [MSSQL]TLS support #605 - without this, most likely you won't be able to use the client on a cloud environment such as Azure, or on perm where the application server and db server are not the same physical machine.
• [MSSQL]Cursor support #606 - missing cursors, will likely be a blocker for stored procedure users or "power users" that stream results from the db directly.
• [MSSQL]Batch Queries support #607 - this can be a blocker for ETL applications where batching is a common technique to address large ingestion of data.
• [MSSQL]datatype support #608 - A more generic issue the current code only supports a subset of the datatypes MSSQL has. Unlike pg and mysql where we have (if not all) data types handled.
• [MSSQL]Transaction support #610 - Most definitely a blocker, missing transactions will be a blocker unless users are just thinkering or PoC projects where data loss can be allowed/developing simple read only applications.

This back-port will create more maintenance work for the team which is not able to sustain this load. We could think of a back-port in the future if the expectations are clearly defined. I'd suggest as an alternative that downtream projects interested on this module to start using the SNAPSHOT builds on sonatype or the milestone releases instead.

[cescoffier]

The downstream cannot use Vert.x 4.0 milestone or snapshots before the end of the year. So, without a backport an alternative would need to be found. Le ven. 8 mai 2020 à 09:54, Paulo Lopes <[email protected]> a écrit :

…

***@***.**** commented on this pull request. As a personal note, I am not currently in favor of this back-port because it is lacking some important features. These features will almost for sure let users down and we will get more maintenance burden on the backport, than focusing on getting the current master and 4.0.0 out of the door. - #605 <#605> - without this, most likely you won't be able to use the client on a cloud environment such as Azure, or on perm where the application server and db server are not the same physical machine. - #606 <#606> - missing cursors, will likely be a blocker for stored procedure users or "power users" that stream results from the db directly. - #607 <#607> - this can be a blocker for ETL applications where batching is a common technique to address large ingestion of data. - #608 <#608> - A more generic issue the current code only supports a subset of the datatypes MSSQL has. Unlike pg and mysql where we have (if not all) data types handled. - #610 <#610> - Most definitely a blocker, missing transactions will be a blocker unless users are just thinkering or PoC projects where data loss can be allowed/developing simple read only applications. This back-port will create more maintenance work for the team which is not able to sustain this load. We could think of a back-port in the future if the expectations are clearly defined. I'd suggest as an alternative that downtream projects interested on this module to start using the SNAPSHOT builds on sonatype or the milestone releases instead. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#604 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADCG7J33RZKXXJI2MZFQJTRQO3ERANCNFSM4M2GRTUA> .

[gavinking]

@pmlopes are any of those issues blockers for typical usage of the Vert.x client under Hibernate Reactive and especially with Quarkus? (This use case is the motivation for the backport.)

It doesn't look to me like they are, though I'm not sure about #605.

I'd suggest as an alternative that downtream projects interested on this module to start using the SNAPSHOT builds on sonatype or the milestone releases instead.

That's not an option for Quarkus, and therefore also not an option for Hibernate Reactive.

[vietj]

@gavinking can you use test this branch to see how much it fits your current expectations ?

[gavinking]

@vietj I can try it out in standalone mode, I guess, though I'm not the right person to test the Quarkus scenario.

[gavinking]

Alright, so I tried to get a very basic test (SQLServerBasicTest) working on SQL Server using the 3.9.1 snapshot, and here's the result:

https://github.com/gavinking/hibernate-rx/tree/sqlserver

I can see a couple of queries for next sequence values executed and returning successfully, but then as soon as I try to run the first select or insert query, it hangs waiting for a response from the driver.

I have no clue what the cause might be: something to do with us not using with transactions? Something to do with the connection properties I'm using? Something else?

[vietj]

@gavinking can you make a reproducer case ? e.g you add a new test in the sql server tests and then we get an analysis from @BillyYccc

[gavinking]

Folks, I have simplified it down to the following:

package org.hibernate.rx;

import io.vertx.core.Vertx;
import io.vertx.ext.unit.Async;
import io.vertx.ext.unit.TestContext;
import io.vertx.ext.unit.junit.VertxUnitRunner;
import io.vertx.mssqlclient.MSSQLConnectOptions;
import io.vertx.mssqlclient.MSSQLPool;
import io.vertx.sqlclient.PoolOptions;
import org.junit.Test;
import org.junit.runner.RunWith;

@RunWith(VertxUnitRunner.class)
public class ConnectionTest {
	@Test
	public void testConnection(TestContext context) {
		MSSQLConnectOptions options = new MSSQLConnectOptions()
				.setHost( "localhost" )
				.setUser( "sa" )
				.setPassword( "reallyStrongPwd123" );
		MSSQLPool pool = MSSQLPool.pool( Vertx.vertx(), options, new PoolOptions() );
		Async async = context.async();
		pool.preparedQuery( "select * from INFORMATION_SCHEMA.SEQUENCES" ).execute(
				ar -> {
					System.out.println( ar.result().rowCount() );
					System.out.println("...done");
					async.complete();
				}
		);
		System.out.println("waiting...");
	}
}

This test hangs and never completes. Surely this must be something simple somehow related to my setup.

[gavinking]

But the thing that seems strange is that if I replace the query with:

select next value for hibernate_sequence

Then the test completes successfully. Which is why I wonder if this has something to do with txns.

[vietj]

so you mean that "select * from INFORMATION_SCHEMA.SEQUENCES" has a problem ?

[gavinking]

so you mean that "select * from INFORMATION_SCHEMA.SEQUENCES" has a problem ?

Yes, but any table will do. It's not specific to INFORMATION_SCHEMA.SEQUENCES.

[BillyYccc]

I have tried and found some of the data types in the table are not supported for now such as nvarchar.

[gavinking]

It's not just that. Honestly I'm just finding a whole lot of really strange behavior (queries that sometimes work and sometimes don't). But it's really hard to debug because I never get any error.

I think the root problem is that it shouldn't hang when an error occurs. It should send the error back to my callback and/or terminate the async operation. This doesn't seem to happen: I never get errors at all, no matter how broken my query is. I can send the query create to the database and it hangs.

[gavinking]

OK, so apart from the Big problem, which is the failure to report errors, it looks like the other major source of my pain is that it doesn't like inserting null into nullable columns.

Here's the test code I'm using to show this. Note that it uses testcontainers to create the db.

package org.hibernate.rx;

import io.vertx.core.Vertx;
import io.vertx.ext.unit.Async;
import io.vertx.ext.unit.TestContext;
import io.vertx.ext.unit.junit.VertxUnitRunner;
import io.vertx.mssqlclient.MSSQLConnectOptions;
import io.vertx.mssqlclient.MSSQLPool;
import io.vertx.sqlclient.PoolOptions;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.testcontainers.containers.MSSQLServerContainer;

@RunWith(VertxUnitRunner.class)
public class ConnectionTest {
	public static final MSSQLServerContainer<?> sqlserver = new MSSQLServerContainer<>()
			.withReuse( true );

	@Test
	public void testConnection(TestContext context) {
		sqlserver.start();
		System.out.println( sqlserver.getJdbcUrl() );
		System.out.println( sqlserver.getFirstMappedPort() );
		MSSQLConnectOptions options = new MSSQLConnectOptions()
				.setHost( "localhost" )
				.setPort( sqlserver.getFirstMappedPort() )
				.setUser( sqlserver.getUsername() )
				.setPassword( sqlserver.getPassword() );
		MSSQLPool pool = MSSQLPool.pool( Vertx.vertx(), options, new PoolOptions() );
		Async async = context.async();
		pool.preparedQuery( "create table WithNull (nullint int)" ).execute(
				ar1 -> {
					System.out.println( ar1.result().rowCount() );
					System.out.println("...inserting...");
					pool.preparedQuery("insert into WithNull (nullint) values (null)").execute(
						ar2 -> {
							System.out.println( ar2.result().rowCount() );
							System.out.println("...querying...");
							pool.preparedQuery("select * from WithNull").execute(
								ar3 -> {
									System.out.println( ar3.result().rowCount() );
									System.out.println("...done");
									sqlserver.stop();
									async.complete();
								}
							);
						}
					);
				}
		);
		System.out.println("creating table...");
	}
}

[BillyYccc]

I have create an issue for this, see #627.
Some unsupported behaviors will throw UnsupportedOperationException which may not be captured correctly by the callback, they should be improved and be logged as an error maybe.