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

where in with empty array produces invalid SQL #1284

Closed
rafaelalmeidatk opened this issue Nov 28, 2024 · 3 comments
Closed

where in with empty array produces invalid SQL #1284

rafaelalmeidatk opened this issue Nov 28, 2024 · 3 comments
Labels
duplicate This issue or pull request already exists enhancement New feature or request

Comments

@rafaelalmeidatk
Copy link

rafaelalmeidatk commented Nov 28, 2024

Related issue comment: #447 (comment)
Reproduction of the issue: https://old.kyse.link/?p=s&i=rdvxZDgcBQ7FzTmMG4Cx

When using where('foo', 'in', []) I would expect the query builder to not produce invalid SQL. This is the behavior with knex:

const query = knex
  .select("id", "last_name")
  .from("user")
  .whereIn("last_name", [])

query.toSQL().sql
// 'select "id", "last_name" from "user" where 1 = ?'
// bindings: [ 0 ]

Having to check the length of the array to conditionally add the where clause considerably increases the complexity of the query specially in situations where you have more than a simple where:

// The desired API
const users = await ky
  .selectFrom("users")
  .selectAll()
  .where((eb) => eb(sql`lower(email)`, "in", emails).or("phone", "in", phones))
  .execute()

// What needs to be done today
const users = await ky
  .selectFrom("users")
  .selectAll()
  .where((eb) => {
    const ors: Expression<SqlBool>[] = []

    if (emails.length) {
      ors.push(eb(sql`lower(email)`, "in", emails))
    }
    if (phones.length) {
      ors.push(eb("phone", "in", phones))
    }

    return eb.or(ors)
  })
  .execute()

FWIW kysely already does something similar when both conditions in the above query are false:

const emails = []
const phones = []
const query = ky
  .selectFrom("users")
  .selectAll()
  .where((eb) => {
    const ors: Expression<SqlBool>[] = []

    if (emails.length) {
      ors.push(eb(sql`lower(email)`, "in", emails))
    }
    if (phones.length) {
      ors.push(eb("phone", "in", phones))
    }

    return eb.or(ors)
  })
  .compile()

query.sql
// 'select * from "users" where 1 = 0'
@koskimas koskimas added the duplicate This issue or pull request already exists label Nov 29, 2024
@rafaelalmeidatk
Copy link
Author

rafaelalmeidatk commented Nov 29, 2024

Hi @koskimas first of all I appreciate the work on the library, I don't think this issue is a duplicate. The other issue I linked was related to accepting anything other than an array as the 3rd argument, and the comment I linked to was related to providing type errors for empty arrays which you said is not worth it and I agree.

What I am proposing here is that kysely correctly fixes the query output when an empty array is provided to where in because it is a cognitive overload to always be aware of empty arrays when using where (which is almost aways possible when doing some filtering). Let me know if you still think this is not common enough and should be handled on the user side, but if you think the proposed behavior makes sense then I would happly try to open a PR

@igalklebanov
Copy link
Member

Hey 👋

This is a duplicate. There's even a pull request with a plugin that aims to handle this.

@igalklebanov igalklebanov added the enhancement New feature or request label Nov 29, 2024
@rafaelalmeidatk
Copy link
Author

Hey thanks for commenting about the plugin, I found the PR 🙏 Sorry for not finding the other issue (#709) before, for some reason it didn't show up the first time I searched it. Having this as a plugin would be great, for the time being I am using this:

export function whereInNonEmpty<
  DB,
  TB extends keyof DB,
  RE extends ReferenceExpression<DB, TB>,
>(
  eb: ExpressionBuilder<DB, TB>,
  lhs: RE,
  rhs: ValueExpressionOrList<
    DB,
    TB,
    ExtractTypeFromReferenceExpression<DB, TB, RE>
  >,
): Expression<SqlBool> {
  if (Array.isArray(rhs) && rhs.length > 0) {
    return eb(lhs, "in", rhs)
  } else {
    return sql`false`
  }
}

// .where((eb) => whereInNonEmpty(eb, 'foo', []))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants