Skip to content

fix: isWriteType() to recognize CTE; always excluding RETURNING #8599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 7, 2024
Merged

fix: isWriteType() to recognize CTE; always excluding RETURNING #8599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e70b389
fix: isWriteType() to recognize CTE; [Postgre] allow beginning whites…
markconnellypro Mar 2, 2024
ad4d54d
Fix linting errors in tests
markconnellypro Mar 2, 2024
8b05cde
Refine isWriteType regex between WITH and other operation keyword
markconnellypro Mar 2, 2024
123d987
Separate each test into its own function;
markconnellypro Mar 4, 2024
8e1f62f
Remove RETURNING from testInsertMulti()
markconnellypro Mar 4, 2024
d2c6162
Fix rector errors in tests
markconnellypro Mar 4, 2024
b0dd905
Change testAssertTypeReturning to testReturning; remove dynamic $asse…
markconnellypro Mar 4, 2024
feb7f81
Remove isWriteType override for Postgre;
markconnellypro Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion system/Database/BaseConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1657,7 +1657,7 @@ public function resetDataCache()
*/
public function isWriteType($sql): bool
{
return (bool) preg_match('/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|MERGE)\s/i', $sql);
return (bool) preg_match('/^\s*(WITH\s.+(\s|[)]))?"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|MERGE)\s(?!.*\sRETURNING\s)/is', $sql);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why "? is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, but since it was already there, I left it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it came from CI3.
https://github.com/bcit-ci/CodeIgniter/blob/70d0a0edbee5e5814aefddb77f67628e68de080f/system/database/DB_driver.php#L999
So it is not easy to remove it.

But I don't imagine a use case. And it seems there is no test case for it.

}

/**
Expand Down
16 changes: 0 additions & 16 deletions system/Database/Postgre/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,20 +567,4 @@ protected function _transRollback(): bool
{
return (bool) pg_query($this->connID, 'ROLLBACK');
}

/**
* Determines if a query is a "write" type.
*
* Overrides BaseConnection::isWriteType, adding additional read query types.
*
* @param string $sql
*/
public function isWriteType($sql): bool
{
if (preg_match('#^(INSERT|UPDATE).*RETURNING\s.+(\,\s?.+)*$#is', $sql)) {
return false;
}

return parent::isWriteType($sql);
}
}
287 changes: 274 additions & 13 deletions tests/system/Database/Live/WriteTypeQueryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function testSet(): void
$this->assertTrue($this->db->isWriteType($sql));
}

public function testInsert(): void
public function testInsertBuilder(): void
{
$builder = $this->db->table('jobs');

Expand All @@ -47,37 +47,295 @@ public function testInsert(): void
$sql = $builder->getCompiledInsert();

$this->assertTrue($this->db->isWriteType($sql));
}

if ($this->db->DBDriver === 'Postgre') {
$sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id;";
public function testInsertOne(): void
{
$sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool');";

$this->assertFalse($this->db->isWriteType($sql));
}
$this->assertTrue($this->db->isWriteType($sql));
}

public function testInsertMulti(): void
{
$sql = <<<'SQL'
INSERT INTO my_table (col1, col2)
VALUES ('Joe', 'Cool');
SQL;

$this->assertTrue($this->db->isWriteType($sql));
}

public function testInsertWithOne(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval) INSERT INTO my_table (col1, col2) SELECT 'Joe', seqval FROM seqvals;";

$this->assertTrue($this->db->isWriteType($sql));
}

public function testInsertWithOneNoSpace(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval)INSERT INTO my_table (col1, col2) SELECT 'Joe', seqval FROM seqvals;";

$this->assertTrue($this->db->isWriteType($sql));
}

public function testUpdate(): void
public function testInsertWithMulti(): void
{
$sql = <<<'SQL'
WITH seqvals AS (SELECT '3' AS seqval)
INSERT INTO my_table (col1, col2)
SELECT 'Joe', seqval
FROM seqvals;
SQL;

$this->assertTrue($this->db->isWriteType($sql));
}

public function testInsertOneReturning(): void
{
$sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id;";

$this->assertFalse($this->db->isWriteType($sql));
}

public function testInsertMultiReturning(): void
{
$sql = <<<'SQL'
INSERT INTO my_table (col1, col2)
VALUES ('Joe', 'Cool')
RETURNING id;
SQL;

$this->assertFalse($this->db->isWriteType($sql));
}

public function testInsertWithOneReturning(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval) INSERT INTO my_table (col1, col2) SELECT 'Joe', seqval FROM seqvals RETURNING id;";

$this->assertFalse($this->db->isWriteType($sql));
}

public function testInsertWithOneReturningNoSpace(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval)INSERT INTO my_table (col1, col2) SELECT 'Joe', seqval FROM seqvals RETURNING id;";

$this->assertFalse($this->db->isWriteType($sql));
}

public function testInsertWithMultiReturning(): void
{
$sql = <<<'SQL'
WITH seqvals AS (SELECT '3' AS seqval)
INSERT INTO my_table (col1, col2)
SELECT 'Joe', seqval
FROM seqvals
RETURNING id;
SQL;

$this->assertFalse($this->db->isWriteType($sql));
}

public function testUpdateBuilder(): void
{
$builder = new BaseBuilder('jobs', $this->db);
$builder->testMode()->where('id', 1)->update(['name' => 'Programmer'], null, null);
$sql = $builder->getCompiledInsert();

$this->assertTrue($this->db->isWriteType($sql));
}

