Last Comment Bug 412074 - Ability to add attachments to a bug via the WebService (Bug.add_attachment)
: Ability to add attachments to a bug via the WebService (Bug.add_attachment)
Status: RESOLVED FIXED
[roadmap: 3.8]
:
Product: Bugzilla
Classification: Server Software
Component: WebService (show other bugs)
: 3.1.2
: All All
: P1 enhancement with 7 votes (vote)
: Bugzilla 4.0
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
Depends on: 388251 454251
Blocks: 411996 657290 813191
  Show dependency treegraph
 
Reported: 2008-01-12 08:10 PST by Max Kanat-Alexander
Modified: 2014-01-10 08:45 PST (History)
12 users (show)
mkanat: approval+
mkanat: approval4.0+
mkanat: blocking4.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (7.02 KB, patch)
2010-06-23 17:18 PDT, Max Kanat-Alexander
timello: review-
Details | Diff | Review
v2 (8.18 KB, patch)
2010-07-05 19:10 PDT, Max Kanat-Alexander
timello: review-
Details | Diff | Review
v3 (4.0) (8.95 KB, patch)
2010-07-07 19:40 PDT, Max Kanat-Alexander
timello: review+
Details | Diff | Review

Description Max Kanat-Alexander 2008-01-12 08:10:06 PST
The WebService should have the ability to add an attachment to a bug.

Probably this depends on Bugzilla::Attachment using Bugzilla::Object->create().
Comment 1 Greg Hendricks 2008-02-26 13:17:49 PST
We need to have a way to either throttle the data or limit the number of attachments that can be made using this method in a given time period or from a particular IP. I see the potential for abuse of this method if someone wishes to do harm to your bugzilla. It would take very little time if your attachment size limit is even as small as 1MB to fill your disk if someone wanted to launch a malicious upload script.
Comment 2 Max Kanat-Alexander 2008-02-26 13:53:52 PST
(In reply to comment #1)
> It would take very little time if your attachment
> size limit is even as small as 1MB to fill your disk if someone wanted to
> launch a malicious upload script.

  That's no different from how things work now. You could do the same with WWW::Mechanize without the XML-RPC interface.
Comment 3 Frédéric Buclin 2008-02-26 13:57:20 PST
Selenium automatically uploads many files during QA tests. ;)
Comment 4 Alexej Hirsch 2008-10-09 14:11:37 PDT
Hello, i am developer of a screenshot tool named „Bug Shooting“ (www.bugshooting.com). This tool can capture screen, edit screenshot and send it to a Bug Tracker system. At this time the application can send screenshots to Bug Tracker like FogBugz, Gemini, BugTracker.NET or OnTime. 

