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.

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

Removed explicit escaping for pgsql driver in FilterPartial#maybeSpecifyEscapeChar. Fixes #941 #968

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Talpx1
Copy link
Contributor

@Talpx1 Talpx1 commented Sep 9, 2024

As mentioned in the issue #941, my previous PR #927 causes the following error when using multiple partial filters in combination with Postgres: SQLSTATE[HY093]: Invalid parameter number: parameter was not defined

Before opening this PR, I investigated on why this may be happening, since from my previous tests (see attachment in PR #927), Postgres supports ESCAPE and the query result is the same with or without specifying it.

I've not been able to exactly identify the problem, but I managed to figure out it has something to do with the \ char specifically.
In fact, if FilterPartials#maybeSpecifyEscapeChar was to be modified as follows, the Invalid parameter number error would not happen:

protected static function maybeSpecifyEscapeChar(string $driver): string
    {
        if(! in_array($driver, ['sqlite','sqlsrv', 'pgsql'])) { //DOES NOT include the removal of pgsql, that is the fix proposed in this PR
            return '';
        }

        return " ESCAPE 'ß'";  //previously: return " ESCAPE '\'";
    }

Basically, this code (so the code currently implemented, not modified if not for the ß char) would work just fine by just replacing ESCAPE '\' with ESCAPE 'ß' (I used ß as an example since it wouldn't collide with other special-meaning chars such as *, but it could be any char).

I guess this indicates that the problem does not live in the maybeSpecifyEscapeChar or in the package itself, but I suspect it resides in some Eloquent parsing.

On that regard, even if not related to the parsing of queries, I found a place in the Eloquent source (vendor/laravel/framework/src/Illuminate/Database/Query/Grammars/Grammar.php function substituteBindingsIntoRawSql), where expressions like \' are treated with special behaviors (tweaking that function, in fact, allowed me to parse the missing parameter in the ddRawSql while debuging).
So my vague guess is that a similar behavior is influencing the parsing of the query when encountering ESCAPE '\'.

The proposed fix is really simple, I just removed pgsql from the drivers to explicitly escape, since the query results with or without ESCAPE are the same for that driver (see testing in PR#927 attachment)

In addition, in the maybeSpecifyEscapeChar phpdoc, I also added 'mariadb' as a possible value for the $driver param, since L11 introduced that driver.
After all the changes I updated the tests and, since the composer.json of this package specify "illuminate/database": "^10.0|^11.0", but mariadb is a valid driver only in v11, I used the following condition to run the mariadb test only if the driver is supported (skipped with relevant message otherwise):

//FilterTest.php line 93

if($dbDriver === 'mariadb' && !in_array('mariadb', DB::supportedDrivers())){
        $this->markTestSkipped('mariadb driver not supported in the installed version of illuminate/database dependency');
    }

This fix solves #941, allowing Postgres users to update the dependency and access the new features.

Hope this helps!

Talpx1 and others added 3 commits September 9, 2024 10:26
…ifyEscapeChar. Fixes spatie#941

Added mariadb driver in FilterPartial#maybeSpecifyEscapeChar phpdoc for param $driver. Also adjusted test in order to run with mariadb driver only if the installed version of illuminate/database dependency supports the driver.
@max13fr
Copy link

max13fr commented Sep 23, 2024

Hello,

Thanks for the fix !
I hope it will be merged soon, as we are stucked at 5.8 version currently.

Best regards

@4H1R
Copy link

4H1R commented Sep 28, 2024

Thank you for the fix
Just waiting for the approval of the Spatie team to merge this

Best regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants