From 01a6545ec1a041e21f4a2542fc4c2c0eeeec52ba Mon Sep 17 00:00:00 2001 From: Max Kanat-Alexander Date: Thu, 28 Oct 2010 15:38:45 -0700 Subject: Bug 602456: Make Search.pm not quote numeric input for numeric fields when generating SQL. r=glob, a=mkanat --- Bugzilla/DB/Schema.pm | 2 ++ Bugzilla/Field.pm | 69 +++++++++++++++++++++++++++++++++++++++----------- Bugzilla/Install/DB.pm | 5 ++++ Bugzilla/Search.pm | 64 ++++++++++++++++++++++++++++++++++++++++++---- 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 + +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. + +=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); -- cgit v1.2.3-24-g4f1b