[FEATURE]: Allow to pass empty values to inArray

[nounder]

Describe what you want

Passing empty array to inArray throws:

inArray requires at least one value

I think it would be more ergonomic to allow empty array as it would avoid additional checks outside of query builder.

Also, throwing an error causes unexpected runtime error if the array changes dynamically.

That's actually what have happened to me with following code:

const topPosts = await DB.select(posts.text, posts.mentions).from(posts)
// following will throw if there happen to be no mentions in dev database, but may surely be on production
// causing unexpected runtime error
const mentionedPeople = DB.select().from(people).where(inArray(people.id, posts.flatMap(v => v.mentions))

doHeavyweightCalculationsUnsuitableToADatabase(topPosts, mentionedPeople)

[bpinney]

I believe such a change would lead to an easier developer experience. That being said, similar changes to other aspects of the API have been closed by the team with no action.

See #1078

[sys13]

I would expect notInArray with an empty array to basically be as if there was no filter at all. It is true for every row that they don't match any elements in the array.

[Angelelz]

I believe such a change would lead to an easier developer experience. That being said, similar changes to other aspects of the API have been closed by the team with no action.

See #1078

I'm not part of the team. I'm just trying to help cleanup the issues.

There are two options that have been discussed:

1. Make inArray and notInArray accept a tuple with at least one value, ie: [T, ...T[]]. That will make sure the user has to be certain that the array has at least one value. The user will probably have to implement something like this to please the compiler:

function isTuple<T extends any[]>(array: T): T is [T, ...T[]] {
   return array.length > 0;
}

And handle the case accordingly in userland.

2. Include the array.length > 0 in the inArray and noInArray functions so that they return false and true respectively.

The down side of the first is that the user has to deal with it (like they are right now, just now the types help them figure it out they need to do it), and handle the case when the array is empty, like this for example:

...
.where(isTuple(myArray) ? inArray(column, myArray) : sql`false`)
...

The down side of the second one is that it introduces hidden behavior, we just don't know down the line what other issues it will start.

[sys13]

I think it's reasonable for a user to assume the following functionality:

• inArray with an empty array would filter out all values
• notInArray with an empty array would keep all values

In JS: [].includes('hi') -> false, which is consistent with the above proposed functionality. The functionality does not prevent a developer from checking for an empty array if they would so choose. Forcing them to check is the current state as notInArray with an empty array results in a runtime error (which I was not expecting and did not get a type error for), but also leads to needing more code to check for empty arrays.

[rbuchberger]

Adding a +1 for this; throwing an error when passed an empty array is not useful nor expected, nor does it raise a typescript error which especially sucks.

[mikkelwf]

I've been using inArray extensively in my recent project and handling this behavior is my single biggest pain point with drizzle.

Throwing an error because the array is empty is not expected at all, and I've spent a fair amount of time fixing production code just because of this "feature".

I honestly don't see why (as a benefit to the developer) the empty array is not handled internally by drizzle and that it is expected that this should be handled externally outside drizzle instead.

[DenisBessa]

This could be a solution: https://moderndash.io/docs/arrayminlength

[RemiPeruto]

Hello guys !
Since @Angelelz seems to be busy I recreated a merge request to close this issue.

Do not hesitate to add your thumb-ups ;)

[kenn]

Recently migrated an app from Prisma to Drizzle, and this has been a big problem.

Major ORMs handle this case gracefully, like with Prisma:

await prisma.user.findMany({ where: { id: { in: [] }}})
// => just return empty result

With Rails:

User.where(id: [])
# => converted to WHERE 1=0 and return empty result

---

I'll try to explain how bad this can get.

This innocuous query:

await db.query.users.findMany({
  where: (users, { inArray, or }) =>
    or(
      inArray(users.a, arrayA),
      inArray(users.b, arrayB),
    ),
})

has to be rewritten with a guard logic like this:

let where

