Skip to content
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

WIP: PostgreSQL DSL #1456

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

WIP: PostgreSQL DSL #1456

wants to merge 7 commits into from

Conversation

stengvac
Copy link
Contributor

@stengvac stengvac commented Feb 15, 2022

PostgreSQL DSL
Still Work in progress: more tests, docs in README, kotlin docs etc, test insert with ON CONFLICT DO UPDATE ... .
Rather big Pull request I know, but changes in core were needed like a lot of them. If needed I can split it to smaller ones.

I would like to introduce into framework also DB specific DSL not only common one. Rough idea: Is to add gradle modules for each supported database with DB specific DSL. So developers can import DSL for DB they use. DB specific DSL will be in gradle module with its own package in postgresql case it is module exposed-postgresql and package is postgresql. Insert, update and delete statements got reworked a bit and support postgresql features like RETURNING and ON CONFLICT. Common DSL would be still available in another module so developer can choose Exposed flavour needed for his project. But would not be part of core so won't be available by default. Selects are just reimported from core.

Insert examples
Update examples
Delete examples

Open for discussion. Changes in DSL or so.

For testing I went for test-container. Usually when I run tests on Exposed project at least one docker with testing DB is running even after tests are finished. Usually change setting for my locally running docker. So in this module testing is kinda reworked.

V

Stengl Vaclav added 7 commits February 6, 2022 17:13
# Conflicts:
#	exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/InsertStatement.kt
#	exposed-postgresql/src/main/kotlin/org/jetbrains/exposed/postgresql/sql/insert-DSL-impl.kt
#	exposed-postgresql/src/main/kotlin/org/jetbrains/exposed/postgresql/sql/returning.kt
#	exposed-postgresql/src/test/kotlin/org/jetbrains/exposed/postgresql/sql/BasePostgresTest.kt
#	exposed-postgresql/src/test/kotlin/org/jetbrains/exposed/postgresql/sql/PostgresqlInsertDSLTest.kt
#	exposed-postgresql/src/test/kotlin/org/jetbrains/exposed/postgresql/sql/test-table.kt
@stengvac stengvac mentioned this pull request Feb 16, 2022
@stengvac
Copy link
Contributor Author

stengvac commented Feb 16, 2022

Hello @naftalmm ,

