adds BaseResult::getNumRows(). adds getNumRows to various DBMS Result classes

[sneakyimp]

Added unit tests/system/Database/Live/GetNumRowsTest.php for this newly added method. Added an entry in the user docs for getNumRows method. Tweaks MockResult to fix phpstan problem and class definition conflict. This is my signed initial commit.

Description
Per Lonnie Ezell's message in #109, I have attempted my first CodeIgniter fork/branch/pull request here. It adds a getNumRows method to the ResultInterface and the various DBMS classes that extend it. This pull request supercedes a couple of messy ones (for which I apologize).

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

@sneakyimp

https://github.com/codeigniter4/CodeIgniter4/pull/4049/checks?check_run_id=1632239856

There was 1 error:

1) Builder\GetNumRowsTest::testGetRowNum
Exception: SQLite3Result does not support a numRows method. Use Builder->countAllResults() instead.

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/SQLite3/Result.php:192
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/Live/GetNumRowsTest.php:39

SQLite3 test fails, because it throws an exception.
Add code to skip the test like this:

if ($this->db->DBDriver === 'SQLite3')
{
	$this->markTestSkipped('SQLite3 does not support comments on tables or columns.');
}

It is from ForgeTest.

[sneakyimp]

@kenjis THANK YOU for the very helpful information. Does that mean I should close this PR and create yet another? OR should I make that one little change and push to that branch again? Will this re-trigger the tests?

[kenjis]

@sneakyimp

Does that mean I should close this PR and create yet another?

No.

should I make that one little change and push to that branch again?

Yes.

Will this re-trigger the tests?

Yes.

[kenjis]

@sneakyimp https://github.com/codeigniter4/CodeIgniter4/pull/4049/checks?check_run_id=1632239808
Rector complains about three files.

1. system/Database/Postgre/Result.php
2. system/Database/SQLSRV/Result.php
3. system/Database/SQLite3/Result.php

Run rector and commit the changes.

$ vendor/bin/rector process system/Database/

[kenjis]

Use CodeIgniter\Database\Exceptions\DatabaseException instead of Exception.

[michalsn]

I think you should use/update a public variable $numRows that is available in the BaseResult class so that the result could be accessed later on.

I don't like the idea of throwing an exception for SQLite3. We probably should "simulate" this functionality when it's not available. That being said, we can add the "global" method getNumRows() to the BaseResult class that will handle these cases, and in child classes, we can have a separate implementation for every "driver" that supports it.

[MGatner]

I like @michalsn 's suggestion - we also cannot add a method to the ResultInterface without a major release, so this will either need to be implemented in the base class and be "optional" (i.e. no existing functionality relies on it) or be marked internal for now.

[MGatner]

Actually even better would be to do what Michal said but deprecate $numRows as a public variable and makes this a getter for it.

[sneakyimp]

I think you should use/update a public variable $numRows that is available in the BaseResult class so that the result could be accessed later on.

My first thought is that this approach would require more changes to other CodeIgniter classes. In particular, we would need to make sure we set this $numRows property any time a relevant query is run. My other thought is that only the MySQLi module offers a similar OOP approach which already effectively does what you want. The SQLSRV and Postgres modules only offer a procedural approach. It seems more intuitive and less invasive to me if this function reflects the behavior of its underlying module. Also, but I'm not certain, I believe you can already call this function "later on" and it should repeatedly return the same value unless you alter the query result/mysqli_result/SQLSRV result/PostgreSQL result object in question.

Put another way, I think these DBMS modules are already storing some value in memory and that these various num_rows functions already function as efficient getters for some value stored in memory. Consider taking a look at the source code for mysqli_num_rows. It's a bit of a chore to chase down the macro definitions, but it leads to what looks like a struct reference.

I don't like the idea of throwing an exception for SQLite3. We probably should "simulate" this functionality when it's not available. That being said, we can add the "global" method getNumRows() to the BaseResult class that will handle these cases, and in child classes, we can have a separate implementation for every "driver" that supports it.

