Bug 455632: Add Bugzilla::Field::Choice->create and have editvalues.cgi use it

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=bbaetz, a=mkanat

5 files changed, 205 insertions, 103 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 1d12ee2cf..d620f222c 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -54,7 +54,6 @@ use base qw(Bugzilla::Object Exporter);
     RemoveVotes CheckIfVotedConfirmed
     LogActivityEntry
     editable_bug_fields
-    SPECIAL_STATUS_WORKFLOW_ACTIONS
 );
 
 #####################################################################
@@ -234,13 +233,6 @@ use constant MAX_LINE_LENGTH => 254;
 # Used in _check_comment(). Gives the max length allowed for a comment.
 use constant MAX_COMMENT_LENGTH => 65535;
 
-use constant SPECIAL_STATUS_WORKFLOW_ACTIONS => qw(
-    none
-    duplicate
-    change_resolution
-    clearresolution
-);
-
 #####################################################################
 
 sub new {
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index abe1fe248..601ea52b2 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -151,6 +151,7 @@ use File::Basename;
     MAX_PRODUCT_SIZE
     MAX_MILESTONE_SIZE
     MAX_COMPONENT_SIZE
+    MAX_FIELD_VALUE_SIZE
     MAX_FREETEXT_LENGTH
 );
 
@@ -427,6 +428,9 @@ use constant MAX_MILESTONE_SIZE => 20;
 # The longest component name allowed.
 use constant MAX_COMPONENT_SIZE => 64;
 
+# The maximum length for values of <select> fields.
+use constant MAX_FIELD_VALUE_SIZE => 64;
+
 # Maximum length allowed for free text fields.
 use constant MAX_FREETEXT_LENGTH => 255;
 
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index bb516b8f4..5d8e5adc0 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -75,7 +75,6 @@ use base qw(Exporter Bugzilla::Object);
 
 use Bugzilla::Constants;
 use Bugzilla::Error;
-use Bugzilla::Field::Choice;
 use Bugzilla::Util;
 
 ###############################