I did not found way to contact you on github so i will write it here. You seems active on this repo. If you could look at this PR and leave some thoughts would be appreciated. I don`t mean code review for not I will tune this MR some more, but more like about idea - DB specific DSL.

Thx,
V

@naftalmm
Copy link
Contributor

naftalmm commented Feb 17, 2022

Hello!

I've also thought about DB-specific DSLs recently. Let me share my opinion about suggested ideas since you're explicitly asking for it)

Dialect-specific submodules

The whole idea of ORM is to create a unified abstraction, independent of specific SQL dialects.
Yes, there are some features, which present only in some DBMSs, but Exposed is trying to support them all in a common API (and even tries to emulate absent ones, but throws runtime exception if it's impossible).

This is achieved by the following design cornerstones:

  1. There is an abstract class FunctionProvider, which defines common API for all supported SQL functions with default implementation throwing exception for non-common operations/operation parameters (like REPLACE operation or IGNORE parameter for DELETE/INSERT).
  2. There are derived classes for each of the supported DBMS which provides an actual implementation for supported features.
    image
  3. Also there is a DatabaseDialect interface, which defines a set of features varying across different dialects
    image

Some non-standard SQL-features are not exclusive for only one DBMS, so this approach allows to not reimplement them in each dialect submodule supporting it, but write code like this:

if (currentDialect.supportsIfNotExists) {
    append("IF EXISTS ")
}
  1. In runtime Exposed "looks at" connected DB, figures out its dialect, and uses a respectful implementation of FunctionProvider and DatabaseDialect.

DB-specific DSLs

However, this "common API" approach has its downsides:

  • Lack of features available only in a single/few DBMS
    or, otherwise,
  • IDEs auto-completion "pollution" and runtime exceptions

Probably it could be mitigated with some additional DSL, defining dialect-specific context, which extends basic "common API":

transaction(forDB = /*...*/) {
    //...
}

But I believe the "common API" approach is enough for ON CONFLICT and RETURNING clauses.

ON CONFLICT clause support

Actually, ON CONFLICT DO UPDATE is already implemented as an emulation for REPLACE operation. And ON CONFLICT DO NOTHING - as emulation for INSERT IGNORE.

RETURNING clause support

For DBMS not supporting the RETURNING clause, it could be emulated with an additional SELECT query under the hood (maybe with some warning in logs).

Suggested DSL is rather complicated, looking almost like plain SQL.
I'd suggest reuse existing DSL for insert, update, deleteWhere and define their overloaded versions with signature like this:

fun <T : Table> T.insert(body: T.(InsertReturningStatement) -> FieldSet): List<ResultRow>

which will be chosen if the last expression of lambda passed into insert is a FieldSet:

Table.insert {
    it[column] = value
    it.returning(...) //or it.returningAll(), both methods should to be defined in InsertReturningStatement class and return `FieldSet`
}

Not sure if it's possible to create such an overload in Kotlin currently.

Alternatively, it could be:

fun <T : Table> T.insert(returning: List<Expression<*>>, body: T.(InsertReturningStatement) -> Unit): List<ResultRow>

But its usage requires boiler-plate listOf call, it's not that readable without explicit argument name and doesn't cover returningAll case:

Table.insert(returning = listOf(...)) {
    it[column] = value
}

Another alternative:

fun <T : Table> T.insert(returning: ReturningDSL.() -> List<Expression<*>>, body: T.(InsertReturningStatement) -> Unit): List<ResultRow>

object ReturningDSL {
    fun returning(column: Expression<*>, vararg columns: Expression<*>) = listOf(column) + columns
    fun returningAll() = emptyList<Expression<*>>() // special value, needs to be treated specifically
}

It forces user to explicitly type returning/returningAll word, making the resulting DSL more readable, but introduces another lambda parameter:

Table.insert({ returning(...) }) {
    it[column] = value
}

@stengvac
Copy link
Contributor Author

stengvac commented Feb 20, 2022

Thanks for answer,

Till today I did not consider Exposed as ORM framework (now i see it is in readme :) ). For me this lib was Dynamic SQL written in Kotlin with lambda DSL. I was looking for one written in Kotlin for some time.

But yes, Exposed claims itself as ORM. But imho in the end it depend what parts of Exposed are used in project. I usually go for DSL only, because I don't like DAO module approach. For me is enought Dynamic SQL which allow easier mapping of data to Kotlin classes via ResultRow.

Even if we will consider Exposed as ORM framework only, unified approach for all databases is nice to have, but not necessary. ORM is mapping between Database results and class instance in Kotlin I does not denote how DB API should look.

And this is where problem lies. As you mention Exposed address DB specific features with common API, which is imho misleading, because some databases may not support that feature and exception may occur. Like using limit in update for PostreSQL. DSL stand for Domain Specific Language. Which in this case for now means common/abstract DSL for all DBs. But also can mean specific for each DB. It depends if we want to or not. I would like to.

Point 2 and 3, Yes I have read and little modified how SQL is rendered for my needs. Basically I did create interface with callbacks, which may or may not be invoked for SQL Dialect. It allows to implement any DB specific features. You can see it for example SQLRenderCallbacks, specific SQL render then in dialect those callbacks are invoked. I did not find other solution for now, because rendering parameters has to be same for all dialects. Maybe checkout code and see changes as whole.

Next you are mentioning specifying DB specific DSL via transaction { }. That could work, but now DSL is build on FieldSet, Table etc this change would break a lot. Meaning now there is no connection between dialect and transaction when statement is written.Yes during execution current DB dialect is retrieved via transaction but it is during runtime not compile time. In the end I don't think it is possible. I did start with different approach and it was DB specific tables but later on I drop it and went for package only.

Function naming for sure is open for discussion and further changes no problem.
About returning functions naming I can try to change from insertReturning to insert, but I think it won`t work bcs ambiguous argument - at least I think, when only argument is lambda, then 2 functions with same name are no go. Still had to try.
One of my goals was to make DSL more SQL like. meaning one lambda, with other lambdas inside. Like following example

table.insertReturning {
      values { insertStatement ->
            insertStatement[this.fullName] = fullName
      }

      returning(returning = table.slice(table.fullName))
}

in this order I would write SQL itself.

Table.insert({ returning(...) }) {
    it[column] = value
}

Works same, but it look less like produced SQL to me. Mainly statement order. Returning before values are set. Yes my example does not force any order at all it is just my flow.

What I want to know and don't see here is opinion whether this changes makes sense and would be welcome or this lib/framework will stick only to common DSL.

@naftalmm
Copy link
Contributor

naftalmm commented Feb 21, 2022

As you mention Exposed address DB specific features with common API, which is imho misleading, because some databases may not support that feature and exception may occur. Like using limit in update for PostreSQL

Actually, for PostgreSQL (and, probably other DBMS not supporting limit in update) it could be emulated with an additional subquery (see https://stackoverflow.com/questions/11432155/limiting-postgresql-update-command).
But, ideally, yes - all not supported/emulable by all DBs features should be moved from common API into DB-specific DSLs.

The obvious advantage of common API for users is the ease of DBMS migration - "write once, run everywhere". DB-specific DSLs as a set of extensions methods for common API could be a compromise between portability and feature-fullness - "write once, rewrite only DB-specific parts, run everywhere".

Next you are mentioning specifying DB specific DSL via transaction { }. That could work, but now DSL is build on FieldSet, Table etc this change would break a lot.

Kotlin allows to define extentions not only globally, but also in the context of another class/interface. To keep Transaction intact we may just declare a set of dedicated objects to represent DB-specific contexts:

object PostgresExtraDSL {
    fun <T : Table> T.somePostgresSpecificStaff() {
        // ...
    }
}

So this will be compiled:

transaction {
    with(PostgresExtraDSL) {
        MyTable.somePostgresSpecificStaff()
    }
}

while this not:

transaction {
    MyTable.somePostgresSpecificStaff()
}

Later, when KEEP-259 will be implemented, additional functions could be defined:

fun <T> postgresTransaction(db: Database? = null, statement: context(PostgresExtraDSL) Transaction.() -> T): T = 
    with(PostgresExtraDSL) { transaction(db, statement) }

for even better API:

postgresTransaction {
    MyTable.somePostgresSpecificStaff()
}

when only argument is lambda, then 2 functions with same name are no go

There is an experimental @OverloadResolutionByLambdaReturnType annotation for this, but it will result in dozens of "Candidate was chosen only by @OverloadResolutionByLambdaReturnType annotation" warnings in users code, so it's not perfectly backward-compatible change. Probably, when this feature will be stabilized, these warnings won't be issued by the compiler as well.

One of my goals was to make DSL more SQL like.

API, strictly following SQL syntax, could be more natural for users with respectful background, but the resulting code is not as concise and readable, as it could be.
I believe good DSL should be a compromise between SQL syntax and Kotlin DSL patterns, taking the best from two worlds.
Yes, statement order in proposed syntax:

Table.insert({ returning(...) }) {
    it[column] = value
}

is not convenient, returning before values are set. Ideally, it should be methods chaining:

Table
    .insert{
        it[column] = value
    }
    .returning(...)

But this API will make insert a non-terminal operation (moving side-effect of DB call into returning), which is a breaking change.

So, maybe, arguments order swap will be enought?

Table.insert({
    it[column] = value
}) {
    returning(...) 
}

What I want to know and don't see here is opinion whether this changes makes sense and would be welcome or this lib/framework will stick only to common DSL.

In my opinion, it's better to implement as many features as possible via common DSL (emulating absent ones in specific dialects by their means). Dialect-specific extra DSL (with aforementioned syntax) makes sense too, but only for non-emulable features, also allowing to gradually implement emulation, moving originally dialect-specific features into common API when all emulations are done.

@Tapac, what do you think?

@stengvac
Copy link
Contributor Author

stengvac commented Feb 21, 2022

The obvious advantage of common API for users is the ease of DBMS migration - "write once, run everywhere". DB-specific DSLs as a set of extensions methods for common API could be a compromise between portability and feature-fullness - "write once, rewrite only DB-specific parts, run everywhere".

I don't follow. My intention was DSL for query/update data not for DB DDL meaning migrations (in my case we don't use Exposed for migrations at all) it is used strictly for data queries/update/delete and migrations are done before app even start by init container by manually written scripts with liquibase/flyway. Also DB specific DSL is intended for projects, which has set database type strictly (in our case PostgreSQL). Otherwise go for common SQL.

