Merge pull request #76 from hajime-matsumoto/fix/sql-line-comment-detection

Fix line comment handling in query type detection

Author: koriym

src/SqlQuery.php

@@ -36,6 +36,7 @@
 final class SqlQuery implements SqlQueryInterface
 {
     private const C_STYLE_COMMENT = '/\/\*(.*?)\*\//u';
+    private const LINE_COMMENT = '/^\s*--[^\r\n]*/m';
 
     private PDOStatement|null $pdoStatement = null;
 
@@ -123,8 +124,8 @@ 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;
+        $queryForDetection = $this->removeCommentsForDetection($lastQuery);
+        $isSelect = stripos($queryForDetection, 'select') === 0 || stripos($queryForDetection, 'with') === 0;
         $result = $isSelect ? $this->fetchAll($pdoStatement, $fetch) : [];
         /** @var array<string, mixed> $values */
         $this->logger->log($sqlId, $values);
@@ -143,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<string> */
     private function getSqls(string $sqlFile): array
     {

tests/SqlCommentTest.php

@@ -0,0 +1,78 @@
+<?php
+
+declare(strict_types=1);
+
+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 function file_get_contents;
+
+class SqlCommentTest extends TestCase
+{
+    private SqlQuery $sqlQuery;
+
+    /** @var array<string, mixed> */
+    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);
+    }
+
+    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);
+    }
+}

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
 )

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
 )

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);

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;

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;

tests/sql/todo_with_dashes_in_string.sql

@@ -0,0 +1 @@
+SELECT * FROM todo WHERE title = 'test--value' AND id = :id;