From 13e2abe1de9a79bf5bba95820616e0cebbf81542 Mon Sep 17 00:00:00 2001 From: Hajime MATSUMOTO Date: Sun, 23 Nov 2025 06:35:22 +0900 Subject: [PATCH 1/3] Fix line comment handling in query type detection Fixed an issue where line comments (--) in SQL files caused incorrect query type detection. For example, an INSERT statement with a comment like "-- SELECT for xxx" at the beginning would be incorrectly identified as a SELECT query. Changes: - Add LINE_COMMENT regex pattern to remove -- style comments - Strip both /* */ and -- comments during query type detection only - Preserve comments when sending queries to the database (important for MySQL performance hints and annotations) - Add comprehensive tests for comment handling edge cases Queries sent to the database remain unchanged (comments preserved), while query type detection (SELECT vs INSERT/UPDATE/DELETE) now correctly ignores comment content. --- src/SqlQuery.php | 7 +- tests/SqlCommentTest.php | 69 +++++++++++++++++++ tests/sql/create_promise.sql | 7 +- tests/sql/create_todo.sql | 5 +- tests/sql/todo_insert_with_select_comment.sql | 3 + .../sql/todo_select_with_leading_comment.sql | 3 + tests/sql/todo_update_with_insert_comment.sql | 2 + 7 files changed, 88 insertions(+), 8 deletions(-) create mode 100644 tests/SqlCommentTest.php create mode 100644 tests/sql/todo_insert_with_select_comment.sql create mode 100644 tests/sql/todo_select_with_leading_comment.sql create mode 100644 tests/sql/todo_update_with_insert_comment.sql diff --git a/src/SqlQuery.php b/src/SqlQuery.php index 76ce9268..c5276d39 100644 --- a/src/SqlQuery.php +++ b/src/SqlQuery.php @@ -36,6 +36,7 @@ final class SqlQuery implements SqlQueryInterface { private const C_STYLE_COMMENT = '/\/\*(.*?)\*\//u'; + private const LINE_COMMENT = '/--[^\r\n]*/'; private PDOStatement|null $pdoStatement = null; @@ -123,8 +124,10 @@ private function perform(string $sqlId, array $values, FetchInterface|null $fetc $this->pdoStatement = $pdoStatement; $lastQuery = $pdoStatement->queryString; - $query = trim((string) preg_replace(self::C_STYLE_COMMENT, '', $lastQuery)); - $isSelect = stripos($query, 'select') === 0 || stripos($query, 'with') === 0; + // Remove comments only for query type detection, not for execution + $queryForDetection = (string) preg_replace(self::C_STYLE_COMMENT, '', $lastQuery); + $queryForDetection = trim((string) preg_replace(self::LINE_COMMENT, '', $queryForDetection)); + $isSelect = stripos($queryForDetection, 'select') === 0 || stripos($queryForDetection, 'with') === 0; $result = $isSelect ? $this->fetchAll($pdoStatement, $fetch) : []; /** @var array $values */ $this->logger->log($sqlId, $values); diff --git a/tests/SqlCommentTest.php b/tests/SqlCommentTest.php new file mode 100644 index 00000000..63b58445 --- /dev/null +++ b/tests/SqlCommentTest.php @@ -0,0 +1,69 @@ + */ + private array $insertData = ['id' => '1', 'title' => 'test']; + + protected function setUp(): void + { + $sqlDir = __DIR__ . '/sql'; + $pdo = new ExtendedPdo('sqlite::memory:', '', '', [PDO::ATTR_STRINGIFY_FETCHES => true]); + $pdo->query((string) file_get_contents($sqlDir . '/create_todo.sql')); + $pdo->perform((string) file_get_contents($sqlDir . '/todo_add.sql'), $this->insertData); + + $this->sqlQuery = new SqlQuery( + $pdo, + __DIR__ . '/sql', + new MediaQueryLogger(), + new AuraSqlPagerFactory(new AuraSqlPager(new DefaultView(), [])), + new ParamConverter(new ToArray()), + new Injector(), + new PerformTemplatedSql('{{ sql }}'), + ); + } + + public function testInsertWithSelectInComment(): void + { + // Critical test: -- SELECT comment should not cause INSERT to be detected as SELECT + $data = ['id' => '2', 'title' => 'test2']; + $this->sqlQuery->exec('todo_insert_with_select_comment', $data); + + // Verify the INSERT worked + $result = $this->sqlQuery->getRow('todo_item', ['id' => '2']); + $this->assertSame($data, $result); + } + + public function testSelectWithLeadingDashComment(): void + { + $result = $this->sqlQuery->getRow('todo_select_with_leading_comment', ['id' => '1']); + $this->assertSame($this->insertData, $result); + } + + public function testUpdateWithInsertComment(): void + { + $updatedTitle = 'updated'; + $this->sqlQuery->exec('todo_update_with_insert_comment', ['id' => '1', 'title' => $updatedTitle]); + + // Verify the UPDATE worked + $result = $this->sqlQuery->getRow('todo_item', ['id' => '1']); + $this->assertSame(['id' => '1', 'title' => $updatedTitle], $result); + } +} diff --git a/tests/sql/create_promise.sql b/tests/sql/create_promise.sql index 07b22e42..a7f3885e 100644 --- a/tests/sql/create_promise.sql +++ b/tests/sql/create_promise.sql @@ -1,5 +1,6 @@ -CREATE TABLE IF NOT EXISTS todo +CREATE TABLE IF NOT EXISTS promise ( - id INTEGER, - title TEXT + id TEXT, + title TEXT, + time TEXT ) \ No newline at end of file diff --git a/tests/sql/create_todo.sql b/tests/sql/create_todo.sql index a7f3885e..4dde87bc 100644 --- a/tests/sql/create_todo.sql +++ b/tests/sql/create_todo.sql @@ -1,6 +1,5 @@ -CREATE TABLE IF NOT EXISTS promise +CREATE TABLE IF NOT EXISTS todo ( id TEXT, - title TEXT, - time TEXT + title TEXT ) \ No newline at end of file diff --git a/tests/sql/todo_insert_with_select_comment.sql b/tests/sql/todo_insert_with_select_comment.sql new file mode 100644 index 00000000..4b40c6b4 --- /dev/null +++ b/tests/sql/todo_insert_with_select_comment.sql @@ -0,0 +1,3 @@ +-- SELECT for testing INSERT detection +-- This comment should not affect query type detection +INSERT INTO todo (id, title) VALUES (:id, :title); diff --git a/tests/sql/todo_select_with_leading_comment.sql b/tests/sql/todo_select_with_leading_comment.sql new file mode 100644 index 00000000..069ee3c4 --- /dev/null +++ b/tests/sql/todo_select_with_leading_comment.sql @@ -0,0 +1,3 @@ +-- Query to select todo by id +-- Author: Test +SELECT * FROM todo WHERE id = :id; diff --git a/tests/sql/todo_update_with_insert_comment.sql b/tests/sql/todo_update_with_insert_comment.sql new file mode 100644 index 00000000..660b0d54 --- /dev/null +++ b/tests/sql/todo_update_with_insert_comment.sql @@ -0,0 +1,2 @@ +-- INSERT comment that should not trigger INSERT detection +UPDATE todo SET title = :title WHERE id = :id; From 81484562062b43b3f61265a24521ef7d40c25479 Mon Sep 17 00:00:00 2001 From: Hajime MATSUMOTO Date: Sun, 23 Nov 2025 07:12:41 +0900 Subject: [PATCH 2/3] Address code review feedback: improve comment handling - Tighten LINE_COMMENT regex to only match -- at line start (after optional whitespace) to avoid stripping -- inside string literals. Changed from '/--[^\r\n]*/' to '/^\s*--[^\r\n]*/m' - Extract comment removal logic to dedicated private method removeCommentsForDetection() for better reusability and testability - Add test case for -- inside string literals to verify the regex improvement works correctly All 86 tests pass. --- src/SqlQuery.php | 23 +++++++++++++++++++---- tests/SqlCommentTest.php | 9 +++++++++ tests/sql/todo_with_dashes_in_string.sql | 1 + 3 files changed, 29 insertions(+), 4 deletions(-) create mode 100644 tests/sql/todo_with_dashes_in_string.sql diff --git a/src/SqlQuery.php b/src/SqlQuery.php index c5276d39..c267110f 100644 --- a/src/SqlQuery.php +++ b/src/SqlQuery.php @@ -36,7 +36,7 @@ final class SqlQuery implements SqlQueryInterface { private const C_STYLE_COMMENT = '/\/\*(.*?)\*\//u'; - private const LINE_COMMENT = '/--[^\r\n]*/'; + private const LINE_COMMENT = '/^\s*--[^\r\n]*/m'; private PDOStatement|null $pdoStatement = null; @@ -124,9 +124,7 @@ private function perform(string $sqlId, array $values, FetchInterface|null $fetc $this->pdoStatement = $pdoStatement; $lastQuery = $pdoStatement->queryString; - // Remove comments only for query type detection, not for execution - $queryForDetection = (string) preg_replace(self::C_STYLE_COMMENT, '', $lastQuery); - $queryForDetection = trim((string) preg_replace(self::LINE_COMMENT, '', $queryForDetection)); + $queryForDetection = $this->removeCommentsForDetection($lastQuery); $isSelect = stripos($queryForDetection, 'select') === 0 || stripos($queryForDetection, 'with') === 0; $result = $isSelect ? $this->fetchAll($pdoStatement, $fetch) : []; /** @var array $values */ @@ -146,6 +144,23 @@ private function fetchAll(PDOStatement $pdoStatement, FetchInterface|null $fetch return $fetch->fetchAll($pdoStatement, $this->injector); } + /** + * Remove comments from SQL query for query type detection + * + * This strips both C-style (/* *\/) and line comments (--) to prevent + * misidentification of query type. The original query sent to the database + * remains unchanged to preserve performance hints and annotations. + */ + private function removeCommentsForDetection(string $sql): string + { + // Remove C-style comments + $sql = (string) preg_replace(self::C_STYLE_COMMENT, '', $sql); + // Remove line comments (-- at start of line, optionally after whitespace) + $sql = (string) preg_replace(self::LINE_COMMENT, '', $sql); + + return trim($sql); + } + /** @return non-empty-array */ private function getSqls(string $sqlFile): array { diff --git a/tests/SqlCommentTest.php b/tests/SqlCommentTest.php index 63b58445..3da08519 100644 --- a/tests/SqlCommentTest.php +++ b/tests/SqlCommentTest.php @@ -66,4 +66,13 @@ public function testUpdateWithInsertComment(): void $result = $this->sqlQuery->getRow('todo_item', ['id' => '1']); $this->assertSame(['id' => '1', 'title' => $updatedTitle], $result); } + + public function testDashesInStringLiteral(): void + { + // Test that -- inside string literals is not stripped + // This query should work correctly despite having -- in the WHERE clause + $result = $this->sqlQuery->getRow('todo_with_dashes_in_string', ['id' => '1']); + // Since 'test--value' won't match 'test', we expect null + $this->assertNull($result); + } } diff --git a/tests/sql/todo_with_dashes_in_string.sql b/tests/sql/todo_with_dashes_in_string.sql new file mode 100644 index 00000000..8bba5502 --- /dev/null +++ b/tests/sql/todo_with_dashes_in_string.sql @@ -0,0 +1 @@ +SELECT * FROM todo WHERE title = 'test--value' AND id = :id; From 89ead58d987ea79af836d426dc0697b4b0e25b4c Mon Sep 17 00:00:00 2001 From: Hajime MATSUMOTO Date: Sun, 23 Nov 2025 12:43:54 +0900 Subject: [PATCH 3/3] Fix use statement ordering in SqlCommentTest Sort use statements alphabetically to comply with coding standards. --- tests/SqlCommentTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/SqlCommentTest.php b/tests/SqlCommentTest.php index 3da08519..e1f601ce 100644 --- a/tests/SqlCommentTest.php +++ b/tests/SqlCommentTest.php @@ -5,13 +5,13 @@ namespace Ray\MediaQuery; use Aura\Sql\ExtendedPdo; +use Pagerfanta\View\DefaultView; use PDO; use PHPUnit\Framework\TestCase; use Ray\AuraSqlModule\Pagerfanta\AuraSqlPager; use Ray\AuraSqlModule\Pagerfanta\AuraSqlPagerFactory; use Ray\Di\Injector; use Ray\InputQuery\ToArray; -use Pagerfanta\View\DefaultView; use function file_get_contents;