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

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.
   
3. Also there is a DatabaseDialect interface, which defines a set of features varying across different dialects
   

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 ")
}

4. 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
}

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.

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?

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.

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.

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

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.