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

[tafkanator]

Issue type:
bug report

Database system/driver:
mysql / mariadb

TypeORM version:
0.2.5

example code:

import { In } from 'typeorm'
// ...
const res = await Entity.find({ where: { id: In([]) } })
// ER_PARSE_ERROR: You have an error in your SQL syntax; ...

Generated SQL is similar to

SELECT * FROM users WHERE id IN();

In MySql IN () (empty IN) is a syntax error. To fix it i propose that either:

• 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)
• In method converts empty array to FALSE so the result would be similar to

SELECT * FROM users WHERE id IN(FALSE); -- FALSE seems to work if used also in NOT IN(FALSE)

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

SELECT * FROM users WHERE id;

[pleerock]

Uhh hard to decide which one should be applied. I need more opinions on this topic.

[shusson]

I'd prefer parameters to be validated but I think it's also ok as it is as long as there is a clearer exception message and explicit documentation explaining that parameters should be validated before passing them to typeorm.

[davewasmer]

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.

[benmesirow]

@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.

[davewasmer]

@benmesirow I agree that if we had to translate IN () to something that is valid SQL syntax, WHERE FALSE is the closest thing.

However - I don't think the goal here should be "TypeORM will take your invalid SQL syntax and try it's best to transform it into something that is valid".

If the user is calling In(someEmptyArray), that is fundamentally invalid. If they want WHERE FALSE, they can supply that.

So why might someone do In(someEmptyArray)? My gut is that the vast majority of instances will be when the user supplied someEmptyArray but thought it wasn't empty. Since their request ("please add an IN clause with this empty set") is invalid SQL, it seems most appropriate to just immediately throw.

[Kononnable]

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]

we can't guarantee if someone is aware that array might be null

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.

In that case executing query with success but without results is expected.

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

I go back to my last point above. Perhaps someone might want the "return empty set rather than error" behavior, but those folks will be a vanishingly small group, they will be aware of their hack (they won't accidentally encounter that behavior), and can achieve it other ways.

Meanwhile, the vast majority of people who encounter this behavior will be people like me: they passed an array expecting it to always have values, and it didn't. Having TypeORM silently change the semantics of my query just to avoid an error could lead to maddeningly difficult-to-diagnose bugs.

[benmesirow]

After thinking about this more I have to agree with @davewasmer; for better or for worse, SQL doesn't support filtering by membership in an empty set, and there's no reason why an empty check on an array can't be preformed prior to composing the typeorm query, which will result in a more efficient query for cases where the array is in fact empty.

[Kononnable]

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.

[davewasmer]

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 😉)

[anodynos]

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' = ''".

[charmsRace]

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'),
  };
};

[Ginden]

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)

[bogdan]

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

[1valdis]

@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).

[bogdan]

@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.

[Anubhavjain786]

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

[MultiWar]

I'll have to agree with people saying it's better to throw an error. I've encountered this problem some days ago and, while I was able to see what's wrong, it would have been much faster if Typeorm had told me about the issue. However, it may be best to just return a warning instead of an error, if that's possible. By doing this, it would help people who encountered the problem accidentally (which I believe is most people) and wouldn't impede people who were doing it on purpose. Just adapting it to some other query would probably be disastrous, though

[imnotjames]

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.

[MultiWar]

I really didn't expect this result in the search of how other frameworks handle this, interesting. And yeah, I get that it might be weird to do something so different than others have been doing. I also have to admit that I don't have much experience, so my POV may be a bit skewed because of that. However, I still think that a warning really would be the best of both worlds, as it neither breaks any code nor keeps developers in the dark. Anyway, cool to learn that it will be handled one way or another. I'm looking forward to seeing how it will work

[imnotjames]

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

typeorm/src/query-builder/QueryBuilder.ts

Lines 919 to 922 in 0280cdc

	if ((this.connection.driver instanceof OracleDriver || this.connection.driver instanceof MysqlDriver) && parameters.length === 0) {
	return `${aliasPath} IN (null)`;
	}
	return `${aliasPath} IN (${parameters.join(", ")})`;

But not all of them.

[MultiWar]

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

[imnotjames]

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

[MultiWar]

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

[syuilo]

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

[Ginden]

@syuilo Use:

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

[nazar-xendit]

@syuilo Use:

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

@Ginden if the array is empty, the where clause will be foo.bar NOT IN (NULL, ), which is still a QueryFailedError

[voltane]

@syuilo Use:

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

@Ginden if the array is empty, the where clause will be foo.bar NOT IN (NULL, ), which is still a QueryFailedError

Thats true. Quick workaround is to pass new array like [null, ...<original_array>] which will never put empty array to query builder.