kylefarris / node-querybuilder

Node QueryBuilder Adapter for Node.js (tags: nodejs, node, mysql, active record, activerecord, querybuilder, query builder)
49 stars 19 forks source link

calling where_in with an empty array returns all rows #49

Open Flamenco opened 5 years ago

Flamenco commented 5 years ago
qb.where_in('id',[])
qb.get(...)

This results in all rows returned, instead of no rows returned.

kylefarris commented 5 years ago

Personally, I think that is the appropriate result for this case. When an invalid or empty list is provided, it should probably be ignored.

Flamenco commented 5 years ago

When one query returns the empty array, it is not appropriate to return all rows in a second query. This should mirror the behavior of a select where foo in (select ...). or a join

I end up having to micro-manage every time I use the result set of queries by checking if they are empty.

Please reconsider.

kylefarris commented 5 years ago

Sorry man, this is a query builder not something that's going to cover every edge case. Really, it should probably throw an error since that's what writing a query like this: SELECT * FROM foo WHERE id in () would do. I chose to take the route of assuming it was a mistake and gracefully ignoring the clause.

If you want it to work like a join or a sub-query, you should probably write it as such. Otherwise, as a general practice for all programming (not just this module), I would suggest checking the outcome of previous queries before using those results in something else. If you don't do that in general, you going to have a lot of surprises in your program.

Flamenco commented 5 years ago

The correct generated SQL should be:

SELECT * FROM foo WHERE false

IMO your query-builder should not return an error when it is a conclusion that no results can be returned. It should return an empty resultset. Certainly not all rows. This is not an edge case issue.