Exception thrown if comments are in SQL string

[bwbond87]

Running any query with a comment (using either -- or /**/) will throw an exception. We tend to read more complex SQL from a file on the classpath so having the ability to run queries with comments is very beneficial.

Example query: "/* test */ top 100 user_id from users" will throw the following:
"java.sql.SQLException: com.microsoft.sqlserver.jdbc.SQLServerException: Unable to identify the table /* test */"

[Suraiya-Hameed]

Hi @bwbond87, I tried executing simple queries with comments, it is working fine, no exception was thrown. Even "select /* test */ top 10 user_id from users" runs without exception. Can you share a code snippet or repo code, so we can narrow down the cause?

The exception com.microsoft.sqlserver.jdbc.SQLServerException you mentioned is from SQL Server, so the problem could likely be because of an incorrect query sent or the way query is formed while reading from the external file.

[bwbond87]

I was using a library to execute the SQL with a provided DataSource. I did some digging into the library and the offending exception occurs when the method getParameterMetaData() is called from the PreparedStatement implementation.

The database I am connecting to is running SQL Server 2005, so that may be a factor. The parseStatement method that the SQLServerParameterMetaData constructor defers to looks like it's failing to parse the SQL string for SQL Server versions <= 2008.

This snippet triggers the exception for me:

try (Connection conn = dataSource.getConnection()) {
    preparedStatement = conn.prepareStatement(" /* test */ select top 100 user_id from users");
    preparedStatement.getParameterMetaData();
    ResultSet resultSet = preparedStatement.executeQuery();
}

[Suraiya-Hameed]

Thanks for the details! I was using the latest SQL Server for testing, so I wasn't able to reproduce it early.

It is a shortcoming in the parsing of SQL query, as you pointed out. The exception is thrown only on accessing ParameterMetaData for SQL Server <=2008, as the sp_describe_undeclared_parameters stored procedure used by driver for getting metadata was introduced in the later version of SQL SERVER.

The input query needs to be parsed for comments and nested comments, we will consider it for future enhancement. You are welcome to make changes and submit a PR.

[xiangyushawn]

@bwbond87 hello. we have created a PR for this issue. Thank you for filing this issue. Could you please let us know if the PR works for you. Thank you.

[bwbond87]

It looks like the case where single line comments are used ("#") is not covered by the change. We tend to place these in our SQL files frequently so it may cause issues.

[xiangyushawn]

@bwbond87 thank you very much for confirming with us! I added a commit to filter out single line comments ("--"). Could you please verify if that works for you now?

[v-nisidh]

@bwbond87 Are you referring to single line comments -- having #? Because as per my understanding single line comment in SQL is --.

Ref: -- (Comment) (Transact-SQL)

[xiangyushawn]

@bwbond87 right. I am referring to --. From my understanding, # is not for commenting on SQL queries, right?

[bwbond87]

@v-nisidh @v-xiangs That was my mistake (My brain was in Python syntax mode). Definitely "--". I'll try the fix first thing tomorrow morning.

[bwbond87]

@v-xiangs

I tested the change. Unfortunately, there are still exceptions being thrown. We have multiple single line comments at the beginning of some statements. Here is an example query:

-- Retrieves all users for the special case
-- Test comment 123
select * 
from TestDatabase.dbo.users
where user_type=36

We also have single line comments at the end of certain statements. Here is one of our more complex examples:

-- Test comment 123
select q.* from (
    select
        u.user_id,
        state=coalesce(st_local_defaults.status,
                        st_global_defaults.status)
    from
        TestDatabase.dbo.Users as u
    left outer join
        services.dbo.st_local_defaults as st_local_defaults
        on (u.user_id = st_local_defaults.user_id) and (st_local_defaults.st_id = 0) -- should be 1?
    left outer join
        services.dbo.st_global_defaults as st_global_defaults
        on (u.user_id = st_global_defaults.user_id)
) as q

where q.state is not null

[xiangyushawn]

@bwbond87 Thank you for providing the query samples. From my testing, the new commits fixed both of the query samples that you provided. Could you please let me know how your project reads the sql file? for example, read the sql file line by line? If it reads line by line, did you add \n at the end of each line?

[bwbond87]

We read an entire file into a string using Apache's IO Utils, here is a snippet:

InputStream sqlStream = this.getClass().getResourceAsStream("/test.sql");
String sql = IOUtils.toString(sqlStream, StandardCharsets.UTF_8);

I traced through the execution and the problem appears to be newlines. One of the tokens was pa.area_id\nfrom. It will therefore fail to identify FROM as a token. The StringTokenizer instantiaion doesn't look like it contains carriage returns and newlines as delimiters. Link to StringTokenizer instantiation.

[xiangyushawn]

@bwbond87 thank you very much for finding the cause for us. I added a new commit to fix it. I hope this will work for you. Thank you.

[bwbond87]

@v-xiangs Thank you for fixing that defect. That fix prevents the exception from being thrown. However, there is an additional issue now. If you run the second SQL example I posted earlier, the table name is identified as "(" in the MetaInfo object. This makes the parsing problem interesting as the "table" is actually a derived table. The fields member in MetaInfo is also empty.

The good news is this issue does not affect our use case at all. I do not know if this warrants creating a completely separate issue or not.

[xiangyushawn]

@bwbond87 I ran the second SQL example with the original driver, it also identifies "(" as the table name. Therefore, this is not an issue introduced by this fix. We will create an another issue for it and close this issue since you have confirmed the issue is fixed. Thank you.

[bwbond87]

OK, thank you!