-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 whereILike issue with sqlite (#5604) #5687
Changes from all commits
49f6ce7
cef6f51
ce722ae
d34a266
b12b1ae
e98edcb
b58d757
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -556,7 +556,14 @@ describe('Where', function () { | |
|
||
describe('where like', async function () { | ||
beforeEach(function () { | ||
if (!(isPostgreSQL(knex) || isMssql(knex) || isMysql(knex))) { | ||
if ( | ||
!( | ||
isPostgreSQL(knex) || | ||
isSQLite(knex) || | ||
isMssql(knex) || | ||
isMysql(knex) | ||
) | ||
) { | ||
return this.skip(); | ||
} | ||
}); | ||
|
@@ -630,10 +637,23 @@ describe('Where', function () { | |
expect(result[7].email).to.equal('test8@example.com'); | ||
}); | ||
|
||
it("doesn't find data using whereLike when different case sensitivity", async () => { | ||
it("doesn't find data using whereLike when different case sensitivity", async function () { | ||
const result = await knex('accounts').whereLike('email', 'Test1%'); | ||
// sqlite only supports case-insensitive search | ||
if (isSQLite(knex)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add comment that sqlite only supports case-insensitive search There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's done @rluvaton |
||
this.skip(); | ||
} | ||
expect(result).to.deep.equal([]); | ||
}); | ||
|
||
it('supports only case-insensitive searches in sqlite', async function () { | ||
const result = await knex('accounts').whereILike('email', 'Test1%'); | ||
if (!isSQLite(knex)) { | ||
this.skip(); | ||
} | ||
expect(result.length).to.equal(1); | ||
expect(result[0].email).to.equal('test1@example.com'); | ||
}); | ||
}); | ||
|
||
it('Retains array bindings, #228', function () { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add test for the not branch too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused, if I understood correctly the
_not
should add anot
to the sql statementto use it with an ilike query with sqlite or any other dialect it has to look like
Since there's no a
whereNotILike
function, what the test should be?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to set that "not" flag currently at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this query? no I kept it in case there will be a support for functions like
whereNotILike