From 03c3ca3618acc28b4440047be0895f90daa962ba Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 26 Feb 2024 17:15:07 +0900 Subject: [PATCH 01/17] docs: add empty lines --- user_guide_src/source/models/entities.rst | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/user_guide_src/source/models/entities.rst b/user_guide_src/source/models/entities.rst index 6e683dcafe0b..d3443093d47e 100644 --- a/user_guide_src/source/models/entities.rst +++ b/user_guide_src/source/models/entities.rst @@ -60,9 +60,13 @@ Create the model first at **app/Models/UserModel.php** so that we can interact w .. literalinclude:: entities/002.php -The model uses the ``users`` table in the database for all of its activities. We've set the ``$allowedFields`` property +The model uses the ``users`` table in the database for all of its activities. + +We've set the ``$allowedFields`` property to include all of the fields that we want outside classes to change. The ``id``, ``created_at``, and ``updated_at`` fields -are handled automatically by the class or the database, so we don't want to change those. Finally, we've set our Entity +are handled automatically by the class or the database, so we don't want to change those. + +Finally, we've set our Entity class as the ``$returnType``. This ensures that all methods on the model that return rows from the database will return instances of our User Entity class instead of an object or array like normal. From 053929712f55742341839d6e3e6f42f3b68610a8 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 26 Feb 2024 17:15:38 +0900 Subject: [PATCH 02/17] docs: break long lines --- user_guide_src/source/models/entities.rst | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/user_guide_src/source/models/entities.rst b/user_guide_src/source/models/entities.rst index d3443093d47e..202e39ed7365 100644 --- a/user_guide_src/source/models/entities.rst +++ b/user_guide_src/source/models/entities.rst @@ -62,13 +62,14 @@ Create the model first at **app/Models/UserModel.php** so that we can interact w The model uses the ``users`` table in the database for all of its activities. -We've set the ``$allowedFields`` property -to include all of the fields that we want outside classes to change. The ``id``, ``created_at``, and ``updated_at`` fields -are handled automatically by the class or the database, so we don't want to change those. - -Finally, we've set our Entity -class as the ``$returnType``. This ensures that all methods on the model that return rows from the database will return -instances of our User Entity class instead of an object or array like normal. +We've set the ``$allowedFields`` property to include all of the fields that we +want outside classes to change. The ``id``, ``created_at``, and ``updated_at`` +fields are handled automatically by the class or the database, so we don't want +to change those. + +Finally, we've set our Entity class as the ``$returnType``. This ensures that all +methods on the model that return rows from the database will return instances of +our User Entity class instead of an object or array like normal. Working with the Entity Class ============================= From 25fb054fa2fd55a6db9be779789bee7469d5cf49 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 26 Feb 2024 17:20:05 +0900 Subject: [PATCH 03/17] docs: add description --- user_guide_src/source/models/model.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/user_guide_src/source/models/model.rst b/user_guide_src/source/models/model.rst index d40c07ba6e6b..cdf32f5baeda 100644 --- a/user_guide_src/source/models/model.rst +++ b/user_guide_src/source/models/model.rst @@ -145,7 +145,7 @@ This requires either a DATETIME or INTEGER field in the database as per the mode `$dateFormat`_ setting. The default field name is ``deleted_at`` however this name can be configured to any name of your choice by using `$deletedField`_ property. -.. important:: The ``deleted_at`` field must be nullable. +.. important:: The ``deleted_at`` field in the database must be nullable. $allowedFields -------------- From 8b1e3a0e06ee899e34de2638bda40684e7d74ec8 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 26 Feb 2024 17:22:44 +0900 Subject: [PATCH 04/17] docs: add empty lines --- user_guide_src/source/models/model.rst | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/user_guide_src/source/models/model.rst b/user_guide_src/source/models/model.rst index cdf32f5baeda..91d39cd6a4fe 100644 --- a/user_guide_src/source/models/model.rst +++ b/user_guide_src/source/models/model.rst @@ -124,10 +124,14 @@ $returnType ----------- The Model's CRUD methods will take a step of work away from you and automatically return -the resulting data, instead of the Result object. This setting allows you to define +the resulting data, instead of the Result object. + +This setting allows you to define the type of data that is returned. Valid values are '**array**' (the default), '**object**', or the **fully qualified name of a class** that can be used with the Result object's ``getCustomResultObject()`` -method. Using the special ``::class`` constant of the class will allow most IDEs to +method. + +Using the special ``::class`` constant of the class will allow most IDEs to auto-complete the name and allow functions like refactoring to better understand your code. .. _model-use-soft-deletes: From 16842062bddbf236b541c6accc789d3dd378b82b Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 26 Feb 2024 17:23:23 +0900 Subject: [PATCH 05/17] docs: break long lines --- user_guide_src/source/models/model.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/user_guide_src/source/models/model.rst b/user_guide_src/source/models/model.rst index 91d39cd6a4fe..de188fa74f67 100644 --- a/user_guide_src/source/models/model.rst +++ b/user_guide_src/source/models/model.rst @@ -123,16 +123,16 @@ default value is ``true``. $returnType ----------- -The Model's CRUD methods will take a step of work away from you and automatically return -the resulting data, instead of the Result object. +The Model's CRUD methods will take a step of work away from you and automatically +return the resulting data, instead of the Result object. -This setting allows you to define -the type of data that is returned. Valid values are '**array**' (the default), '**object**', or the **fully -qualified name of a class** that can be used with the Result object's ``getCustomResultObject()`` -method. +This setting allows you to define the type of data that is returned. Valid values +are '**array**' (the default), '**object**', or the **fully qualified name of a class** +that can be used with the Result object's ``getCustomResultObject()`` method. Using the special ``::class`` constant of the class will allow most IDEs to -auto-complete the name and allow functions like refactoring to better understand your code. +auto-complete the name and allow functions like refactoring to better understand +your code. .. _model-use-soft-deletes: From 6615763349f5b1d01f1d0f752af4a7102539d5c4 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 26 Feb 2024 17:23:50 +0900 Subject: [PATCH 06/17] docs: fix description --- user_guide_src/source/models/model.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/user_guide_src/source/models/model.rst b/user_guide_src/source/models/model.rst index de188fa74f67..41a754bc25ee 100644 --- a/user_guide_src/source/models/model.rst +++ b/user_guide_src/source/models/model.rst @@ -123,7 +123,7 @@ default value is ``true``. $returnType ----------- -The Model's CRUD methods will take a step of work away from you and automatically +The Model's **find*()** methods will take a step of work away from you and automatically return the resulting data, instead of the Result object. This setting allows you to define the type of data that is returned. Valid values From a5be00b6be4d1d8857c37e4c01c3056099ef78f4 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 26 Feb 2024 17:37:27 +0900 Subject: [PATCH 07/17] docs: fix misleading description and add note --- user_guide_src/source/models/entities.rst | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/user_guide_src/source/models/entities.rst b/user_guide_src/source/models/entities.rst index 202e39ed7365..bcc0c2ec57d0 100644 --- a/user_guide_src/source/models/entities.rst +++ b/user_guide_src/source/models/entities.rst @@ -68,8 +68,12 @@ fields are handled automatically by the class or the database, so we don't want to change those. Finally, we've set our Entity class as the ``$returnType``. This ensures that all -methods on the model that return rows from the database will return instances of -our User Entity class instead of an object or array like normal. +built-in methods on the model that return rows from the database will return +instances of our User Entity class instead of an object or array like normal. + +.. note:: + Of course, if you add a custom method to your model, you must implement it + so that instances of ``$returnType`` are returned. Working with the Entity Class ============================= From 549db022d033f6590a1fbe956c9498a2208efccf Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 29 Feb 2024 10:43:39 +0900 Subject: [PATCH 08/17] test: fix tests that fails on Feb 29 --- tests/system/I18n/TimeLegacyTest.php | 9 +++++++-- tests/system/I18n/TimeTest.php | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/system/I18n/TimeLegacyTest.php b/tests/system/I18n/TimeLegacyTest.php index 36907988349d..9f013a0ddcbe 100644 --- a/tests/system/I18n/TimeLegacyTest.php +++ b/tests/system/I18n/TimeLegacyTest.php @@ -399,10 +399,15 @@ public function testGetTimestamp(): void public function testGetAge(): void { + // setTestNow() does not work to parse(). $time = TimeLegacy::parse('5 years ago'); - $this->assertSame(5, $time->getAge()); - $this->assertSame(5, $time->age); + // Considers leap year + $now = TimeLegacy::now(); + $expected = ($now->day === '29' && $now->month === '2') ? 4 : 5; + + $this->assertSame($expected, $time->getAge()); + $this->assertSame($expected, $time->age); } public function testAgeNow(): void diff --git a/tests/system/I18n/TimeTest.php b/tests/system/I18n/TimeTest.php index d60fb4410a03..f5bca98268fb 100644 --- a/tests/system/I18n/TimeTest.php +++ b/tests/system/I18n/TimeTest.php @@ -406,10 +406,15 @@ public function testGetTimestamp(): void */ public function testGetAge(): void { + // setTestNow() does not work to parse(). $time = Time::parse('5 years ago'); - $this->assertSame(5, $time->getAge()); - $this->assertSame(5, $time->age); + // Considers leap year + $now = Time::now(); + $expected = ($now->day === '29' && $now->month === '2') ? 4 : 5; + + $this->assertSame($expected, $time->getAge()); + $this->assertSame($expected, $time->age); } public function testAgeNow(): void From a14ea360e203a0d6ae965ea470155e75622db1ca Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 29 Feb 2024 10:45:52 +0900 Subject: [PATCH 09/17] test: remove unnecessary timezone America/Chicago It is wrong to change the timezone only to setTestNow(). Remnants of the old code? Now UTC is the default timezone. --- tests/system/I18n/TimeLegacyTest.php | 8 ++++---- tests/system/I18n/TimeTest.php | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/system/I18n/TimeLegacyTest.php b/tests/system/I18n/TimeLegacyTest.php index 9f013a0ddcbe..5814683ba6c7 100644 --- a/tests/system/I18n/TimeLegacyTest.php +++ b/tests/system/I18n/TimeLegacyTest.php @@ -419,7 +419,7 @@ public function testAgeNow(): void public function testAgeFuture(): void { - TimeLegacy::setTestNow('June 20, 2022', 'America/Chicago'); + TimeLegacy::setTestNow('June 20, 2022'); $time = TimeLegacy::parse('August 12, 2116 4:15:23pm'); $this->assertSame(0, $time->getAge()); @@ -427,7 +427,7 @@ public function testAgeFuture(): void public function testGetAgeSameDayOfBirthday(): void { - TimeLegacy::setTestNow('December 31, 2022', 'America/Chicago'); + TimeLegacy::setTestNow('December 31, 2022'); $time = TimeLegacy::parse('December 31, 2020'); $this->assertSame(2, $time->getAge()); @@ -435,7 +435,7 @@ public function testGetAgeSameDayOfBirthday(): void public function testGetAgeNextDayOfBirthday(): void { - TimeLegacy::setTestNow('January 1, 2022', 'America/Chicago'); + TimeLegacy::setTestNow('January 1, 2022'); $time = TimeLegacy::parse('December 31, 2020'); $this->assertSame(1, $time->getAge()); @@ -443,7 +443,7 @@ public function testGetAgeNextDayOfBirthday(): void public function testGetAgeBeforeDayOfBirthday(): void { - TimeLegacy::setTestNow('December 30, 2021', 'America/Chicago'); + TimeLegacy::setTestNow('December 30, 2021'); $time = TimeLegacy::parse('December 31, 2020'); $this->assertSame(0, $time->getAge()); diff --git a/tests/system/I18n/TimeTest.php b/tests/system/I18n/TimeTest.php index f5bca98268fb..db34590eadeb 100644 --- a/tests/system/I18n/TimeTest.php +++ b/tests/system/I18n/TimeTest.php @@ -426,7 +426,7 @@ public function testAgeNow(): void public function testAgeFuture(): void { - Time::setTestNow('June 20, 2022', 'America/Chicago'); + Time::setTestNow('June 20, 2022'); $time = Time::parse('August 12, 2116 4:15:23pm'); $this->assertSame(0, $time->getAge()); @@ -434,7 +434,7 @@ public function testAgeFuture(): void public function testGetAgeSameDayOfBirthday(): void { - Time::setTestNow('December 31, 2022', 'America/Chicago'); + Time::setTestNow('December 31, 2022'); $time = Time::parse('December 31, 2020'); $this->assertSame(2, $time->getAge()); @@ -442,7 +442,7 @@ public function testGetAgeSameDayOfBirthday(): void public function testGetAgeNextDayOfBirthday(): void { - Time::setTestNow('January 1, 2022', 'America/Chicago'); + Time::setTestNow('January 1, 2022'); $time = Time::parse('December 31, 2020'); $this->assertSame(1, $time->getAge()); @@ -450,7 +450,7 @@ public function testGetAgeNextDayOfBirthday(): void public function testGetAgeBeforeDayOfBirthday(): void { - Time::setTestNow('December 30, 2021', 'America/Chicago'); + Time::setTestNow('December 30, 2021'); $time = Time::parse('December 31, 2020'); $this->assertSame(0, $time->getAge()); From e70b389f16f4e35f521fc011762dc564a3eb4e42 Mon Sep 17 00:00:00 2001 From: "Mark A. Connelly" <16519297+markconnellypro@users.noreply.github.com> Date: Sat, 2 Mar 2024 01:47:04 -0500 Subject: [PATCH 10/17] fix: isWriteType() to recognize CTE; [Postgre] allow beginning whitespace at query start, RETURNING with DELETE --- system/Database/BaseConnection.php | 2 +- system/Database/Postgre/Connection.php | 2 +- .../Database/Live/WriteTypeQueryTest.php | 141 +++++++++++++++++- 3 files changed, 137 insertions(+), 8 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 9b131152210b..00f39efb1be3 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -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|.)+(\s|[)]))?"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|MERGE)\s/i', $sql); } /** diff --git a/system/Database/Postgre/Connection.php b/system/Database/Postgre/Connection.php index 0f761db197b9..1a30e993abfb 100644 --- a/system/Database/Postgre/Connection.php +++ b/system/Database/Postgre/Connection.php @@ -577,7 +577,7 @@ protected function _transRollback(): bool */ public function isWriteType($sql): bool { - if (preg_match('#^(INSERT|UPDATE).*RETURNING\s.+(\,\s?.+)*$#is', $sql)) { + if (preg_match('#^\s*(WITH\s(\s|.)+(\s|[)]))?(INSERT|UPDATE|DELETE).*RETURNING\s.+(\,\s?.+)*$#is', $sql)) { return false; } diff --git a/tests/system/Database/Live/WriteTypeQueryTest.php b/tests/system/Database/Live/WriteTypeQueryTest.php index 9228288ac22d..bf524981563e 100644 --- a/tests/system/Database/Live/WriteTypeQueryTest.php +++ b/tests/system/Database/Live/WriteTypeQueryTest.php @@ -48,11 +48,49 @@ public function testInsert(): void $this->assertTrue($this->db->isWriteType($sql)); - if ($this->db->DBDriver === 'Postgre') { - $sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id;"; + $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)); + + $sql = <<assertFalse($this->db->isWriteType($sql)); + $this->assertTrue($this->db->isWriteType($sql)); + + $assertionType = 'assertTrue'; + if ($this->db->DBDriver === 'Postgre') { + $assertionType = 'assertFalse'; } + + $sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id;"; + + $this->$assertionType($this->db->isWriteType($sql)); + + $sql = "WITH seqvals AS (SELECT '3' AS seqval)INSERT INTO my_table (col1, col2) SELECT 'Joe', seqval FROM seqvals RETURNING id;"; + + $this->$assertionType($this->db->isWriteType($sql)); + + $sql = <<$assertionType($this->db->isWriteType($sql)); + + $sql = <<$assertionType($this->db->isWriteType($sql)); } public function testUpdate(): void @@ -63,11 +101,52 @@ public function testUpdate(): void $this->assertTrue($this->db->isWriteType($sql)); - if ($this->db->DBDriver === 'Postgre') { - $sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2 RETURNING *;"; + $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)); + + $sql = <<assertFalse($this->db->isWriteType($sql)); + $this->assertTrue($this->db->isWriteType($sql)); + + $assertionType = 'assertTrue'; + if ($this->db->DBDriver === 'Postgre') { + $assertionType = 'assertFalse'; } + + $sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2 RETURNING *;"; + + $this->$assertionType($this->db->isWriteType($sql)); + + $sql = "WITH seqvals AS (SELECT '3' AS seqval)UPDATE my_table SET col1 = seqval FROM seqvals WHERE id = 2 RETURNING *;"; + + $this->$assertionType($this->db->isWriteType($sql)); + + $sql = <<$assertionType($this->db->isWriteType($sql)); + + $sql = <<$assertionType($this->db->isWriteType($sql)); } public function testDelete(): void @@ -76,6 +155,56 @@ public function testDelete(): void $sql = $builder->testMode()->delete(['id' => 1], null, true); $this->assertTrue($this->db->isWriteType($sql)); + + $sql = "DELETE FROM my_table WHERE id = 2;"; + + $this->assertTrue($this->db->isWriteType($sql)); + + $sql = "WITH seqvals AS (SELECT '3' AS seqval)DELETE FROM my_table JOIN seqvals ON col1 = seqval;"; + + $this->assertTrue($this->db->isWriteType($sql)); + + $sql = <<assertTrue($this->db->isWriteType($sql)); + + $assertionType = 'assertTrue'; + if ($this->db->DBDriver === 'Postgre') { + $assertionType = 'assertFalse'; + } + + $sql = "DELETE FROM my_table WHERE id = 2 RETURNING *;"; + + $this->$assertionType($this->db->isWriteType($sql)); + + $sql = "WITH seqvals AS (SELECT '3' AS seqval)DELETE FROM my_table JOIN seqvals ON col1 = seqval RETURNING *;"; + + $this->$assertionType($this->db->isWriteType($sql)); + + $sql = <<$assertionType($this->db->isWriteType($sql)); + + $sql = <<$assertionType($this->db->isWriteType($sql)); } public function testReplace(): void From ad4d54d2027856997d9360bc133744ad864d9f66 Mon Sep 17 00:00:00 2001 From: "Mark A. Connelly" <16519297+markconnellypro@users.noreply.github.com> Date: Sat, 2 Mar 2024 03:28:21 -0500 Subject: [PATCH 11/17] Fix linting errors in tests --- .../Database/Live/WriteTypeQueryTest.php | 146 +++++++++--------- 1 file changed, 73 insertions(+), 73 deletions(-) diff --git a/tests/system/Database/Live/WriteTypeQueryTest.php b/tests/system/Database/Live/WriteTypeQueryTest.php index bf524981563e..264528edb7f7 100644 --- a/tests/system/Database/Live/WriteTypeQueryTest.php +++ b/tests/system/Database/Live/WriteTypeQueryTest.php @@ -52,12 +52,12 @@ public function testInsert(): void $this->assertTrue($this->db->isWriteType($sql)); - $sql = <<assertTrue($this->db->isWriteType($sql)); @@ -68,29 +68,29 @@ public function testInsert(): void $sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id;"; - $this->$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); $sql = "WITH seqvals AS (SELECT '3' AS seqval)INSERT INTO my_table (col1, col2) SELECT 'Joe', seqval FROM seqvals RETURNING id;"; - $this->$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); - $sql = <<$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); - $sql = <<$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); } public function testUpdate(): void @@ -105,13 +105,13 @@ public function testUpdate(): void $this->assertTrue($this->db->isWriteType($sql)); - $sql = <<assertTrue($this->db->isWriteType($sql)); @@ -122,31 +122,31 @@ public function testUpdate(): void $sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2 RETURNING *;"; - $this->$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); $sql = "WITH seqvals AS (SELECT '3' AS seqval)UPDATE my_table SET col1 = seqval FROM seqvals WHERE id = 2 RETURNING *;"; - $this->$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); - $sql = <<$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); - $sql = <<$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); } public function testDelete(): void @@ -156,7 +156,7 @@ public function testDelete(): void $this->assertTrue($this->db->isWriteType($sql)); - $sql = "DELETE FROM my_table WHERE id = 2;"; + $sql = 'DELETE FROM my_table WHERE id = 2;'; $this->assertTrue($this->db->isWriteType($sql)); @@ -164,13 +164,13 @@ public function testDelete(): void $this->assertTrue($this->db->isWriteType($sql)); - $sql = <<assertTrue($this->db->isWriteType($sql)); @@ -179,32 +179,32 @@ public function testDelete(): void $assertionType = 'assertFalse'; } - $sql = "DELETE FROM my_table WHERE id = 2 RETURNING *;"; + $sql = 'DELETE FROM my_table WHERE id = 2 RETURNING *;'; - $this->$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); $sql = "WITH seqvals AS (SELECT '3' AS seqval)DELETE FROM my_table JOIN seqvals ON col1 = seqval RETURNING *;"; - $this->$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); - $sql = <<$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); - $sql = <<$assertionType($this->db->isWriteType($sql)); + $this->{$assertionType}($this->db->isWriteType($sql)); } public function testReplace(): void From 8b05cde6870236a9d10653161281de0c1c73ab40 Mon Sep 17 00:00:00 2001 From: "Mark A. Connelly" <16519297+markconnellypro@users.noreply.github.com> Date: Sat, 2 Mar 2024 03:55:14 -0500 Subject: [PATCH 12/17] Refine isWriteType regex between WITH and other operation keyword --- system/Database/BaseConnection.php | 2 +- system/Database/Postgre/Connection.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 00f39efb1be3..b289c1a06a5d 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1657,7 +1657,7 @@ public function resetDataCache() */ public function isWriteType($sql): bool { - return (bool) preg_match('/^\s*(WITH\s(\s|.)+(\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/is', $sql); } /** diff --git a/system/Database/Postgre/Connection.php b/system/Database/Postgre/Connection.php index 1a30e993abfb..3c2f28825388 100644 --- a/system/Database/Postgre/Connection.php +++ b/system/Database/Postgre/Connection.php @@ -577,7 +577,7 @@ protected function _transRollback(): bool */ public function isWriteType($sql): bool { - if (preg_match('#^\s*(WITH\s(\s|.)+(\s|[)]))?(INSERT|UPDATE|DELETE).*RETURNING\s.+(\,\s?.+)*$#is', $sql)) { + if (preg_match('#^\s*(WITH\s.+(\s|[)]))?(INSERT|UPDATE|DELETE).*RETURNING\s.+(\,\s?.+)*$#is', $sql)) { return false; } From 123d987ca6a3f77ac2faa8eab87b7927536a9781 Mon Sep 17 00:00:00 2001 From: "Mark A. Connelly" <16519297+markconnellypro@users.noreply.github.com> Date: Sun, 3 Mar 2024 22:21:22 -0500 Subject: [PATCH 13/17] Separate each test into its own function; add testAssertTypeReturning function for which databases support RETURNING --- .../Database/Live/WriteTypeQueryTest.php | 258 +++++++++++++++--- 1 file changed, 220 insertions(+), 38 deletions(-) diff --git a/tests/system/Database/Live/WriteTypeQueryTest.php b/tests/system/Database/Live/WriteTypeQueryTest.php index 264528edb7f7..befb0327b89b 100644 --- a/tests/system/Database/Live/WriteTypeQueryTest.php +++ b/tests/system/Database/Live/WriteTypeQueryTest.php @@ -28,6 +28,25 @@ final class WriteTypeQueryTest extends CIUnitTestCase protected $refresh = true; protected $seed = CITestSeeder::class; + /** + * Whether CodeIgniter ignores RETURNING for isWriteType. + * + * Currently, only Postgre is supported by CodeIgniter. + * This method should be updated if support for RETURNING + * is expanded to other databases. + * + * @param string $dbDriver + */ + private function testAssertTypeReturning($dbDriver): string + { + $assertType = 'assertTrue'; + if ($dbDriver === 'Postgre') { + $assertType = 'assertFalse'; + } + + return $assertType; + } + public function testSet(): void { $sql = 'SET FOREIGN_KEY_CHECKS=0'; @@ -35,7 +54,7 @@ public function testSet(): void $this->assertTrue($this->db->isWriteType($sql)); } - public function testInsert(): void + public function testInsertBuilder(): void { $builder = $this->db->table('jobs'); @@ -47,11 +66,42 @@ public function testInsert(): void $sql = $builder->getCompiledInsert(); $this->assertTrue($this->db->isWriteType($sql)); + } + + public function testInsertOne(): void + { + $sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool');"; + + $this->assertTrue($this->db->isWriteType($sql)); + } + + public function testInsertMulti(): void + { + $sql = <<<'SQL' + INSERT INTO my_table (col1, col2) + VALUES ('Joe', 'Cool') + RETURNING id; + 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 testInsertWithMulti(): void + { $sql = <<<'SQL' WITH seqvals AS (SELECT '3' AS seqval) INSERT INTO my_table (col1, col2) @@ -60,28 +110,50 @@ public function testInsert(): void SQL; $this->assertTrue($this->db->isWriteType($sql)); + } - $assertionType = 'assertTrue'; - if ($this->db->DBDriver === 'Postgre') { - $assertionType = 'assertFalse'; - } - + public function testInsertOneReturning(): void + { $sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id;"; - $this->{$assertionType}($this->db->isWriteType($sql)); - - $sql = "WITH seqvals AS (SELECT '3' AS seqval)INSERT INTO my_table (col1, col2) SELECT 'Joe', seqval FROM seqvals RETURNING id;"; + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - $this->{$assertionType}($this->db->isWriteType($sql)); + $this->{$assertType}($this->db->isWriteType($sql)); + } + public function testInsertMultiReturning(): void + { $sql = <<<'SQL' INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id; SQL; - $this->{$assertionType}($this->db->isWriteType($sql)); + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($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;"; + + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($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;"; + + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($this->db->isWriteType($sql)); + } + public function testInsertWithMultiReturning(): void + { $sql = <<<'SQL' WITH seqvals AS (SELECT '3' AS seqval) INSERT INTO my_table (col1, col2) @@ -90,21 +162,54 @@ public function testInsert(): void RETURNING id; SQL; - $this->{$assertionType}($this->db->isWriteType($sql)); + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($this->db->isWriteType($sql)); } - public function testUpdate(): void + 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)); + } + + public function testUpdateOne(): void + { + $sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2;"; + + $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 @@ -114,20 +219,19 @@ public function testUpdate(): void SQL; $this->assertTrue($this->db->isWriteType($sql)); + } - $assertionType = 'assertTrue'; - if ($this->db->DBDriver === 'Postgre') { - $assertionType = 'assertFalse'; - } - + public function testUpdateOneReturning(): void + { $sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2 RETURNING *;"; - $this->{$assertionType}($this->db->isWriteType($sql)); - - $sql = "WITH seqvals AS (SELECT '3' AS seqval)UPDATE my_table SET col1 = seqval FROM seqvals WHERE id = 2 RETURNING *;"; + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - $this->{$assertionType}($this->db->isWriteType($sql)); + $this->{$assertType}($this->db->isWriteType($sql)); + } + public function testUpdateMultiReturning(): void + { $sql = <<<'SQL' UPDATE my_table SET col1 = 'foo' @@ -135,8 +239,31 @@ public function testUpdate(): void RETURNING *; SQL; - $this->{$assertionType}($this->db->isWriteType($sql)); + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($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 *;"; + + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($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 *;"; + + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + $this->{$assertType}($this->db->isWriteType($sql)); + } + + public function testUpdateWithMultiReturning(): void + { $sql = <<<'SQL' WITH seqvals AS (SELECT '3' AS seqval) UPDATE my_table @@ -146,24 +273,52 @@ public function testUpdate(): void RETURNING *; SQL; - $this->{$assertionType}($this->db->isWriteType($sql)); + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($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) @@ -173,28 +328,50 @@ public function testDelete(): void SQL; $this->assertTrue($this->db->isWriteType($sql)); + } - $assertionType = 'assertTrue'; - if ($this->db->DBDriver === 'Postgre') { - $assertionType = 'assertFalse'; - } - + public function testDeleteOneReturning(): void + { $sql = 'DELETE FROM my_table WHERE id = 2 RETURNING *;'; - $this->{$assertionType}($this->db->isWriteType($sql)); - - $sql = "WITH seqvals AS (SELECT '3' AS seqval)DELETE FROM my_table JOIN seqvals ON col1 = seqval RETURNING *;"; + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - $this->{$assertionType}($this->db->isWriteType($sql)); + $this->{$assertType}($this->db->isWriteType($sql)); + } + public function testDeleteMultiReturning(): void + { $sql = <<<'SQL' DELETE FROM my_table WHERE id = 2 RETURNING *; SQL; - $this->{$assertionType}($this->db->isWriteType($sql)); + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($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 *;"; + + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($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 *;"; + + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($this->db->isWriteType($sql)); + } + public function testDeleteWithMultiReturning(): void + { $sql = <<<'SQL' WITH seqvals AS (SELECT '3' AS seqval) @@ -204,7 +381,9 @@ public function testDelete(): void RETURNING *; SQL; - $this->{$assertionType}($this->db->isWriteType($sql)); + $assertType = $this->testAssertTypeReturning($this->db->DBDriver); + + $this->{$assertType}($this->db->isWriteType($sql)); } public function testReplace(): void @@ -225,19 +404,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)); From 8e1f62fc2670a0db2e7a4241d7ad846fab17f3fb Mon Sep 17 00:00:00 2001 From: "Mark A. Connelly" <16519297+markconnellypro@users.noreply.github.com> Date: Sun, 3 Mar 2024 22:25:18 -0500 Subject: [PATCH 14/17] Remove RETURNING from testInsertMulti() --- tests/system/Database/Live/WriteTypeQueryTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/system/Database/Live/WriteTypeQueryTest.php b/tests/system/Database/Live/WriteTypeQueryTest.php index befb0327b89b..2ab956781d7f 100644 --- a/tests/system/Database/Live/WriteTypeQueryTest.php +++ b/tests/system/Database/Live/WriteTypeQueryTest.php @@ -79,8 +79,7 @@ public function testInsertMulti(): void { $sql = <<<'SQL' INSERT INTO my_table (col1, col2) - VALUES ('Joe', 'Cool') - RETURNING id; + VALUES ('Joe', 'Cool'); SQL; $this->assertTrue($this->db->isWriteType($sql)); From d2c6162ed3e4ccdfda0873323116addb4f62d41d Mon Sep 17 00:00:00 2001 From: "Mark A. Connelly" <16519297+markconnellypro@users.noreply.github.com> Date: Sun, 3 Mar 2024 23:12:12 -0500 Subject: [PATCH 15/17] Fix rector errors in tests --- tests/system/Database/Live/WriteTypeQueryTest.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/system/Database/Live/WriteTypeQueryTest.php b/tests/system/Database/Live/WriteTypeQueryTest.php index 2ab956781d7f..3a4e5a4ba50c 100644 --- a/tests/system/Database/Live/WriteTypeQueryTest.php +++ b/tests/system/Database/Live/WriteTypeQueryTest.php @@ -39,12 +39,11 @@ final class WriteTypeQueryTest extends CIUnitTestCase */ private function testAssertTypeReturning($dbDriver): string { - $assertType = 'assertTrue'; if ($dbDriver === 'Postgre') { - $assertType = 'assertFalse'; + return 'assertFalse'; } - return $assertType; + return 'assertTrue'; } public function testSet(): void From b0dd905a68e83b6ccfad362ba488bd632ed07eab Mon Sep 17 00:00:00 2001 From: "Mark A. Connelly" <16519297+markconnellypro@users.noreply.github.com> Date: Sun, 3 Mar 2024 23:30:17 -0500 Subject: [PATCH 16/17] Change testAssertTypeReturning to testReturning; remove dynamic $assertType and instead use if statement --- .../Database/Live/WriteTypeQueryTest.php | 132 +++++++++++------- 1 file changed, 79 insertions(+), 53 deletions(-) diff --git a/tests/system/Database/Live/WriteTypeQueryTest.php b/tests/system/Database/Live/WriteTypeQueryTest.php index 3a4e5a4ba50c..df3ea16d02c6 100644 --- a/tests/system/Database/Live/WriteTypeQueryTest.php +++ b/tests/system/Database/Live/WriteTypeQueryTest.php @@ -29,21 +29,17 @@ final class WriteTypeQueryTest extends CIUnitTestCase protected $seed = CITestSeeder::class; /** - * Whether CodeIgniter ignores RETURNING for isWriteType. + * Whether CodeIgniter considers RETURNING in isWriteType. * * Currently, only Postgre is supported by CodeIgniter. * This method should be updated if support for RETURNING - * is expanded to other databases. + * is expanded for other databases. * * @param string $dbDriver */ - private function testAssertTypeReturning($dbDriver): string + private function testReturning($dbDriver): bool { - if ($dbDriver === 'Postgre') { - return 'assertFalse'; - } - - return 'assertTrue'; + return $dbDriver === 'Postgre'; } public function testSet(): void @@ -114,9 +110,11 @@ public function testInsertOneReturning(): void { $sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id;"; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testInsertMultiReturning(): void @@ -127,27 +125,33 @@ public function testInsertMultiReturning(): void RETURNING id; SQL; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($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;"; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($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;"; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testInsertWithMultiReturning(): void @@ -160,9 +164,11 @@ public function testInsertWithMultiReturning(): void RETURNING id; SQL; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testUpdateBuilder(): void @@ -223,9 +229,11 @@ public function testUpdateOneReturning(): void { $sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2 RETURNING *;"; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testUpdateMultiReturning(): void @@ -237,27 +245,33 @@ public function testUpdateMultiReturning(): void RETURNING *; SQL; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($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 *;"; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($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 *;"; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testUpdateWithMultiReturning(): void @@ -271,9 +285,11 @@ public function testUpdateWithMultiReturning(): void RETURNING *; SQL; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testDeleteBuilder(): void @@ -332,9 +348,11 @@ public function testDeleteOneReturning(): void { $sql = 'DELETE FROM my_table WHERE id = 2 RETURNING *;'; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testDeleteMultiReturning(): void @@ -345,27 +363,33 @@ public function testDeleteMultiReturning(): void RETURNING *; SQL; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($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 *;"; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($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 *;"; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testDeleteWithMultiReturning(): void @@ -379,9 +403,11 @@ public function testDeleteWithMultiReturning(): void RETURNING *; SQL; - $assertType = $this->testAssertTypeReturning($this->db->DBDriver); - - $this->{$assertType}($this->db->isWriteType($sql)); + if ($this->testReturning($this->db->DBDriver)) { + $this->assertFalse($this->db->isWriteType($sql)); + } else { + $this->assertTrue($this->db->isWriteType($sql)); + } } public function testReplace(): void From feb7f81ec0ff4de6da3e66cc8ad8aebb4efb5a9d Mon Sep 17 00:00:00 2001 From: "Mark A. Connelly" <16519297+markconnellypro@users.noreply.github.com> Date: Mon, 4 Mar 2024 00:20:00 -0500 Subject: [PATCH 17/17] Remove isWriteType override for Postgre; Omit RETURNING from isWriteType for all database types; Modify tests for uniform handling for all database types --- system/Database/BaseConnection.php | 2 +- system/Database/Postgre/Connection.php | 16 --- .../Database/Live/WriteTypeQueryTest.php | 104 +++--------------- 3 files changed, 16 insertions(+), 106 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index b289c1a06a5d..ae5c4b3805f1 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1657,7 +1657,7 @@ public function resetDataCache() */ public function isWriteType($sql): bool { - 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/is', $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); } /** diff --git a/system/Database/Postgre/Connection.php b/system/Database/Postgre/Connection.php index 3c2f28825388..d488c96ec1d4 100644 --- a/system/Database/Postgre/Connection.php +++ b/system/Database/Postgre/Connection.php @@ -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('#^\s*(WITH\s.+(\s|[)]))?(INSERT|UPDATE|DELETE).*RETURNING\s.+(\,\s?.+)*$#is', $sql)) { - return false; - } - - return parent::isWriteType($sql); - } } diff --git a/tests/system/Database/Live/WriteTypeQueryTest.php b/tests/system/Database/Live/WriteTypeQueryTest.php index df3ea16d02c6..fe59f19ffd2e 100644 --- a/tests/system/Database/Live/WriteTypeQueryTest.php +++ b/tests/system/Database/Live/WriteTypeQueryTest.php @@ -28,20 +28,6 @@ final class WriteTypeQueryTest extends CIUnitTestCase protected $refresh = true; protected $seed = CITestSeeder::class; - /** - * Whether CodeIgniter considers RETURNING in isWriteType. - * - * Currently, only Postgre is supported by CodeIgniter. - * This method should be updated if support for RETURNING - * is expanded for other databases. - * - * @param string $dbDriver - */ - private function testReturning($dbDriver): bool - { - return $dbDriver === 'Postgre'; - } - public function testSet(): void { $sql = 'SET FOREIGN_KEY_CHECKS=0'; @@ -110,11 +96,7 @@ public function testInsertOneReturning(): void { $sql = "INSERT INTO my_table (col1, col2) VALUES ('Joe', 'Cool') RETURNING id;"; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testInsertMultiReturning(): void @@ -125,33 +107,21 @@ public function testInsertMultiReturning(): void RETURNING id; SQL; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($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;"; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $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;"; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testInsertWithMultiReturning(): void @@ -164,11 +134,7 @@ public function testInsertWithMultiReturning(): void RETURNING id; SQL; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testUpdateBuilder(): void @@ -229,11 +195,7 @@ public function testUpdateOneReturning(): void { $sql = "UPDATE my_table SET col1 = 'foo' WHERE id = 2 RETURNING *;"; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testUpdateMultiReturning(): void @@ -245,33 +207,21 @@ public function testUpdateMultiReturning(): void RETURNING *; SQL; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($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 *;"; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $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 *;"; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testUpdateWithMultiReturning(): void @@ -285,11 +235,7 @@ public function testUpdateWithMultiReturning(): void RETURNING *; SQL; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testDeleteBuilder(): void @@ -348,11 +294,7 @@ public function testDeleteOneReturning(): void { $sql = 'DELETE FROM my_table WHERE id = 2 RETURNING *;'; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testDeleteMultiReturning(): void @@ -363,33 +305,21 @@ public function testDeleteMultiReturning(): void RETURNING *; SQL; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($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 *;"; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $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 *;"; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testDeleteWithMultiReturning(): void @@ -403,11 +333,7 @@ public function testDeleteWithMultiReturning(): void RETURNING *; SQL; - if ($this->testReturning($this->db->DBDriver)) { - $this->assertFalse($this->db->isWriteType($sql)); - } else { - $this->assertTrue($this->db->isWriteType($sql)); - } + $this->assertFalse($this->db->isWriteType($sql)); } public function testReplace(): void