if (arrayA.length === 0 && arrayB.length === 0) {
  return []
}
if (arrayA.length > 0 && arrayB.length === 0) {
  where = inArray(users.a, arrayA)
}
if (arrayA.length === 0 && arrayB.length > 0) {
  where = inArray(users.b, arrayB)
}
if (arrayA.length > 0 && arrayB.length > 0) {
  where = or(inArray(users.a, arrayA), inArray(users.b, arrayB))
}

await db.query.users.findMany({ where })

combinatorial explosion awaits. :)

[Hansenq]

I agree this should be fixed, since it's a big barrier to adoption of drizzle for those moving from other ORMs. It's one of those edge cases where matching SQL is not worth the tradeoff.

For those who need a temporary solution, I would recommend re-exporting inArray with your own logic, so that you can perform a runtime check and handle the empty array case yourself:

re-exports.ts

import { 
  inArray as drizzleInArray,
  notInArray as drizzleNotInArray,
  sql,
} from "drizzle-orm"

/**
 * Test whether the first parameter, a column or expression,
 * has a value from a list passed as the second argument.
 *
 * ## Examples
 *
 * ```ts
 * // Select cars made by Ford or GM.
 * db.select().from(cars)
 *   .where(inArray(cars.make, ['Ford', 'GM']))
 * ```
 *
 * @see notInArray for the inverse of this test
 */
const inArray: typeof drizzleInArray = (
  ...args: Parameters<typeof inArray>
): ReturnType<typeof inArray> => {
  const [column, values] = args;
  // https://github.com/drizzle-team/drizzle-orm/issues/1295
  if (Array.isArray(values) && values.length === 0) {
    return sql`false`;
  }
  return drizzleInArray(...args);
};

/**
 * Test whether the first parameter, a column or expression,
 * has a value that is not present in a list passed as the
 * second argument.
 *
 * ## Examples
 *
 * ```ts
 * // Select cars made by any company except Ford or GM.
 * db.select().from(cars)
 *   .where(notInArray(cars.make, ['Ford', 'GM']))
 * ```
 *
 * @see inArray for the inverse of this test
 */
const notInArray: typeof drizzleNotInArray = (
  ...args: Parameters<typeof drizzleNotInArray>
): ReturnType<typeof drizzleNotInArray> => {
  const [column, values] = args;
  // https://github.com/drizzle-team/drizzle-orm/issues/1295
  if (Array.isArray(values) && values.length === 0) {
    return sql`true`;
  }
  return drizzleNotInArray(...args);
};

export { inArray, notInArray };

Then in your file where you export db:

export * from "./re-exports";

[kenn]

For those who need a temporary solution, I would recommend re-exporting inArray with your own logic, so that you can perform a runtime check and handle the empty array case yourself:

re-exports.ts

Oh this monkey patch is a lifesaver! Thanks a ton @Hansenq !

[RemiPeruto]

@AndriiSherman reviewed the MR.
The feature should be released soon 🚀 !

[AndriiSherman]

Was released in https://github.com/drizzle-team/drizzle-orm/releases/tag/0.32.1

[jakeleventhal]

@AndriiSherman @RemiPeruto I am looking to update some code to account for this. one example looks like this:

if (idsToDelete.length) {
  await db.delete(someTable).where(inArray(someTable.id, idsToDelete));
}

With this PR, does it make sense to keep the code as is to avoid a useless query? or will drizzle abort a DB call and just resolve the promise if the sql evaluates to nothing. it would be nice to be able to remove the if statement and just run the delete statement knowing that no calls to the db will be made if the resulting SQL results in:

sql`false`

[fstaffa]

@jakeleventhal looking at the MR, it does nothing to cancel the query. It just changes a part of the generated SQL to false. To be able to not make a DB call, drizzle would have to be able to prove that the query will do nothing. From what I understood, this is not a design goal of drizzle.

In your case the SQL would look simply like

DELETE FROM sometable WHERE false

which will always fail, but the query might be more complicated like:

await db.delete(someTable).where(or(inArray(someTable.id, idsToDelete), true));

which would look

DELETE FROM sometable WHERE false OR true

and suddenly the query would have to be executed

[jakeleventhal]

#2765