Bug 4069 - query returns Result even when query result is FALSE #4176
Conversation
|
Looking at the test errors, it would appear that some of the tests expect a
The errors from my last commit: For example, in DeleteModelTest, we see this: public function testDeleteFail(): void
{
$this->setPrivateProperty($this->db, 'DBDebug', false);
$this->createModel(JobModel::class);
$this->seeInDatabase('job', ['name' => 'Developer']);
$result = $this->model->where('name123', 'Developer')->delete();
$this->assertFalse($result->resultID);
$this->seeInDatabase('job', ['name' => 'Developer']);
}Neither model docs nor the query builder docs give any details about the return values of their respective delete methods. We can see by reading the source that the Model delete method utilizes either the builder::update or builder::delete functions. We can see from the update and delete source code that those functions return the result of I would point out that the testDeleteWithSoftDeleteFail test doesn't expect a result object, it expects a boolean. This boolean is currently extracted in the BaseBuilder::update function where the code anticipates a ResultInterface object even though the query is not a read-type query. This behavior contradicts the behavior of the query function specified in the documentation. I haven't troubled yet to examine the other test cases. I am hoping to get some feedback from the project bosses before I charge too far down this rabbit hole. Clearly, my pull request isn't ready yet. I'd like to further modify the query method to reflect the behavior specified in the documentation: a ResultInterface object for "read" type queries, a boolean for "write" type queries. We'd also need to locate and fix any code that anticipates a ResultInterface object for a "read" type query. |
--- a/tests/system/Database/Live/BadQueryTest.php
+++ b/tests/system/Database/Live/BadQueryTest.php
@@ -15,6 +15,7 @@ class BadQueryTest extends CIDatabaseTestCase
{
// this throws an exception in this testing environment, but in production it'll return FALSE
// perhaps check $this->db->DBDebug for different test?
+ $this->setPrivateProperty($this->db, 'DBDebug', false);
$query = $this->db->query('SELECT * FROM table_does_not_exist');
$this->assertEquals(false, $query);
} |
|
Thanks @paulbalandan for requesting a review from @michalsn This commit is clearly not ready -- aside from the nearly trivial change to BaseConnection::query, some classes and tests that use this function will need to be examined and the function should probably be further altered to return
It might be a bit of a chore to return an actual boolean -- bad query
UPDATE table_does_not_exist SET name='Developers' WHERE id=1
-- good query
UPDATE db_job SET name='Developer' WHERE id=1The results of the various native DBMS functions are as follows:
I was thinking of mimicking the old CI3 function which returns TRUE for successful write-type queries (using a function to detect them) or which can be forced through an optional parameter to return a result object. Any comments welcome. |
|
I think CI3's behavior is reasonable. The CI4 User Guide says like the following, and no contradictions with it.
|
|
Thanks, @sneakyimp for your research on the subject! Sorry that I'm answering so late, but I haven't had time to properly sit and read about this problem earlier. I guess we can do something like this: if ($query->isWriteType())
{
return false;
}
return new $resultClass($this->connID, $this->resultID);and handle the scenario similarly when the query is successful. I wouldn't worry about BC, since we're fixing a bug here. |
|
To start, I was hoping to define one centralized isWriteType function but the existing CI4 codebase has a bit of a code redundancy issue. It defines isWriteType in three different places, none of which is accessible to the other: The SQLite3 and Query classes use an identical regex. The SQLSRV is almost identical but adds MERGE queries as well like the old CI3 function. I've taken the liberty of adding a static It looks like there's a phpunit test method defined in tests/system/Database/BaseQueryTest, I don't think we need to check I'm looking through the source code for references to resultID to determine which should change and am not entirely sure in some cases: Mock/MockConnection I made a couple of changes to the unit tests impacted by the changes. I've also tried to improve the return types specified for the various insert/delete/query functions to reflect the actual possibilities. |
|
So it looks like the badQueryTest I've added needs some massage. This behaves very differently depending on whether $query = $this->db->query('SELECT * FROM table_does_not_exist');
If |
|
I have modified my BadQueryTest in an attempt to make it check for the public function testBadQuery()
{
if ($this->db->DBDebug) {
// expect an exception, class and message varies by DBMS
$this->expectException(\Exception::class);
$query = $this->db->query('SELECT * FROM table_does_not_exist');
} else {
// this throws an exception in this testing environment, but in production it'll return FALSE
// perhaps check $this->db->DBDebug for different test?
$query = $this->db->query('SELECT * FROM table_does_not_exist');
$this->assertEquals(false, $query);
}
}I wonder if the code should maybe have a try/catch block somewhere to normalize the exception thrown into a single DatabaseException type? Please advise. |
|
The commit I just pushed is resulting in a Rector failure. It looks like Rector is removing a single line? I am fairly mystified at the change it's making, which doesn't seem consistent with the rest of the file -- why would it delete that one blank line when there are so many other identical ones? Also, I've tried running Rector on the system folder locally but it hangs on test 231 of 1148: |
|
Thank you for your investigation. It seems CI4 is not properly designed for exceptions. |
Should we perhaps start a separate issue/bug report for the lack of exception standardization? I don't imagine it would be too difficult to roll that change in with the ones I've already made here. Also, @kenjis, could you suggest any way to deal with the Rector test that is failing? It's not clear to me what change it wants to make -- if it's just trying to delete a single blank line, that's pretty confusing because there are many other blank lines just like that one already in the file which do not get changed. Also, I'm curious why I cannot get Rector to complete locally. It freezes when I try to run it and will not complete. |
$ git fetch codeigniter
$ git checkout develop
$ git merge codeigniter/develop
$ composer update
$ vendor/bin/rector process tests
Rector 0.9.21
Config file: rector.php
235/1164 [▓▓▓▓▓░░░░░░░░░░░░░░░░░░░░░░░] 20%The progress rate doesn't move from there. |
|
The command was not the same in GitHub Actions. $ vendor/bin/rector process --dry-run
Rector 0.9.21
Config file: rector.php
1500/1500 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
[OK] Rector is done! No problem. @sneakyimp Why don't you rebase this branch? |
|
@kenjis I'm confused by your posts. I hope you might clarify some things for me:
Any additional detail would be much appreciated. |
|
@sneakyimp Okay. Your base CI4 is very old. So your rector's version is old even you run Author: Abdul Malik Ikhsan <samsonasik@gmail.com>
Date: Tue Jan 5 02:10:45 2021 +0700
[Static Analysis] Use Rector 0.9 for 4.1 branch with php version feature 7.3
diff --git a/composer.json b/composer.json
index 9516cd390..a845905ef 100644
--- a/composer.json
+++ b/composer.json
@@ -21,7 +21,7 @@
"phpstan/phpstan": "^0.12",
"phpunit/phpunit": "^9.1",
"predis/predis": "^1.1",
- "rector/rector": "^0.8",
+ "rector/rector": "^0.9",
"squizlabs/php_codesniffer": "^3.3"
},
"suggest": {
So rebasing helps you. I'm not sure what the rector error is. But it was the past error. And you are not familiar with git. I would show you how to do it. |
|
Clone your repos: Add remote branch Fetch the latest CI4 (remote branch): Checkout your PR branch: Rebase your branch: |
|
Thank you!
I guess 13 days is pretty old. I see that the latest commit to the develop branch is a mere 8 hours ago.
You've been quite helpful. Let me read up on my own and I think I can figure it out. |
|
@kenjis I have rebased as you so kindly suggested. Sadly, I cannot run I think it's a mistake to remove support for php 7.2. |
|
Unfortunately CI 4.1 requires PHP 7.3. And it was decided in the past. commit 22e34b190f24ff21735f6dcf7bb2671181234165
Author: MGatner <mgatner@icloud.com>
Date: Fri Oct 30 18:13:27 2020 +0000
Bump PHP req, PHPUnit version
diff --git a/admin/framework/composer.json b/admin/framework/composer.json
index df3468dba..e5e88487c 100644
--- a/admin/framework/composer.json
+++ b/admin/framework/composer.json
@@ -5,7 +5,7 @@
"homepage": "https://codeigniter.com",
"license": "MIT",
"require": {
- "php": "^7.2 || ^8.0",
+ "php": "^7.3||^8.0",
"ext-curl": "*",
"ext-intl": "*",
"ext-json": "*",
@@ -18,7 +18,7 @@
"codeigniter4/codeigniter4-standard": "^1.0",
"fakerphp/faker": "^1.9",
"mikey179/vfsstream": "^1.6",
- "phpunit/phpunit": "^8.5",
+ "phpunit/phpunit": "^9.1",
"predis/predis": "^1.1",
"squizlabs/php_codesniffer": "^3.3"
}, |
|
@sneakyimp Can you use |
|
@kenjis thank you very much for the kind suggestion. I'm always reluctant to use non-official repos for my workstation. I ended up upgrading to Ubuntu 20, which has an apt package for PHP 7.4. I still think it's a poor decision to abandon PHP 7.2. Ubuntu 18.04 LTS, which only offers PHP 7.2 as an apt package will provide maintenance updates for two more years and extended security maintenance until 2028. I have rebased as advised and committed the latest changes. That was a lot of work just so rector could remove a single blank line. I still don't understand why rector would take issue with that one empty line when there are at least a dozen others just like it. Before: public function getVersion(): string;
//--------------------------------------------------------------------
/**
* Orchestrates a query against the database. Queries must use
* Database\Statement objects to store the query and build it.
* This method works with the cache.
*
* Should automatically handle different connections for read/write
* queries if needed.
*
* @param string $sql
* @param mixed ...$binds
*
* @return BaseResult|Query|boolean
*/
public function query(string $sql, $binds = null);after: public function getVersion(): string;
//--------------------------------------------------------------------
/**
* Orchestrates a query against the database. Queries must use
* Database\Statement objects to store the query and build it.
* This method works with the cache.
*
* Should automatically handle different connections for read/write
* queries if needed.
*
* @param string $sql
* @param mixed ...$binds
*
* @return BaseResult|Query|boolean
*/
public function query(string $sql, $binds = null); |
|
It seems your branch is not to be expected status, because it has 168 commits and Files changed 195. You have the unneeded merge commit: 0cf7b27 |
I tried rebasing as you suggested -- although I didn't re-clone the repo because I already had it checked out. I haven't the foggiest idea why this PR is so big. |
|
@sneakyimp Do you understand the difference between In this situation, you don't have to use After all, you need to fix this PR commits. I would explain it. |
|
It seems you need the commits 3e55588, 094d003, f599492, d9f568d. |
I am dimly aware of the difference, having read the documentation here. I confess I didn't understand why you suggested that I rebase. I did not understand why i should re-clone a repo that I already had locally, so I attempted to rebase my local repo.
I did not use any merge command. I tried to follow your example above except I chose to operate on my local repository instead of cloning again. I forget the actual sequence of commands, but the merge happened automatically at one point. To be clear, I did not manually type any merge commands at all.
I will certainly need help to sort this out. As I mentioned before, I am not familiar with rebasing. |
It seems I wrote it wrong. You don't have to re-clone a repo at all. I just showed all the flow of the process. You have to clone a repo only once. |
|
Update your Create new branch from the latest Cherry-pick needed commits: Back up the original PR branch: Remove the original PR branch: Rename the new branch: Force push the new branch: |
sneakyimp
force-pushed
the
bugs/issue-4069
branch
from
d9f568d
to
bae798e
Compare
sneakyimp
force-pushed
the
bugs/issue-4069
branch
from
5fd8e70
to
227ad3a
Compare
…C sp_rename into SQLSRV\Connection, tweaked WriteTypeQueryTest
|
@kenjis, I have made the changes as you suggested -- avoiding any static methods. I had a closer look and the MockConnection, because it extends the I have added MERGE type queries to the I added an I added a new unit test, Database/Live/WriteTypeQueryTest where I have made an initial attempt at unit tests. This test could probably be enhanced, and it overlaps with Database/BaseQueryTest.php somewhat, but it does test a couple of DBMS-specific isWriteType functions and it tests actual queries generated by the query builder. In running the unit tests locally, I noticed that it seems like the code assumes the tables/data structures don't exist before the test runs but then doesn't eliminate the tables when tests are complete, so you get some tests failing if you run these tests repeatedly. |
I sent PR: #4334 |
| if ($this->db->DBDebug) { | ||
| // expect an exception, class and message varies by DBMS | ||
| $this->expectException(\Exception::class); | ||
| $query = $this->db->query('SELECT * FROM table_does_not_exist'); | ||
| } else { | ||
| // this throws an exception in this testing environment, but in production it'll return FALSE | ||
| // perhaps check $this->db->DBDebug for different test? | ||
| $query = $this->db->query('SELECT * FROM table_does_not_exist'); | ||
| $this->assertEquals(false, $query); | ||
| } |
There was a problem hiding this comment.
In test execution, the if condition is true or false, not both.
You should test both cases, $this->db->DBDebug is true and false.
There was a problem hiding this comment.
I still don't understand. If $this->db-DBDEbug casts as true, code execution starts at line 17 -- we must expect the exception before executing the query. Otherwise code execution goes to line 23, and we must run the query before we assert that its result is false.
There was a problem hiding this comment.
When we run the test case, we really test either of below:
- $this->expectException(\Exception::class);
- $this->assertEquals(false, $query);
But we should test both cases.
In other words, we should do:
- set
$this->db->DBDebugtrue and test - set
$this->db->DBDebugfalse and test
There was a problem hiding this comment.
Please create separate test cases for DBDebug option enabled and disabled.
There was a problem hiding this comment.
Aha! I think I understand now -- sorry.
I should remove the if statement and instead explicitly set DBDebug=true and test that and then set it to false and run that test also so that the test performs both tests every time. I was under the mistaken impression that the tests that ran would try it both ways.
There was a problem hiding this comment.
I have had some success if I make $origDebug a static variable because it then persists between different instances of DBUtilsTest:
protected static $origDebug;
/**
* This test must run first to store the inital debug value before we tinker with it below
*/
public function testFirst() {
$this::$origDebug = $this->getPrivateProperty($this->db, 'DBDebug');
$this->assertIsBool($this::$origDebug);
}and then if I can be sure these tests will always run in the order they are defined in the PHP file, this should avoid changing DBDebug from whatever default it may have except for the scope of these tests:
public function testUtilsOptimizeTableFalseOptimizeDatabaseDebugTrue()
{
$util = (new Database())->loadUtils($this->db);
$this->setPrivateProperty($util, 'optimizeTable', false);
// set debug to true -- WARNING this change will persist!
$this->setPrivateProperty($this->db, 'DBDebug', true);
$this->expectException(DatabaseException::class);
$this->expectExceptionMessage('Unsupported feature of the database platform you are using.');
$util->optimizeDatabase();
// this point in code execution will never be reached
}
public function testUtilsOptimizeTableFalseOptimizeDatabaseDebugFalse()
{
$util = (new Database())->loadUtils($this->db);
$this->setPrivateProperty($util, 'optimizeTable', false);
// set debug to false -- WARNING this change will persist!
$this->setPrivateProperty($this->db, 'DBDebug', false);
$result = $util->optimizeDatabase();
$this->assertFalse($result);
// restore original value grabbed from testFirst -- WARNING this change will persist!
$this->setPrivateProperty($this->db, 'DBDebug', $this::$origDebug);
}There was a problem hiding this comment.
I.e., the unit tests start with whatever DBDebug default (default value, from Config, from .env, etc) and if you change DBDebug in one unit test method, that changed value will still be in effect for the following test.
Yes. Ideally, all the state before a test run should be completely identical.
But currently, CI4 tests have a lot of hidden dependencies, and it will be difficult to control them perfectly,
so the best approach is to save the DBDebug value before execution and restore it.
There was a problem hiding this comment.
HOWEVER it would appear that phpunit instantiates a new instance of DbUtilsTest for each and every test method it contains so $this->origDebug will be null inside every other test function inside it even while any changes to $this->db->DBDebug persist between tests.
PHPUnit resets properties in a test case, because the state before each test should be identical
and ideally a test should not depend on another test.
If you want to keep a value between test methods, you can use static properties or Test Dependencies
https://phpunit.readthedocs.io/en/9.5/writing-tests-for-phpunit.html#test-dependencies
There was a problem hiding this comment.
PHPUnit resets properties in a test case, because the state before each test should be identical
and ideally a test should not depend on another test.
I did test whether a change to the DBDebug value in one test method would persist when PHPUnit moves on to the next test and it did
I tried adding a class property that was non-static, public $origDebug and I set this value in a test method but, to my surprise, it was NULL in every subsequent test method in that file. This is what made me think it was creating a new instance of the test class for each method. I added a constructor function and this was called repeatedly for a single test file -- presumably once for each test. I surmised from these experiments that the db object would retain any changes made to DBDebug until all the subsequent tests had run. PHP passes objects by reference, so if you modify them inside a function, the changes can persist:
class foo {
public $prop = "default";
}
function myFunc($v) {
$v->prop = "inside";
echo $v->prop . "\n";
}
$v = new foo;
echo $v->prop . "\n";
myFunc($v);
echo $v->prop . "\n";output:
default
inside
inside
If you want to keep a value between test methods, you can use static properties or Test Dependencies
https://phpunit.readthedocs.io/en/9.5/writing-tests-for-phpunit.html#test-dependencies
I did indeed end up using a public static $origDebug var to hold the original/default value for DBDebug, and this seems to work, but I would point out that this assumes the tests will be executed in the order they are defined in the file.
There was a problem hiding this comment.
I did test whether a change to the DBDebug value in one test method would persist when PHPUnit moves on to the next test and it did
⚠️ ⚠️ ⚠️ .
$this->db comes from the static method (it was moved to trait recently):
CodeIgniter4/DatabaseTestTrait.php at develop · codeigniter4/CodeIgniter4
CodeIgniter4/system/Test/DatabaseTestTrait.php
Lines 72 to 76 in e466127
this assumes the tests will be executed in the order they are defined in the file.
If I remember correctly, phpunit runs tests in the order.
|
@sneakyimp I deleted the previous comments and I re-wrote them. |
|
This is another issue, because CI3 does not have prepared statement functionality. When we use prepared statements, we prepare and execute a query. When a prepared query fails, what value should be returned? |
Thank you for digging this deeply into the code, and thank you for your very kind help. I would not be able to contribute without your guidance. I have not looked into the prepared statement functionality. Would it be acceptable to create a separate issue/bug report for that? I very badly need to get back to work on my CI3 to CI4 site migration which has been stymied by these bugs I've been working on. I could probably look at it after March 1st. |
|
Yeah, I think prepared statement issue is another separate issue/bug. To be honest, I'm tired of this big PR. |
There was a problem hiding this comment.
Thanks, both of you @sneakyimp and @kenjis for taking your time and working on this.
Besides small tweaks in tests, it looks like it's almost ready.
The last thing we need is a changelog update and migration instructions.
Please make an entry here: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.1.2.rst under Bugs fixed - when you will be referring to the fixed return status.
We need to make the migration instructions when it comes to the changed ConnectionInterface, so please copy this file: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/installation/upgrade_410.rst and based on it create upgrade_412.rst and add instructions for adding a new method.
| if ($this->db->DBDebug) { | ||
| // expect an exception, class and message varies by DBMS | ||
| $this->expectException(\Exception::class); | ||
| $query = $this->db->query('SELECT * FROM table_does_not_exist'); | ||
| } else { | ||
| // this throws an exception in this testing environment, but in production it'll return FALSE | ||
| // perhaps check $this->db->DBDebug for different test? | ||
| $query = $this->db->query('SELECT * FROM table_does_not_exist'); | ||
| $this->assertEquals(false, $query); | ||
| } |
There was a problem hiding this comment.
Please create separate test cases for DBDebug option enabled and disabled.
…or certain tests. Added changelog and upgrade notes.
remove incorrect markdown
|
@sneakyimp Thank you for this. |
I'm happy to contribute, and feel this is important. I think it was probably overlooked because the phpunit tests get run with Also, I hope you guys will review & accept PR #4307 . @paulbalandan took a look but I haven't heard anything in five days. I think autorouting is a very serious consideration for code organization, to reduce complicated routes files, for seo optimization, and for security to prevent malicious code execution issues. |
New changes were introduced to the BaseModel when fixing a bug in codeigniter4: codeigniter4/CodeIgniter4#4176 fixes michalsn#2
Initial commit fixing wrong return value in Database/BaseConnection:query. When underlying resultID if false, this function should return false as described in #4069. This is a preliminary (and trivial) correction. In order for this change to be run, the db must be configured with DBDebug=false (e.g., in production mode).
NOTE that this code change is based on the CI 3 behavior. This may seem like a fundamental change but I truly believe that returning a Result object when the query failed was a mistake because these cast to
true. I have been unable to detect any change in the behavior of the phpunit tests from this one change, so it's my guess that the phpunit tests do not ever run with DBDebug=false. I've constructed a phpunit test, BadQueryTest which seems to work on my machine, but I have added this line to my .env file:I suspect my BadQueryTest might have trouble with the automate tests that will run, and would ask help (@kenjis ? @michalsn ?) in massaging things here to achieve the right test and make sure this isn't a breaking change. I've only made this change to try and make sure CI4 behaves as the documentation says it will. There are no changes required to the user docs. On the contrary, there is probably still more work to be done to the code to make sure it behaves as the documentation says it does. In particular, the code will not currently return
trueunder any circumstances.Description
I changed one line (line 680) to return
falseinstead of aResultobject. This conforms both to CI3 behavior but also to the behavior described in the CI4 documentation. I added a BadQueryTest which probably needs work.Checklist: