Bug 602456: Make Search.pm not quote numeric input for numeric fields

when generating SQL.
r=glob, a=mkanat

4 files changed, 120 insertions, 20 deletions

diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index cfa1608f9..56e763d20 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -683,6 +683,8 @@ use constant ABSTRACT_SCHEMA => {
             reverse_desc => {TYPE => 'TINYTEXT'},
             is_mandatory => {TYPE => 'BOOLEAN', NOTNULL => 1,
                              DEFAULT => 'FALSE'},
+            is_numeric    => {TYPE => 'BOOLEAN', NOTNULL => 1,
+                             DEFAULT => 'FALSE'},
         ],
         INDEXES => [
             fielddefs_name_idx    => {FIELDS => ['name'],
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index 1229d31cb..052717f00 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -103,6 +103,7 @@ use constant DB_COLUMNS => qw(
     value_field_id
     reverse_desc
     is_mandatory
+    is_numeric
 );
 
 use constant VALIDATORS => {
@@ -120,9 +121,11 @@ use constant VALIDATORS => {
     visibility_field_id => \&_check_visibility_field_id,
     visibility_values => \&_check_visibility_values,
     is_mandatory => \&Bugzilla::Object::check_boolean,
+    is_numeric   => \&_check_is_numeric,
 };
 
 use constant VALIDATOR_DEPENDENCIES => {
+    is_numeric => ['type'],
     name => ['custom'],
     type => ['custom'],
     reverse_desc => ['type'],
@@ -141,6 +144,7 @@ use constant UPDATE_COLUMNS => qw(
     value_field_id
     reverse_desc
     is_mandatory
+    is_numeric
     type
 );
 
@@ -161,7 +165,7 @@ use constant SQL_DEFINITIONS => {
 # the fielddefs table.
 use constant DEFAULT_FIELDS => (
     {name => 'bug_id',       desc => 'Bug #',      in_new_bugmail => 1,
-     buglist => 1},
+     buglist => 1, is_numeric => 1},
     {name => 'short_desc',   desc => 'Summary',    in_new_bugmail => 1,
      is_mandatory => 1, buglist => 1},
     {name => 'classification', desc => 'Classification', in_new_bugmail => 1,
@@ -199,15 +203,20 @@ use constant DEFAULT_FIELDS => (
     {name => 'qa_contact',   desc => 'QAContact',  in_new_bugmail => 1,
      buglist => 1},
     {name => 'cc',           desc => 'CC',         in_new_bugmail => 1},
-    {name => 'dependson',    desc => 'Depends on', in_new_bugmail => 1},
-    {name => 'blocked',      desc => 'Blocks',     in_new_bugmail => 1},
+    {name => 'dependson',    desc => 'Depends on', in_new_bugmail => 1,
+     is_numeric => 1},
+    {name => 'blocked',      desc => 'Blocks',     in_new_bugmail => 1,
+     is_numeric => 1},
 
     {name => 'attachments.description', desc => 'Attachment description'},
     {name => 'attachments.filename',    desc => 'Attachment filename'},
     {name => 'attachments.mimetype',    desc => 'Attachment mime type'},
-    {name => 'attachments.ispatch',     desc => 'Attachment is patch'},
-    {name => 'attachments.isobsolete',  desc => 'Attachment is obsolete'},
-    {name => 'attachments.isprivate',   desc => 'Attachment is private'},
+    {name => 'attachments.ispatch',     desc => 'Attachment is patch',
+     is_numeric => 1},
+    {name => 'attachments.isobsolete',  desc => 'Attachment is obsolete',
+     is_numeric => 1},
+    {name => 'attachments.isprivate',   desc => 'Attachment is private',
+     is_numeric => 1},
     {name => 'attachments.submitter',   desc => 'Attachment creator'},
 
     {name => 'target_milestone',      desc => 'Target Milestone',
@@ -217,26 +226,32 @@ use constant DEFAULT_FIELDS => (
     {name => 'delta_ts',              desc => 'Last changed date',
      buglist => 1},
     {name => 'longdesc',              desc => 'Comment'},
-    {name => 'longdescs.isprivate',   desc => 'Comment is private'},
+    {name => 'longdescs.isprivate',   desc => 'Comment is private',
+     is_numeric => 1},
     {name => 'longdescs.count',       desc => 'Number of Comments',
-     buglist => 1},
+     buglist => 1, is_numeric => 1},
     {name => 'alias',                 desc => 'Alias', buglist => 1},
-    {name => 'everconfirmed',         desc => 'Ever Confirmed'},
-    {name => 'reporter_accessible',   desc => 'Reporter Accessible'},
-    {name => 'cclist_accessible',     desc => 'CC Accessible'},
+    {name => 'everconfirmed',         desc => 'Ever Confirmed',
+     is_numeric => 1},
+    {name => 'reporter_accessible',   desc => 'Reporter Accessible',
+     is_numeric => 1},
+    {name => 'cclist_accessible',     desc => 'CC Accessible',
+     is_numeric => 1},
     {name => 'bug_group',             desc => 'Group', in_new_bugmail => 1},
     {name => 'estimated_time',        desc => 'Estimated Hours',
-     in_new_bugmail => 1, buglist => 1},
-    {name => 'remaining_time',        desc => 'Remaining Hours', buglist => 1},
+     in_new_bugmail => 1, buglist => 1, is_numeric => 1},
+    {name => 'remaining_time',        desc => 'Remaining Hours', buglist => 1,
+     is_numeric => 1},
     {name => 'deadline',              desc => 'Deadline',
      type => FIELD_TYPE_DATETIME, in_new_bugmail => 1, buglist => 1},
     {name => 'commenter',             desc => 'Commenter'},
     {name => 'flagtypes.name',        desc => 'Flags', buglist => 1},
     {name => 'requestees.login_name', desc => 'Flag Requestee'},
     {name => 'setters.login_name',    desc => 'Flag Setter'},
-    {name => 'work_time',             desc => 'Hours Worked', buglist => 1},
+    {name => 'work_time',             desc => 'Hours Worked', buglist => 1,
+     is_numeric => 1},
     {name => 'percentage_complete',   desc => 'Percentage Complete',
-     buglist => 1},
+     buglist => 1, is_numeric => 1},
     {name => 'content',               desc => 'Content'},
     {name => 'attach_data.thedata',   desc => 'Attachment data'},
     {name => "owner_idle_time",       desc => "Time Since Assignee Touched"},
@@ -273,6 +288,13 @@ sub _check_description {
 
 sub _check_enter_bug { return $_[1] ? 1 : 0; }
 
+sub _check_is_numeric {
+    my ($invocant, $value, undef, $params) = @_;
+    my $type = blessed($invocant) ? $invocant->type : $params->{type};
+    return 1 if $type == FIELD_TYPE_BUG_ID;
+    return $value ? 1 : 0;
+}
+
 sub _check_mailhead { return $_[1] ? 1 : 0; }
 
 sub _check_name {
@@ -779,6 +801,21 @@ a boolean specifying whether or not the field is mandatory;
 
 sub is_mandatory { return $_[0]->{is_mandatory} }
 
+=over
+
+=item C<is_numeric>
+
+A boolean specifying whether or not this field logically contains
+numeric (integer, decimal, or boolean) values. By "logically contains" we
+mean that the user inputs numbers into the value of the field in the UI.
+This is mostly used by L<Bugzilla::Search>.
+
+=back
+
+=cut
+
+sub is_numeric { return $_[0]->{is_numeric} }
+
 
 =pod
 
@@ -821,6 +858,7 @@ They will throw an error if you try to set the values to something invalid.
 
 sub set_description    { $_[0]->set('description', $_[1]); }
 sub set_enter_bug      { $_[0]->set('enter_bug',   $_[1]); }
+sub set_is_numeric     { $_[0]->set('is_numeric',  $_[1]); }
 sub set_obsolete       { $_[0]->set('obsolete',    $_[1]); }
 sub set_sortkey        { $_[0]->set('sortkey',     $_[1]); }
 sub set_in_new_bugmail { $_[0]->set('mailhead',    $_[1]); }
@@ -1095,6 +1133,7 @@ sub populate_field_definitions {
             $field->set_buglist($def->{buglist});
             $field->_set_type($def->{type}) if $def->{type};
             $field->set_is_mandatory($def->{is_mandatory});
+            $field->set_is_numeric($def->{is_numeric});
             $field->update();
         }
         else {
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index 4dcf0d637..47c8873fa 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -115,6 +115,11 @@ sub update_fielddefs_definition {
     # 2010-04-05 dkl@redhat.com - Bug 479400
     _migrate_field_visibility_value();
 
+    $dbh->bz_add_column('fielddefs', 'is_numeric',
+        {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});
+    $dbh->do('UPDATE fielddefs SET is_numeric = 1 WHERE type = '
+             . FIELD_TYPE_BUG_ID);
+
     # Remember, this is not the function for adding general table changes.
     # That is below. Add new changes to the fielddefs table above this
     # comment.
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index d81e26c16..7dc0f2c00 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -129,6 +129,26 @@ use Storable qw(dclone);
 # Constants #
 #############
 
+# This is the regex for real numbers from Regexp::Common, modified to be
+# more readable.
+use constant NUMBER_REGEX => qr/
+    ^[+-]?      # A sign, optionally.
+
+    (?=\d|\.)   # Then either a digit or "."
+    \d*         # Followed by many other digits
+    (?:
+        \.      # Followed possibly by some decimal places
+        (?:\d*)
+    )?
+ 
+    (?:         # Followed possibly by an exponent.
+        [Ee]
+        [+-]?
+        \d+
+    )?
+    $
+/x;
+
 # If you specify a search type in the boolean charts, this describes
 # which operator maps to which internal function here.
 use constant OPERATORS => {
@@ -188,6 +208,17 @@ use constant OPERATOR_REVERSE => {
     # casesubstring, anyexact, allwords, allwordssubstr
 };
 
+# For these operators, even if a field is numeric (is_numeric returns true),
+# we won't treat the input like a number.
+use constant NON_NUMERIC_OPERATORS => qw(
+    changedafter
+    changedbefore
+    changedfrom
+    changedto
+    regexp
+    notregexp
+);
+
 use constant OPERATOR_FIELD_OVERRIDE => {
     # User fields
     'attachments.submitter' => {
@@ -1568,9 +1599,9 @@ sub _handle_chart {
     # parameters. If the user does pass multiple parameters, they will
     # become a space-separated string for those search functions.
     #
-    # all_values and all_quoted are for search functions that do operate
+    # all_values is for search functions that do operate
     # on multiple values, like anyexact.
-
+    
     my %search_args = (
         chart_id   => $sql_chart_id,
         sequence   => $or_id,
@@ -1578,11 +1609,11 @@ sub _handle_chart {
         full_field => $full_field,
         operator   => $operator,
         value      => $string_value,
-        quoted     => $dbh->quote($string_value),
         all_values => $value,
         joins      => [],
         having     => [],
     );
+    $search_args{quoted} = $self->_quote_unless_numeric(\%search_args);
     # This should add a "term" selement to %search_args.
     $self->do_search_function(\%search_args);
     
@@ -1596,7 +1627,7 @@ sub _handle_chart {
     
     return \%search_args;
 }
-   
+
 ##################################
 # do_search_function And Helpers #
 ##################################
@@ -1713,6 +1744,29 @@ sub _get_operator_field_override {
 # Search Function Helpers #
 ###########################
 
+# When we're doing a numeric search against a numeric column, we want to
+# just put a number into the SQL instead of a string. On most DBs, this
+# is just a performance optimization, but on SQLite it actually changes
+# the behavior of some searches.
+sub _quote_unless_numeric {
+    my ($self, $args, $value) = @_;
+    if (!defined $value) {
+        $value = $args->{value};
+    }
+    my ($field, $operator) = @$args{qw(field operator)};
+    
+    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;
+    if ($is_numeric) {
+        my $quoted = $value;
+        trick_taint($quoted);
+        return $quoted;
+    }
+    return Bugzilla->dbh->quote($value);
+}
+
 sub build_subselect {
     my ($outer, $inner, $table, $cond) = @_;
     return "$outer IN (SELECT $inner FROM $table WHERE $cond)";
@@ -2726,7 +2780,7 @@ sub _anyexact {
     my $dbh = Bugzilla->dbh;
     
     my @list = $self->_all_values($args, ',');
-    @list = map { $dbh->quote($_) } @list;
+    @list = map { $self->_quote_unless_numeric($args, $_) } @list;
     
     if (@list) {
         $args->{term} = $dbh->sql_in($full_field, \@list);