Bug 579514: Make Bug.attachments also return attachment data

r=dkl, a=mkanat

4 files changed, 64 insertions, 25 deletions

diff --git a/Bugzilla/WebService.pm b/Bugzilla/WebService.pm
index 9e83a5a64..0ca5da267 100644
--- a/Bugzilla/WebService.pm
+++ b/Bugzilla/WebService.pm
@@ -269,11 +269,11 @@ the structs, to possibly improve performance or save some bandwidth.
 
 =over
 
-=item C<include_fields> (array)
+=item C<include_fields> 
 
-An array of strings, representing the (case-sensitive) names of fields.
-Only the fields specified in this hash will be returned, the rest will
-not be included.
+C<array> An array of strings, representing the (case-sensitive) names of
+fields in the return value. Only the fields specified in this hash will
+be returned, the rest will not be included.
 
 If you specify an empty array, then this function will return empty
 hashes.
@@ -288,10 +288,11 @@ would return something like:
 
   { users => [{ id => 1, name => 'user@domain.com' }] }
 
-=item C<exclude_fields> (array)
+=item C<exclude_fields>
 
-An array of strings, representing the (case-sensitive) names of fields.
-The fields specified will not be included in the returned hashes.
+C<array> An array of strings, representing the (case-sensitive) names of
+fields in the return value. The fields specified will not be included in
+the returned hashes.
 
 If you specify all the fields, then this function will return empty
 hashes.
diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index 6065ee493..625ca2541 100644
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -29,7 +29,7 @@ use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Field;
 use Bugzilla::WebService::Constants;
-use Bugzilla::WebService::Util qw(filter validate);
+use Bugzilla::WebService::Util qw(filter filter_wants validate);
 use Bugzilla::Bug;
 use Bugzilla::BugMail;
 use Bugzilla::Util qw(trick_taint trim);
@@ -834,11 +834,11 @@ sub _attachment_to_hash {
     # Skipping attachment flags for now.
     delete $attach->{flags};
 
-    return filter $filters, {
+    my $item = filter $filters, {
         creation_time    => $self->type('dateTime', $attach->attached),
         last_change_time => $self->type('dateTime', $attach->modification_time),
         id               => $self->type('int', $attach->id),
-        bug_id           => $self->type('int', $attach->bug->id),
+        bug_id           => $self->type('int', $attach->bug_id),
         file_name        => $self->type('string', $attach->filename),
         summary          => $self->type('string', $attach->description),
         description      => $self->type('string', $attach->description),
@@ -846,9 +846,21 @@ 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),
-        creator          => $self->type('string', $attach->attacher->login),
-        attacher         => $self->type('string', $attach->attacher->login),
     };
+
+    # 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) {
+            $item->{$field} = $self->type('string', $attach->attacher->login);
+        }
+    }
+
+    if (filter_wants $filters, 'data') {
+        $item->{'data'} = $self->type('base64', $attach->data);
+    }
+
+    return $item;
 }
 
 1;
@@ -1145,6 +1157,9 @@ C<array> An array of integer attachment ids.
 
 =back
 
+Also accepts the L<include_fields|Bugzilla::WebService/include_fields>,
+and L<exclude_fields|Bugzilla::WebService/exclude_fields> arguments.
+
 =item B<Returns>
 
 A hash containing two elements: C<bugs> and C<attachments>. The return
@@ -1188,6 +1203,10 @@ diagram above) are:
 
 =over
 
+=item C<data>
+
+C<base64> The raw data of the attachment, encoded as Base64.
+
 =item C<creation_time>
 
 C<dateTime> The time the attachment was created.
@@ -1270,6 +1289,8 @@ C<creator>.
 =item In Bugzilla B<4.0>, the C<description> return value was renamed to
 C<summary>.
 
+=item The C<data> return value was added in Bugzilla B<4.0>.
+
 =item In Bugzilla B<4.2>, the C<is_url> return value was removed
 (this attribute no longer exists for attachments).
 
