Skip Menu |
 

This queue is for tickets about the RT-Client-REST CPAN distribution.

Report information
The Basics
Id: 90112
Status: open
Priority: 0/
Queue: RT-Client-REST

People
Owner: DMITRI [...] cpan.org
Requestors: melmothx [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Attachment retrieval returns wrongly decoded files
Date: Thu, 07 Nov 2013 11:25:54 +0100
To: bug-RT-Client-REST [...] rt.cpan.org
From: Marco Pessotto <melmothx [...] gmail.com>
Download (untitled) / with headers
text/plain 2.5k
As far as I can see, when retrieving binary attachments, the response of RT is a text/plain response in any case, with a declared content type: Content-Type: text/plain; charset=utf-8 The response looks like so: RT/4.0.7 200 Ok id: 873 Subject: Creator: 12 Created: 2013-11-06 07:15:36 Transaction: 1457 Parent: 871 MessageId: Filename: prova2.png ContentType: image/png ContentEncoding: base64 Headers: Content-Type: image/png; name="prova2.png" Content-Disposition: attachment; filename="prova2.png" Content-Transfer-Encoding: base64 Content-Length: 16175 Content: <here starts the binary data> Now, the get_attachment method will call ->decoded_content on the response, and from what I can see, the binary data will be mangled, as will be considered utf8 data and decoded accordingly. From my testing, encoding it back doesn't restore the original binary data. Please consider the following patch, which adds an option to retrieve the undecoded data and keeps the existing behaviour unmodified. diff --git a/lib/RT/Client/REST.pm b/lib/RT/Client/REST.pm index 8ebef0f..ebbd669 100644 --- a/lib/RT/Client/REST.pm +++ b/lib/RT/Client/REST.pm @@ -154,9 +154,16 @@ sub get_attachment { my $parent_id = $self->_valid_numeric_object_id(delete($opts{parent_id})); my $id = $self->_valid_numeric_object_id(delete($opts{id})); - my $form = form_parse( - $self->_submit("$type/$parent_id/attachments/$id")->decoded_content - ); + my $res = $self->_submit("$type/$parent_id/attachments/$id"); + my $content; + if ($opts{undecoded}) { + $content = $res->content; + } + else { + $content = $res->decoded_content; + } + my $form = form_parse($content); + my ($c, $o, $k, $e) = @{$$form[0]}; if (!@$o && $c) { @@ -951,12 +958,16 @@ B<bcc>, and B<attachments> parameters (see C<comment> above). Get a list of numeric attachment IDs associated with ticket C<$id>. -=item get_attachment (parent_id => $parent_id, id => $id) +=item get_attachment (parent_id => $parent_id, id => $id, undecoded => $bool) Returns reference to a hash with key-value pair describing attachment C<$id> of ticket C<$parent_id>. (parent_id because -- who knows? -- maybe attachments won't be just for tickets anymore in the future). +If the option undecoded is set to a true value, the attachment will be +returned verbatim and undecoded (this is probably what you want with +images and binary data). + =item get_transaction_ids (parent_id => $id, %opts) Get a list of numeric IDs associated with parent ID C<$id>. C<%opts> Best wishes -- Marco
Download (untitled) / with headers
text/plain 251b
Hi, Did this behaviour start after upgrading RT::Client::REST to the latest version? Or did you just detect it on a new project? Could you maybe try with v 0.42 to see if you get the same behavior with attachments? Best Regards, Jose Luis Martinez
Subject: Re: [rt.cpan.org #90112] Attachment retrieval returns wrongly decoded files
Date: Thu, 14 Nov 2013 08:35:20 +0100
To: bug-RT-Client-REST [...] rt.cpan.org
From: Marco Pessotto <melmothx [...] gmail.com>
Download (untitled) / with headers
text/plain 1020b
"Jose Luis Martinez Torres via RT" <bug-RT-Client-REST@rt.cpan.org> writes: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=90112 > > > Hi, > > Did this behaviour start after upgrading RT::Client::REST to the > latest version? Or did you just detect it on a new project?
Show quoted text
> Could you maybe try with v 0.42 to see if you get the same behavior > with attachments?
I detected it on a new project, against RT 4.0.7. I've tried the 0.42 version, but I get the same behaviour: If I try to write a file with the returned string, I get "Wide character in syswrite" (given that is undecoded), if I try to encode that string to utf8 or latin (even if that's just the wrong thing to do, given that's a binary file) I get a file with garbage. It's quite possible that the behaviour of 4.0.x changed from 3.8.x, but my patch addresses it providing the option "undecoded" and avoiding to modify the existing behaviour. If I find the time I'll try to write a test for this but it's not so trivial to do. Best wishes -- Marco
Download (untitled) / with headers
text/plain 288b
Hi, I'm actually thinking of turning your patch the other way around, providing undecoded as default seems like a more sane way of handling attachments by default, and giving the option of encoding for "backwards compatibility" reasons (just if someone was relying on the old behaviour)
Download (untitled) / with headers
text/plain 288b
Hi, I'm actually thinking of turning your patch the other way around, providing undecoded as default seems like a more sane way of handling attachments by default, and giving the option of encoding for "backwards compatibility" reasons (just if someone was relying on the old behaviour)
Subject: Re: [rt.cpan.org #90112] Attachment retrieval returns wrongly decoded files
Date: Thu, 14 Nov 2013 15:16:20 +0100
To: bug-RT-Client-REST [...] rt.cpan.org
From: "Stefan Hornburg (Racke)" <racke [...] linuxia.de>
Download (untitled) / with headers
text/plain 768b
On 11/14/2013 03:10 PM, Jose Luis Martinez Torres via RT wrote: Show quoted text
> Queue: RT-Client-REST > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=90112 > > > Hi, > > I'm actually thinking of turning your patch the other way around, providing undecoded as default seems like a more sane way of handling attachments by default, and giving the option of encoding for "backwards compatibility" reasons (just if someone was relying on the old behaviour) >
Shouldn't be there a way to determine the MIME-Type of an attachment? It would make sense to decode text/* types. Regards Racke -- LinuXia Systems => http://www.linuxia.de/ Expert Interchange Consulting and System Administration ICDEVGROUP => http://www.icdevgroup.org/ Interchange Development Team
Subject: Re: [rt.cpan.org #90112] Attachment retrieval returns wrongly decoded files
Date: Thu, 14 Nov 2013 15:34:27 +0100
To: bug-RT-Client-REST [...] rt.cpan.org
From: Marco Pessotto <melmothx [...] gmail.com>
"Stefan Hornburg via RT" <bug-RT-Client-REST@rt.cpan.org> writes: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=90112 > > > On 11/14/2013 03:10 PM, Jose Luis Martinez Torres via RT wrote:
>> Queue: RT-Client-REST >> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=90112 > >> >> Hi, >> >> I'm actually thinking of turning your patch the other way around, >> providing undecoded as default seems like a more sane way of >> handling attachments by default, and giving the option of encoding >> for "backwards compatibility" reasons (just if someone was relying >> on the old behaviour) >>
> > Shouldn't be there a way to determine the MIME-Type of an attachment? > It would make sense to decode text/* types.
The response will always declare itself as "text/plain; encoding: utf8" (at least on 4.0.7), so it will not work out of the box. The binary data are sent incapsulated (and splat by lines, with indentation): see my recent commit for a visual exaplanation (in the heredoc at the end). https://github.com/RT-Client-REST/RT-Client-REST/blob/master/rt-client-rest/t/83-attachments.t A better solution for binary attachment would be fetching the /content url, instead, which is the verbatim binary data, with a sensible HTTP header, and just 2 spurios lines to be stripped at the beginning of the body response. So maybe we can add a method for this, without altering existing behaviour in a way or another (I'd keep the undecoded option, though, which is handy to fetch the filename and the body in one run, for example). wget -S -q --keep-session-cookies --load-cookies cookies.txt http://localhost/rt/REST/1.0/ticket/130/attachments/873/content -O- | head -3 HTTP/1.1 200 OK Server: nginx/1.2.1 Content-Type: image/png Transfer-Encoding: chunked Connection: keep-alive Set-Cookie: RT_SID_rt.universe.krase.net.80=bf42e792575dd83805497d702570cb59; path=/rt; HttpOnly Date: Thu, 14 Nov 2013 14:31:20 GMT X-Frame-Options: DENY Expires: Thu, 01 Jan 1970 00:00:01 GMT Cache-Control: no-cache RT/4.0.7 200 Ok ?PNG [...] Best wishes -- Marco
Download (untitled) / with headers
text/plain 792b
Hi, Show quoted text
> > Shouldn't be there a way to determine the MIME-Type of an attachment? > > It would make sense to decode text/* types.
> > The response will always declare itself as "text/plain; encoding: > utf8" > (at least on 4.0.7), so it will not work out of the box. The binary > data > are sent incapsulated (and splat by lines, with indentation): see my > recent commit for a visual exaplanation (in the heredoc at the end). > > https://github.com/RT-Client-REST/RT-Client-REST/blob/master/rt- > client-rest/t/83-attachments.t > > A better solution for binary attachment would be fetching the /content > url, instead, which is the verbatim binary data, with a sensible HTTP > header, and just 2 spurios lines to be stripped at the beginning of > the > body response.
+1 for that approach.
Download (untitled) / with headers
text/plain 734b
On Thu Nov 14 09:34:44 2013, melmothx@gmail.com wrote: Show quoted text
> The response will always declare itself as "text/plain; encoding: > utf8" > (at least on 4.0.7), so it will not work out of the box. The binary > data > are sent incapsulated (and splat by lines, with indentation): see my > recent commit for a visual exaplanation (in the heredoc at the end). > > https://github.com/RT-Client-REST/RT-Client-REST/blob/master/rt- > client-rest/t/83-attachments.t
Marco, If you check out test reports, you'll see that the test you added fails a lot, especially on OpenBSD: http://www.cpantesters.org/distro/R/RT-Client-REST.html?grade=3&perlmat=2&patches=2&oncpan=2&distmat=2&perlver=ALL&osname=ALL&version=0.45 Any idea why? - Dmitri.
Subject: Re: [rt.cpan.org #90112] Attachment retrieval returns wrongly decoded files
Date: Sat, 26 Apr 2014 19:03:35 +0200
To: bug-RT-Client-REST [...] rt.cpan.org
From: Marco Pessotto <melmothx [...] gmail.com>
Download (untitled) / with headers
text/plain 517b
"Dmitri Tikhonov via RT" <bug-RT-Client-REST@rt.cpan.org> writes: Show quoted text
> Marco, > > If you check out test reports, you'll see that the test you added > fails a lot, especially on OpenBSD: > > http://www.cpantesters.org/distro/R/RT-Client-REST.html?grade=3&perlmat=2&patches=2&oncpan=2&distmat=2&perlver=ALL&osname=ALL&version=0.45 > > Any idea why?
Ideas right away? No. As far as I can see, it fails consistently only on OpenBSB, the other failures are unrelated. Anyone with an openbsd box? Best wishes -- Marco
Download (untitled) / with headers
text/plain 224b
On Sat Apr 26 13:03:49 2014, melmothx@gmail.com wrote: Show quoted text
> As far as I can see, it fails consistently only on OpenBSB, the other > failures are unrelated.
I fixed the OpenBSD issue (RT#95223), will be out in the next version.
RT-Send-CC: racke [...] linuxia.de
Download (untitled) / with headers
text/plain 799b
The RT folks must have discovered the same issues in 3.8 time frame. From rt-3.8.17/share/html/REST/1.0/Forms/attachment/default: if ( $attachment->ContentType =~ m|^text/| ) { push @data, [ Content => $attachment->Content ]; } else { push @data, [ Content => "Content is not text and will not be displayed!\n" . "Use \"rt show attachment/<id>/content [> file.ext]\" to get the content." ]; } This code (as a matter of fact, this file) is not present in 3.6. When I originally wrote RT::Client::REST, it was 3.6.x that was current. I guess the solution is indeed to issue an additional HTTP request (transparently) for attachments whose content type !~ /^text Thank you for your analysis and suggestion, Stefan.
RT-Send-CC: racke [...] linuxia.de
Download (untitled) / with headers
text/plain 419b
On Sun May 11 00:29:53 2014, DMITRI wrote: Show quoted text
> Thank you for your analysis and suggestion, Stefan.
I meant Marco... I was replying to https://rt.cpan.org/Ticket/Display.html?id=90112#txn-1287853 and got confused by the way rt displays messages: "Stefan Hornburg via RT" <bug-RT-Client-REST@rt.cpan.org> writes: - Show quoted text- The response will always declare itself as "text/plain; encoding: utf8" I blame RT! :)
Subject: Re: [rt.cpan.org #90112] Attachment retrieval returns wrongly decoded files
Date: Sun, 11 May 2014 08:58:49 +0200
To: bug-RT-Client-REST [...] rt.cpan.org
From: Marco Pessotto <melmothx [...] gmail.com>
Download (untitled) / with headers
text/plain 1.3k
"Dmitri Tikhonov via RT" <bug-RT-Client-REST@rt.cpan.org> writes: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=90112 > > > The RT folks must have discovered the same issues in 3.8 time frame. > > From rt-3.8.17/share/html/REST/1.0/Forms/attachment/default: > > if ( $attachment->ContentType =~ m|^text/| ) { > push @data, [ Content => $attachment->Content ]; > } else { > push @data, > [ Content => > "Content is not text and will not be displayed!\n" > . "Use \"rt show attachment/<id>/content [> file.ext]\" to get the content." > ]; > } > > This code (as a matter of fact, this file) is not present in 3.6. When I originally > wrote RT::Client::REST, it was 3.6.x that was current. > > I guess the solution is indeed to issue an additional HTTP request (transparently) for > attachments whose content type !~ /^text
I was testing the thing against 4.0.7 (as per my original bug report). The problem is that, as far as I remember, getting the attachment you're required to strip the RT headers (the first two lines of the content), it looks like you just can't get the whole, unmodified file in a single HTTP request and call ->content on it. So there is, again, the need to parse the content in an (slightly) hacky way to get a binary file. But I could be wrong. Cheers -- Marco
Download (untitled) / with headers
text/plain 729b
On Sun May 11 02:59:01 2014, melmothx@gmail.com wrote: Show quoted text
> So there is, again, the need to > parse the content in an (slightly) hacky way to get a binary file.
Yes, that is true. I realized it last night as I was drifting off to sleep, after I posted the comment :) However, I'd still like to get rid of undecoded => [0|1] option to get_attachment() and do what Stefan suggested: decide on whether to decode content based on MIME type inside the function itself: content = res->decoded_content # OK if binary attachment is mangled at this point, since first part of # the response is ASCII and isn't mangled. parse_form(content) if (content-type !~ m~^text/~) { content = res->content parse_form(content) }


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.