Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Bug: Query Builder limit() problem with OCI8 #9469

Closed
rutgers-master opened this issue Feb 27, 2025 · 6 comments · Fixed by #9472
Closed

Bug: Query Builder limit() problem with OCI8 #9469

rutgers-master opened this issue Feb 27, 2025 · 6 comments · Fixed by #9472
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@rutgers-master
Copy link

PHP Version

8.3

CodeIgniter4 Version

4.6.0

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

Oracle

What happened?

When using Query Builder (OCI8) on an Oracle database, it seems like the limit() function is not working properly when returning results for anything after the first page. In other words, if I do a limit(2,0), I'll get back the first 2 results correctly, however if I do a limit(2,2), instead of getting back the next page of 2 results, instead I get back 3 results and one of the results is a duplicate from the previous page of results.

Steps to Reproduce

Using a Model that is for an Oracle table, simply do:

$results = $builder->select("field")->limit(2,0)->get()->getResult();
// Print the results
print_r($results);
// Then do it again to get the next 2 results:
$results = $builder->select("field")->limit(2,2)->get()->getResult();
// Print the results
print_r($results);
// You'll get back 3 results instead of 2 results.

Expected Output

The expected output would be to get back just 2 results when doing limit(2,2), and this can easily be fixed by modifying the Codeigniter4/system/Database/OCI8/Builder.php file and changing this line:
$limitTemplateQuery = 'SELECT * FROM (SELECT INNER_QUERY.*, ROWNUM RNUM FROM (%s) INNER_QUERY WHERE ROWNUM < %d)' . ($offset !== 0 ? ' WHERE RNUM >= %d' : '');
To this line:
$limitTemplateQuery = 'SELECT * FROM (SELECT INNER_QUERY.*, ROWNUM RNUM FROM (%s) INNER_QUERY WHERE ROWNUM < %d)' . ($offset !== 0 ? ' WHERE RNUM > %d' : '');

Note: All I did was change "RNUM >= %d" to "RNUM > %d".

Anything else?

If you need more details, please let me know. Unfortunately I don't know the specific version of the Oracle server I am accessing, I don't manage the Oracle server itself.
@rutgers-master rutgers-master added the bug Verified issues on the current code behavior or pull requests that will fix them label Feb 27, 2025
@michalsn
Copy link
Member

A version would be useful, as our tests already cover this: https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/Database/Live/LimitTest.php#L25

@michalsn
Copy link
Member

Try to run:

SELECT BANNER FROM V$VERSION;

@rutgers-master
Copy link
Author

Thank you for that SQL statement:

Oracle Database 19c Enterprise Edition Release 19.0.0.0.0 - Production

@michalsn
Copy link
Member

That is strange. Can you check what you get, when you call:

// assuming you use the default connection
$db = db_connect();
echo $db->getVersion();
// and
echo oci_server_version($db->connID);

In general, the $limitTemplateQuery should not be used to determine the LIMIT part in your query. Since your version is greater than 12.1.

https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Database/OCI8/Builder.php#L217-L224

Can you change this method and check if it will work correctly?

protected function _limit(string $sql, bool $offsetIgnore = false): string
{
    $offset = (int) ($offsetIgnore === false ? $this->QBOffset : 0);

    // OFFSET-FETCH can be used only with the ORDER BY clause
    if (empty($this->QBOrderBy)) {
        $sql .= ' ORDER BY 1';
    }

    return $sql . ' OFFSET ' . $offset . ' ROWS FETCH NEXT ' . $this->QBLimit . ' ROWS ONLY';
}

Since we no longer support versions below 12.1 it would make everything simpler.

@rutgers-master
Copy link
Author

Interesting results:

$db = \Config\Database::connect([ "DSN" => "//my.server.here:1521/mydb", "hostname" => "my.server.here", "username" => "my_username", "password" => "my_password", "database" => "mydb", "DBDriver" => "OCI8", "DBPrefix" => "", "pConnect" => true, "DBDebug" => true, "cacheOn" => false, "cacheDir" => "", "charset" => "utf8", "DBCollat" => "utf8_general_ci", "swapPre" => "", "encrypt" => false, "compress" => false, "strictOn" => false, "failover" => [], "port" => 1521 ]);
echo "DB Version = ".$db->getVersion();
echo "DB Version = ".oci_server_version($db->connID);

Results are:

DB Version = 
With an error: TypeError: oci_server_version(): Argument #1 ($connection) must be of type resource, null given

Looking in that getVersion() method, $this->dataCache is an empty array for me, and it's getting tripped up on the $this->connID check because it is null. If I look at the contents of $db, it contains:

{"connID":false,"resultID":false,"protectIdentifiers":true,"escapeChar":"\"","likeEscapeStr":" ESCAPE '%s' ","likeEscapeChar":"!","dataCache":[],"transEnabled":true,"transStrict":true,"commitMode":32,"lastInsertedTableName":null}

Yet I'm able to immediately do a $builder = $db->table("mytable"); $results = $builder->select("field")->get()-getResult(); and get back results from the database. So even though it says there's no connID...the connection is still there.

And yes, if I change the _limit method to your new version that you provided, that does work correctly and provides the correct results.

Thank you for your help with this and if there's any more testing I can do for you, please let me know.

@michalsn
Copy link
Member

Oh, ok I see it now. The connection is established only right before we execute the query, so obviously there is no connID when we try to get the version. The same problem occurred in the code I posted to debug the issue.

I will prepare a fix later.

Thank you for the report and feedback.

This was referenced Feb 28, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: OCI8 limit() method michalsn/CodeIgniter4
2 participants
@michalsn @rutgers-master