Fix whereILike issue with sqlite (#5604) #5687
Conversation
mohammedbalila
commented
Sep 13, 2023
mohammedbalila
commented
- This PR is an attempt to fix whereILike query with sqlite mentioned in here whereILike and orWhereILike don't work with sqlite3 #5604 (comment)
- The change is not big, but I need some help with writing tests before merging
|
@mustafabalila I'm happy to help with tests, what can I do? |
|
Hello @kibertoad, I found that the unit tests for Also do you think changing any |
|
@mustafabalila Using "LIKE" should be fine, SQLite LIKE is case-insensitive unless compiled with custom flags. Let me take a look which tests fail in CI now |
| @@ -632,7 +639,12 @@ describe('Where', function () { | |||
|
|
|||
| it("doesn't find data using whereLike when different case sensitivity", async () => { | |||
| const result = await knex('accounts').whereLike('email', 'Test1%'); | |||
| expect(result).to.deep.equal([]); | |||
| if (isSQLite(knex)) { | |||
There was a problem hiding this comment.
please add comment that sqlite only supports case-insensitive search
…ustafabalila/knex into fixing-whereILike-in-sqlite-issue-5604
| @@ -329,6 +329,13 @@ class QueryCompiler_SQLite3 extends QueryCompiler { | |||
| onJsonPathEquals(clause) { | |||
| return this._onJsonPathEquals('json_extract', clause); | |||
| } | |||
|
|
|||
| whereILike(statement) { | |||
| return `${this._columnClause(statement)} ${this._not( | |||
There was a problem hiding this comment.
can you add test for the not branch too?
There was a problem hiding this comment.
I'm a bit confused, if I understood correctly the _not should add a not to the sql statement
to use it with an ilike query with sqlite or any other dialect it has to look like
knex('table').whereNot('col', 'ilike', 'test_string').select();Since there's no a whereNotILike function, what the test should be?
There was a problem hiding this comment.
Is it possible to set that "not" flag currently at all?
There was a problem hiding this comment.
For this query? no I kept it in case there will be a support for functions like whereNotILike
|
Hey @kibertoad If the work on these two PRs is okay but the merge is delayed for any reason, I think I can pick something else, but if you think they need more work, please let me know |
|
@rluvaton Could you please take a look? |
|
Yes, will look at it in few hours, not near a computer |
| @@ -632,7 +639,13 @@ describe('Where', function () { | |||
|
|
|||
| it("doesn't find data using whereLike when different case sensitivity", async () => { | |||
| const result = await knex('accounts').whereLike('email', 'Test1%'); | |||
| expect(result).to.deep.equal([]); | |||
| // sqlite only supports case-insensitive search | |||
| if (isSQLite(knex)) { | |||
There was a problem hiding this comment.
I would split this into 2 tests, as the test name no longer represents the test behavior when in SQLite
There was a problem hiding this comment.
It's done @rluvaton
|
if I'm not responding, ping me on twitter, I have a lot of GitHub notification |
DM is closed |
Fixed |
|
Thanks @rluvaton |
