typeorm: SQL Syntax error when empty array is given into "In" method

There is workaround. You can do:

const res = await Entity.find({ where: { id: In([null, ...ids]) } })

X IN (NULL) is equivalent to 1=0, and X IN (NULL, 3) is equivalent to X IN (3)

IMO the way typeorm treats In(null) is counter intuitive to people who are not 10 years in the SQL world. Ideally I would want ORM to understand what I mean like so:

find({where: {key: [null, 1, 2]}) // => where key is null or key in (1,2)
find({where: {key: [undefined]}) // => where key is null
find({where: {key: []) // => where 1 = 0

@syuilo Use:

Foos.createQueryBuilder('foo')
	.where(`foo.bar NOT IN (NULL, :...xs)`, { xs: [] }) // QueryFailedError

This Issue is closed, but there is still a similar problem with the query builder.

Foos.createQueryBuilder('foo')
	.where(`foo.bar NOT IN (:...xs)`, { xs: [] }) // QueryFailedError

I’ve been spending time looking through how other similar frameworks handle it:

• Laravel: 1=0
• SQLAlchemy: 1!=1
• Knex: 1=0
• Sequel: 1!=1
• Sequelize: IN (NULL)

I was back and forth thinking about this and the best way to approach the problem - to throw an error or to keep on trucking on. In a lot of ways it’s nice to throw an error because it’d be explicit - but I think it’s also good to think through the intent of the empty array - If you say “I want anything that matches the things in here” and there’s nothing there - then there can be no matches.

Also, I can’t find anything that actually throws an error - and I am always iffy on building an API that goes against the grain. I’ll be opening a PR shortly to work on this.

I don’t think we should throw an error here - maybe in most cases users would assume passed array isn’t empty, but we can’t guarantee if someone is aware that array might be null. In that case executing query with success but without results is expected.

But if we know that query will have no results do we really have to execute it? Or maybe just execute it with WHERE 1=0 to make it easier to implement.

@davewasmer @tafkanator @pleerock I believe a semantically equivalent clause that could replace the IN (:...) part would be WHERE FALSE, since, by definition of empty an empty set, a checking for membership of any element in an empty set will return false.

improve type annotations not to allow empty arrays to be inserted inside In method with guard for checking array.length to be > 0 (this would be closest to mysql spec)

I don’t think it will be possible to catch this at the Typescript layer - Typescript doesn’t differentiate between empty arrays vs. arrays with entries, so we can’t type-check this.

I do think that have some kind of runtime check would be valuable. Something that throws if you supply an empty array to In, since it is fundamentally a nonsensical operation. And TypeORM errors can be much more helpful than letting the database handle the error, since you get a stack trace right to the invocation site.

In method converts empty array to FALSE so the result would be similar to

Strongly disagree. This is a semantically different query. I.e. it’s possible false is a valid value for that column, which could result in rows matching that shouldn’t match.

remove IN() clause (may or may not create other sql syntax errors)

Same here. This fundamentally changes the query, leading to potentially dangerous results. What happens if I do an UPDATE ... WHERE id IN (...), and the IN clause gets stripped? Now I’ve updated every row in that table.

There is workaround. You can do:

const res = await Entity.find({ where: { id: In([null, ...ids]) } })

X IN (NULL) is equivalent to 1=0, and X IN (NULL, 3) is equivalent to X IN (3)

It’s Working, Thanx @Ginden

Both solutions seems logical to me.

If TypeORM’s API was a higher level abstraction over SQL, then I would agree. But the QueryBuilder API is pretty close to a 1:1 mapping over SQL concepts.

In other words - I think transforming to a filter would be fine if the method was called Filter(). But it’s not - it’s In(), because you’re adding an SQL IN clause, which fundamentally does not support empty sets.

I think I’ve made my case at this point, and it doesn’t seem like there’s a lot of new information coming from further discussion. I’m happy to elaborate further if any of my points are unclear, but I also don’t want to belabor the point if you’re not interested in changing this behavior.

Thanks for your work on TypeORM @Kononnable - it’s great project (regardless of how this issue shakes out 😉)

Cool, I’ll definitely check those links out when I get to my computer tomorrow. Thanks for letting us know how things are going.

There’s a lotta issues so I sometimes miss some.

Of course, it’s probably almost impossible to keep track of everything. I sometimes can’t even keep track of my own small repos hahaha

I really didn’t expect this result in the search of how other frameworks handle this, interesting.

I was actually doing the research separately for opening a PR and then found this issue as well 😃 There’s a lotta issues so I sometimes miss some.

Also, I just realized that this comment section is talking specifically about mysql and mariadb so I think it’s nice to add that it happens in mssql too, which is where I experienced it. Sorry if this makes my comments irrelevant or if I should be commenting on another issue/creating my own for this. But it’s the same issue, anyway

This issue is across all dialects, so don’t worry about that.

I have a branch where I’ll be getting this change in & opening a PR - it’s currently only got the change to our tests to verify the behavior is currently not working & the tests for that are running here

Also - for some dialects we are already handling an empty array:

https://github.com/typeorm/typeorm/blob/0280cdc451c35ef73c830eb1191c95d34f6ce06e/src/query-builder/QueryBuilder.ts#L919-L922

But not all of them.

@1valdis SQL is 30 years old. There are many brave enough to fix its design issues in other ORMs that are already doing that. TypeORM is behind by not doing that and denying progress.

@bogdan I disagree. in (null) giving nothing was one of the first things I learnt when learning about in in the first place. The way it works now is closer to SQL level (which is QueryBuilder all about).

On that note, here’s a functional solution for FindOptions instead of QueryBuilder:

private createTypeConditions = async (type: string) => {
  const ids = await this.getIdsForType(type);

  return {
    id: ids.length > 0 ? In(ids) : Raw(() => 'FALSE'),
  };
};

A naive but useful userland solution, is using @davewasmer 1=0 with a few helpers like :

export const querySafe_AndWhereInIds = <T extends WhereExpression>(query: T, ids: any[] = []) => {
  return _.isEmpty(ids) ? query.andWhere('1=0') : query.andWhereInIds(ids);
};

export const querySafe_OrWhereInIds = <T extends WhereExpression>(query: T, ids: any[] = []) => {
  return _.isEmpty(ids) ? query.orWhere('1=0') : query.orWhereInIds(ids);
};

I just wish we had this sort of sanity checks inside the lib, since it lib can tell at runtime that translating an empty array [] to an SQL IN () will cause an SQL execution error.

And this doesn’t break the query at all, it’s semantically equivalent and would return the same result of the query if IN () wasn’t an SQL idiocracy. To help debugging SQL, we could change it to "'EMPTY_ARRAY_IN_EXPRESSION' = ''".

But if that’s the case, they are essentially telling TypeORM to build invalid syntax. There are other ways to get TypeORM to build a query that returns no results.

Depends on how you look at it. You can try to ‘translate’ each part of the queryBuilder syntax to strict sql(entity.Id IN ()). But you can also ‘translate’ it to more Javascript-like solution([].some(v=>v===entity.Id)). Both solutions seems logical to me.

If you want a query with no results, you can add your own .andWhere(‘1=0’).

I could say something similar - you can always check length of the array before running the query. But it’s not the point. We just have to assume which solution is more common practice because both are good(and bad) based just on which are you used to.

Or we could just see how other ORM deals with such situations.