Spring Add support for R2DBC

This commit introduces support for R2DBC ("Reactive Relational Database Connectivity") with custom ConnectionFactory implementations, a functional DatabaseClient for SQL execution, transaction management, a bind marker abstraction database initialization utilities and exception translation.

Open issues:

• [X] Discuss reuse of ScriptUtils and friends :: to be addressed in #25275.
• [ ] Potential reuse of named parameters utilities
• [x] Documentation (should it go side by side with the JDBC documentation)

Comment From: rstoyanchev

Documentation (should it go side by side with the JDBC documentation)

The Data Access section would be a logical place for it. A short section could also be added under WebFlux mostly for discoverability, similar to how Spring MVC has a section about REST clients which are otherwise in other sections.

Comment From: jhoeller

A few notes from a first pass of mine: * I'm not sure we really need the connectionfactory.lookup package, its JDBC equivalent was mostly introduced for JPA setup purposes. * We might be able to trim the connectionfactory handles and also the connectionfactory.init stuff a bit, not needing some of the flexibility that the JDBC equivalents have there. * I wonder whether the exception translation abstraction in the support package even needs to be an abstraction here since we effectively only really need the exception subclass check. * Since connectionfactory is a bit of a mouthful for a package name and support is so light, I propose to collapse those two into a unified support package, simply with connectionfactory root merging into support root and connectionfactory.init becoming support.init.

Comment From: sbrannen

Discuss reuse of ScriptUtils and friends

Aside from missing original @author tags (which is a very minor documentation detail), that is indeed a lot of copy-n-paste there. A bug fix in one of those will require a bug fix in the parallel implementation. (note: we used to have the same problem when some of that support also lived duplicated in spring-test) So perhaps we should look into a way to avoid the duplication. Though, we don't currently have a general purpose "data access" module in which that could reside. Plus, changing the packages would be a breaking change for existing applications. And... the standard implementation of course doesn't use reactive types. So coming up with a general purpose base implementation might be a big hurdle.

Comment From: jhoeller

Specifically about the connection handling business in connectionfactory, I don't see the need for the ConnectionHandle abstraction here, could simply always hold a Connection directly in ConnectionHolder. Its JDBC equivalent was primarily introduced for JPA connection borrowing.

Also, I wonder whether we need EE-style decoration of a transaction-aware ConnectionFactory here. This primarily makes sense for plain R2DBC access code that would transparently work with declarative transaction management; the question being whether that is a common scenario. For code that is Spring-aware to begin with, ConnectionFactoryUtils.getConnection does the job as well.

Comment From: jhoeller

A specific note on R2dbcTransactionManager: We got a similar JdbcTransactionManager in the jdbc.support package in 5.3 now, including exception translation which DataSourceTransactionManager doesn't have (and cannot depend on for package cycle reasons between jdbc.datasource and jdbc.support), so placing the R2DBC equivalent in r2dbc.support would be well aligned.

Talking about exception translation there, JdbcTransactionManager actually propagates a translated DataAccessException as-is, similar to JpaTransactionManager flush exceptions. R2dbcTransactionManager should follow that pattern, not wrapping a translated DataAccessException into a TransactionSystemException but rather only wrapping non-translated exceptions that way.

Comment From: mp911de

Thanks for having a look. Here are my thoughts on the mentioned items:

• connectionfactory.lookup: We've seen feature requests for a routing ConnectionFactory implementation (see spring-projects/spring-data-r2dbc#98), so following the JDBC design seemed the most appropriate choice to remain consistent.
• Most R2DBC drivers use R2dbcException subclasses to distinguish between exceptions. Falling back to SQLState-based exception translation seemed a viable alternative for those which do not follow the class-based approach.
• ScriptUtils and named parameter handling: Really, a lot of this code duplicates what's provided by spring-jdbc already. I would like to aim for an approach, where at least the common parts (script splitting, named parameter identification) would be available from a org.springframework.dao-like package so that we can remove the duplications. Clearly, execution-specifics such as methods returning a Mono cannot be unified.
• transaction-aware ConnectionFactory: Currently, we cannot properly answer this question. There are too little clients available. We've been in touch with users that use the R2DBC API directly and so a transaction-aware variant of ConnectionFactory provides value. Thinking a few steps ahead, R2DBC libraries would not be required to provide a Spring-specific customization on their side if a supplied ConnectionFactory already works according to transaction rules.
• R2dbcTransactionManager/ConnectionFactoryTransactionManager: We can move things around and align these with JdbcTransactionManager. We should follow at least the exception wrapping/translation rules.
• Regarding the connectionfactory package, how about reducing its name to just connection following the JMS design?

