Comment 6 for bug 1164575

Dan,

I consider this fix a backstop only, and that you're right and the call points should be evaluated closely. It should plug any holes caused by jsonNumberToDBString callers, but not all potential users of that function may be making use of it.

The problem, ISTM, stems from an assumption about for what purposes jsonNumberToDBString should be used. The function itself was relying on the caller to make sure that it was really passed a number-ish user-supplied value, and therefore was just testing that the DB field to which that value would be tied in some way was, indeed, number-ish as well. It's used like that in some cases, but (as we now know) the IN-predicate case wants to use it the other way around -- we know this is a number-ish DB field, now quote some user-supplied value if it is non-numeric. But, of course, that's not what the function does.

So, we can look at it two ways: retain the old definition and fix the call sites to check the user-supplied value, or make jsonNumberToDBString more robust (but, potentially, badly named) and have it quote anything that's either 1) not a number-ish user-supplied value or 2) is trying to be used with a non-number-ish DB field.

The path of least code is the latter, obv, but a plan to attack it from the former angle (or, fresh eyes on the internal architecture of the query building in oils_sql.c, leading to a plan first, and action later) wouldn't be a bad thing. It would be a time-costly thing, I think.

Anyway, thanks much for testing and commit-ifying that, Dan. How shall we coordinate the back-branch releases we'll need to do? And, shall we wait on those before pushing out 2.4RC? I think yes on the latter, so we don't risk widening the exposure window to those on 2.3 and before.

Also, I know we're outside the 2.1 maintenance window, but should we consider putting out a source branch and instructions on how to build and install an updated oils_sql.so?