Closed Bug 412074 Opened 16 years ago Closed 14 years ago

Ability to add attachments to a bug via the WebService (Bug.add_attachment)

Categories

(Bugzilla :: WebService, enhancement, P1)

3.1.2
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

(Whiteboard: [roadmap: 3.8])

Attachments

(1 file, 2 obsolete files)

The WebService should have the ability to add an attachment to a bug.

Probably this depends on Bugzilla::Attachment using Bugzilla::Object->create().
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.
(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.
Selenium automatically uploads many files during QA tests. ;)
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.
(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!
Priority: -- → P1
Summary: Ability to add attachments to a bug → Ability to add attachments to a bug via the WebService
Target Milestone: --- → Bugzilla 4.0
This is just what I'm looking for. Good luck Alexej! My team will be rooting for you.
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.
Whiteboard: [roadmap: 4.0]
Depends on: 454251
Currently working on this bug.
Status: NEW → ASSIGNED
(In reply to comment #8)
> Currently working on this bug.

timello, are you still working on it?
(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.
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Whiteboard: [roadmap: 4.0] → [roadmap: 3.8]
Target Milestone: Bugzilla 4.0 → Bugzilla 3.8
Assignee: webservice → timello
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.)
Assignee: timello → mkanat
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.)
Attached patch v1 (obsolete) — Splinter Review
Okay, this allows both XML-RPC and JSON-RPC to add attachments to Bugzilla.
Attachment #453585 - Flags: review?(timello)
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 on attachment 453585 [details] [diff] [review]
v1

r- due Dave comments.
Attachment #453585 - Flags: review?(timello) → review-
(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.
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.
Attached patch v2 (obsolete) — Splinter Review
Okay, this fixes all of Dave's comments and various consistency issues.
Attachment #453585 - Attachment is obsolete: true
Attachment #456133 - Flags: review?(timello)
Flags: blocking4.0+
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.
Attachment #456133 - Flags: review?(timello) → review-
Ok. I was not using SOAP::Data->type(base64 => $data) when passing the data. It doesn't need the decode in the XMLRPC method.
Attached patch v3 (4.0)Splinter Review
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.
Attachment #456133 - Attachment is obsolete: true
Attachment #456377 - Flags: review?(timello)
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.
  That will only happen on trunk, though. That's due to a patch that was checked in after I wrote this patch.
Attachment #456377 - Attachment description: v3 → v3 (4.0)
Attachment #456377 - Flags: review?(timello) → review?(dkl)
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.
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?
  timello: See the docs about line breaks in base64, in the patch.
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.
Attachment #456377 - Flags: review+
Flags: approval4.0+
Flags: approval+
Attachment #456377 - Flags: review?(dkl)
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Summary: Ability to add attachments to a bug via the WebService → Ability to add attachments to a bug via the WebService (Bug.add_attachment)
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.
(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.
(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.
Keywords: relnote
Added to the release notes in bug 604256.
Keywords: relnote
Blocks: 657290
Blocks: 813191
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 ?
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
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: