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

fix for incorrect sql LIKE escaping for some db drivers while performing partial filter #927

Merged
merged 1 commit into from
May 10, 2024

Conversation

Talpx1
Copy link
Contributor

@Talpx1 Talpx1 commented Mar 14, 2024

Hi everyone!
First of all, thanks for the amazing package:)

So, I stumbled upon what I think it's a bug in the sql LIKE escaping for special chars (_, %, \).

Consider the following example:
There's a table, which we'll call companies, having a column for the company name: name.
In this column, we store company names with underscores (maybe a slug, for example).

Here's the problem:
when the table gets filtered from the QueryBuilder by the name column, if the search value contains an underscore (for example), the underlying query, rightfully, escape this underscore (this happens in FilterPartials::escapeLike) making the query become something like SELECT * FROM companies WHERE name LIKE '%some\_value%'.
According to my testing, in some database drivers (mysql, pgsql) this escape is automatically interpreted and all is fine, but in some other cases (sqlite, sqlsrv) the \ is literally interpreted, if not explicitly told otherwise. (I was using sqlite as a test database, so that's why I noticed something off).
In the latter cases, the filtering will happen while looking for a company with name containing some\_value (with the \ NOT being an escape char but a literal char) and, for this reason, failing the matching.

Solution:
some databases support explicitly telling what's the escaper char, using ESCAPE, so appending ESCAPE '\' at the end of the query, will tell the db to use the \ char as escape char and not as literal char.
The mysql driver doesn't seem to support such feature, so here's why the check for the db driver (mysql/mariadb would throw a syntax error if used with ESCAPE).

Here's the file containing my testing mentioned above:
escaping_test.txt

Just a note to finish:
some tests were failing before this fix: Tests: 37 failed, 2 skipped, 174 passed (267 assertions)
after the fix: Tests: 37 failed, 2 skipped, 178 passed (271 assertions)
So I don't think my implementation is causing the fails.
The only test i changed is in FilterTest, line 90, as it was checking for an exact string match, but after the change to the query, it wouldn't pass, so i made it look for a partial match (before: ...->toEqual(...), now ...->toContian(...)).

I apologize if my English for the explanation of the problem/solution may lack of clarity.

@AlexVanderbist
Copy link
Member

Thanks! Good catch :) I'll probably modify this just a bit to respect the model's specific DB connection and tag this soon.

@AlexVanderbist AlexVanderbist merged commit 769f3c4 into spatie:main May 10, 2024
1 check passed
@Talpx1
Copy link
Contributor Author

Talpx1 commented May 10, 2024

Glad to contribute and thanks for your amazing package! :)

@dwightwatson
Copy link

dwightwatson commented Jul 6, 2024

Just flagging that this change has caused a break when using multiple partial matches (#941).

Not sure if it's something that can be easily rectified.

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.

4 participants