diff --git a/Bugzilla/WebService/Server/JSONRPC.pm b/Bugzilla/WebService/Server/JSONRPC.pm
index 3ff875361..b55194fda 100644
--- a/Bugzilla/WebService/Server/JSONRPC.pm
+++ b/Bugzilla/WebService/Server/JSONRPC.pm
@@ -29,7 +29,7 @@ use Bugzilla::WebService::Constants;
 use Bugzilla::WebService::Util qw(taint_data);
 use Bugzilla::Util qw(correct_urlbase trim);
 
-use MIME::Base64 qw(decode_base64);
+use MIME::Base64 qw(decode_base64 encode_base64);
 
 #####################################
 # Public JSON::RPC Method Overrides #
@@ -191,7 +191,10 @@ sub type {
         # ISO-8601 "YYYYMMDDTHH:MM:SS" with a literal T
         $retval = $self->datetime_format_outbound($value);
     }
-    # XXX Will have to implement base64 if Bugzilla starts using it.
+    elsif ($type eq 'base64') {
+        utf8::encode($value) if utf8::is_utf8($value);
+        $retval = encode_base64($value, '');
+    }
 
     return $retval;
 }
diff --git a/Bugzilla/WebService/Util.pm b/Bugzilla/WebService/Util.pm
index dbf5ec593..e94feb490 100644
--- a/Bugzilla/WebService/Util.pm
+++ b/Bugzilla/WebService/Util.pm
@@ -28,7 +28,8 @@ use base qw(Exporter);
 require Test::Taint;
 
 our @EXPORT_OK = qw(
-    filter 
+    filter
+    filter_wants
     taint_data
     validate
 );
@@ -36,21 +37,29 @@ our @EXPORT_OK = qw(
 sub filter ($$) {
     my ($params, $hash) = @_;
     my %newhash = %$hash;
-    my %include = map { $_ => 1 } @{ $params->{'include_fields'} || [] };
-    my %exclude = map { $_ => 1 } @{ $params->{'exclude_fields'} || [] };
 
     foreach my $key (keys %$hash) {
-        if (defined $params->{include_fields}) {
-            delete $newhash{$key} if !$include{$key};
-        }
-        if (defined $params->{exclude_fields}) {
-            delete $newhash{$key} if $exclude{$key};
-        }
+        delete $newhash{$key} if !filter_wants($params, $key);
     }
 
     return \%newhash;
 }
 
+sub filter_wants ($$) {
+    my ($params, $field) = @_;
+    my %include = map { $_ => 1 } @{ $params->{'include_fields'} || [] };
+    my %exclude = map { $_ => 1 } @{ $params->{'exclude_fields'} || [] };
+
+    if (defined $params->{include_fields}) {
+        return 0 if !$include{$field};
+    }
+    if (defined $params->{exclude_fields}) {
+        return 0 if $exclude{$field};
+    }
+
+    return 1;
+}
+
 sub taint_data {
     my @params = @_;
     return if !@params;
@@ -115,20 +124,25 @@ internally in the WebService code.
 
  filter({ include_fields => ['id', 'name'], 
           exclude_fields => ['name'] }, $hash);
-
+ my $wants = filter_wants $params, 'field_name';
  validate(@_, 'ids');
 
 =head1 METHODS
 
 =over
 
-=item C<filter_fields>
+=item C<filter>
 
 This helps implement the C<include_fields> and C<exclude_fields> arguments
 of WebService methods. Given a hash (the second argument to this subroutine),
 this will remove any keys that are I<not> in C<include_fields> and then remove
 any keys that I<are> in C<exclude_fields>.
 
+=item C<filter_wants>
+
+Returns C<1> if a filter would preserve the specified field when passing
+a hash to L</filter>, C<0> otherwise.
+
 =item C<validate>
 
 This helps in the validation of parameters passed into the WebSerice