From: Max Kanat-Alexander <mkanat@bugzilla.org>
Date: Thu, 24 Jun 2010 00:39:11 +0000 (-0700)
Subject: Bug 572602: Change the way that Bugzilla::Object determines what fields
X-Git-Tag: bugzilla-3.7.1~8
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8f33041e6542f12e6897ef6ed7a67c43a118c504;p=thirdparty%2Fbugzilla.git

Bug 572602: Change the way that Bugzilla::Object determines what fields
are required for create(). It now assumes that any column that is NOT NULL
and has not DEFAULT in the database is required. We also shift the burden
of throwing errors about empty values to the validators. This fixes the bug
that Bugzilla::Bug->create() wasn't populating default values for fields
if they weren't specified in the create() parameters.
r=timello, a=mkanat
---

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 33cb12bb6e..930495d427 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -87,13 +87,9 @@ sub DB_COLUMNS {
         $dbh->sql_date_format('attachments.creation_ts', '%Y.%m.%d %H:%i') . ' AS creation_ts';
 }
 
-use constant REQUIRED_CREATE_FIELDS => qw(
-    bug
-    data
-    description
-    filename
-    mimetype
-);
+use constant REQUIRED_FIELD_MAP => {
+    bug_id => 'bug',
+};
 
 use constant UPDATE_COLUMNS => qw(
     description
@@ -515,6 +511,10 @@ sub _check_bug {
     my $user = Bugzilla->user;
 
     $bug = ref $invocant ? $invocant->bug : $bug;
+
+    $bug || ThrowCodeError('param_required', 
+                           { function => "$invocant->create", param => 'bug' });
+
     ($user->can_see_bug($bug->id) && $user->can_edit_product($bug->product_id))
       || ThrowUserError("illegal_attachment_edit_bug", { bug_id => $bug->id });
 
@@ -526,7 +526,7 @@ sub _check_content_type {
 
     $content_type = 'text/plain' if (ref $invocant && ($invocant->isurl || $invocant->ispatch));
     my $legal_types = join('|', LEGAL_CONTENT_TYPES);
-    if ($content_type !~ /^($legal_types)\/.+$/) {
+    if (!$content_type or $content_type !~ /^($legal_types)\/.+$/) {
         ThrowUserError("invalid_content_type", { contenttype => $content_type });
     }
     trick_taint($content_type);
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 614d83a642..58901c57ec 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -115,15 +115,6 @@ sub DB_COLUMNS {
     return @columns;
 }
 
-use constant REQUIRED_CREATE_FIELDS => qw(
-    component
-    product
-    short_desc
-    version
-);
-
-# There are also other, more complex validators that are called
-# from run_create_validators.
 sub VALIDATORS {
 
     my $validators = {
@@ -290,6 +281,11 @@ use constant FIELD_MAP => {
     offset           => 'OFFSET',
 };
 
+use constant REQUIRED_FIELD_MAP => {
+    product_id   => 'product',
+    component_id => 'component',
+};
+
 #####################################################################
 
 sub new {
@@ -1696,8 +1692,8 @@ sub _check_reporter {
     else {
         # On bug creation, the reporter is the logged in user
         # (meaning that he must be logged in first!).
+        Bugzilla->login(LOGIN_REQUIRED);
         $reporter = Bugzilla->user->id;
-        $reporter || ThrowCodeError('invalid_user');
     }
     return $reporter;
 }
diff --git a/Bugzilla/Classification.pm b/Bugzilla/Classification.pm
index 322b5cf990..c7cda11be8 100644
--- a/Bugzilla/Classification.pm
+++ b/Bugzilla/Classification.pm
@@ -40,10 +40,6 @@ use constant DB_COLUMNS => qw(
     sortkey
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(
-    name
-);
-
 use constant UPDATE_COLUMNS => qw(
     name
     description
diff --git a/Bugzilla/Component.pm b/Bugzilla/Component.pm
index a1f5144e5d..e5eb78a2d9 100644
--- a/Bugzilla/Component.pm
+++ b/Bugzilla/Component.pm
@@ -47,13 +47,6 @@ use constant DB_COLUMNS => qw(
     description
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(
-    name
-    product
-    initialowner
-    description
-);
-
 use constant UPDATE_COLUMNS => qw(
     name
     initialowner
@@ -61,6 +54,10 @@ use constant UPDATE_COLUMNS => qw(
     description
 );
 
+use constant REQUIRED_FIELD_MAP => {
+    product_id => 'product',
+};
+
 use constant VALIDATORS => {
     create_series    => \&Bugzilla::Object::check_boolean,
     product          => \&_check_product,
@@ -234,6 +231,8 @@ sub _check_initialqacontact {
 
 sub _check_product {
     my ($invocant, $product) = @_;
+    $product || ThrowCodeError('param_required', 
+                    { function => "$invocant->create", param => 'product' });
     return Bugzilla->user->check_can_admin_product($product->name);
 }
 
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index 3f3e3d145d..9ab5c49b9e 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -105,8 +105,6 @@ use constant DB_COLUMNS => qw(
     is_mandatory
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(name description sortkey);
-
 use constant VALIDATORS => {
     custom       => \&_check_custom,
     description  => \&_check_description,
diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm
index e4cb4406ae..0c44134ef2 100644
--- a/Bugzilla/Field/Choice.pm
+++ b/Bugzilla/Field/Choice.pm
@@ -55,8 +55,6 @@ use constant UPDATE_COLUMNS => qw(
 use constant NAME_FIELD => 'value';
 use constant LIST_ORDER => 'sortkey, value';
 
-use constant REQUIRED_CREATE_FIELDS => qw(value);
-
 use constant VALIDATORS => {
     value   => \&_check_value,
     sortkey => \&_check_sortkey,
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 3dccd7a02e..308eb64d1b 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -87,14 +87,6 @@ use constant DB_COLUMNS => qw(
     status
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(
-    attach_id
-    bug_id
-    setter_id
-    status
-    type_id
-);
-
 use constant UPDATE_COLUMNS => qw(
     requestee_id
     setter_id
@@ -429,6 +421,7 @@ sub create {
     $params->{$_} = $flag->{$_} foreach @columns;
 
     $params->{creation_date} = $params->{modification_date} = $timestamp;
+
     $flag = $class->SUPER::create($params);
     return $flag;
 }
diff --git a/Bugzilla/Group.pm b/Bugzilla/Group.pm
index f24eef7359..f047ef365e 100644
--- a/Bugzilla/Group.pm
+++ b/Bugzilla/Group.pm
@@ -60,8 +60,6 @@ use constant VALIDATORS => {
     icon_url    => \&_check_icon_url,
 };
 
-use constant REQUIRED_CREATE_FIELDS => qw(name description isbuggroup);
-
 use constant UPDATE_COLUMNS => qw(
     name
     description
diff --git a/Bugzilla/Keyword.pm b/Bugzilla/Keyword.pm
index 882adaf02f..e2ecc29e57 100644
--- a/Bugzilla/Keyword.pm
+++ b/Bugzilla/Keyword.pm
@@ -35,8 +35,6 @@ use constant DB_COLUMNS => qw(
 
 use constant DB_TABLE => 'keyworddefs';
 
-use constant REQUIRED_CREATE_FIELDS => qw(name description);
-
 use constant VALIDATORS => {
     name        => \&_check_name,
     description => \&_check_description,
@@ -106,7 +104,9 @@ sub _check_name {
     my ($self, $name) = @_;
 
     $name = trim($name);
-    $name eq "" && ThrowUserError("keyword_blank_name");
+    if (!defined $name or $name eq "") {
+        ThrowUserError("keyword_blank_name");
+    }
     if ($name =~ /[\s,]/) {
         ThrowUserError("keyword_invalid_name");
     }
@@ -124,7 +124,9 @@ sub _check_name {
 sub _check_description {
     my ($self, $desc) = @_;
     $desc = trim($desc);
-    $desc eq '' && ThrowUserError("keyword_blank_description");
+    if (!defined $desc or $desc eq '') {
+        ThrowUserError("keyword_blank_description");
+    }
     return $desc;
 }
 
diff --git a/Bugzilla/Milestone.pm b/Bugzilla/Milestone.pm
index fafd14ad01..cb7d53da3f 100644
--- a/Bugzilla/Milestone.pm
+++ b/Bugzilla/Milestone.pm
@@ -45,10 +45,9 @@ use constant DB_COLUMNS => qw(
     sortkey
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(
-    value
-    product
-);
+use constant REQUIRED_FIELD_MAP => {
+    product_id => 'product',
+};
 
 use constant UPDATE_COLUMNS => qw(
     value
@@ -195,6 +194,8 @@ sub _check_sortkey {
 
 sub _check_product {
     my ($invocant, $product) = @_;
+    $product || ThrowCodeError('param_required',
+                    { function => "$invocant->create", param => "product" });
     return Bugzilla->user->check_can_admin_product($product->name);
 }
 
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 67517c4056..29effd7deb 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -39,6 +39,8 @@ use constant UPDATE_VALIDATORS => {};
 use constant NUMERIC_COLUMNS   => ();
 use constant DATE_COLUMNS      => ();
 use constant VALIDATOR_DEPENDENCIES => {};
+# XXX At some point, this will be joined with FIELD_MAP.
+use constant REQUIRED_FIELD_MAP  => {};
 
 # This allows the JSON-RPC interface to return Bugzilla::Object instances
 # as though they were hashes. In the future, this may be modified to return
@@ -447,10 +449,9 @@ sub check_required_create_fields {
     Bugzilla::Hook::process('object_before_create', { class => $class,
                                                       params => $params });
 
-    foreach my $field ($class->REQUIRED_CREATE_FIELDS) {
-        ThrowCodeError('param_required',
-            { function => "${class}->create", param => $field })
-            if !exists $params->{$field};
+    my @check_fields = $class->_required_create_fields();
+    foreach my $field (@check_fields) {
+        $params->{$field} = undef if !exists $params->{$field};
     }
 }
 
@@ -616,6 +617,30 @@ sub _get_validators {
     return $cache->{$cache_key};
 }
 
+# These are all the fields that need to be checked, always, when
+# calling create(), because they have no DEFAULT and they are marked
+# NOT NULL.
+sub _required_create_fields {
+    my $class = shift;
+    my $dbh = Bugzilla->dbh;
+    my $table = $class->DB_TABLE;
+
+    my @columns = $dbh->bz_table_columns($table);
+    my @required;
+    foreach my $column (@columns) {
+        my $def = $dbh->bz_column_info($table, $column);
+        if ($def->{NOTNULL} and !defined $def->{DEFAULT}
+            # SERIAL fields effectively have a DEFAULT, but they're not
+            # listed as having a DEFAULT in DB::Schema.
+            and $def->{TYPE} !~ /serial/i) 
+        {
+            my $field = $class->REQUIRED_FIELD_MAP->{$column} || $column;
+            push(@required, $field);
+        }
+    }
+    return @required;
+}
+
 1;
 
 __END__
@@ -687,11 +712,6 @@ The order that C<new_from_list> and C<get_all> should return objects
 in. This should be the name of a database column. Defaults to
 L</NAME_FIELD>.
 
-=item C<REQUIRED_CREATE_FIELDS>
-
-The list of fields that B<must> be specified when the user calls
-C<create()>. This should be an array.
-
 =item C<VALIDATORS>
 
 A hashref that points to a function that will validate each param to
@@ -742,6 +762,15 @@ A list of columns to update when L</update> is called.
 If a field can't be changed, it shouldn't be listed here. (For example,
 the L</ID_FIELD> usually can't be updated.)
 
+=item C<REQUIRED_FIELD_MAP>
+
+This is a hashref that maps database column names to L</create> argument
+names. You only need to specify values for fields where the argument passed
+to L</create> has a different name in the database than it does in the
+L</create> arguments. (For example, L<Bugzilla::Bug/create> takes a
+C<product> argument, but the column name in the C<bugs> table is
+C<product_id>.)
+
 =item C<NUMERIC_COLUMNS>
 
 When L</update> is called, it compares each column in the object to its
@@ -915,17 +944,13 @@ Description: Creates a new item in the database.
              are invalid.
 
 Params:      C<$params> - hashref - A value to put in each database
-               field for this object. Certain values must be set (the 
-               ones specified in L</REQUIRED_CREATE_FIELDS>), and
-               the function will throw a Code Error if you don't set
-               them.
+             field for this object.
 
 Returns:     The Object just created in the database.
 
 Notes:       In order for this function to work in your subclass,
              your subclass's L</ID_FIELD> must be of C<SERIAL>
-             type in the database. Your subclass also must
-             define L</REQUIRED_CREATE_FIELDS> and L</VALIDATORS>.
+             type in the database.
 
              Subclass Implementors: This function basically just
              calls L</check_required_create_fields>, then
@@ -940,8 +965,10 @@ Notes:       In order for this function to work in your subclass,
 
 =item B<Description>
 
-Part of L</create>. Throws an error if any of the L</REQUIRED_CREATE_FIELDS>
-have not been specified in C<$params>
+Part of L</create>. Modifies the incoming C<$params> argument so that
+any field that does not have a database default will be checked
+later by L</run_create_validators>, even if that field wasn't specified
+as an argument to L</create>.
 
 =item B<Params>
 
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index 4426dda38b..5146497634 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -52,12 +52,6 @@ use constant DB_COLUMNS => qw(
    allows_unconfirmed
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(
-    name
-    description
-    version
-);
-
 use constant UPDATE_COLUMNS => qw(
     name
     description
diff --git a/Bugzilla/Search/Recent.pm b/Bugzilla/Search/Recent.pm
index 1498af0340..79257a8519 100644
--- a/Bugzilla/Search/Recent.pm
+++ b/Bugzilla/Search/Recent.pm
@@ -34,8 +34,6 @@ use Bugzilla::Util;
 use constant DB_TABLE => 'profile_search';
 use constant LIST_ORDER => 'id';
 
-use constant REQUIRED_CREATE_FIELDS => ();
-
 use constant DB_COLUMNS => qw(
     id
     user_id
@@ -120,7 +118,7 @@ sub _check_user_id {
 sub _check_bug_list {
     my ($invocant, $list) = @_;
 
-    my @bug_ids = ref($list) ? @$list : split(',', $list);
+    my @bug_ids = ref($list) ? @$list : split(',', $list || '');
     detaint_natural($_) foreach @bug_ids;
     return join(',', @bug_ids);
 }
diff --git a/Bugzilla/Search/Saved.pm b/Bugzilla/Search/Saved.pm
index cf043beb1b..cb63714691 100644
--- a/Bugzilla/Search/Saved.pm
+++ b/Bugzilla/Search/Saved.pm
@@ -48,8 +48,6 @@ use constant DB_COLUMNS => qw(
     query_type
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(name query);
-
 use constant VALIDATORS => {
     name       => \&_check_name,
     query      => \&_check_query,
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 6c7be22411..cb3f75fa8f 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -102,8 +102,6 @@ use constant NAME_FIELD => 'login_name';
 use constant ID_FIELD   => 'userid';
 use constant LIST_ORDER => NAME_FIELD;
 
-use constant REQUIRED_CREATE_FIELDS => qw(login_name cryptpassword);
-
 use constant VALIDATORS => {
     cryptpassword => \&_check_password,
     disable_mail  => \&_check_disable_mail,
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index bfe630a8f6..f5ab51d2ba 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -592,8 +592,11 @@ sub is_7bit_clean {
 }
 
 sub clean_text {
-    my ($dtext) = shift;
-    $dtext =~  s/[\x00-\x1F\x7F]+/ /g;   # change control characters into a space
+    my $dtext = shift;
+    if ($dtext) {
+        # change control characters into a space
+        $dtext =~ s/[\x00-\x1F\x7F]+/ /g;
+    }
     return trim($dtext);
 }
 
diff --git a/Bugzilla/Version.pm b/Bugzilla/Version.pm
index e40a022c43..4270b1e5fd 100644
--- a/Bugzilla/Version.pm
+++ b/Bugzilla/Version.pm
@@ -46,10 +46,9 @@ use constant DB_COLUMNS => qw(
     product_id
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(
-    value
-    product
-);
+use constant REQUIRED_FIELD_MAP => {
+    product_id => 'product',
+};
 
 use constant UPDATE_COLUMNS => qw(
     value
@@ -188,6 +187,8 @@ sub _check_value {
 
 sub _check_product {
     my ($invocant, $product) = @_;
+    $product || ThrowCodeError('param_required',
+                    { function => "$invocant->create", param => 'product' });
     return Bugzilla->user->check_can_admin_product($product->name);
 }
 
diff --git a/Bugzilla/Whine/Schedule.pm b/Bugzilla/Whine/Schedule.pm
index be0f2fae8e..63148856c9 100644
--- a/Bugzilla/Whine/Schedule.pm
+++ b/Bugzilla/Whine/Schedule.pm
@@ -42,8 +42,6 @@ use constant DB_COLUMNS => qw(
     mailto_type
 );
 
-use constant REQUIRED_CREATE_FIELDS => qw(eventid mailto mailto_type);
-
 use constant UPDATE_COLUMNS => qw(
     eventid 
     run_day 
diff --git a/email_in.pl b/email_in.pl
index 52ad27642e..6033c31c42 100755
--- a/email_in.pl
+++ b/email_in.pl
@@ -152,13 +152,6 @@ sub post_bug {
 
     my $user = Bugzilla->user;
 
-    # Bugzilla::Bug->create throws a confusing CodeError if
-    # the REQUIRED_CREATE_FIELDS are missing, but much more
-    # sensible errors if the fields exist but are just undef.
-    foreach my $field (Bugzilla::Bug::REQUIRED_CREATE_FIELDS) {
-        $fields->{$field} = undef if !exists $fields->{$field};
-    }
-
     my ($retval, $non_conclusive_fields) =
       Bugzilla::User::match_field({
         'assigned_to'   => { 'type' => 'single' },