In general, I'm happy to reduce the API surface and move things around for the parts that lived in JDBC only to cater for JPA reasons.

Comment From: jhoeller

Shortening r2dbc.connectionfactory to r2dbc.connection sounds like a fine option as well. I wonder what to do with the very light r2dbc.support package though, we could also try to get rid of that one completely, moving R2dbcExceptionTranslator to r2dbc.connection (where it fits usage-wise).

As for the SQL state exception parsing, I'm rather strongly aiming to avoid this here. Over there in JDBC land, it is an old and hard-coded arrangement of very specific vendor codes, largely outlived by the JDBC 4 exception subclass support for a long time already. Since this is just about throwing more specific exception subclasses, it's not too bad if a particular driver does not distinguish much at all. I do not want our code to make up for a lack of sophistication in the drivers here... again. The appropriate target for a user to request finer-grained exception translation is the driver vendor.

Collapsing R2dbcExceptionTranslator into an interface plus single default implementation might therefore be worthwhile since other benefits of the equivalent JDBC separation - JDBC 4 runtime dependencies (for the SQLException subclasses) vs JDBC 2/3 compatibility, or the customizable error code files - don't really show here and are also outlived on the JDBC side already. For R2DBC, we could arguably have a single default implementation checking the exception subclasses and leave the rest up to the drivers. Even further, we could turn it into a translation utility like with our JmsUtils.convertJmsAccessException and Hibernate SessionFactoryUtils.convertHibernateAccessException methods, with no SPI interface and no configuration options for users, just doing internal exception subclass adaptation.

Our JDBC support was very maintenance intensive over the years, in particular due to its many vendor-specific checks (also for parameter type determination, value extraction, stored procedure handling). We have a clean plate to start with here, let's keep it as streamlined as possible.

Comment From: jhoeller

Some further notes about the connection package: * SingleConnectionConnectionFactory could be named SingleConnectionFactory like with JMS. * SmartConnectionFactory is based on the old SmartDataSource.shouldClose idea which only really makes sense if avoiding a proxy is essential, e.g. for code that does a hard cast to OracleConnection. Since such hard casts are not expected here and common connection proxying always suppresses close calls anyway, I don't think we need this mechanism with R2DBC. * ConnectionProxy predates the JDBC 4 unwrap mechanism, and since R2DBC comes with a similar unwrap mechanism out of the box, I see no need for preserving this separate mechanism. * Going even further, I wonder whether we need a single connection factory for R2DBC at all. With JDBC, this is mostly for testing, and even there most people use a DriverManagerDataSource or a simple pool instead. I'm inclined to drop SingleConnectionConnectionFactory completely, along with SmartConnectionFactory and the DelegatingConnectionFactory base class. * Since I also expect ConnectionHandle and SimpleConnectionHandle to go away, the resulting connection package would be significantly lighter: just containing ConnectionFactoryUtils, ConnectionHolder, R2dbcTransactionManager, and potentially TransactionAwareConnectionFactoryProxy (incorporating the delegation parts directly). This would easily allow for putting the R2dbcExceptionTranslator mechanism into the same package, in particular when condensed to an interface plus single default implementation, or when turning it into a utility method in ConnectionFactoryUtils (as suggested above).

Comment From: mp911de

I applied the discussed changes. Let me know if there's anything missing.

I also experimented with the method naming of DatabaseClient. Switching execute(String sql) to sql(String sql) aligns nicely here. Renaming fetch(…) to execute(…) causes that the code becomes less readable. Example:

databaseClient.sql().execute().all()
databaseClient.sql().execute().first()

The fetch variant feels more natural in this context:

Flux<Map<String, Object>> flux = databaseClient.sql(…).fetch().all();
Flux<Map<T, Object>> flux = databaseClient.sql(…).map(…).all();
Mono<Void> then = databaseClient.sql(…).then();

I will add the documentation bits early next week.

Comment From: jhoeller

@mp911de This looks good to me so far, ready to get merged as a first cut.

@sbrannen Any immediate input on the connection.init package that we should address before the merge still? Seems a bit of a shame that we have to duplicate five exceptions there. Was there a specific need for diffentiating that much in the JDBC variant? I guess that's also related to the general question of ScriptUtils reuse...

Comment From: mp911de

FTR: I omitted the init package from the documentation for now until we're entirely happy with the init package.

Comment From: sbrannen

@sbrannen Any immediate input on the connection.init package that we should address before the merge still?

I'm reviewing it now and will get back to you.

Seems a bit of a shame that we have to duplicate five exceptions there. Was there a specific need for diffentiating that much in the JDBC variant?

Two of them were already in place since 3.0. When I reworked ScriptUtils, there was a need for two additional exceptions that I introduced in 4.0.3 along with the ScriptException base type to group them all together. So, it may seem like a lot, but I think it's reasonable, though indeed unfortunate that we have to duplicate all of them.

I guess that's also related to the general question of ScriptUtils reuse...

Yes, indeed: directly related.

Comment From: sbrannen

@sbrannen Any immediate input on the connection.init package that we should address before the merge still?

I don't think there's anything urgent that needs to be addressed before the initial merge.

However, I reviewed the status quo and came up with some topics for discussion.

• DatabasePopulatorUtils.execute(DatabasePopulator, ConnectionFactory) could likely be a default method in DatabasePopulator -- no need to copy that utils pattern from spring-jdbc.
• ResourceDatabasePopulator should support multiple commentPrefixes like in the JDBC variant.
• Javadoc for ResourceDatabasePopulator.setBlockCommentEndDelimiter(String) uses improper escaping with {@code } element.
• DatabasePopulator was originally only intended for "populating" a database (i.e., creating it), but the Javadoc points out how the use cases changed over time: "Strategy used to populate, initialize, or clean up a database." Thus, if we are creating something completely new for R2DBC, it may be a good idea to come up with a better name than DatabasePopulator, since cleanup often entails the opposite of "population".
• I think it would be worthwhile to take a shot at extracting the common code from ScriptUtils into something that resides in spring-tx so that spring-jdbc and spring-r2dbc could delegate to or extend that common functionality. The challenge, however, likely lies in the fact that ScriptUtils in spring-jdbc is closely coupled to the ScriptException hierarchy in org.springframework.jdbc.datasource.init, but perhaps we can come up with a way around that. Let's brainstorm.
• I also think we should investigate extracting common functionality from ResourceDatabasePopulator since the only differences between those two variants are the implementations of populate() and execute(). Maybe we need an abstraction that encapsulates the "resource-based configuration of scripts" since that is 90% of the code in those ResourceDatabasePopulator implementations -- then we could have "execution-specific" subtypes.

Comment From: jhoeller

Let's merge this after the 5.2.7 release tomorrow (which unfortunately keeps us busier than expected), maybe on Wednesday this week? We may then follow up with refinements on master right afterwards.

Comment From: jhoeller

@mp911de Since we seem to have a conflict in the dependency declarations, it'd be great if you could rebase your branch and update to Arabba-SR4 in preparation for a merge to master... I'd also appreciate a quick final review pass over it from your side before Sam and I take over :-)

Comment From: mp911de

Done. Rebased and ready for you to take over.

Comment From: sbrannen

This first cut for the R2DBC support has been merged into master in time for Spring Framework 5.3 M1.

Kudos to @mp911de and @christophstrobl!

Comment From: sbrannen

The ScriptUtils topic will be addressed in #25275.