My dream ist o support BugZilla. So i need the feature to add an attachment to a bug too.
Comment 5 Max Kanat-Alexander 2008-10-09 17:20:57 PDT
(In reply to comment #4)
> Hello, i am developer of a screenshot tool named „Bug Shooting“
> (www.bugshooting.com). This tool can capture screen, edit screenshot and send
> it to a Bug Tracker system. At this time the application can send screenshots
> to Bug Tracker like FogBugz, Gemini, BugTracker.NET or OnTime.

  Great! We'd love to have that available for our users!
Comment 6 Estevan 2008-11-24 14:25:38 PST
This is just what I'm looking for. Good luck Alexej! My team will be rooting for you.
Comment 7 Alexej Hirsch 2008-11-24 23:46:14 PST
Hello guys,
the problem is i don't know how implement this feature in bugzilla. I need help from you all. So if someone is able to write the bugzilla code and want to see bugzilla support in Bug Shooting please contact me.
Comment 8 Tiago Mello [:timello] 2009-05-05 06:35:25 PDT
Currently working on this bug.
Comment 9 Frédéric Buclin 2009-08-14 17:31:13 PDT
(In reply to comment #8)
> Currently working on this bug.

timello, are you still working on it?
Comment 10 Tiago Mello [:timello] 2009-08-16 19:32:41 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Currently working on this bug.
> 
> timello, are you still working on it?

Not really, but I'm interested in. Just waiting for some time to back working on this bug.
Comment 11 Max Kanat-Alexander 2010-06-23 14:59:16 PDT
I consider this to be a part of fully implementing Bug.update, so I'm going to work on it for Bugzilla 3.8.

(LpSolit and I decided about a week ago that Bug.update stuff can continue to be implemented and reviewed through the freezes.)
Comment 12 Max Kanat-Alexander 2010-06-23 15:55:52 PDT
I consider this to be a part of fully implementing Bug.update, so I'm going to work on it for Bugzilla 3.8.

(LpSolit and I decided about a week ago that Bug.update stuff can continue to be implemented and reviewed through the freezes.)
Comment 13 Max Kanat-Alexander 2010-06-23 17:18:43 PDT
Created attachment 453585 [details] [diff] [review]
v1

Okay, this allows both XML-RPC and JSON-RPC to add attachments to Bugzilla.
Comment 14 David Lawrence [:dkl] 2010-06-25 12:11:15 PDT
Comment on attachment 453585 [details] [diff] [review]
v1

>=== modified file 'Bugzilla/WebService.pm'
>+# Used by the JSON-RPC server to convert incoming bsae64 fields appropriately.
>+use constant BASE64_FIELDS => {};

base64 instead of bsae64.

>+        my $attachment = Bugzilla::Attachment->create({
>+            bug => $bug,
>+            data        => $params->{data},
>+            description => $params->{summary},
>+            filename    => $params->{file_name},

Why not $params->{filename} instead of $params->{file_name} to be consistent?

>+        push(@created, $attachment);
>+    }
>+    $_->update() foreach @bugs;
>+    $dbh->bz_commit_transaction();
>+
>+    $_->send_changes() foreach @bugs;

Why not have $_->update be $bug->update within the foreach loop? I can understand
send_changes needs to be outside of the transaction but the update?

>+=item B<Params>
>+
>+=over
>+
>+=item C<ids>
>+
>+B<Required> C<array> An array of ints or strings--the ids

Nit: s/ints/integers/. Also maybe say and/or since the list can be a mix
of integers and strings.

>+=item C<is_private>
>+
>+C<boolean> True if the attachment should be private (restricted
>+to the "insidergroup"), False if the attachment should be public.
>+
>+=item C<is_url>
>+
>+C<boolean> True if the attachment is just a URL, pointing to data elsewhere.
>+If so, the C<data> item should just contain the URL.

Should denote that they default to 'false' if omitted.

>+=item 600 (Attachment Too Large)
>+
>+You tried to attach a file that was than Bugzilla will accept.

You tried to attach a file that was *larger* than Bugzilla will accept.

>=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'
>+    my @base64_fields = @{ $pkg->BASE64_FIELDS->{$method} || [] };
>+    foreach my $field (@base64_fields) {
>+        if (defined $params->{$field}) {
>+            $params->{$field} = decode_base64($params->{$field});
>+        }
>+    }

What does this do if the 'data' argument is a URL and not base64 encoded?
Does it just leave it alone?

Dave
Comment 15 Tiago Mello [:timello] 2010-06-30 06:48:03 PDT
Comment on attachment 453585 [details] [diff] [review]
v1

r- due Dave comments.
Comment 16 Max Kanat-Alexander 2010-07-05 19:05:34 PDT
(In reply to comment #14)
> Why not have $_->update be $bug->update within the foreach loop? I can
> understand
> send_changes needs to be outside of the transaction but the update?

  update() is supposed to happen after all validation is done, to avoid rollbacks. (That is, in general, rollbacks are never supposed to happen.) I would be doing the same with Attachment->create, but we don't have an architecturally-ideal way to do that right now.

> Nit: s/ints/integers/. 

  Oh, I actually think that I'm going to keep that as "ints", since it's a technical type name for the WebService.

> What does this do if the 'data' argument is a URL and not base64 encoded?
> Does it just leave it alone?

  Under JSON-RPC, you will always have to pass the data argument as base64-encoded, since JSON-RPC doesn't have an explicit base64 type. The parameter is documented as base64, so it's not inconsistent with its documentation.
Comment 17 Max Kanat-Alexander 2010-07-05 19:06:25 PDT
Oh, and also, file_name is consistent with Bug.attachments. "summary" was not, so I changed Bug.attachments and left in "description" for backwards-compat. New patch coming.
Comment 18 Max Kanat-Alexander 2010-07-05 19:10:21 PDT
Created attachment 456133 [details] [diff] [review]
v2

Okay, this fixes all of Dave's comments and various consistency issues.
Comment 19 Tiago Mello [:timello] 2010-07-07 13:05:12 PDT
Comment on attachment 456133 [details] [diff] [review]
v2

Attachment->create() does not except base64 data type, you probably forgot to call decode_base64 in the add_attachment XMLRPC method. Also I think 'is_patch' parameter is missing.

Nit: The error is saying: "You must enter a description for the attachment." but the documentation says 'summary' instead. I know this error msg is shared between the webservices and the cgis too... just mentioning here.
Comment 20 Tiago Mello [:timello] 2010-07-07 13:13:50 PDT
Ok. I was not using SOAP::Data->type(base64 => $data) when passing the data. It doesn't need the decode in the XMLRPC method.
Comment 21 Max Kanat-Alexander 2010-07-07 19:40:32 PDT
Created attachment 456377 [details] [diff] [review]
v3 (4.0)

Okay, this adds is_patch and refactors slightly to avoid more rollbacks, and also I made sure that the comment now "happens" at the exact same time as the attachment was added.
Comment 22 David Lawrence [:dkl] 2010-07-11 19:27:40 PDT
not ok 178 - WS_ERROR_CODE has 2 error(s):
# Error tag 'local_file_too_large' is used in WS_ERROR_CODE in Bugzilla/WebService/Constants.pm but not defined in any template, and not used in any code.
# Error tag 'patch_too_large' is used in WS_ERROR_CODE in Bugzilla/WebService/Constants.pm but not defined in any template, and not used in any code.
#   Failed test 'WS_ERROR_CODE has 2 error(s):
# Error tag 'local_file_too_large' is used in WS_ERROR_CODE in Bugzilla/WebService/Constants.pm but not defined in any template, and not used in any code.
# Error tag 'patch_too_large' is used in WS_ERROR_CODE in Bugzilla/WebService/Constants.pm but not defined in any template, and not used in any code.'
#   at t/012throwables.t line 208.
Comment 23 Max Kanat-Alexander 2010-07-12 13:30:31 PDT
  That will only happen on trunk, though. That's due to a patch that was checked in after I wrote this patch.
Comment 24 Tiago Mello [:timello] 2010-07-12 13:33:56 PDT
I tested it using XML-RPC and it is working fine. I still need to check the behavior with JSON, thought, I'm ok with the patch.
Comment 25 Tiago Mello [:timello] 2010-07-13 07:49:25 PDT
I tested it using JSON-RPC. I could successful send attachment in text/plain type but it fails when sending binaries like image/jpg, application/octet-stream... I'm using encode_base64($data) for both cases... reading the file as:

open($fh, '/home/timello/file...' ) or die $!;
read( $fh, $data, -s $fh );

The same code works for XML-RPC (apart from the encode_base64 which is unnecessary...)

When it does the my $result = $rpc->call($uri, {.... , it doesn't even return any object instance ... it looks like it fails in the $rpc->call or something... maybe the string with the base64 content is too long?
Comment 26 Max Kanat-Alexander 2010-07-13 12:27:21 PDT
  timello: See the docs about line breaks in base64, in the patch.
Comment 27 Tiago Mello [:timello] 2010-07-13 13:42:42 PDT
Comment on attachment 456377 [details] [diff] [review]
v3 (4.0)

Ok. I removed all newlines and it worked. I sniffed the traffic and used the response as a positive answer for the review since it contains all expected returning data. I'm ok with the patch.
Comment 28 Max Kanat-Alexander 2010-07-13 15:45:10 PDT
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Attachment.pm
modified Bugzilla/WebService.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/Server/JSONRPC.pm
Committed revision 7350.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Attachment.pm
modified Bugzilla/WebService.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/Server/JSONRPC.pm
Committed revision 7319.
Comment 29 Kevin Soukup 2010-10-10 21:57:34 PDT
Please correct me if I'm interpreting the bug incorrectly.

The request was to add the ability to add attachments to a bug via the WebService. I was interpreting this to mean something like inline pasting of an image. 

I'm imagining I'm a software user with a problem, and I want to provide a screen capture of the problem I'm seeing. I'd like to be able to insert that image straight into the incident (copy/paste) so it's viewable as part of the bug description. This would be similar to what one can do in Jira, and would be accomplished using something like "KSnapshot" or BugShooting" (see comment #4 above).

The status of this request is "Resolved Fixed" with roadmap of 3.8?

I just logged into the 4.1 landfill, but I do not not see that that I can insert a screen capture. I do see that I can attach a supporting document (OK, but clumsy), but that's not what I thought this bug was addressing.

Am I reading this wrong then? Should the ability to do so exist in this demo veeersion of software?

Thanks much for setting me straight.
Comment 30 Frédéric Buclin 2010-10-11 07:54:20 PDT
(In reply to comment #29)
> Please correct me if I'm interpreting the bug incorrectly.

You are! :)  This bug has nothing to do with the web UI. What you are talking about is bug 602313.
Comment 31 Max Kanat-Alexander 2010-10-11 22:16:39 PDT
(In reply to comment #29)
> The request was to add the ability to add attachments to a bug via the
> WebService. I was interpreting this to mean something like inline pasting of an
> image. 

  To be clear, a "WebService" is a remote procedure call service like JSON-RPC, XML-RPC, or some form of REST API.
Comment 32 Max Kanat-Alexander 2010-10-21 19:40:01 PDT
Added to the release notes in bug 604256.
Comment 33 Khokhar 2014-01-10 05:50:46 PST
Hi,
By using add_attachment method I can upload files with different extensions (.zip, .jpg etc) , but uploaded files got corrupted. 

I've google it and found that many people have this problem with add_attachment method of Bugzilla API.

Ref: 
http://stackoverflow.com/questions/7131197/uploading-attachments-to-bugzilla-using-the-web-services-api-and-perl

http://stackoverflow.com/questions/13516982/add-an-attachment-to-bugzilla-using-xml-rpc-in-vba

My Bugzilla version is 4.2.3

I am using a python module for interacting with Bugzilla over XMLRPC
https://git.fedorahosted.org/git/python-bugzilla.git


I am just copying here the attachfile function from base.py
Its very straightforward python code. 

    def attachfile(self, idlist, attachfile, description, **kwargs):

        if isinstance(attachfile, str):
            f = open(attachfile)
        elif hasattr(attachfile, 'read'):
            f = attachfile
        else:
            raise TypeError("attachfile must be filename or file-like object")

        # Back compat
        if "contenttype" in kwargs:
            kwargs["content_type"] = kwargs.pop("contenttype")
        if "ispatch" in kwargs:
            kwargs["is_patch"] = kwargs.pop("ispatch")
        if "isprivate" in kwargs:
            kwargs["is_private"] = kwargs.pop("isprivate")
        if "filename" in kwargs:
            kwargs["file_name"] = kwargs.pop("filename")

        kwargs['summary'] = description
        if 'file_name' not in kwargs:
            kwargs['file_name'] = os.path.basename(f.name)

        if 'content_type' not in kwargs:
            kwargs['content_type'] = 'application/octet-stream'

        kwargs['data'] = self._attachment_encode(f)
        kwargs['ids'] = self._listify(idlist)

        ret = self._proxy.Bug.add_attachment(kwargs)

        if "attachments" in ret:
            # Up to BZ 4.2
            ret = [int(k) for k in ret["attachments"].keys()]
        elif "ids" in ret:
            # BZ 4.4+
            ret = ret["ids"]

        if type(ret) is list and len(ret) == 1:
            ret = ret[0]
        return ret

Is this the correct way to write this comment here on already Resolved bug or shall I report a separate bug ?
Comment 34 David Lawrence [:dkl] 2014-01-10 07:11:35 PST
Please open a new fresh bug for this issue and we can track it there. Also in the new bug please provide some sample attachments that are getting corrupted for us to reproduce.

(In reply to Khokhar from comment #33)
> 
>         kwargs['data'] = self._attachment_encode(f)

I assume this is base64 encoding the data before sending?

dkl
Comment 35 Khokhar 2014-01-10 08:45:09 PST
(In reply to David Lawrence [:dkl] from comment #34)
> Please open a new fresh bug for this issue and we can track it there. Also
> in the new bug please provide some sample attachments that are getting
> corrupted for us to reproduce.

Thanks for the quick reply

new fresh bug#958559 has been created.

I've uploaded sample attachments as well.

Note You need to log in before you can comment on or make changes to this bug.