@@ -376,7 +375,8 @@ sub legal_values {
     my $self = shift;
 
     if (!defined $self->{'legal_values'}) {
-        my @values = Bugzilla::Field::Choice->get_all({ field => $self });
+        require Bugzilla::Field::Choice;
+        my @values = Bugzilla::Field::Choice->type($self)->get_all();
         $self->{'legal_values'} = \@values;
     }
     return $self->{'legal_values'};
diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm
index 6ef911941..7705258eb 100644
--- a/Bugzilla/Field/Choice.pm
+++ b/Bugzilla/Field/Choice.pm
@@ -26,6 +26,8 @@ use base qw(Bugzilla::Object);
 
 use Bugzilla::Constants;
 use Bugzilla::Error;
+use Bugzilla::Field;
+use Bugzilla::Util qw(trim detaint_natural);
 
 use Scalar::Util qw(blessed);
 
@@ -42,104 +44,141 @@ use constant DB_COLUMNS => qw(
 use constant NAME_FIELD => 'value';
 use constant LIST_ORDER => 'sortkey, value';
 
-##########################################
-# Constructors and Database Manipulation #
-##########################################
+use constant REQUIRED_CREATE_FIELDS => qw(value);
 
-# When calling class methods, we aren't based on just one table,
-# so we need some slightly hacky way to do DB_TABLE. We do it by overriding
-# class methods and making them specify $_new_table. This is possible
-# because we're always called from inside Bugzilla::Field, so it always
-# has a $self to pass us which contains info about which field we're
-# attached to.
-#
-# This isn't thread safe, but Bugzilla isn't a threaded application.
-our $_new_table;
-our $_current_field;
-sub DB_TABLE {
-    my $invocant = shift;
-    if (blessed $invocant) {
-        return $invocant->field->name;
+use constant VALIDATORS => {
+    value   => \&_check_value,
+    sortkey => \&_check_sortkey,
+};
+
+use constant CLASS_MAP => {
+    bug_status => 'Bugzilla::Status',
+};
+
+#################
+# Class Factory #
+#################
+
+# Bugzilla::Field::Choice is actually an abstract base class. Every field
+# type has its own dynamically-generated class for its values. This allows
+# certain fields to have special types, like how bug_status's values
+# are Bugzilla::Status objects.
+
+sub type {
+    my ($class, $field) = @_;
+    my $field_obj = blessed $field ? $field : Bugzilla::Field->check($field);
+    my $field_name = $field_obj->name;
+
+    if ($class->CLASS_MAP->{$field_name}) {
+        return $class->CLASS_MAP->{$field_name};
     }
-    return $_new_table;
+
+    # For generic classes, we use a lowercase class name, so as
+    # not to interfere with any real subclasses we might make some day.
+    my $package = "Bugzilla::Field::Choice::$field_name";
+
+    # We check defined so that the package and the stored field are only
+    # created once globally (at least per request). We prefix it with
+    # field_ (instead of suffixing it) so that we don't somehow conflict
+    # with the names of custom fields.
+    if (!defined Bugzilla->request_cache->{"field_$package"}) {
+        eval <<EOC;
+            package $package;
+            use base qw(Bugzilla::Field::Choice);
+            use constant DB_TABLE => '$field_name';
+EOC
+        Bugzilla->request_cache->{"field_$package"} = $field_obj;
+    }
+
+    return $package;
 }
 
+################
+# Constructors #
+################
+
+# We just make new() enforce this, which should give developers 
+# the understanding that you can't use Bugzilla::Field::Choice
+# without calling type().
 sub new {
     my $class = shift;
-    my ($params) = @_;
-    _check_field_arg($params);
-    my $self = $class->SUPER::new($params);
-    _fix_return($self);
-    return $self;
+    if ($class eq 'Bugzilla::Field::Choice') {
+        ThrowCodeError('field_choice_must_use_type');
+    }
+    $class->SUPER::new(@_);
 }
 
-sub new_from_list {
-    my $class = shift;
-    my ($ids, $params) = @_;
-    _check_field_arg($params);
-    my $list = $class->SUPER::new_from_list(@_);
-    _fix_return($list);
-    return $list;
-}
+#########################
+# Database Manipulation #
+#########################
 
-sub match {
+# Our subclasses can take more arguments than we normally accept.
+# So, we override create() to remove arguments that aren't valid
+# columns. (Normally Bugzilla::Object dies if you pass arguments
+# that aren't valid columns.)
+sub create {
     my $class = shift;
     my ($params) = @_;
-    _check_field_arg($params);
-    my $results = $class->SUPER::match(@_);
-    _fix_return($results);
-    return $results;
+    foreach my $key (keys %$params) {
+        if (!grep {$_ eq $key} $class->DB_COLUMNS) {
+            delete $params->{$key};
+        }
+    }
+    return $class->SUPER::create(@_);
 }
 
-sub get_all {
-    my $class = shift;
-    _check_field_arg(@_);
-    my @list = $class->SUPER::get_all(@_);
-    _fix_return(\@list);
-    return @list;
-}
+#############
+# Accessors #
+#############
 
-sub _check_field_arg {
-    my ($params) = @_;
-    my ($class, undef, undef, $func) = caller(1);
-    if (!defined $params->{field}) {
-        ThrowCodeError('param_required',
-                       { function => "${class}::$func",
-                         param    => 'field' });
-    }
-    elsif (!blessed $params->{field}) {
-        ThrowCodeError('bad_arg', { function => "${class}::$func",
-                                    argument => 'field' });
-    }
-    $_new_table = $params->{field}->name;
-    $_current_field = $params->{field};
-    delete $params->{field};
+sub sortkey { return $_[0]->{'sortkey'}; }
+sub field {
+    my $invocant = shift;
+    my $class = ref $invocant || $invocant;
+    my $cache = Bugzilla->request_cache;
+    # This is just to make life easier for subclasses. Our auto-generated
+    # subclasses from type() already have this set.
+    $cache->{"field_$class"} ||=  
+        new Bugzilla::Field({ name => $class->DB_TABLE });
+    return $cache->{"field_$class"};
 }
 
-sub _fix_return {
-    my $retval = shift;
-    if (ref $retval eq 'ARRAY') {
-        foreach my $obj (@$retval) {
-            $obj->{field} = $_current_field;
-        }
-    }
-    elsif (defined $retval) {
-        $retval->{field} = $_current_field;
+##############
+# Validators #
+##############
+
+sub _check_value {
+    my ($invocant, $value) = @_;
+
+    my $field = $invocant->field;
+
+    $value = trim($value);
+    ThrowUserError('fieldvalue_undefined') if !defined $value || $value eq "";
+    ThrowUserError('fieldvalue_name_too_long', { value => $value })
+        if length($value) > MAX_FIELD_VALUE_SIZE;
+
+    my $exists = $invocant->type($field)->new({ name => $value });
+    if ($exists) {
+        ThrowUserError('fieldvalue_already_exists', 
+                       { field => $field, value => $value });
     }
 
-    # We do this just to avoid any possible bugs where $_new_table or
-    # $_current_field are set from a previous call. It also might save
-    # a little memory under mod_perl by releasing $_current_field explicitly.
-    undef $_new_table;
-    undef $_current_field;
+    return $value;
 }
 
-#############
-# Accessors #
-#############
+sub _check_sortkey {
+    my ($invocant, $value) = @_;
+    $value = trim($value);
+    return 0 if !$value;
+    # Store for the error message in case detaint_natural clears it.
+    my $orig_value = $value;
+    detaint_natural($value)
+        || ThrowUserError('fieldvalue_sortkey_invalid',
+                          { sortkey => $orig_value,
+                            field   => $invocant->field });
+    return $value;
+}
 
-sub sortkey { return $_[0]->{'sortkey'}; }
-sub field   { return $_[0]->{'field'};   }
 
 1;
 
@@ -153,27 +192,47 @@ Bugzilla::Field::Choice - A legal value for a <select>-type field.
 
  my $field = new Bugzilla::Field({name => 'bug_status'});
 
- my $choice = new Bugzilla::Field::Choice({ field => $field, id => 1 });
- my $choice = new Bugzilla::Field::Choice({ field => $field, name => 'NEW' });
+ my $choice = new Bugzilla::Field::Choice->type($field)->new(1);
 
- my $choices = Bugzilla::Field::Choice->new_from_list([1,2,3], 
-                                                      { field => $field});
- my $choices = Bugzilla::Field::Choice->get_all({ field => $field });
- my $choices = Bugzilla::Field::Choice->match({ sortkey => 10, 
-                                                field => $field });
+ my $choices = Bugzilla::Field::Choice->type($field)->new_from_list([1,2,3]);
+ my $choices = Bugzilla::Field::Choice->type($field)->get_all();
+ my $choices = Bugzilla::Field::Choice->type($field->match({ sortkey => 10 }); 
 
 =head1 DESCRIPTION
 
 This is an implementation of L<Bugzilla::Object>, but with a twist.
-All the class methods require that you specify an additional C<field>
-argument, which is a L<Bugzilla::Field> object that represents the
-field whose value this is.
+You can't call any class methods (such as C<new>, C<create>, etc.) 
+directly on C<Bugzilla::Field::Choice> itself. Instead, you have to
+call C<Bugzilla::Field::Choice-E<gt>type($field)> to get the class
+you're going to instantiate, and then you call the methods on that.
+
+We do that because each field has its own database table for its values, so
+each value type needs its own class.
 
-You can look at the L</SYNOPSIS> to see where this extra C<field>
-argument goes in each function.
+See the L</SYNOPSIS> for examples of how this works.
 
 =head1 METHODS
 
+=head2 Class Factory
+
+In object-oriented design, a "class factory" is a method that picks
+and returns the right class for you, based on an argument that you pass.
+
+=over
+
+=item C<type>
+
+Takes a single argument, which is either the name of a field from the
+C<fielddefs> table, or a L<Bugzilla::Field> object representing a field.
+
+Returns an appropriate subclass of C<Bugzilla::Field::Choice> that you
+can now call class methods on (like C<new>, C<create>, C<match>, etc.)
+
+B<NOTE>: YOU CANNOT CALL CLASS METHODS ON C<Bugzilla::Field::Choice>. You
+must call C<type> to get a class you can call methods on.
+
+=back
+
 =head2 Accessors
 
 These are in addition to the standard L<Bugzilla::Object> accessors.
diff --git a/Bugzilla/Status.pm b/Bugzilla/Status.pm
index f8b77331c..5f37974e7 100644
--- a/Bugzilla/Status.pm
+++ b/Bugzilla/Status.pm
@@ -22,13 +22,28 @@ use strict;
 
 package Bugzilla::Status;
 
-use base qw(Bugzilla::Object Exporter);
-@Bugzilla::Status::EXPORT = qw(BUG_STATE_OPEN is_open_state closed_bug_statuses);
+use Bugzilla::Error;
+
+use base qw(Bugzilla::Field::Choice Exporter);
+@Bugzilla::Status::EXPORT = qw(
+    BUG_STATE_OPEN
+    SPECIAL_STATUS_WORKFLOW_ACTIONS
+
+    is_open_state 
+    closed_bug_statuses
+);
 
 ################################
 #####   Initialization     #####
 ################################
 
+use constant SPECIAL_STATUS_WORKFLOW_ACTIONS => qw(
+    none
+    duplicate
+    change_resolution
+    clearresolution
+);
+
 use constant DB_TABLE => 'bug_status';
 
 use constant DB_COLUMNS => qw(
@@ -39,8 +54,24 @@ use constant DB_COLUMNS => qw(
     is_open
 );
 
-use constant NAME_FIELD => 'value';
-use constant LIST_ORDER => 'sortkey, value';
+sub VALIDATORS {
+    my $invocant = shift;
+    my $validators = $invocant->SUPER::VALIDATORS;
+    $validators->{is_open} = \&Bugzilla::Object::check_boolean;
+    $validators->{value} = \&_check_value;
+    return $validators;
+}
+
+#########################
+# Database Manipulation #
+#########################
+
+sub create {
+    my $class = shift;
+    my $self = $class->SUPER::create(@_);
+    add_missing_bug_status_transitions();
+    return $self;
+}
 
 ###############################
 #####     Accessors        ####
@@ -51,6 +82,22 @@ sub sortkey   { return $_[0]->{'sortkey'};  }
 sub is_active { return $_[0]->{'isactive'}; }
 sub is_open   { return $_[0]->{'is_open'};  }
 
+##############
+# Validators #
+##############
+
+sub _check_value {
+    my $invocant = shift;
+    my $value = $invocant->SUPER::_check_value(@_);
+
+    if (grep { lc($value) eq lc($_) } SPECIAL_STATUS_WORKFLOW_ACTIONS) {
+        ThrowUserError('fieldvalue_reserved_word',
+                       { field => $invocant->field, value => $value });
+    }
+    return $value;
+}
+
+
 ###############################
 #####       Methods        ####
 ###############################