Empty parameter passed to where_in can return ALL items, instead of none

[eluser]Unknown[/eluser]
The db-> where_in function has a major gotcha in it.

If NULL is passed as values to the function, it is possible for the overall db query to return all of the items in the table, rather than the expected none.

Example

$this->db->from('geotargets');
$this->db->where_in('offer_id', $offer_ids);

If $offer_ids is not NULL, the query executes as something like this:

SELECT * FROM (`geotargets`) WHERE `offer_id` IN ('5', '18', '20', '8', '7', '11')

However, if $offer_ids is NULL, I would expect the query to return NULL, or an empty set, as there are no "in" parameters to evaluate. But because _where_in simply "return"s if $offer_ids is NULL, the query that gets executed is:

SELECT * FROM (`geotargets`)

I know I can check for a NULL $offer_ids before I execute the query, it just seems very dangerous for the where_in to simply "return" if the value is NULL, rather than doing something like:

if($values === NULL)
{
/* general idea, append a WHERE $key IN (NULL) to the query so the query returns nothing, rather than everything */
$query .= 'WHERE $key IN (NULL)';
}

which would return an empty set.

[eluser]TheFuzzy0ne[/eluser]
Welcome to the CodeIgniter forums!

That's to be expected. If you don't pass a any values for the WHERE IN, then it doesn't get set, therefore no WHERE clause gets set, and thus all rows are returned.

It's just like doing $this->db->where(//nothing here). You're ommitting the WHERE, so it doesn't get set. Otherwise it would generate SQL along the lines of:

Code:

SELECT *
FROM `my_table`
WHERE

Which isn't valid SQL, and will cause a database error.

My question to you: What would you expect it to do?

[eluser]Unknown[/eluser]
Thanks for the quick reply. I've actually been a CI user for about 6 years now, and I love it!

This issue came up in controller code that looked something like this:

$offer_ids = $this->host_model->GetActiveIds($host_id)
$geotargets = $this->geotarget_model->OfferIds_GetGeotargets($offer_ids);

99.9% of the time $offer_ids had something in it, but in very corner cases, $offer_ids was NULL. When that corner case hit, I would have expected OfferIds_GetGeotargets to return no geotargets, as there were no offer_ids to look for. Instead it returned ALL geotargets.

I guess my point is that _where_in dropping that portion of the query on the floor when $values is NULL is counter intuitive; I would have expected it to return nothing, instead of everything. That is possible if the code does a WHERE $key IN (NULL) - which is valid SQL and returns nothing.

To put it another way, I asked for all objects of color "", and it returned every object of all colors.

At the least, I'd expect a query error, or an error to be logged in this condition, rather than the whole thing being disregarded, or perhaps the documentation for where_in could have a warning about passing NULL for values.

In summary, if $values is NULL, I'd do a "WHERE $key IN (NULL)" rather than just "return"

[eluser]TheFuzzy0ne[/eluser]
I just spent an hour typing a reply, only to be pounced on by Ellis Lab's security measures, which stopped me from posting and caused me to lose the lot. Grr...

In short, I disagree.

$this->db->where_in() expected an array of values, and you didn't pass an array of values, so it returned. Rather than throwing a database error due to a malformed query and breaking your app, it spotted that you didn't pass it an array and gracefully bailed out. The user guide entry for where_in() makes no mention of any other parameter type besides an array, (and I think it would be slightly unreasonable to expect them to ask them to list all of parameter types it won't allow. ) If this->geotarget_model->OfferIds_GetGeotargets() returned an empty array() (as where_in() expects), you'd have got a database error and/or a message in your log. (Isn't this what you wanted?)

The Active Record library is robust, while not being more complex than it needs to be. It does rudimentary parameter checking, to keep any unnecessary errors to a minimum.

I think your model should be checking the parameters provided and not making the query if you're not getting the arguments you expect, just like the Active Record library does, then it's down to you to define how to react when the method is passed a parameter other than what it's expecting (i.e return FALSE, throw an error/exception, log a message, set a default value (e.g. array(NULL)) or whatever else).

I think it encourages defensive coding, which can really help to strengthen your application. I try to always check any parameters if there's even a remote chance that the parameters won't be what I'm expecting them to be. That way it's up to you how you handle errors and where, (i.e in a controller, model, library etc..).

With that said, I see no reason why where_in() can't cast the first value to an array, if it's not an array already. That would be a single line of code. It may be worth suggesting?