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

19 files changed, 87 insertions, 96 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 33cb12bb6..930495d42 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 614d83a64..58901c57e 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 322b5cf99..c7cda11be 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 a1f5144e5..e5eb78a2d 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 3f3e3d145..9ab5c49b9 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 e4cb4406a..0c44134ef 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 3dccd7a02..308eb64d1 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 f24eef735..f047ef365 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 882adaf02..e2ecc29e5 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 fafd14ad0..cb7d53da3 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 67517c405..29effd7de 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 4426dda38..514649763 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 1498af034..79257a851 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 cf043beb1..cb6371469 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 6c7be2241..cb3f75fa8 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 bfe630a8f..f5ab51d2b 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 e40a022c4..4270b1e5f 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 be0f2fae8..63148856c 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 52ad27642..6033c31c4 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' },