summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMax Kanat-Alexander <mkanat@bugzilla.org>2010-10-29 00:38:45 +0200
committerMax Kanat-Alexander <mkanat@bugzilla.org>2010-10-29 00:38:45 +0200
commit01a6545ec1a041e21f4a2542fc4c2c0eeeec52ba (patch)
tree4e1ca26b6b32c8f1795a3bf36fbcf78db24f1c83
parent5ba4b1aff6564126b3d0b1e7741b992d178d8f66 (diff)
downloadbugzilla-01a6545ec1a041e21f4a2542fc4c2c0eeeec52ba.tar.gz
bugzilla-01a6545ec1a041e21f4a2542fc4c2c0eeeec52ba.tar.xz
Bug 602456: Make Search.pm not quote numeric input for numeric fields
when generating SQL. r=glob, a=mkanat
-rw-r--r--Bugzilla/DB/Schema.pm2
-rw-r--r--Bugzilla/Field.pm69
-rw-r--r--Bugzilla/Install/DB.pm5
-rw-r--r--Bugzilla/Search.pm64
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);