Bug 1000913 - Backport upstream bug 540818 to bmo/4.2 to improve include_fields and exclude_fields to accept _default, _all and _custom keywords

r=glob

7 files changed, 151 insertions, 88 deletions

diff --git a/Bugzilla/WebService.pm b/Bugzilla/WebService.pm
index 60642c5e8..86fbe1fe3 100644
--- a/Bugzilla/WebService.pm
+++ b/Bugzilla/WebService.pm
@@ -334,6 +334,34 @@ for GET type requests.
 
 =back
 
+There are several shortcut identifiers to ask for only certain groups of
+fields to be returned or excluded.
+
+=over
+
+=item C<_all>
+
+All possible fields are returned if C<_all> is specified in C<include_fields>.
+
+=item C<_default>
+
+These fields are returned if C<include_fields> is empty or C<_default>
+is specified. All fields described in the documentation are returned
+by default unless specified otherwise.
+
+=item C<_extra>
+
+These fields are not returned by default and need to be manually specified
+in C<include_fields> either by field name, or using C<_extra>.
+
+=item C<_custom>
+
+Only custom fields are returned if C<_custom> is specified in C<include_fields>.
+This is normally specific to bug objects and not relevant for other returned
+objects.
+
+=back
+
 =head1 SEE ALSO
 
 =head2 Server Types
diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index 0ca4e8eac..710bff112 100644
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -351,7 +351,7 @@ sub render_comment {
 
 # Helper for Bug.comments
 sub _translate_comment {
-    my ($self, $comment, $filters) = @_;
+    my ($self, $comment, $filters, $types, $prefix) = @_;
     my $attach_id = $comment->is_about_attachment ? $comment->extra_data
                                                   : undef;
 
@@ -375,7 +375,7 @@ sub _translate_comment {
         ];
     }
 
-    return filter $filters, $comment_hash;
+    return filter($filters, $comment_hash, $types, $prefix);
 }
 
 sub get {
@@ -1231,7 +1231,7 @@ sub _bug_to_hash {
     # All the basic bug attributes are here, in alphabetical order.
     # A bug attribute is "basic" if it doesn't require an additional
     # database call to get the info.
-    my %item = (
+    my %item = %{ filter $params, {
         alias            => $self->type('string', $bug->alias),
         classification   => $self->type('string', $bug->classification),
         component        => $self->type('string', $bug->component),
@@ -1251,15 +1251,14 @@ sub _bug_to_hash {
         url              => $self->type('string', $bug->bug_file_loc),
         version          => $self->type('string', $bug->version),
         whiteboard       => $self->type('string', $bug->status_whiteboard),
-    );
-
+    } };
 
     # First we handle any fields that require extra SQL calls.
     # We don't do the SQL calls at all if the filter would just
     # eliminate them anyway.
     if (filter_wants $params, 'assigned_to') {
         $item{'assigned_to'} = $self->type('email', $bug->assigned_to->login);
-        $item{'assigned_to_detail'} = $self->_user_to_hash($bug->assigned_to, $params, 'assigned_to');
+        $item{'assigned_to_detail'} = $self->_user_to_hash($bug->assigned_to, $params, undef, 'assigned_to');
     }
     if (filter_wants $params, 'blocks') {
         my @blocks = map { $self->type('int', $_) } @{ $bug->blocked };
@@ -1268,11 +1267,11 @@ sub _bug_to_hash {
     if (filter_wants $params, 'cc') {
         my @cc = map { $self->type('email', $_) } @{ $bug->cc || [] };
         $item{'cc'} = \@cc;
-        $item{'cc_detail'} = [ map { $self->_user_to_hash($_, $params, 'cc') } @{ $bug->cc_users } ];
+        $item{'cc_detail'} = [ map { $self->_user_to_hash($_, $params, undef, 'cc') } @{ $bug->cc_users } ];
     }
     if (filter_wants $params, 'creator') {
         $item{'creator'} = $self->type('email', $bug->reporter->login);
-        $item{'creator_detail'} = $self->_user_to_hash($bug->reporter, $params, 'creator');
+        $item{'creator_detail'} = $self->_user_to_hash($bug->reporter, $params, undef, 'creator');
     }
     if (filter_wants $params, 'depends_on') {
         my @depends_on = map { $self->type('int', $_) } @{ $bug->dependson };
@@ -1298,7 +1297,7 @@ sub _bug_to_hash {
         my $qa_login = $bug->qa_contact ? $bug->qa_contact->login : '';
         $item{'qa_contact'} = $self->type('email', $qa_login);
         if ($bug->qa_contact) {
-            $item{'qa_contact_detail'} = $self->_user_to_hash($bug->qa_contact, $params, 'qa_contact');
+            $item{'qa_contact_detail'} = $self->_user_to_hash($bug->qa_contact, $params, undef, 'qa_contact');
         }
     }
     if (filter_wants $params, 'see_also') {
@@ -1315,7 +1314,7 @@ sub _bug_to_hash {
         product => $bug->product_obj, component => $bug->component_obj, bug_id => $bug->id });
     foreach my $field (@custom_fields) {
         my $name = $field->name;
-        next if !filter_wants $params, $name;
+        next if !filter_wants($params, $name, ['default', 'custom']);
         if ($field->type == FIELD_TYPE_BUG_ID) {
             $item{$name} = $self->type('int', $bug->$name);
         }
@@ -1335,37 +1334,47 @@ sub _bug_to_hash {
 
     # Timetracking fields are only sent if the user can see them.
     if (Bugzilla->user->is_timetracker) {
-        $item{'estimated_time'} = $self->type('double', $bug->estimated_time);
-        $item{'remaining_time'} = $self->type('double', $bug->remaining_time);
-        # No need to format $bug->deadline specially, because Bugzilla::Bug
-        # already does it for us.
-        $item{'deadline'} = $self->type('string', $bug->deadline);
-        $item{'actual_time'} = $self->type('double', $bug->actual_time);
+        if (filter_wants $params, 'estimated_time') {
+            $item{'estimated_time'} = $self->type('double', $bug->estimated_time);
+        }
+        if (filter_wants $params, 'remaining_time') {
+            $item{'remaining_time'} = $self->type('double', $bug->remaining_time);
+        }
+        if (filter_wants $params, 'deadline') {
+            # No need to format $bug->deadline specially, because Bugzilla::Bug
+            # already does it for us.
+            $item{'deadline'} = $self->type('string', $bug->deadline);
+        }
+        if (filter_wants $params, 'actual_time') {
+            $item{'actual_time'} = $self->type('double', $bug->actual_time);
+        }
     }
 
     # The "accessible" bits go here because they have long names and it
     # makes the code look nicer to separate them out.
-    $item{'is_cc_accessible'} = $self->type('boolean', 
-                                            $bug->cclist_accessible);
-    $item{'is_creator_accessible'} = $self->type('boolean',
-                                                 $bug->reporter_accessible);
+    if (filter_wants $params, 'is_cc_accessible') {
+        $item{'is_cc_accessible'} = $self->type('boolean', $bug->cclist_accessible);
+    }
+    if (filter_wants $params, 'is_creator_accessible') {
+        $item{'is_creator_accessible'} = $self->type('boolean', $bug->reporter_accessible);
+    }
 
-    return filter $params, \%item;
+    return \%item;
 }
 
 sub _user_to_hash {
-    my ($self, $user, $filters, $prefix) = @_;
+    my ($self, $user, $filters, $types, $prefix) = @_;
     my $item = filter $filters, {
         id        => $self->type('int', $user->id),
         real_name => $self->type('string', $user->name),
         name      => $self->type('email', $user->login),
         email     => $self->type('email', $user->email),
-    }, $prefix;
+    }, $types, $prefix;
     return $item;
 }
 
 sub _attachment_to_hash {
-    my ($self, $attach, $filters) = @_;
+    my ($self, $attach, $filters, $types, $prefix) = @_;
 
     my $item = filter $filters, {
         creation_time    => $self->type('dateTime', $attach->attached),
@@ -1379,25 +1388,25 @@ sub _attachment_to_hash {
         is_private       => $self->type('int', $attach->isprivate),
         is_obsolete      => $self->type('int', $attach->isobsolete),
         is_patch         => $self->type('int', $attach->ispatch),
-    };
+    }, $types, $prefix;
 
     # creator/attacher require an extra lookup, so we only send them if
     # the filter wants them.
     foreach my $field (qw(creator attacher)) {
-        if (filter_wants $filters, $field) {
+        if (filter_wants $filters, $field, $types, $prefix) {
             $item->{$field} = $self->type('email', $attach->attacher->login);
         }
     }
 
-    if (filter_wants $filters, 'data') {
+    if (filter_wants $filters, 'data', $types, $prefix) {
         $item->{'data'} = $self->type('base64', $attach->data);
     }
 
-    if (filter_wants $filters, 'size') {
+    if (filter_wants $filters, 'size', $types, $prefix) {
         $item->{'size'} = $self->type('int', $attach->datasize);
     }
 
-    if (filter_wants $filters, 'flags') {
+    if (filter_wants $filters, 'flags', $types, $prefix) {
         $item->{'flags'} = [ map { $self->_flag_to_hash($_) } @{$attach->flags} ];
     }
 
@@ -2358,6 +2367,9 @@ Two items are returned:
 An array of hashes that contains information about the bugs with 
 the valid ids. Each hash contains the following items:
 
+These fields are returned by default or by specifying C<_default>
+in C<include_fields>.
+
 =over
 
 =item C<actual_time>
@@ -2607,7 +2619,11 @@ C<string> The value of the "status whiteboard" field on the bug.
 
 Every custom field in this installation will also be included in the
 return value. Most fields are returned as C<string>s. However, some
-field types have different return values:
+field types have different return values.
+
+Normally custom fields are returned by default similar to normal bug
+fields or you can specify only custom fields by using C<_custom> in
+C<include_fields>.
 
 =over
 
diff --git a/Bugzilla/WebService/Classification.pm b/Bugzilla/WebService/Classification.pm
index 22358c784..1c4226fd6 100644
--- a/Bugzilla/WebService/Classification.pm
+++ b/Bugzilla/WebService/Classification.pm
@@ -41,39 +41,37 @@ sub get {
         @classification_objs = grep { $selectable_class{$_->id} } @classification_objs;
     }
 
-    my @classifications = map { filter($params, $self->_classification_to_hash($_)) } @classification_objs;
+    my @classifications = map { $self->_classification_to_hash($_, $params) } @classification_objs;
 
     return { classifications => \@classifications };
 }
 
 sub _classification_to_hash {
-    my ($self, $classification) = @_;
+    my ($self, $classification, $params) = @_;
 
     my $user = Bugzilla->user;
     return unless (Bugzilla->params->{'useclassification'} || $user->in_group('editclassifications'));
 
     my $products = $user->in_group('editclassifications') ?
                      $classification->products : $user->get_selectable_products($classification->id);
-    my %hash = (
+
+    return filter $params, {
         id          => $self->type('int',    $classification->id),
         name        => $self->type('string', $classification->name),
         description => $self->type('string', $classification->description),
         sort_key    => $self->type('int',    $classification->sortkey),
-        products    => [ map { $self->_product_to_hash($_) } @$products ],
-    );
-
-    return \%hash;
+        products    => [ map { $self->_product_to_hash($_, $params) } @$products ],
+    };
 }
 
 sub _product_to_hash {
-    my ($self, $product) = @_;
-    my %hash = (
+    my ($self, $product, $params) = @_;
+
+    return filter $params, {
        id          => $self->type('int', $product->id),
        name        => $self->type('string', $product->name),
        description => $self->type('string', $product->description),
-   );
-
-   return \%hash;
+   }, undef, 'products';
 }
 
 1;
diff --git a/Bugzilla/WebService/Product.pm b/Bugzilla/WebService/Product.pm
index ca22e6418..3be46bd6d 100644
--- a/Bugzilla/WebService/Product.pm
+++ b/Bugzilla/WebService/Product.pm
@@ -199,7 +199,7 @@ sub _product_to_hash {
 
 sub _component_to_hash {
     my ($self, $component, $params) = @_;
-    my $field_data = {
+    my $field_data = filter $params, {
         id =>
             $self->type('int', $component->id),
         name =>
@@ -214,8 +214,9 @@ sub _component_to_hash {
             0,
         is_active =>
             $self->type('boolean', $component->is_active),
-    };
-    if (filter_wants($params, 'flag_types', 'components')) {
+    }, undef, 'components';
+
+    if (filter_wants($params, 'flag_types', undef, 'components')) {
         $field_data->{flag_types} = {
             bug =>
                 [map {
@@ -227,12 +228,13 @@ sub _component_to_hash {
                 } @{$component->flag_types->{'attachment'}}],
         };
     }
-    return filter($params, $field_data, 'components');
+
+    return $field_data;
 }
 
 sub _flag_type_to_hash {
     my ($self, $flag_type, $params) = @_;
-    return {
+    return filter $params, {
         id =>
             $self->type('int', $flag_type->id),
         name =>
@@ -255,12 +257,12 @@ sub _flag_type_to_hash {
             $self->type('int', $flag_type->grant_group_id),
         request_group =>
             $self->type('int', $flag_type->request_group_id),
-    };
+    }, undef, 'flag_types';
 }
 
 sub _version_to_hash {
     my ($self, $version, $params) = @_;
-    my $field_data = {
+    return filter $params, {
         id =>
             $self->type('int', $version->id),
         name =>
@@ -269,13 +271,12 @@ sub _version_to_hash {
             0,
         is_active =>
             $self->type('boolean', $version->is_active),
-    };
-    return filter($params, $field_data, 'versions');
+    }, undef, 'versions';
 }
 
 sub _milestone_to_hash {
     my ($self, $milestone, $params) = @_;
-    my $field_data = {
+    return filter $params, {
         id =>
             $self->type('int', $milestone->id),
         name =>
@@ -284,8 +285,7 @@ sub _milestone_to_hash {
             $self->type('int', $milestone->sortkey),
         is_active =>
             $self->type('boolean', $milestone->is_active),
-    };
-    return filter($params, $field_data, 'milestones');
+    }, undef, 'milestones';
 }
 
 1;
diff --git a/Bugzilla/WebService/Server/REST.pm b/Bugzilla/WebService/Server/REST.pm
index d38ede97b..5457b41db 100644
--- a/Bugzilla/WebService/Server/REST.pm
+++ b/Bugzilla/WebService/Server/REST.pm
@@ -312,15 +312,6 @@ sub bz_rest_options {
 sub rest_include_exclude {
     my ($params) = @_;
 
-    # _all is same as default columns
-    if ($params->{'include_fields'}
-        && ($params->{'include_fields'} eq '_all'
-            || $params->{'include_fields'} eq '_default'))
-    {
-        delete $params->{'include_fields'};
-        delete $params->{'exclude_fields'} if $params->{'exclude_fields'};
-    }
-
     if ($params->{'include_fields'} && !ref $params->{'include_fields'}) {
         $params->{'include_fields'} = [ split(/[\s+,]/, $params->{'include_fields'}) ];
     }
diff --git a/Bugzilla/WebService/User.pm b/Bugzilla/WebService/User.pm
index c84d2e07b..988ae3cd5 100644
--- a/Bugzilla/WebService/User.pm
+++ b/Bugzilla/WebService/User.pm
@@ -179,11 +179,11 @@ sub get {
         }
         my $in_group = $self->_filter_users_by_group(
             \@user_objects, $params);
-        @users = map {filter $params, {
+        @users = map { filter $params, {
                      id        => $self->type('int', $_->id),
                      real_name => $self->type('string', $_->name),
                      name      => $self->type('email', $_->login),
-                 }} @$in_group;
+                 } } @$in_group;
 
         return { users => \@users };
     }
@@ -226,7 +226,7 @@ sub get {
 
     my $in_group = $self->_filter_users_by_group(\@user_objects, $params);
     foreach my $user (@$in_group) {
-        my $user_info = {
+        my $user_info = filter $params, {
             id        => $self->type('int', $user->id),
             real_name => $self->type('string', $user->name),
             name      => $self->type('email', $user->login),
@@ -258,7 +258,7 @@ sub get {
             }
         }
 
-        push(@users, filter($params, $user_info));
+        push(@users, $user_info);
     }
 
     Bugzilla::Hook::process('webservice_user_get', 
diff --git a/Bugzilla/WebService/Util.pm b/Bugzilla/WebService/Util.pm
index f5f92e6eb..9f053095c 100644
--- a/Bugzilla/WebService/Util.pm
+++ b/Bugzilla/WebService/Util.pm
@@ -119,19 +119,19 @@ sub extract_flags {
     return (\@old_flags, \@new_flags);
 }
 
-sub filter ($$;$) {
-    my ($params, $hash, $prefix) = @_;
+sub filter($$;$$) {
+    my ($params, $hash, $types, $prefix) = @_;
     my %newhash = %$hash;
 
     foreach my $key (keys %$hash) {
-        delete $newhash{$key} if !filter_wants($params, $key, $prefix);
+        delete $newhash{$key} if !filter_wants($params, $key, $types, $prefix);
     }
 
     return \%newhash;
 }
 
-sub filter_wants ($$;$) {
-    my ($params, $field, $prefix) = @_;
+sub filter_wants($$;$$) {
+    my ($params, $field, $types, $prefix) = @_;
 
     # Since this is operation is resource intensive, we will cache the results
     # This assumes that $params->{*_fields} doesn't change between calls
@@ -142,28 +142,58 @@ sub filter_wants ($$;$) {
         return $cache->{$field};
     }
 
+    # Mimic old behavior if no types provided
+    my %field_types = map { $_ => 1 } (ref $types ? @$types : ($types || 'default'));
+
     my %include = map { $_ => 1 } @{ $params->{'include_fields'} || [] };
     my %exclude = map { $_ => 1 } @{ $params->{'exclude_fields'} || [] };
 
-    my $wants = 1;
-    if (defined $params->{exclude_fields} && $exclude{$field}) {
-        $wants = 0;
+    my %include_types;
+    my %exclude_types;
+
+    # Only return default fields if nothing is specified
+    $include_types{default} = 1 if !%include;
+
+    # Look for any field types requested
+    foreach my $key (keys %include) {
+        next if $key !~ /^_(.*)$/;
+        $include_types{$1} = 1;
+        delete $include{$key};
     }
-    elsif (defined $params->{include_fields} && !$include{$field}) {
-        if ($prefix) {
-            # Include the field if the parent is include (and this one is not excluded)
-            $wants = 0 if !$include{$prefix};
-        }
-        else {
-            # We want to include this if one of the sub keys is included
-            my $key = $field . '.';
-            my $len = length($key);
-            $wants = 0 if ! grep { substr($_, 0, $len) eq $key  } keys %include;
-        }
+    foreach my $key (keys %exclude) {
+        next if $key !~ /^_(.*)$/;
+        $exclude_types{$1} = 1;
+        delete $exclude{$key};
+    }
+
+    # If the user has asked to include all or exclude all
+    return $cache->{$field} = 0 if $exclude_types{'all'};
+    return $cache->{$field} = 1 if $include_types{'all'};
+
+    # Explicit inclusion/exclusion
+    return $cache->{$field} = 0 if $exclude{$field};
+    return $cache->{$field} = 1 if $include{$field};
+
+    # If the user has not asked for any fields specifically or if the user has asked
+    # for one or more of the field's types (and not excluded them)
+    foreach my $type (keys %field_types) {
+        return $cache->{$field} = 0 if $exclude_types{$type};
+        return $cache->{$field} = 1 if $include_types{$type};
+    }
+
+    my $wants = 0;
+    if ($prefix) {
+        # Include the field if the parent is include (and this one is not excluded)
+        $wants = 1 if $include{$prefix};
+    }
+    else {
+        # We want to include this if one of the sub keys is included
+        my $key = $field . '.';
+        my $len = length($key);
+        $wants = 1 if grep { substr($_, 0, $len) eq $key  } keys %include;
     }
 
-    $cache->{$field} = $wants;
-    return $wants;
+    return $cache->{$field} = $wants;
 }
 
 sub taint_data {