-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat: add case-insensitive support in advanced Json filtering #4977
base: main
Are you sure you want to change the base?
Conversation
that would be some awesome to have |
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.
Thanks for this PR! I've left a couple of comments for further iterations and better compatibility with the other database providers.
let contains = expr_string.like(format!("%{value}%")); | ||
let contains = match query_mode { | ||
QueryMode::Default => expr_string.like(format!("%{value}%")), | ||
QueryMode::Insensitive => expr_string.compare_raw("ILIKE", format!("%{value}%")), |
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.
The ILIKE
comparison operator is only available on PostgreSQL and CockroachDB.
Since Prisma supports several other database providers (like SQLite, MySQL, MongoDB), we should steer away from using raw
operators directly.
Usually, we rely on connector-specific primitives wrapped in a common abstraction (quaint::connector::queryable::Queryable
).
But in this case, the way forward is probably to change the implementations of the quaint::visitor::Visitor
trait to align visit_json_extract
and similar methods to prisma/prisma#7390 (comment), lowering the case of both compared values when mode: 'insensitive'
is applied.
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.
Nice catch, thank you!
let starts_with = expr_string.like(format!("{value}%")); | ||
let starts_with = match query_mode { | ||
QueryMode::Default => expr_string.like(format!("{value}%")), | ||
QueryMode::Insensitive => expr_string.compare_raw("ILIKE", format!("{value}%")), |
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 we choose another name for the currently called "query mode"? How about case: 'sensitive' | 'insensitive'
, where case: 'sensitive'
is the default option?
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.
After adding case
enum, it feels a bit incosistent with the rest of where
api. Especially when looking into cases like:
prisma.recipe.findMany({
where: {
allergens: {
not: { contains: "peanuts" },
mode: "insensitive", // <-- mode
},
title: {
path: [locale],
string_contains: filter.search,
case: "insensitive", // <-- case
},
},
})
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.
Hi @lubosmato, after talking with a colleague, and observing this output, I stand corrected. I didn't realise QueryMode
was an already existing enum
in out codebase (
pub enum QueryMode { |
mode: 'default' | 'insensitive'
:)
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.
Hi @jkomyno, thank you for reply. I reverted the commit with case
enum.
76913f0
to
db9cf81
Compare
@jkomyno thank you for review, would you find time for another round? Also I would like to ask you if you could point me to the right direction. I tried to hook up prisma client with locally built bins/libs to test this feature with I tried to set env vars What needs to be done in order to make |
fe1afb1
to
bc780f9
Compare
Hi @jkomyno, what are the next steps/is there anything else that needs to be done from my end? |
CodSpeed Performance ReportMerging #4977 will not alter performanceComparing Summary
|
This is slightly annoying to do, sorry for that. Prisma CLI (and generator) doesn't use the query engine to get the DMMF, it uses the WASM module (published as |
ef092ed
to
105af6b
Compare
Hi @jkomyno, what needs to be done to finish this? |
The PR adds
case
field into Json filtering to allow case-insensitive text filtering with Json fields.E.g.:
Fixes prisma/prisma#7390