[RFC] Report an error when binding a column name in an ORDER BY clause #216

eygraber · 2020-10-07T05:53:18Z

Attempting to address sqldelight/sqldelight#1187

@AlecStrong Is this the right approach, and does it need to be broadened to other areas besides ORDER BY?

eygraber · 2020-10-07T05:53:46Z

core/src/test/kotlin/com/alecstrong/sql/psi/core/FixturesTest.kt

-        DialectPreset.SQLITE_3_18 to arrayOf("src/test/fixtures", "src/test/fixtures_upsert_not_supported"),
-        DialectPreset.SQLITE_3_24 to arrayOf("src/test/fixtures", "src/test/fixtures_sqlite_3_24"),
-        DialectPreset.SQLITE_3_25 to arrayOf("src/test/fixtures", "src/test/fixtures_sqlite_3_24", "src/test/fixtures_sqlite_3_25"),
-        DialectPreset.MYSQL to arrayOf("src/test/fixtures_mysql"),
-        DialectPreset.HSQL to arrayOf("src/test/fixtures_hsql"),
-        DialectPreset.POSTGRESQL to arrayOf("src/test/fixtures_postgresql")
+        DialectPreset.SQLITE_3_18 to arrayOf("src/test/fixtures")
    )

    @Suppress("unused") // Used by Parameterized JUnit runner reflectively.
    @Parameters(name = "{0}: {1}")
    @JvmStatic fun parameters() = dialects.flatMap { (dialect, fixtureFolders) ->
      fixtureFolders.flatMap { fixtureFolder ->
        File(fixtureFolder).listFiles()
-            .filter { it.isDirectory }
+            .filter { it.isDirectory && it.name == "bind-failures" }


Pardon my harness, i.e. is there a better way to do this?

to run a single test what i usually do is run all tests, and then right click the specific one I want from the intellij run panel and run just the one test. It's not a great flow, this works too

AlecKazakova

this is perfect! just the one comment and harness revert but looks great to me

AlecKazakova · 2020-10-07T13:26:00Z

core/src/main/kotlin/com/alecstrong/sql/psi/core/psi/mixins/OrderByMixin.kt

+    val childBindExpr = node.psi.children.find { it is SqlBindExpr }
+    if (childBindExpr != null) {
+      annotationHolder.createErrorAnnotation(childBindExpr, "Cannot bind the name of a column in an ORDER BY clause")
+    }


i think you might be able to be more explicit here

Suggested change

val childBindExpr = node.psi.children.find { it is SqlBindExpr }

if (childBindExpr != null) {

annotationHolder.createErrorAnnotation(childBindExpr, "Cannot bind the name of a column in an ORDER BY clause")

}

if (expr is SqlBindExpr) {

annotationHolder.createErrorAnnotation(expr, "Cannot bind the name of a column in an ORDER BY clause")

}

AlecKazakova · 2020-10-07T13:26:31Z

core/src/test/kotlin/com/alecstrong/sql/psi/core/FixturesTest.kt

-        DialectPreset.MYSQL to arrayOf("src/test/fixtures_mysql"),
-        DialectPreset.HSQL to arrayOf("src/test/fixtures_hsql"),
-        DialectPreset.POSTGRESQL to arrayOf("src/test/fixtures_postgresql")
+        DialectPreset.SQLITE_3_18 to arrayOf("src/test/fixtures")


AlecKazakova · 2020-10-07T13:27:09Z

core/src/test/kotlin/com/alecstrong/sql/psi/core/FixturesTest.kt

-        DialectPreset.SQLITE_3_18 to arrayOf("src/test/fixtures", "src/test/fixtures_upsert_not_supported"),
-        DialectPreset.SQLITE_3_24 to arrayOf("src/test/fixtures", "src/test/fixtures_sqlite_3_24"),
-        DialectPreset.SQLITE_3_25 to arrayOf("src/test/fixtures", "src/test/fixtures_sqlite_3_24", "src/test/fixtures_sqlite_3_25"),
-        DialectPreset.MYSQL to arrayOf("src/test/fixtures_mysql"),
-        DialectPreset.HSQL to arrayOf("src/test/fixtures_hsql"),
-        DialectPreset.POSTGRESQL to arrayOf("src/test/fixtures_postgresql")
+        DialectPreset.SQLITE_3_18 to arrayOf("src/test/fixtures")
    )

    @Suppress("unused") // Used by Parameterized JUnit runner reflectively.
    @Parameters(name = "{0}: {1}")
    @JvmStatic fun parameters() = dialects.flatMap { (dialect, fixtureFolders) ->
      fixtureFolders.flatMap { fixtureFolder ->
        File(fixtureFolder).listFiles()
-            .filter { it.isDirectory }
+            .filter { it.isDirectory && it.name == "bind-failures" }


to run a single test what i usually do is run all tests, and then right click the specific one I want from the intellij run panel and run just the one test. It's not a great flow, this works too

eygraber · 2020-10-08T03:13:54Z

core/src/main/kotlin/com/alecstrong/sql/psi/core/psi/mixins/ResultColumnMixin.kt

+    val bindExpr = expr as? SqlBindExpr
+    if (bindExpr != null) {
+      annotationHolder.createErrorAnnotation(bindExpr, "Cannot bind the name of a column in a SELECT result-column")
+    }


I did it like this because smartcast can't be used on expr. I can do it as a let if you think that would look better.

i think this is good 👍

AlecKazakova · 2020-10-08T03:19:55Z

core/src/main/kotlin/com/alecstrong/sql/psi/core/psi/mixins/SelectStmtMixin.kt

@@ -46,6 +50,25 @@ internal abstract class SelectStmtMixin(
  override fun annotate(annotationHolder: SqlAnnotationHolder) {
    super.annotate(annotationHolder)

+    val invalidBinaryEqualityBindExpr = exprList.find { child ->


its still valid to do WHERE ? = some_column so I wouldn't want to fail here

I was thinking about that. What if the failure only occurs if the RHS is also a SqlBindExpr?

I also updated to test for all binary expressions

yep im cool with LHS and RHS cant both be a bind expr. I'd recommend just doing that in a new mixin for binary expressions then instead of having it live in SelectStmtMixin

I can add a mixin to all the binary comparison expressions, but is there a way to add something that includes all the different types of expressions in a synthetic one?

Something like:

binary_comparison_expr ::= [ binary_boolean_expr | binary_equality_expr | binary_like_expr | is_expr | between_expr | in_expr ]

that would need a ton of changes downstream so probably not ideal. you could have a single mixin kotlin class like

abstract class BinaryMixin : SqlCompositeElement(node) { abstract fun getExprList(): List<SqlExpr> fun annotate() { ... } }

and then you just have to repeat yourself in the bnf but i think thats fine

Just pushed that update

AlecKazakova · 2020-10-08T03:20:37Z

core/src/main/kotlin/com/alecstrong/sql/psi/core/psi/mixins/ResultColumnMixin.kt

+    val bindExpr = expr as? SqlBindExpr
+    if (bindExpr != null) {
+      annotationHolder.createErrorAnnotation(bindExpr, "Cannot bind the name of a column in a SELECT result-column")
+    }


i think this is good 👍

eygraber · 2020-10-08T04:13:32Z

core/src/main/kotlin/com/alecstrong/sql/psi/core/sql.bnf

@@ -259,12 +261,18 @@ collate_expr ::= expr COLLATE collation_name
 binary_like_operator ::= ( LIKE | GLOB | REGEXP | MATCH )
 binary_like_expr ::= expr [ NOT ] ( binary_like_operator ) expr [ ESCAPE expr ] {
  mixin = "com.alecstrong.sql.psi.core.psi.mixins.BinaryLikeExprMixin"
+  mixin = "com.alecstrong.sql.psi.core.psi.mixins.BinaryComparisonExprMixin"


Is it OK to specify two mixins like this?

Looks like it's not. Pushed some changes to address that.

…r in a GROUP BY clause

AlecKazakova · 2020-10-08T12:47:41Z

awesome, thanks!

AlecKazakova · 2020-10-08T15:23:07Z

reverting the result_column part: sqldelight/sqldelight#631

eygraber · 2020-10-08T16:50:06Z

Is it possible to suppress the error just for that scenario?

AlecKazakova · 2020-10-08T17:04:29Z

probably, i'm working on getting a release out so will wait till thats stable before working more on it

in general sql-psi should be as permissive as possible about whats allowed, and if we want to have more fine tuned errors they should land in sqldelight, in retrospect we might want to move these mixins over there so that sql-psi treats these as find but sqldelight complains for invalid arg types

AlecKazakova · 2020-10-08T17:05:29Z

also no idea why but i think this is causing memory leaks downstream: https://github.com/cashapp/sqldelight/pull/2038/checks?check_run_id=1227010895

going to revert for now to derisk the release but yea we can come back to it after

…BY clause (#216)" This reverts commit 5749520.

AlecKazakova · 2020-10-08T18:38:15Z

nevermind that looks like it might be unrelated

eygraber commented Oct 7, 2020

View reviewed changes

eygraber force-pushed the binding-column-name-error branch from 34878e4 to 21290e7 Compare October 7, 2020 05:58

AlecKazakova approved these changes Oct 7, 2020

View reviewed changes

eygraber added 2 commits October 7, 2020 22:57

Report an error when binding a column name in an ORDER BY clause

b951d56

Report an error when binding a column name in a SELECT result-column

825236f

eygraber force-pushed the binding-column-name-error branch from 21290e7 to 7ccaa2f Compare October 8, 2020 03:11

eygraber commented Oct 8, 2020

View reviewed changes

AlecKazakova approved these changes Oct 8, 2020

View reviewed changes

eygraber force-pushed the binding-column-name-error branch 2 times, most recently from 34707a1 to 24e11af Compare October 8, 2020 04:11

eygraber commented Oct 8, 2020

View reviewed changes

Report an error when binding a column name in a SELECT WHERE clause o…

bafdb9b

…r in a GROUP BY clause

eygraber force-pushed the binding-column-name-error branch from 24e11af to bafdb9b Compare October 8, 2020 04:36

AlecKazakova merged commit 5749520 into sqldelight:master Oct 8, 2020

AlecKazakova added a commit that referenced this pull request Oct 8, 2020

Revert "[RFC] Report an error when binding a column name in an ORDER …

0957eb8

…BY clause (#216)" This reverts commit 5749520.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Report an error when binding a column name in an ORDER BY clause #216

[RFC] Report an error when binding a column name in an ORDER BY clause #216

eygraber commented Oct 7, 2020

eygraber Oct 7, 2020

AlecKazakova Oct 7, 2020

AlecKazakova left a comment

AlecKazakova Oct 7, 2020

AlecKazakova Oct 7, 2020

AlecKazakova Oct 7, 2020

eygraber Oct 8, 2020

AlecKazakova Oct 8, 2020

AlecKazakova Oct 8, 2020

eygraber Oct 8, 2020

eygraber Oct 8, 2020

AlecKazakova Oct 8, 2020

eygraber Oct 8, 2020

AlecKazakova Oct 8, 2020

eygraber Oct 8, 2020

AlecKazakova Oct 8, 2020

eygraber Oct 8, 2020

eygraber Oct 8, 2020

AlecKazakova commented Oct 8, 2020

AlecKazakova commented Oct 8, 2020

eygraber commented Oct 8, 2020

AlecKazakova commented Oct 8, 2020

AlecKazakova commented Oct 8, 2020

AlecKazakova commented Oct 8, 2020

[RFC] Report an error when binding a column name in an ORDER BY clause #216

[RFC] Report an error when binding a column name in an ORDER BY clause #216

Conversation

eygraber commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlecKazakova left a 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

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

AlecKazakova commented Oct 8, 2020

AlecKazakova commented Oct 8, 2020

eygraber commented Oct 8, 2020

AlecKazakova commented Oct 8, 2020

AlecKazakova commented Oct 8, 2020

AlecKazakova commented Oct 8, 2020