when passing empty list in .whereIn()

[HurSungYun]

Environment

Knex version: 0.15.0
Database + version: mysql 5.7
OS: mac OS X

Bug

I tried this

slots_db.client('table')
  .select()
  .where({ 'name': 'a' })
  .whereIn('release', []);

expected

select * from `table` where `name` = ?

but actually

select * from `table` where `name` = ? and 1 = ?

I believe it should be either the first one or throwing error.

I'd like to work on this issue.

[wubzz]

Honestly I think its perfectly fine the way it is. When calling .whereIn you are expecting to return rows based on specific criteria, in this case a list of values. If that list happens to be empty, we shouldn't ignore the fact that it was still a criteria for the query to begin with. Right now knex solves this by doing a 1 = 0 query returning no rows at all.

To me this is the same as saying an undefined value in .where({column: undefined}) should ignore the column criteria in the where clause, which feels very dangerous, especially in dynamic situations. Maybe a bad example since this throws Undefined Bindings error, but the point is the same.

[HurSungYun]

@wubzz I agree with your point.

thanks for your explanation in detail :)

[HurSungYun]

I think that throwing error can be a better solution for this situation.

In my use case, it makes business logic broken when ignoring empty list and returning nothing.

I believe it can be safer when knex throws an error. Users can realize their bugs quickly if we throw errors.

How do you think about this? @wubzz

[wubzz]

I still think its wrong to throw an error. You could just simply do this:

const releaseIds = [];
const builder = slots_db.client('table')
  .select()
  .where({ 'name': 'a' });

if(releaseIds.length) {
 builder.whereIn('release', releaseIds);
}

Most use-cases consist around argument to whereIn being a dynamically populated array, which may very well be an empty array sometimes. Changing this to throw an error is not only a breaking change, but also a hazzle in practise. Doing so would result in always having to do the example above unless your list of IDs is always static.

This is just my personal opinion, maybe better to get some more input.

@elhigu @tgriesser @kibertoad

[kibertoad]

@wubzz I wouldn't call it a breaking change since current behaviour doesn't seem to be documented, and I don't think it's the one that most users would expect.
Can you imagine a real-world use-case when you would actually prefer query to return nothing when some filtering criteria is empty?
I agree with @HurSungYun here, if we don't want to surprise users, it makes sense to require them to be more explicit in their intention instead of silently returning data that is different from what most users would expect.

[wubzz]

@kibertoad If we're evaluating breaking changes as something that must exist in documentation then I guess we are free to change or remove a lot in Knex without a breaking-change warning..

The reason I call it a breaking change is because for some applications that may have used Knex extensively will undoubtedly have used .whereIn in this exact context. The lack of issues on this precise issue makes me certain of that. Changing something like this in a minor version (that is to say not a breaking change) could potentially result in applications breaking left right and center. If that's not a breaking change, I don't know what is.

I don't see why .whereIn should silently ignore this scenario if all other .whereX functions throw errors, such as .whereExists(<empty>), .where({column: undefined}), etc etc.. Surely it's more "surprising" the user to completely ignore a requested criteria.

@tgriesser What are your thoughts on this now compared to few years ago #477 ?

[kibertoad]

@wubzz I'm basing this on precedents within the project; previously @elhigu mentioned that he does not consider changing ambiguous undocumented aspects of functionality a breaking change (e. g. how "schema.tableName" syntax became unsupported without a prior warning in some places). Which actually did break some production code :).

I agree that ignoring criteria is also going to be surprising, personally I'm more in favour of throwing an error.

[wubzz]

I feel there's a need to differentiate between accidental and deliberate breaking changes, documented or not. Don't know about the change you're referring to (even if maybe I should) and if it was deliberate or not, but in this case we'd know the outcome beforehand.

We seem to agree that silently ignoring is out of the question. The remaining alternative is to throw an error. Neither of these changes solve what is described in the issue example in terms of output. For dynamic lists you would always have to if(condition) { builder.whereIn() }

Turns out .whereIn is also consistent with the other .where functions when it comes to undefined values, so there's no consistency to be gained from this. An empty array [] surely is technically valid?

So I guess my question is: What is to be gained from changing this given the facts above?

My two cents is to keep it the way it is, but add it to docs.

[kibertoad]

@wubzz I mean #2499 which was caused by 7ff766f - but you are right, there wasn't originally an expectation it will break anything.

Turns out .whereIn is also consistent with the other .where functions when it comes to undefined values

Documentation claims otherwise:
Supplying knex with an undefined value to any of the where functions will cause knex to throw an error during sql compilation. This is both for yours and our sake. Knex cannot know what to do with undefined values in a where clause, and generally it would be a programmatic error to supply one to begin with.

For dynamic lists you would always have to if(condition) { builder.whereIn() }

But you already need to do this for .where() statements.

An empty array [] surely is technically valid?

Yes, but in all cases you would want to short-circuit call and never hit DB in the first place if you already know at query building time that you are getting 0 results no matter what. Meaning that remaining cases are almost always going to be due to users being negligent.

[HurSungYun]

Of course, everyone should write code like this if(condition) { builder.whereIn() } and I agree with it.

However, in my case of using Knex.js for the first time, I didn't know that fact and made a mistake of not checking the list.length.

Because Knex ignores invalid input and builds query like 1 = 0, I spent lots of time to understand the reason my code doesn't work as I expected.

I believe if Knex throws error when empty list is passed, users can figure out the fact that they need to check the length of list more quickly.

Shouldn't we guide users to use Knex.js properly with errors when invalid input is passed? I believe it would be a more generous way than ignore it silently.

[wubzz]

@kibertoad

Documentation claims otherwise

Huh? It explains exactly what I meant. Undefined to .whereIn -> same as .where({col: undefned})

But you already need to do this for .where() statements.

Not sure I'm following, I have never done that afaik.

Yes, but in all cases you would want to short-circuit call and never hit DB in the first place

This isn't really an argument, knex could just not issue the query then and return 0 rows immediately since it knows it will always be empty..

@HurSungYun Sure, except [] is not invalid. While the docs does not mention it builds a 1 = 0 query, it also doesn't mention that empty [] is not allowed. Not to mention it is a completely valid value in JavaScript. Thus, it cannot be considered invalid input in terms of argument structure. .whereIn(column|columns, array|callback|builder)

I'm gonna drop out of this discussion, think I've said all I can. Change it if you want. Personally think it's a mistake.

[elhigu]

This is actually a feature and it works like this by design. So closing as not a bug. Could be documented though.

[elhigu]

Shouldn't we guide users to use Knex.js properly with errors when invalid input is passed? I believe it would be a more generous way than ignore it silently.

It is not an invalid value like for example undefined would be. The generated code is logically valid for case where given list is empty and nothing should be matched.

An empty array [] surely is technically valid?

Yes, but in all cases you would want to short-circuit call and never hit DB in the first place if you already know at query building time that you are getting 0 results no matter what. Meaning that remaining cases are almost always going to be due to users being negligent.

There can be still other where clauses in query which still returns other results and it is pretty common that people are querying multiple where in or other where in in a single query.

[kibertoad]

Right, so we have team majority on the issue and path forward is clear 👍.
I'll update documentation.

[ricardograca]

OTOH, whereIn([]) seems it should logically map to WHERE something IN () which is invalid syntax, so the fact that in knex it still generates a valid syntax could be a bit unexpected.

[kibertoad]

@ricardograca Yup, that's pretty much how I personally see it, for me empty array is pretty much same thing as undefined in this case.

[elhigu]

Main point is that it was done current way by design and it was not an accident and it is the same way that many other ORMs handle it (like e.g. hibernate). It is just the way Tim wanted it to work originally, because it is more convenient for the end user of knex API.

OTOH, whereIn([]) seems it should logically map to WHERE something IN () which is invalid syntax, so the fact that in knex it still generates a valid syntax could be a bit unexpected.

For average user of knex .whereIn('colName', []) throwing an error because of invalid SQL syntax is very unexpected behavior. They would expect it to match with nothing (because average knex user actually seems to have very limited knowledge of SQL).

So it is unexpected in both ways and documentation is the correct place to tell why it behaves the way it does.

[ricardograca]

That makes sense as long as it's documented, so I think this should be reopened until the docs are updated.

[kibertoad]

@ricardograca I've created separate issue in doc repo for that.

[vantaboard]

Is there some shorthand that we could use though? A shorthand that checks the length and then skips the query if the length is falsy?