About

object PostgresExtraDSL {
    fun <T : Table> T.somePostgresSpecificStaff() {  }
}

seems nice. What need more work how to propagate it to DSL. Write with all the time bleh.

postgresTransaction {
    MyTable.somePostgresSpecificStaff()
}

This is not good idea how to propagate scope for bigger projects. We have queries placed in repositories. Transaction is often placed in services because multiple DB operations are often part of 1 transaction. So not bounding transaction with DSL is good approach I would say. Otherwise we have to propagate transaction to lower layers because of DSL? no thanks. I understand, that for specific DSL would be nice to somehow check which dialect was actually used for DSL execution, but it it for developer to make sure that if there is possibility that other DB will be used he has to go for common DSL.

I would rather go for database objects bound DSL. Meaning table.withDialect(Postgres).select { ... }, table.select<Postgres> { }, val postgresDSLTable = table.dsl(Postgres) then use postgresDSLTable as DSL root or even Table<DLS>?

About DSL API I like most

Table
    .insert{
        it[column] = value
    }
    .returning(...)

it does not have to be breaking change if it is make like version 2 or so. Also this allow to use context extensions more fluent.

@naftalmm
Copy link
Contributor

naftalmm commented Feb 21, 2022

I don't follow. My intention was DSL for query/update data not for DB DDL meaning migrations

Yes, data migration is a completely separate task. I mean that apart from data migration, the SQL code to work with new DBMS should be tweaked too (and ORM seamlessly does it for you, unless you're using DB-specific DSL).

DB specific DSL is intended for projects, which has set database type strictly

Business decisions may change - architectural decisions should expect this to minimize possible code modifications.

Write with all the time bleh.
We have queries placed in repositories. Transaction is often placed in services.

You may declare queries separately from their calls in transactions.
With KEEP-259 you can just mark functions, declaring queries accessing DB-specific DSL, with context(PostgresExtraDSL) and then import in another place and pass them into postgresTransaction { }. So, there will be no user-written with(PostgresExtraDSL) at all.
Until that, all functions, declaring queries accessing DB-specific DSL have to wrap their body into with(PostgresExtraDSL) { }

it does not have to be breaking change if it is make like version 2 or so

It still would be a breaking change since it would require users to somehow migrate their code to keep its semantics.
Making it in a major revision is just not that unexpected for users.

@a2xchip
Copy link

a2xchip commented May 13, 2023

@naftalmm @stengvac What is the status of this PR?

@stengvac
Copy link
Contributor Author

Hey,

as we never got any response on topic: how Exposed will behave in future regarding DB specific dialects (either won`t support them -> use only comm SQL or will somehow). Then this PR is sleeping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants