-
Notifications
You must be signed in to change notification settings - Fork 496
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
Compiler: Add Defaults support in bindings #3375
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
46838fb
Fix DEFAULT in binding
hfhbd 293db31
Remove isDefault api
hfhbd 5c9670a
Fix grammar
hfhbd ad350f2
Add mysql async test
hfhbd bf605c0
Add postgresql async test
hfhbd b583718
Fix wildards
hfhbd bf3de38
Fix code style
hfhbd 980d85b
Merge branch 'master' into defaults
hfhbd b2593f7
Add default to mysql too
hfhbd f5619f6
Downgrade r2dbc spi due binary incompatible with drivers
hfhbd 466a934
Fix DEFAULT in binding
hfhbd 053327b
Remove isDefault api
hfhbd 19b3723
Fix grammar
hfhbd c3169c1
Add mysql async test
hfhbd cd163e9
Add postgresql async test
hfhbd a280a97
Fix wildards
hfhbd 18caef1
Fix code style
hfhbd 115b2ff
Add default to mysql too
hfhbd e3b9eba
Downgrade r2dbc spi due binary incompatible with drivers
hfhbd e218001
Merge remote-tracking branch 'old/defaults' into defaults
hfhbd c909a2f
Merge branch 'cashapp:master' into defaults
hfhbd 727c911
Merge remote-tracking branch 'old/defaults' into defaults
hfhbd 8dcc05f
Remove useless check for default
hfhbd File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 changes: 11 additions & 0 deletions
11
.../main/kotlin/app/cash/sqldelight/dialects/postgresql/grammar/mixins/BindParameterMixin.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
package app.cash.sqldelight.dialects.postgresql.grammar.mixins | ||
|
||
import app.cash.sqldelight.dialect.grammar.mixins.BindParameterMixin | ||
import com.intellij.lang.ASTNode | ||
|
||
abstract class BindParameterMixin(node: ASTNode) : BindParameterMixin(node) { | ||
override fun replaceWith(isAsync: Boolean, index: Int): String = when { | ||
isAsync -> "$$index" | ||
else -> "?" | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
15 changes: 15 additions & 0 deletions
15
.../dialect/src/main/kotlin/app/cash/sqldelight/dialect/grammar/mixins/BindParameterMixin.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package app.cash.sqldelight.dialect.grammar.mixins | ||
|
||
import com.alecstrong.sql.psi.core.psi.SqlBindParameter | ||
import com.alecstrong.sql.psi.core.psi.SqlCompositeElementImpl | ||
import com.intellij.lang.ASTNode | ||
|
||
abstract class BindParameterMixin(node: ASTNode) : SqlCompositeElementImpl(node), SqlBindParameter { | ||
hfhbd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* Overwrite, if the user provided sql parameter should be overwritten by sqldelight with [replaceWith]. | ||
* | ||
* Some sql dialects support other bind parameter besides `?`, but sqldelight should still replace the | ||
* user provided parameter with [replaceWith] for a homogen generated code. | ||
*/ | ||
open fun replaceWith(isAsync: Boolean, index: Int): String = "?" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
sqldelight-gradle-plugin/src/test/integration-mysql-async/settings.gradle
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
apply from: "../settings.gradle" | ||
|
||
rootProject.name = 'sqldelight-mysql-integration' | ||
rootProject.name = 'sqldelight-mysql-integration-async' |
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like a driver implementation detail, that specifically for R2DBC we need indexed parameters.
We're getting closer and I'm very happy to have functioning tests for this stuff now, so this is really the only thing I'm still hung up on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only r2dbc. Postgres native needs indexed parameters too: https://github.com/hfhbd/postgres-native-sqldelight/blob/1a82ac52d6ab6246644fe8216932fd348afd1a9a/postgres-native-sqldelight-driver/src/commonMain/kotlin/app/softwork/sqldelight/postgresdriver/PostgresNativeDriver.kt#L165
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like that approach more, doing it at the driver level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But at driver level you only have strings. So "SELECT '?'" or "LIKE 'foo?%'" does not work. (Except using sql-psi at driver level during runtime again before executing each stmt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, I buy that
last suggestion then: could we just always replace arguments for the postgres driver with their indexed argument, even if its not the async driver? So that parameter can be removed and we're just saying for this dialect this is the expected parameter format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What parameter do you want to drop?
index => How do you want to do this? Hardcode a check in sqldelight-core to replace the parameter wildcard with a specific string for postgresql only?
isAsync => I have to check it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As expected, using
$1
does not work on JDBC and results in an org.postgresql.util.PSQLException, because you try to bind a parameter to an unknown one:In JDBC, the question mark (?) is the placeholder for the positional parameters of a PreparedStatement. There are, however, a number of PostgreSQL® operators that contain a question mark. To keep such question marks in an SQL statement from being interpreted as positional parameters, use two question marks ( ?? ) as escape sequence. You can also use this escape sequence in a Statement , but that is not required. Specifically only in a Statement a single ( ? ) can be used as an operator.
https://jdbc.postgresql.org/documentation/query/#using-the-statement-or-preparedstatement-interface
But I removed some useless check/binding implementations because I already check for
DEFAULT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. I'm still wishing that this logic was in the driver somewhere instead of the dialect since it looks like a driver constraint, not a postgres one, but lets just merge to fix the underlying issue and then if it becomes a problem later we can revisit.