602456 - Search.pm should insert numbers for numeric field/operator combinations

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Max Kanat-Alexander]

Right now Search.pm inserts all values as strings. On some databases, this may affect performance when you're actually searching a numeric column.

However, the real problem is that in SQLite, searching numeric values using strings often does not work, mostly when the numeric values are computed (like percentage_complete or longdescs.count). So, in order for SQLite to work properly (and possibly better performance elsewhere) we should be inserting numbers without quotes when we can.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

I need a review on this because it touches Bugzilla::Field.

Also, technically I'm doing something rather dangerous, which is deciding to not quote certain DB input. I'm pretty confident that my regex is good and safe, and xt/search.t doesn't indicate any problems with it, but I could always be overlooking something.

Comment 2

[Max Kanat-Alexander]

Attachment: v2

The regexp operators should also be marked as NON_NUMERIC. (Otherwise xt/search.t fails on Pg.)

Comment 3

[Frédéric Buclin]

Comment on attachment 481464 [details] [diff] [review]
v2

>=== modified file 'Bugzilla/Search.pm'
>+    my $numeric_operator = !grep { $_ eq $operator } NON_NUMERIC_OPERATORS;
>+    my $numeric_field = $self->_chart_fields->{$field}->is_numeric;
>+    my $numeric_value = ($value =~ NUMBER_REGEX) ? 1 : 0;
>+    my $is_numeric = $numeric_operator && $numeric_field && $numeric_value;

This code doesn't make sense to me. There is no check that custom fields will have all their values numeric. This code is pretty much for hardcoded fields only, plus custom fields of type BUG_ID, but there is no way to prevent e.g. an extension to use Bugzilla::Field->create({..., is_numeric => 1}) and add strings instead of numbers as legal values. Your regexp will prevent to fall into this security hole, but wouldn't it be better to check the DB column type? INT1-4 + decimal() would be numeric fields; not the other ones.

Comment 4

[Max Kanat-Alexander]

Yeah, an extension absolutely could set a field as is_numeric => 1, if they chose to. That's actually one of the reasons that I made it a database column, so that extensions could make that decision if they wanted to.

The regex will prevent any actual security issues, and is_numeric keeps things flexible.

Unfortunately, checking the column type would be very complex and wouldn't work. For example, assigned_to is an INT, but it should not behave like is_numeric.

Comment 5

[mackyle]

Is there any reason the NUMBER_REGEX expression requires non-integers > -1 and < 1 to start with 0?

The current NUMBER_REGEX clearly allows both 0.5 and -0.5 but doesn't seem to allow either .5 or -.5 to be used as numbers.  Was that intentional?  SQLite, for example, does not require the leading 0 and happily accepts both .5 and -.5 as numbers.

Comment 6

[Max Kanat-Alexander]

It was intentional, yes. If you can confirm that MySQL, PostgreSQL, and MS-SQL all support that syntax (or show it to me in the ANSI SQL standard) then I can modify the regex.

Comment 7

[Max Kanat-Alexander]

Also, one of the reasons it was done was to make it simple to exclude "." alone as being valid.

Comment 8

[mackyle]

From the SQL-92 spec (available from <http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt>) pp. 89-90:

>    <literal> ::=
>        <signed numeric literal>
>        | <general literal>
>
>    <unsigned literal> ::=
>        <unsigned numeric literal>
>        | <general literal>
>
>    <signed numeric literal> ::=
>        [ <sign> ] <unsigned numeric literal>
>    
>    <unsigned numeric literal> ::=
>        <exact numeric literal>
>        | <approximate numeric literal>
>    
>    <exact numeric literal> ::=
>        <unsigned integer> [ <period> [ <unsigned integer> ] ]
>        | <period> <unsigned integer>
>    
>    <sign> ::= <plus sign> | <minus sign>
>    
>    <approximate numeric literal> ::= <mantissa> E <exponent>
>    
>    <mantissa> ::= <exact numeric literal>
>    
>    <exponent> ::= <signed integer>
>    
>    <signed integer> ::= [ <sign> ] <unsigned integer>
>    
>    <unsigned integer> ::= <digit>...

Would seem to indicate that numbers like .5, -.5, +.5, .5e0 and -.5E+0 are all allowed by the SQL standard.  When a value is marked as numeric, it might be simpler to just insert an SQL fragment like this:

>    CAST('the-quoted-numeric-value' AS NUMERIC)

for example:

>    SELECT CAST('.5e0' AS NUMERIC);

to make the database do the conversion and validation rather than trying to write a REGEXP that accounts for any quirks that may be present in the various databases' implementation of the SQL standard.  To really handle approximate numeric literals well it might also be desirable to change the NUMERIC to DOUBLE PRECISION in the CAST if the numeric value matches /[eE]/ (see pg. 19 of the spec. referenced earlier).

Comment 9

[Max Kanat-Alexander]

Okay, thanks for the info! I may not be available to update the patch for a little while, but I can see if I can come up with a workable regex that also allows .\d+ alone.

As far as CAST goes, we wouldn't do that because CAST does not work interoperably between existing SQL servers that we support, and also may have performance implications in some situations (where NUMERIC would translate to something different than the actual column type being compared against).

Comment 10

[Max Kanat-Alexander]

Attachment: v3

Okay, here it is with a re-worked regex that should support everything.

Comment 11

[:glob ✱]

Comment on attachment 486308 [details] [diff] [review]
v3

r=glob

Comment 12

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Field.pm
modified Bugzilla/Search.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
Committed revision 7582.