if ($this->db->DBDriver === 'Postgre') {
$sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2 RETURNING *;";
public function testUpdateOne(): void
{
$sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2;";

$this->assertFalse($this->db->isWriteType($sql));
}
$this->assertTrue($this->db->isWriteType($sql));
}

public function testUpdateMulti(): void
{
$sql = <<<'SQL'
UPDATE my_table
SET col1 = 'foo'
WHERE id = 2;
SQL;

$this->assertTrue($this->db->isWriteType($sql));
}

public function testUpdateWithOne(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval) UPDATE my_table SET col1 = seqval FROM seqvals WHERE id = 2;";

$this->assertTrue($this->db->isWriteType($sql));
}

public function testUpdateWithOneNoSpace(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval)UPDATE my_table SET col1 = seqval FROM seqvals WHERE id = 2;";

$this->assertTrue($this->db->isWriteType($sql));
}

public function testUpdateWithMulti(): void
{
$sql = <<<'SQL'
WITH seqvals AS (SELECT '3' AS seqval)
UPDATE my_table
SET col1 = seqval
FROM seqvals
WHERE id = 2;
SQL;

$this->assertTrue($this->db->isWriteType($sql));
}

public function testUpdateOneReturning(): void
{
$sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2 RETURNING *;";

$this->assertFalse($this->db->isWriteType($sql));
}

public function testUpdateMultiReturning(): void
{
$sql = <<<'SQL'
UPDATE my_table
SET col1 = 'foo'
WHERE id = 2
RETURNING *;
SQL;

$this->assertFalse($this->db->isWriteType($sql));
}

public function testUpdateWithOneReturning(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval) UPDATE my_table SET col1 = seqval FROM seqvals WHERE id = 2 RETURNING *;";

$this->assertFalse($this->db->isWriteType($sql));
}

public function testUpdateWithOneReturningNoSpace(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval)UPDATE my_table SET col1 = seqval FROM seqvals WHERE id = 2 RETURNING *;";

$this->assertFalse($this->db->isWriteType($sql));
}

public function testUpdateWithMultiReturning(): void
{
$sql = <<<'SQL'
WITH seqvals AS (SELECT '3' AS seqval)
UPDATE my_table
SET col1 = seqval
FROM seqvals
WHERE id = 2
RETURNING *;
SQL;

$this->assertFalse($this->db->isWriteType($sql));
}

public function testDelete(): void
public function testDeleteBuilder(): void
{
$builder = $this->db->table('jobs');
$sql = $builder->testMode()->delete(['id' => 1], null, true);

$this->assertTrue($this->db->isWriteType($sql));
}

public function testDeleteOne(): void
{
$sql = 'DELETE FROM my_table WHERE id = 2;';

$this->assertTrue($this->db->isWriteType($sql));
}

public function testDeleteMulti(): void
{
$sql = <<<'SQL'
DELETE FROM my_table
WHERE id = 2;
SQL;

$this->assertTrue($this->db->isWriteType($sql));
}

public function testDeleteWithOne(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval) DELETE FROM my_table JOIN seqvals ON col1 = seqval;";

$this->assertTrue($this->db->isWriteType($sql));
}

public function testDeleteWithOneNoSpace(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval)DELETE FROM my_table JOIN seqvals ON col1 = seqval;";

$this->assertTrue($this->db->isWriteType($sql));
}

public function testDeleteWithMulti(): void
{
$sql = <<<'SQL'
WITH seqvals AS
(SELECT '3' AS seqval)
DELETE FROM my_table
JOIN seqvals
ON col1 = seqval;
SQL;

$this->assertTrue($this->db->isWriteType($sql));
}

public function testDeleteOneReturning(): void
{
$sql = 'DELETE FROM my_table WHERE id = 2 RETURNING *;';

$this->assertFalse($this->db->isWriteType($sql));
}

public function testDeleteMultiReturning(): void
{
$sql = <<<'SQL'
DELETE FROM my_table
WHERE id = 2
RETURNING *;
SQL;

$this->assertFalse($this->db->isWriteType($sql));
}

public function testDeleteWithOneReturning(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval) DELETE FROM my_table JOIN seqvals ON col1 = seqval RETURNING *;";

$this->assertFalse($this->db->isWriteType($sql));
}

public function testDeleteWithOneReturningNoSpace(): void
{
$sql = "WITH seqvals AS (SELECT '3' AS seqval)DELETE FROM my_table JOIN seqvals ON col1 = seqval RETURNING *;";

$this->assertFalse($this->db->isWriteType($sql));
}

public function testDeleteWithMultiReturning(): void
{
$sql = <<<'SQL'
WITH seqvals AS
(SELECT '3' AS seqval)
DELETE FROM my_table
JOIN seqvals
ON col1 = seqval
RETURNING *;
SQL;

$this->assertFalse($this->db->isWriteType($sql));
}

public function testReplace(): void
{
if (in_array($this->db->DBDriver, ['Postgre', 'SQLSRV'], true)) {
Expand All @@ -96,19 +354,22 @@ public function testReplace(): void
$this->assertTrue($this->db->isWriteType($sql));
}

public function testCreate(): void
public function testCreateDatabase(): void
{
$sql = 'CREATE DATABASE foo';

$this->assertTrue($this->db->isWriteType($sql));
}

public function testDrop(): void
public function testDropDatabase(): void
{
$sql = 'DROP DATABASE foo';

$this->assertTrue($this->db->isWriteType($sql));
}

public function testDropTable(): void
{
$sql = 'DROP TABLE foo';

$this->assertTrue($this->db->isWriteType($sql));
Expand Down