DB_query_builder->_where_in() produce an db error if $values array is…#4186
DB_query_builder->_where_in() produce an db error if $values array is…#4186j0inty wants to merge 1 commit into
Conversation
… empty Hi, today I saw an database error coming up to me as a result of the DB_query_builder->_where_in() function. The function only checks is the parameter $values !== NULL and is $values an array, but not the count of elements. ``` Error Number: 42601/7 ERROR: Syntaxerror at „)“ LINE 3: WHERE domain IN() ^ SELECT * FROM "domain" WHERE domain IN() AND "domain" NOT LIKE 'ALL' Filename: F:/workspace/postfixadmin-ng/build/dev/application/models/Domains_model.php Line Number: 56 ```
|
Why would you pass an empty array to |
|
Sample use-case: A search form with a multi-select control could produce such an empty array eventually, if a user has not been selected anything. |
|
-----BEGIN PGP SIGNED MESSAGE----- Hi, that was a situation in one of my scripts while I was develop on the But that's not a reason to not fix this annoying bug in your Framework. regards PS: I used my fixes since yesterday for the PA project and it works Am 23.10.2015 um 13:09 schrieb Andrey Andreev:
iQIcBAEBCAAGBQJWKhfKAAoJEIyoWxU/fWMU2dAP/A9LtvzglUDxF83tDv0h7lWY |
|
@ivantcholakov That could imply using unvalidated user input ... deffinately not one in favor of accepting the patch. @j0inty You're not at all answering my question with that. And it may be annoying to you for whatever reasons, but it's not a bug ... not compared to what you're suggesting. If you were suggesting to explicitly reject empty inputs, that would be another story. You're not supposed to pass an empty array to that function and silently discarding such calls is not doing you a favor. Quite the opposite, if you unintentionally pass an empty array to |
|
"...could produce such an empty array eventually..." - an empty array could be valid in the described case. Of course, if numeric values are expected in a non-empty array, the received array should be filtered, etc., but this is an outside topic. The query builder for its own purpose is going to make this check (for an empty array) for me, it would not be necessary insertion of additional code at the concrete place. The suggestion gives convenience, especially for those that like method chaining. |
|
-----BEGIN PGP SIGNED MESSAGE----- Hi,
No Question that it would help you out to find the empty array(). ;) But I think that the QueryBuilder has to check the Input, too. ;) And I my case where I found this problem for me was that I work with admin https://github.com/j0inty/postfixadmin-ng/blob/develop/src/application/m As you can see I could previously check the $this->auth->admin_domains regards PS: I know, some kind of CS Fixer is missing at time. ;) .... Am 23.10.2015 um 13:35 schrieb Andrey Andreev:
iQIcBAEBCAAGBQJWKiJXAAoJEIyoWxU/fWMUHi0QAIJ6K0uT//OuCsM+sLsKWZky |
In order to know whether fo "filter" (and this should be "validate", not "filter") or not, you have to make a check anyway. And since you have to do that, you can skip the @j0inty Here's the thing ...
Should the Query Builder check the input? I can agree that it should. But if SQL's In your particular case, a simple This explicitness even makes your code more readable, because now everybody who looks at it will know that there's a possible case where there are no domains to filter by. |
|
In the submitted code, while you check for an empty array, you don't check whether @narfbg while I agree that I can see both sides of the argument. While I like to write very forgiving code at times, I don't like the idea of my database getting hammered with an unfiltered query (or data being exposed to the wrong user) because I forgot to check a variable before passing it to one of query builder's where methods. I can see more potential for harm in receiving an unfiltered result set than an error, so I would lean more towards making the arguments required and erroring out on invalid input, when the method is called, rather than waiting for the database to return an error. Since changing the behavior in this way would probably be considered a BC break, I don't really think that's likely to happen in CI3, either. |
|
@mwhitneysdsu I too have no idea why it accepts nulls and I did notice that, but didn't mention it just to avoid arguing over two different problems at the same time. So far, I'm thinking of rejecting this one and removing the |
|
"But if SQL's IN() triggers an error in case of an empty output, why should CI do a different thing and silently discard it?" Hm..., probably this behavior has been chosen to prevent somebody from dumping lots of data by a mistake. |
|
A possible alternative, if you like it: // Class CI_DB_query_builder
/**
* An empty method that keeps chaining, the parameter does the desired operation as a side-effect.
*
* Sample usage if you want to build the query using one PHP sentence:
*
* $to_who = array('male', 'female', 'child'); // Let us allow this to be an empty array too.
*
* $found_products = $this->db
* ->select('id, name')
* ->from('products')
* ->where('in_stock', 1)
* ->that(empty($to_who) ? null : $this->db->where_in('to_who', $to_who)) // Edit: where -> where_in, my bad.
* ->limit(20)
* ->order_by('price', 'asc')
* ->get()
* ->result_array();
*
* var_dump($found_products);
*
* @param mixed $expression An expression for nesting context/scope.
* @return object Returns a reference to the created model instance.
*/
public function that($expression = NULL)
{
return $this;
} |
|
Today I've also encountered this inconsistency in CI's Query Builder, in that it will silently generate invalid SQL containing "IN ()". In order to make it consistent while also not producing invalid SQL or changing the logic of the produced SQL statements, I propose the following solution: For For This fix prevents CI from producing invalid SQL for empty($values) IN()s, which makes it self-consitent with handling P.S. CI should probably not be silently dropping _where_in() variations for NULL arguments, I'm not sure why it doesn't first |
|
@DaneelTrevize Sorry, but no. The issue is still open just as a reminder to drop the |
|
Being as you know that bad SQL is going to error, why would you not throw an exception beforehand instead and more cleanly isolate the issue sooner? There is no purpose to querying the DB when you already know what the outcome will be, other than taking more time & resources. |
|
That's kind of a loaded question ... Almost any question that asks "why not" is. It assumes that we have to do anything about that. Truth is both we don't really have to, and noone really thought of the possibility in the first place. And honestly, I don't think it's a problem of any kind. Again, there's no valid reason to pass an empty array as the input, under no circumstances. If it ever happens it's an oversight on the part of the developer, and it's not a common thing either, so why should we bother with it? Even the argument that we shouldn't unnecessarily hit the DB is kinda silly, because it means we have to add at least one more function call for all cases, so if we're talking about overhead, that doesn't net us any benefits. There's also another answer though: because CI hasn't been built to do exceptions, yet. That's something we plan for 3.2, but to be consistent with the currently used style of coding, it would be a simple We probably will indeed throw an exception for this in 3.2, but even if we didn't, that really wouldn't be an actual problem for anybody. |
|
I'll admit I fed you enough rope to hang yourself with, as a quick way to assess how worthwhile this framework is long term. I already mentioned error logging
Quibbling the performance overhead of adding an Expressing no desire to hold this framework to a scalable or sustainable standard, such as ensuring all calls to the QB return cleanly (via succeeding, or logging errors[, or silent ignoring of inputs]), seems shortsighted. If you can't see the difference in value between it at least doing the consistent silent |
Eh? I don't know what that means or what you're talking about.
Fair enough, you did mention error logging. But I'm not talking about logging ... I'm talking about error reporting, in the sense that an exception would give you a better indication of what's wrong than an SQL error. And really, you came here with an elaborate plan for how to translate an empty array into valid SQL; that's certainly not suggesting what I meant.
I'm sorry, but that's a load of bullshit.
What I'm saying is that if there's "time and resources" (as you called it) that you should care about, it's during valid code execution paths. It is ridiculous to care about that in the one odd case where you made a programming mistake.
Those are some fancy, but snarky words that you've used here, and written in quite an accusatory way. Did I do something to you that I don't know of? But putting that aside, the "standard" you're talking about is something I see very often - people wanting to hide errors, as if that solves all the problems. And that's a fallacy. I'll say this yet again: we're talking about an error condition, and not one where the user made a mistake, but one where we have a programming error. It is not ok to be silent in those cases. I agree that the error is ugly, but at least it tells you that you did something wrong - it helps you to debug the problem.
Another loaded question ... It's your obligation as a developer to validate all user input, and yes I sure do want to compel you to do that. Also, the default NULL input is crap that shouldn't have existed in the first place. I don't know if backi in the day the EllisLab guys had an internal rule to always have default parameter values or something (because I've seen and removed that a lot), but it's absolutely the wrong thing to try to be consistent with.
And more accusations encapsulated into yet another loaded question ... Literally in your last sentence you advocated against "duplication", and now you're talking about recovery? You know damn well that if you wouldn't do simple validation on the input, you wouldn't check for a return value or an exception either. And I'm repeating myself here for the countless time, but we're talking about a programming error. The recovery is you fixing your code. Also, entry-level set theory logic? Wat?
I do see the difference in value - a silent See, at first I thought you wanted an exception and as I've already said, I'd agree with that - gives you the feedback from the SQL error in a nicer way and allows for recovery. But now you insist on silently returning false?! That would be the absolutely worst choice and if you're at a loss of words (though it hardly seems that way), I guarantee you it's not because you know it all. |
|
TL;DR:
Elaborate? No, more like
CodeIgniter offers Query Builder as a feature, which has a family of where_in() functions.
Whether the functions are designed to always just return silently, or additionally sometimes log errors, or throw exceptions, is a different issue than what they're actually trying/designed to do with the given inputs in the first place. And for the record, I'm of the opinion that an empty array should be handled as per the relational-equiv. SQL, and that null $values could either throw invalid argument exception, for not being a set/array (a array containing just 1 null value would be valid input), or would result in the SQL that filters all rows( WHERE FALSE). W.r.t. code duplication by CI users, and the actual value of using CI/any framework at all vs simply reimplementing pieces of it every they time they're needed, I will reference Postel's law from the TCP specification:
If a framework offer a guarantee of consistency, e.g. always success or fails but regardless does return from the function call & return control to the user, then the user has the choice & responsibility to handle inputs & outputs or not. They are not forced to check outputs & return values but the framework can make that clearly the user's fault if they do not. But if the framework can't offer any guarantee that it won't blow up, emit arbitrary SQL & end the user's control via an emitted query error, then every sane user is forced to try duplicate the obvious input checks in each of their codebases (which some will omit/get wrong), probably by defining some wrapper layer that would be better suited being an official part of the framework specification in the first place & folded back into the input validation of these functions. |
|
I don't want to jump in too far on this, since I haven't been around or using CI much, but I keep getting notified and wanted to drop a gentle reminder that just slapping an
So, you're probably looking at something more along the lines of: If you want to argue that the other empty values shouldn't be permitted, either, you could save yourselves the extra code and just knock one of the equal signs off the $values === NULL comparison and add a comment to explain the variance from the code style guidelines. The code is so far removed from the original design decisions at this point that it's primarily a question of what makes sense for continued maintenance and whether similar functionality should change in CI 4. Finally, any appeal to Postel's Law in PHP-land needs a link to The Harmful Consequences of the Robustness Principle. |
|
Or just type hint the argument as Ok, Postel's was something of an appeal to authority, although that rebuttal seems to focus on related problems with divergence of actual network protocol implementations.
In our context of CI & QB, I'd say that means it should expect to handle an empty $values, even if that means silently ignoring it, rather than producing SQL that errors hard. However: |
I guess I missed that part. As an added bonus, if you declare the type of $values as an array, you can remove the creation of an array from $values as well. I wonder how many people are using the various where_in methods with non-array values... Regarding Postel's, I was trying to avoid specifics and go to the generalized point that it can cause problems if extended far enough. We probably all agree that it might be nice if something less bad happened, and maybe there could be something good that could happen, but the amount of change that might actually be permitted in this method is likely very different if you're talking about CI 3.1.x vs. 3.2 vs. 4.0. It's also pretty pointless to talk about whether people can find value in the framework without a change when some of the code we're discussing is over 10 years old and the issue itself is a couple years old. |
|
Sorry that I won't be addressing everything that was written, but it's not worth it for a minor issue like this. I'll just say this:
|
5 participants
Hi,
today I saw an database error coming up to me as a result of the DB_query_builder->_where_in() function.
The function only checks is the parameter $values !== NULL and is $values an array, but not the count of elements.