Sqldelight: Issues with comments

[endanke]

Hi!

I'm trying to use comments in my .sq files, but I've noticed two small problems:

• Comments only seems to get included in the generated query files, but not in the interfaces. So every comment is ignored what I write before the end of the create table block.
• I can't use more than one comments for the queries. One works fine, but if I include another one, it seems like the second comment's end tag is recognized for the first comment, so it disables the things between the two. (Attached an image about this.)

I've checked the relevant test in the repo, it seems that there's no case which covers either of these.

[endanke]

@AlecStrong I've noticed that the issue might be in the flex configuration of your parser at this line:
https://github.com/AlecStrong/sql-psi/blob/13ce4d77c5c1afcc1593959967171c31b0eb9855/core/src/main/kotlin/com/alecstrong/sql/psi/core/SqlLexer.flex#L30

I'm not too experienced with lexical analysis, but I suspect it works like a regex. I've tried the current version on https://regex101.com and I think the issue might be that the comment tag has a greedy match. When I added a question mark after the capture block it seems to work as expected. (See the attached image for reference.)
Hope this helps finding the bug!

[AlecStrong]

hell yea, thanks for finding the fix!

[AlecStrong]

ugh looks like jflex has its own regex implementation that it isn't working with. hmmmmm

[endanke]

That's unfortunate.
Not sure if It helps, but there's an example in the manual of jFlex for a similar comment format:

DocumentationComment = "/**" {CommentContent} "*"+ "/"
CommentContent       = ( [^*] | \*+ [^/*] )*

[AlecStrong]

that worked! cheers

[endanke]

I've tried the latest version today. Unfortunately it looks like the issue it still not fully solved.
The good news is that the comments seems to be recognized by the lexer, but there might be still some other problems after the tokenization.

Case 1: If I add a Javadoc-style comment in the create table call, then I get an error in the IDE:
<column name real> or <table constraint real> expected, got ...

Case 2: If I add the comment above the query definitions there's no error in the IDE, but the generated code is incorrect. It seems to add an extra closing tag in the output.

This is my input in the .sq file:

And this is the generated code:

[AlecStrong]

Pooooooop

[endanke]

A small update on this issue:
After some trials and errors I've realized that there's a simple fix for the second problem. It seems like the Javadoc comment works on the query definitions if the lines of the comments are indented with 1 leading space starting from the second line. So for example, this works:

/**
 * Comment test
 */
insertSomeData:
INSERT INTO deviceEvent
VALUES ?;

(Although it's still a bug because afaik the leading space is not a requirement for Javadoc comments.)

However this still doesn't fix the first problem, when comments are used in a create table statement.
For that I've created a new test case (in the JavadocTest class) if someone would like to investigate it further:

@Test fun `create - properly formatted javadoc`() {
        val result = FixtureCompiler.compileSql("""
          |CREATE TABLE test (
          |  /**
          |   * Test comment one
          |   */
          |  sample TEXT
          |);
          |""".trimMargin(), tempFolder)
        assertThat(result.errors).isEmpty()
        val generatedInterface = result.compilerOutput.get(File(result.outputDirectory, "com/example/Test.kt"))
        assertThat(generatedInterface).isNotNull()
        assertThat(generatedInterface.toString()).isEqualTo("""
          |package com.example
          |
          |import kotlin.String
          |
          |data class Test(
          |  /**
          |   * Test comment one
          |   */
          |  val sample: String?
          |) {
          |  override fun toString(): String = ""${'"'}
          |  |Test [
          |  |  sample: ${"$"}sample
          |  |]
          |  ""${'"'}.trimMargin()
          |}
          |""".trimMargin())
    }

This currently fails with the following error:

expected to be empty
but was:
    [Test.sq line 2:2 - <column name real> expected, got '/**
       * Test commen...']

I'm trying to test some solutions but I think the problem might be still somewhere in the SQL-PSI project and I'm not really sure about the development setup there.

[endanke]

@AlecStrong seems like I've finally managed to fix the issue. This time I've also tested it locally!
I could make a pull request, but I'm not sure if it's the right way to apply it and also it's a relatively small change anyway.

I had to modify both projects. In SQL-PSI I've added two optional javadoc element in the sql.bnf file on line 99:

create_table_stmt ::= CREATE [ TEMP | TEMPORARY ] TABLE [ IF NOT EXISTS ] [ database_name '.' ] table_name ( '(' [ javadoc ] column_def ( ',' [ javadoc ] column_def ) * ( ',' table_constraint ) * ')' [ table_options ] | AS compound_select_stmt ) {

Also had to adjust some of the test cases to make it build. This fixed the parsing issue, but then I also had to add the comment to the generated files.
For that I've modified the TableInterfaceGenerator.kt file in SQLDelight on line 69:
I've replaced constructor.addParameter(columnName, column.type().javaType) with:

val param = ParameterSpec.builder(columnName, column.type().javaType)
column.prevSibling.prevSibling.let { javadoc ->
    if ((javadoc as LeafPsiElement).elementType == SqlTypes.JAVADOC) {
        javadocText(javadoc)?.let { param.addKdoc(it) }
    }
}
constructor.addParameter(param.build())

This finally works, although I'm pretty sure it's not an optimal solution. Instead the comments could be added to the SqlColumnDef class somehow, but I'm not that familiar with the whole setup to make those changes. I hope this information is useful enough for you to fix this issue in a future release.

PS.: I've noticed that the output doesn't fit the test case I've posted above. For some reason it also adds an extra * to the lines between the opening and closing tags. Probably the regex still doesn't have a proper match on the comment. At least it works, this could be also fixed later.

[AlecStrong]

this sounds like the right path forward to me. I think for the sqldelight change there might be something like PsiTreeUtil.prevSiblingIgnoringWhitspace(column)

the other option is including [ javadoc ] in the column_def rule, as long as that works in sql-psi, it'll make the sqldelight change a lot easier

column.javadoc?.let(::javadocText)?.let { param.addKdoc(it) }

either sounds good to me, if you want to make a PR with those changes I'd be happy to accept!

[endanke]

Thanks for the quick reply!
I've created the PRs, hopefully it should fix these issues without any problem.