feat!: Add support for r2dbc driver connections #2299

bog-walk · 2024-11-08T07:50:52Z

Description

Second draft
First draft: #2266

Summary of the change: Provide a concise summary of this PR. Describe the changes made in a single sentence or short paragraph.

Detailed description:

What: Detail what changes have been made in the PR.
Why: Explain the reasons behind the changes. Why were they necessary?
How: Describe how the changes were implemented, including any key aspects of the code modified or new features added.

Type of Change

Please mark the relevant options with an "X":

Bug fix
New feature
Documentation update

Updates/remove existing public API methods:

Is breaking change

Affected databases:

Checklist

Unit tests are in place
The build is green (including the Detekt check)
All public methods affected by my PR has up to date API docs
Documentation for my change is up to date

Related Issues

- Implement all API interfaces and abstract classes - Extract base logic from Database to new DatabaseApi and implement r2dbc versi> - Add dialect-specific metadata providers and example tests (Oracle and SQl Ser> are mostly not implemented) - Implement custom transaction manager, but need to decide on what to do with transaction (existing blocks).

- Add SqlTypeProvider and PropertyProvider objects to be held by a MetadataProv> instance representative of each supported database. - Add missing KDocs. - Add tests for all metadata queries.

- Add notes about restrictions related to Oracle, SQL Server, and PostgreSQL-NG

…odule

…ltApi - Introduce ResultApi interface that wraps actual database result - Introduce JdbcResult and R2dbcResult - Since only getting object by index and/or type is supported by R2DBC, all uses of ResultSet specific getType() functions in source code have been replaced by getObject()

- The test getKeywords() passes locally but occasionally fails on TC build

… plain sql - Set connection url as proprty of R2dbcDatabase so it can be accessed without retrieving metadata - Remove getUrl() from MetadataProvider and subclasses - Introduce Transaction.execQuery() that returns a Flow based on a transformation - Introduce Transaction.execQuery() that executes without transformation - Add unit tests (commented out as conflict between driver dependencies)

…ls to jdbc module - executeQuery(), executeBatch(), and executeMultiple() now suspend, as do many internal or private functions - Transaction.exec() variants should also suspend as they are used frequently, but this is problematic for jdbc and trickling up - Any use of exec() or Statement.execute() in vendor dialects has been moved to jdbc module - Since Statement.execute() is used heavily in Queries.kt, creation of each statement has been extracted to a common StatementBuilder object. That way execution can be separated by introducing suspend variants of insert/update/delete etc. - A decision needs to be made about query statements and SchemaUtils and all other internal uses. - Marked all potential breaking changes and places that may need suspend

…rsions.

…tionManager

e5l · 2024-11-08T08:59:50Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/AbstractQuery.kt


-/** Base class representing an SQL query that returns a [ResultSet] when executed. */
+/** Base class representing an SQL query that returns a result when executed. */


I would keep [ResultApi] reference in KDoc

obabichevjb · 2024-11-12T07:58:09Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/api/ResultApi.kt

+import kotlinx.coroutines.flow.Flow
+
+/** Base class for wrapping data generated by executing a statement that queries the database. */
+interface ResultApi {


Minor: I'd prefer to avoid Api in class/interface names. Probably something like QueryResult/StatementResult, but it's minor

obabichevjb · 2024-11-12T08:02:09Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/api/ResultApi.kt

+/** Base class for wrapping data generated by executing a statement that queries the database. */
+interface ResultApi {
+    /** The actual data object returned by the database after statement execution. */
+    val result: Any


I haven't checked yet how it's used, but from api perspective it could be better to remove that from API. Here is the interface with common methods for any type of connection, and here we say that every implementation must provide result field with something.

I would say that such a field can be public but inside implementation, so it will have more specific type, and will not be in common interface. If the user of implementation knows that it should be there and he wants to get it he can always cast his entity to specific type.

obabichevjb · 2024-11-12T08:15:37Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/api/ResultApi.kt

+    fun <T> collectAll(transform: (ResultApi) -> T): Flow<T>
+
+    /** Performs the specified [transform] on the first element of the result, returned as a [Flow]. */
+    fun <T> collectSingle(transform: (ResultApi) -> T): Flow<T>


As I can see there no usages of collectSingle() in the project. Would it throw exceptions if there are not rows anymore?

As I know we have no way to check if the current row is the last one (at least isLast in jdbc is not recommended for usage because it could be too expensive if I'm right), so there is no way to call this method safely? Probably I miss somthing, but I don't see what now

obabichevjb · 2024-11-12T08:19:54Z

exposed-r2dbc/src/main/kotlin/org/jetbrains/exposed/sql/statements/r2dbc/R2dbcResult.kt

+    override val result: Result
+) : ResultApi {
+    private var currentRow: Row? = null
+    private var currentRowMetadata: RowMetadata? = null


Minor: to avoid problems with synchronization between currentRow and currentRowMetadata it could be moved to one field like currentEntry: R2dbcEntry(row: Row, metadata: RowMetadata)

obabichevjb · 2024-11-12T08:36:52Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnType.kt

-            value is Array<*> -> recursiveValueFromDB(value, dimensions) as R?
+        return when (value) {
+            is Array<*> -> recursiveValueFromDB(value, dimensions) as R?
+            is java.sql.Array -> recursiveValueFromDB(value.array, dimensions) as R?


I have a PR with this fix: #2296, it also contains a test for it (probably your PR also but anyway), just to say that here can be a merge conflict later

obabichevjb · 2024-11-12T11:29:29Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Transaction.kt

@@ -51,7 +52,7 @@ open class UserDataHolder {
 open class Transaction(
    private val transactionImpl: TransactionInterface
 ) : UserDataHolder(), TransactionInterface by transactionImpl {
-    final override val db: Database = transactionImpl.db
+    final override val db: DatabaseApi = transactionImpl.db


Also Api in class name looks quite meaningless.

I see that it's kind of a way to save back compatibility. And it could be logic that should be Database, R2dbcDatabase, and JdbcDatabase, but people who already use Database would be surpised that now they need to use JdbcDatabse.

Database uses ExposedConnection, so probably it would be better to name DatabaseApi as ExposedDatabase also

obabichevjb · 2024-11-12T12:00:38Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Transaction.kt

+        var answer: Pair<T?, List<StatementContext>> = null to emptyList()
+        stmt::executeIn.startCoroutine(
+            this,
+            object : Continuation<Pair<T?, List<StatementContext>>> {


That part looks quite suspicious of course, I'd discuss it separately.

Am I right that's the way to perform asynchronous code inside non-suspend function, and for jdbc it would be executed simply synchronouse?

obabichevjb · 2024-11-12T12:10:22Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/StatementBuilder.kt

+}
+
+/** Builder object for creating SQL statements. */
+object StatementBuilder : IStatementBuilder {


Not sure if we often use invocable objects across the project as a pattern. For me it looks like kind of shortcut for creating statements instead of calling constructors directly, but it means that we should not forget to add new statements here, and not forget to use that object instead of statements directly... I have a feeling that maintaining that object would be not easy

obabichevjb · 2024-11-12T12:34:54Z

exposed-crypt/build.gradle.kts

@@ -19,6 +19,7 @@ dependencies {
    api(libs.spring.security.crypto)
    testImplementation(project(":exposed-dao"))
    testImplementation(project(":exposed-tests"))
+    testImplementation(project(":exposed-jdbc"))


Is it intentional, looks like I can remove and build the module without problem

obabichevjb · 2024-11-12T12:36:30Z

exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/EntityCache.kt

+ *
+ * None of the functions rely directly on the underlying driver.
+ */
+internal object TableUtils : SchemaUtilityApi() {


Should it be also inside core's TableUtils?

bog-walk added 13 commits November 7, 2024 10:03

Refactor vendors package to hold metadata properties and queries

4608329

- Add SqlTypeProvider and PropertyProvider objects to be held by a MetadataProv> instance representative of each supported database. - Add missing KDocs. - Add tests for all metadata queries.

Configure initial setup of exposed-tests TestDB constants

b983aa6

- Add notes about restrictions related to Oracle, SQL Server, and PostgreSQL-NG

Add drop column support API and remove pgJDBC-ng support from r2dbc m…

2b2e101

…odule

Exclude metadata test for SQL keywords

b2a0659

- The test getKeywords() passes locally but occasionally fails on TC build

Replace ResultSet with ResultApi in all placesexcept Transaction.exec()

5ce46a0

Fix detekt issues and revert internal naming

71230af

Fix MySQL constraint test

3cbc6e9

Separate SchemaUtils and MigrationUtils in suspend and non-suspend ve…

6cdaea8

…rsions.

Separate into driver modules: Transaction, Queries, Database, Transac…

34a7fe7

…tionManager

bog-walk marked this pull request as draft November 8, 2024 07:51

e5l self-requested a review November 8, 2024 08:58

e5l reviewed Nov 8, 2024

View reviewed changes

e5l requested a review from obabichevjb November 8, 2024 10:10

obabichevjb reviewed Nov 12, 2024

View reviewed changes

bog-walk closed this Dec 9, 2024

bog-walk deleted the bog-walk/r2dbc-support branch December 9, 2024 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Add support for r2dbc driver connections #2299

feat!: Add support for r2dbc driver connections #2299

bog-walk commented Nov 8, 2024 •

edited

Loading

e5l Nov 8, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024

obabichevjb Nov 12, 2024


		/** Base class representing an SQL query that returns a [ResultSet] when executed. */
		/** Base class representing an SQL query that returns a result when executed. */

feat!: Add support for r2dbc driver connections #2299

feat!: Add support for r2dbc driver connections #2299

Conversation

bog-walk commented Nov 8, 2024 • edited Loading

Description

Type of Change

Checklist

Related Issues

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

bog-walk commented Nov 8, 2024 •

edited

Loading