spatie / laravel-query-builder

Easily build Eloquent queries from API requests
https://spatie.be/docs/laravel-query-builder
MIT License
4k stars 392 forks source link

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

Closed Talpx1 closed 3 months ago

Talpx1 commented 5 months ago

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 commented 3 months ago

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

Talpx1 commented 3 months ago

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

dwightwatson commented 1 month ago

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.