The comments for the SQLite3Result class suggest this code to obtain a numRows count, but this requires the code to loop thru every record of the result set, and would alter the state of the query/result object that has been returned:

$nrows = 0;
$result->reset();
while ($result->fetchArray())
    $nrows++;
$result->reset();
return $nrows;

Note how it resets the result twice. The looping might be a hint of the 'performance and memory issues' by @lonnieezell in #109. Perhaps we could alter this code to take a snapshot of the current result index and then do a dataSeek to that same index after this function runs.

[sneakyimp]

I don't like the idea of throwing an exception for SQLite3. We probably should "simulate" this functionality when it's not available. That being said, we can add the "global" method getNumRows() to the BaseResult class that will handle these cases, and in child classes, we can have a separate implementation for every "driver" that supports it.

In the interest of simulating a numRows function for SQLite3, I took a peek at the source code for the SQLite3\Result::dataSeek source and see that it throws a DatabaseException if you try to use that function with any value of $n other than zero:

	public function dataSeek(int $n = 0)
	{
		if ($n !== 0)
		{
			throw new DatabaseException('SQLite3 doesn\'t support seeking to other offset.');
		}

		return $this->resultID->reset(); // @phpstan-ignore-line
	}

My original thinking was that, because SQLite3Result has no numRows function, throwing an exception which suggests the use of builder->countAllResults() would be the most helpful information to developers using SQLite3. The downside of this approach is that any CI4 project using my getNumRows function would break if they switched the driver to SQLite3. This seems very unlikely to happen in practice, but who knows?

I think the best approach would be to implement the loop-count approach above and, if possible, emit a warning. However, CodeIgniter throws ErrorException by default when you emit even the tiniest of errors:

trigger_error("using getNumRows with SQLite3 resets the result array and is discouraged.", E_USER_NOTICE);

Is there some recommended best practice for warning developers? Perhaps we can write a log or something?

[michalsn]

I don't really understand how relevant is the fact the database module takes the OOP approach or procedural.

I believe we can easily make $numRows protected since it wasn't used at all before. We can set value for this variable only on demand - when someone will call the getNumRows() method.

The idea behind storing this value separately in the $numRows variable is because developers may use freeResult() method and then the resource won't be available anymore. The second one (as you mentioned), is that in some cases there is no built-in support for this feature, and getting the result may be time-consuming.

For SQLite3 we can take a more "brutal" approach and just force to count the result array - just an idea.

Personally, I just can't imagine not supporting such a method for all database drivers. SQLite3 is commonly used for testing environments (when the project isn't too complex) and the lack of this method would be a big pain for developers.

[sneakyimp]

I don't really understand how relevant is the fact the database module takes the OOP approach or procedural.

The point I was expressing (rather poorly) is that the underlying database drivers that support any numRows method are already maintaining such a variable in memory and the various driver functions (mysql_num_rows, sqlsrv_num_rows, and pg_num_rows) are already acting efficiently as a getter method. I don't think adding a private/protected numRows property to the CodeIgniter classes gains any appreciable functionality. On the contrary, maintaining a property $numRows in our PHP classes just requires more code, more memory, and more CPU cycles than just using the underlying driver functions.

I believe we can easily make $numRows protected since it wasn't used at all before. We can set value for this variable only on demand - when someone will call the getNumRows() method.

I was trying to imagine how we might use a 'lazy loading' approach for this numRows property and I suppose we could set it to NULL by default and then check is_null($this->numRows) in each getNumRows() method, however, we would need to identify every situation that would reset this value. As you mention, freeResult() might be one such case.

I still don't see any good reason for us to maintain a numRows private/protected property except for the SQLite3 situation, where the getNumRows method is somewhat costly. In every other case, I believe the underlying DBMS is performing an efficient memory read operation on some record count integer stored in memory.

The idea behind storing this value separately in the $numRows variable is because developers may use freeResult() method and then the resource won't be available anymore. The second one (as you mentioned), is that in some cases there is no built-in support for this feature, and getting the result may be time-consuming.

The code I've supplied in this pull request will currently throw an exception if you try to call $query->getNumRows() after you have called $query->freeResult. I think this is entirely correct. You shouldn't be calling getNumRows when you have freed the query result. Here's an example:

	public function foo() {
		$db = \Config\Database::connect("tests");
		$query = $db->query("select * from db_job");
		$query->freeResult();
		var_dump("Rows: " . $query->getNumRows());
	}

This throws an ErrorException:

[ErrorException]
Trying to get property 'num_rows' of non-object
at SYSTEMPATH/Database/MySQLi/Result.php:186

For SQLite3 we can take a more "brutal" approach and just force to count the result array - just an idea.

I posted some example code above to do exactly this and tried to point out the tradeoffs. I'd appreciate any thoughts you have. Also please note the Exception SQLite3 throws in the dataSeek function.

Personally, I just can't imagine not supporting such a method for all database drivers. SQLite3 is commonly used for testing environments (when the project isn't too complex) and the lack of this method would be a big pain for developers.

I agree that a getNumRows function is efficient and useful. I'm migrating a CI3 project that uses this function 100 times. I would point out that CI4 already lacks this function (builder->countAllResults is a different beast). Should we adopt this pull request, it should have no impact on any other db functionality as it is currently written. The only impact my pull request would have on existing projects is that instances of a BaseResult or db\Result object will consume slightly more memory due to the presence of a single additional method (which is almost trivial).

No one is under any obligation to use such a function, regardless of which DB driver they use. The use of Result->getNumRows is entirely optional.

[michalsn]

Storing $numRows value doesn't bring any additional costs - it's just one variable with a number. And it's already a null by default. I don't believe we have to reset it - it's a part of the Result class so other queries will have it's own classes.

I will adjust your example a little

	public function foo() {
		$db = \Config\Database::connect("tests");
		$query = $db->query("select * from db_job");
		var_dump("Rows: " . $query->getNumRows());
		$query->freeResult();
		var_dump("Rows: " . $query->getNumRows());
	}

From my perspective, the second call shouldn't return an error. Simply because the number of rows for this query did not change. I find the proposed behavior as strange from a logical point of view. Keep in mind that users usually don't care what is going on under the hood - they want stable api between drivers.

I understand your point of view, but we try to provide consistent solutions whenever it's possible.

Okay, I think you already know my preference for handling this feature. Anyway, if other members will agree with your idea then it will be merged.

[sneakyimp]

@michalsn I really appreciate your input, and hope that you don't find my latest commit off-putting.

Storing $numRows value doesn't bring any additional costs - it's just one variable with a number. And it's already a null by default. I don't believe we have to reset it - it's a part of the Result class so other queries will have it's own classes.

In the current CI4 develop branch, the public property BaseResult::numRows is never set anywhere. One's machine will allocate memory for this value every time such a result is instantiated but the code doesn't currently do anything but check if numRows === 0 which will never be true. If we choose to store the most recently obtained num_rows value in this variable we must burn CPU cycles to check it, keep it updated, etc. I think it makes sense when a num_rows operation is painful, as with SQLite3, but I don't think it makes sense where the underlying db driver is already doing its work efficiently.

Also, the num_rows functions provided by the underlying database drivers may return different values depending on whether one is buffering results. From the documentation:

The behaviour of mysqli_num_rows() depends on whether buffered or unbuffered result sets are being used. For unbuffered result sets, mysqli_num_rows() will not return the correct number of rows until all the rows in the result have been retrieved.

This behavior apparently depends on whether you use MYSQLI_USE_RESULT or MYSQLI_STORE_RESULT. I'm not entirely sure but it looks like CodeIgniter uses the mysqli_query which defaults to MYSQLI_STORE_RESULT. In any case, it seems wise to me to keep CodeIgniter's behavior consistent with the underlying functions as much as possible. Should mysqli_num_rows change its result for a given query operation due to the arrival of more records, I think CodeIgniter should reflect that rather than caching some stale value.

I will adjust your example a little

	public function foo() {
		$db = \Config\Database::connect("tests");
		$query = $db->query("select * from db_job");
		var_dump("Rows: " . $query->getNumRows());
		$query->freeResult();
		var_dump("Rows: " . $query->getNumRows());
	}

From my perspective, the second call shouldn't return an error. Simply because the number of rows for this query did not change. I find the proposed behavior as strange from a logical point of view. Keep in mind that users usually don't care what is going on under the hood - they want stable api between drivers.

If this code were to output Rows: 4 both times, I think that could be problematic. For example, if your code is checking whether a Result object has rows to determine whether or not to loop through its records. If you get a 4 when the underlying result object is gone, you'll probably get an error trying to iterate on a non-existent result set. To me, the Result->getNumRows() function should never be used once you have called freeResult(). That's sort of like trying to query a database connection that has been closed or dereferencing a null pointer. That seems like textbook Undefined Behavior.

I understand your point of view, but we try to provide consistent solutions whenever it's possible.

Since the numRows property is never set to anything in the current develop branch, I took the liberty of moving it to the SQLite3\Result class and made it protected there. I changed the BaseResult class to refer instead to the new getNumRows() method in its methods that were checking numRows===0 (which would always be false). This methods are:

• getCustomResultObject
• getResultArray
• getResultObject

I am aware that some developers may have tried to reference the Result::numRows property, but contend that this would have been a mistake because it would never be anything but null unless explicitly set by the developer themselves.

I've modified the pull request and hope you folks might kindly take a look. I see that the SQLSRV tests are failing for some reason. I'll look into that.

[sneakyimp]

OK so that's weird the SQLSRV checks failed this time. They ran just fine two days ago. It looks from the errors like sqlsrv_num_rows is returning false for some reason in this function:

	public function getNumRows() : int
	{
		$retval = sqlsrv_num_rows($this->resultID);
		if ($retval === false)
		{
			throw new DatabaseException('Error retrieving row count');
		}
		return intval($retval);
	}

This latest function is nearly identical to the one that ran previously except for the type of exception thrown and the if/else statement:

	public function getNumRows() : int
	{
		$retval = sqlsrv_num_rows($this->resultID);
		if ($retval === false)
		{
			throw new \Exception('Error retrieving row count');
		}
		else
		{
			return intval($retval);
		}
	}

Might someone have changed the type of cursor used for these automated tests? The documentation on sqlsrv_num_rows says:

Retrieves the number of rows in a result set. This function requires that the statement resource be created with a static or keyset cursor.

Looking at the option value provided for Scrollable in our source code, it looks like CI4 chooses either false or SQLSRV_CURSOR_CLIENT_BUFFERED depending on whether the constant SQLSRV_CURSOR_CLIENT_BUFFERED is defined or not. I would point out that this option is not listed in the documentation for sqlsrv_query. The options listed there are:

• SQLSRV_CURSOR_FORWARD
• SQLSRV_CURSOR_STATIC
• SQLSRV_CURSOR_DYNAMIC
• SQLSRV_CURSOR_KEYSET

However, SQLSRV_CURSOR_CLIENT_BUFFERED is listed in the Microsoft docs. Sadly, I'm not sure of the implications of these different cursor types. Perhaps someone might explain why the CI4 default is the latter?

[sneakyimp]

Digging deeper into the SQLSRV tests, it looks like my changeup of numRows and getNumRows in BaseResult::getResultArray is the source of this problem. It looks like calls from DBUtilsTest to BaseUtils::optimizeDatabase and optimizeTable make calls to BaseResult::getResultArray() and this works fine for most DB drivers, but the SQLSRV driver appears to have a resultID for those optimize queries that is problematic. Feeding that resultID to sqlsrv_num_rows is yielding false.

This is tricky to sort out because I don't have access to any MSSQL server. Looking at the code in getResultArray(), it looks like the resultID is neither boolean nor empty but for some reason it returns false when fed to sqlsrv_num_rows.

Not sure where the best place is to fix this problem, but leaning toward BaseResult.

[michalsn]

MySQLi - there is no way to use buffered query because we always use the default flag (as you noticed), so calling num_rows() will always return the same value.

For the SQLSVR problem, I'm not sure why it's not working now but the flag we're using is valid per documentation. The flag is inherited from CI3 and it seems a good fit.

As you mentioned there are different flags for SQLSRV so there is also a question should we support the getNumRows() for all of them or not (with some of them sqlsrv_num_rows() will not work).

[michalsn]

As for SQLSRV problem - I've just looked at the code one more time and from what I see we don't use Scrollable option for write-type queries. Since you're forcing the usage of getNumRows() in the place where I commented on your code above - we have a problem.

[sneakyimp]

As for SQLSRV problem - I've just looked at the code one more time and from what I see we don't use Scrollable option for write-type queries. Since you're forcing the usage of getNumRows() in the place where I commented on your code above - we have a problem.

I agree there's a problem, but it's my feeling that folks shouldn't be calling getResultArray for queries that just return boolean values. It seems extremely odd to call mysql_fetch_assoc on the results of a DROP TABLE query, don't you think? As I mentioned before, mysqli_query only returns records sets for SELECT, SHOW, DESCRIBE or EXPLAIN queries. For everything else, the docs say it returns true or false.

[MGatner]

You all have moved beyond my database comfort zone, but I'm grateful for the work you're doing.

[sneakyimp]

You all have moved beyond my database comfort zone, but I'm grateful for the work you're doing.

Thanks for the encouragement, @MGatner. I feel like a bit of a troublemaker, but want to help make CI4 better.

I have reported an issue, #4059, with the DbUtilsTest that is causing difficulty for this pull request. I hope it might be informative and would appreciate anyone's input.

[michalsn]

I agree there's a problem, but it's my feeling that folks shouldn't be calling getResultArray for queries that just return boolean values. It seems extremely odd to call mysql_fetch_assoc on the results of a DROP TABLE query, don't you think? As I mentioned before, mysqli_query only returns records sets for SELECT, SHOW, DESCRIBE or EXPLAIN queries. For everything else, the docs say it returns true or false.

I haven't checked what is going on there - I just pointed out where is the problem, since you were wondering why your tests are failing.

If other drivers work the same way I guess this is something that should be fixed.

[JerryCipriano]

@sneakyimp

This discussion is largely over my head, but for what it is worth, our Codeigniter 3 project code frequently checks num_rows. Keeping this intact would be nice when we migrate to 4.

[sneakyimp]

@michalsn As previously discussed, I have tried implementing the getNumRows functions as closely to the CI3 code as possible. The significant differences are that

1. the SQLSRV one cannot check for a scrollable cursor because our SQLSRV\Result class has no visibility to any scrollable property. To provide this visibility might require more extensive changes to the the BaseConnection class and other classes to inject the scrollable property. This seems unnecessary because function seems to behave even when you call getNumRows on a non-scrollable query. In that case, sqlsrv_num_rows returns FALSE but this is converted to zero due to the int return type.
2. the if/else logic in CI3 was converted automatically by rector and/or code sniffer and/or phpstan to its current form. I found these changes rather surprising and invasive, but I think they should still work because the class defaults resultArray and resultObject to empty arrays and only seems to add additional elements.

[michalsn]

Thanks @sneakyimp. I made some more comments - some of them are related to code style, but I understand that it's because of problems with tools.

You may try to remove the pre-commit hook and then add it again? I'm not really sure what is going on since it's not really my field of knowledge. I checked it locally and code sniffer actually fixed it correctly. Maybe we should try to call @samsonasik for help.

[kenjis]

@sneakyimp #4049 (comment)
Code Sniffer has a bug in if statement rule?

You could commit by git commit --no-verify. The option disables pre-commit hook.

[MGatner]

--no-verify

😮

@kenjis You've just saved me so much heartache. I've been editing the pre-commit hook every time 🤦‍♂️

[sneakyimp]

You could commit by git commit --no-verify. The option disables pre-commit hook.

Thank you, @kenjis. I had managed to figure that out on my own. That said, I definitely did not set up these git hooks on my own. If we have two different automatic code editors that can't agree on each other, that seems bad -- a lot of spurious commits and a lot of failed tests when you finally do commit to github. Maybe someone (more experienced with these auto tests than I am) should make sure that Rector and Code Sniffer don't have some conflict?

[sneakyimp]

OK I believe I've made all the change requests from @michalsn and @samsonasik. Please review and let me know your feelings.

[michalsn]

This is the last round from me.

Just some tiny things, but I'm requesting these changes because it's "now or never" 😄 Probably no one would go back to it after a merge.

[sneakyimp]

I have made the changes requested by @michalsn and am baffled by the test failures I'm seeing. These appear to be caused by files that I have not touched and which have passed the tests on previous commits. What might be causing these failures?

[kenjis]

Why PHP 7.4 - SQLite3 failed?

There was 1 failure:

1) CodeIgniter\Cache\Handlers\PredisHandlerTest::testGetMetaData
Failed asserting that 1 is equal to 0 or is less than 0.

/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/Handlers/PredisHandlerTest.php:105

https://github.com/codeigniter4/CodeIgniter4/pull/4049/checks?check_run_id=1684018456

[michalsn]

Thank you @sneakyimp!

PHPStan error is already fixed in a different PR.

Predis error is probably related to the slow tests. It happens only once in a while.

[sneakyimp]

Thank you @sneakyimp!

I appreciate everyone's guidance and openness to my ideas.

PHPStan error is already fixed in a different PR.

Predis error is probably related to the slow tests. It happens only once in a while.

Do we need to re-trigger the tests somehow? It's so disappointing when any fail -- and so gratifying when they all succeed.

[michalsn]

When we merge the PR that fixes the errors, I will run tests again (unless we merge this earlier).

[michalsn]

@sneakyimp Sorry, I think I misled you - you need to rebase to make tests working properly. If you're not familiar with rebasing, you can follow this article: https://samsonasik.wordpress.com/2015/09/16/practical-git-4-rebasing-conflicted-task-branch-against-primary-branch/

[sneakyimp]

@sneakyimp Sorry, I think I misled you - you need to rebase to make tests working properly. If you're not familiar with rebasing, you can follow this article: https://samsonasik.wordpress.com/2015/09/16/practical-git-4-rebasing-conflicted-task-branch-against-primary-branch/

I started trying the various commands in that article and when I ran get pull origin develop this resulted in no changes, which makes me think that a rebase would not solve the problem. I'm wondering if this is because the fort I created in github needs to somehow pull from its parent repo? Any advice would be much appreciated. This is a bit out of my comfort zone as far as git operations go.

EDIT: I added the CI4 repo as a remote to my local git project and pulled the latest changes to my own develop branch and also to my num-rows-signed branch and then merged those upstream changes into my repo. I then pushed to my fork repo and this has brought my fork current with the CI4 repo and the tests are running again.

[kenjis]

@sneakyimp
How to rebase:

$ git remote add codeigniter git://github.com/codeigniter4/CodeIgniter4.git
$ git fetch codeigniter
$ git rebase codeigniter/develop

[sneakyimp]

How to rebase:

$ git remote add codeigniter git://github.com/codeigniter4/CodeIgniter4.git
$ git fetch codeigniter
$ git rebase codeigniter/develop

@kenjis must I still do this? I performed the first two steps there and merged things and pushed and my commit here has passed all tests.

[kenjis]

@sneakyimp I don't know. But if you rebase it, your commit graph will be clean.
The merge commit will be gone.

before:

after:

[michalsn]

Okay, no worries, we have an option to squash before the merge.

[MGatner]

Thanks so much for all your work!