Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

Query param parsing removes comments from SQL, making them not visible in SQL monitoring tools

Description

The changes in this commit

https://github.com/lucee/Lucee/commit/77f517fe6ede4d0ab37c36cfc71b65583a2b20cb

attempt to strip comments entirely from the SQL String, but this is undesirable for anyone wanting to see these comments in their SQL monitoring tools. There is actually an entire SQL commenter specification for adding debug comments to the end of queries

https://google.github.io/sqlcommenter/

which we have implemented in our qb library

https://qb.ortusbooks.com/query-builder/debugging/sqlcommenter

but this change in Lucee makes the entire feature not work.

It’s also worth noting, the logic seem to have some issues, as a single line comment followed immediately (no whitespace after the line break) by a multi-line comment prior to the SQL statement will remain in the query, but I’m not sure why. This likely means that query marks in these comments will mess up query params so I’d check it out too. (note the whitespace is important-- the multiline comment must be left justified)

<cfquery name="test" datasource="myDSN"> -- foo /* bar */ SELECT 'test' </cfquery>

This exact query produces the following SQL

/* bar */ SELECT 'test'

which appears to “miss' the multi-line comment. And this query fails entirely with the error there are more question marks in the SQL than params defined.

<cfquery name="test" datasource="myDSN"> -- foo /* bar? */ SELECT 'test' </cfquery>

Environment

None

Attachments

3
  • 07 Jun 2024, 10:54 am
  • 07 Jun 2024, 10:54 am
  • 06 Jun 2024, 09:46 pm

Activity

Show:

Brad Wood 1 July 2024 at 20:47

Looks pretty good, thx

Zac Spitzer 1 July 2024 at 17:25
Edited

I have run the tests for this PR against 6.0.4.1, with the comment checking disabled, just to demonstrate the underlying problem with comment parsing, 5 combinations fail, https://github.com/zspitzer/Lucee/actions/runs/9748408232

[INFO] [java] [script] /* bar? */ [INFO] [java] [script] SELECT engine [INFO] [java] [script] from qry [INFO] [java] [script] where id = :id
[INFO] [java] [script] -- foo ? :do [INFO] [java] [script] /* bar? :*/ [INFO] [java] [script] SELECT engine [INFO] [java] [script] from qry [INFO] [java] [script] where id = :id

BTW the tests just stop on the first variation which fails, for a given sql statement, against h2 and qoq, there are 49 variations per sql statement in the test suite

Zac Spitzer 1 July 2024 at 16:55

Ok all tests passing against qoq and H2

as part of this process I have added dumping out the exceptions in the qoq fallback, some of the select parsing seems to be falling over when it shouldn't be, so fixing that should improve performance too

does the updated test case cover all your edge cases?

Zac Spitzer 30 June 2024 at 17:52

needs more work still, i’m converting the tests to use real queries and thus test all the code paths affected

Zac Spitzer 30 June 2024 at 12:57
Edited

I’ve refactored the test to do a whole range of additional whitespace tests, plus I fixed the handling of the last line being a single line comment

https://github.com/lucee/Lucee/pull/2382/commits/da468dbc12acbae07fd04b9e93f8b2c938b325e8 good catch!

Details

Assignee

Reporter

Priority

Labels

New Issue warning screen

Before you create a new Issue, please post to the mailing list first https://dev.lucee.org

Once the issue has been verified, one of the Lucee team will ask you to file an issue

Sprint

Affects versions

Created 22 May 2024 at 19:52
Updated 6 March 2025 at 11:37

Flag notifications