111522 - Provide ability to specify MIME type of attachment when downloading

Status: VERIFIED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P1)

Description

[Alex Vincent [:WeirdAl]]

Spun off from bug 111520.

From that bug:

--- quote ---
Currently, Bugzilla supports a straight-forward attachment link:

Created an attachment (id=58900)

However, if I as a Bugzilla user could choose multiple mime-types via link for
this attactioment, it would be invaluable:

Created an attachment (id=58900, application/xhtml+xml) (application/xml)
(text/xml) (text/html)

--- end quote ---

I'm requesting this because a lot of XHTML attachments get filed as text/xml or
text/html.  If we can provide an easier way to examine these attachments from
multiple mime-types, and encourage using a  mime-type which is for XHTML only,
this would make testing of XHTML-based bugs much easier.

Comment 1

[Matthew Tuck [:CodeMachine]]

I expect you'll find that when you go to download the attachment, you need to
have just one MIME type to tell the browser the attachment is.  What purpose is
there of the multiple MIME types?  Searching?

Comment 2

[Alex Vincent [:WeirdAl]]

The purpose is to help us identify and test bugs which are specific to XHTML
instead of HTML or XML.  The mimetype can have a profound effect, as bug 111397,
bug 111514 and bug 111518 demonstrate.

It'd be nice to have one attachment instead of two, three, or four.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

I think here is a case where the cost would outweigh the benefit. Converting our
database structure/values to support multiple mime types per attachment surely
isn't worth it.  And I am not convinced this would benefit users outside of the
bugzilla.mozilla.org install.  As a matter of fact, the number of instances this
would be used properly even on b.m.o would be relatively small.  This is
probably a wontfix as this bug stands.

Perhaps a better solution would be a (undocumented?) feature allowing the URL to
specify the MIME type:  /attachment.cgi?id=1234&mimetype=text/plain for example.
 That would involve considerably less work to implement than the current proposal.

Removing xhtml keyword as this bug has nothing to do with support for XHTML.  We
support it, and AFAIK there is no standard that dictates we provide multiple
content types for a downloaded file.

Comment 4

[Alex Vincent [:WeirdAl]]

That's all I was really asking for, a way to call a particular mime-type from
the server.  Sending the mime-type via HTTP GET is fine with me.  I was simply
hoping b.m.o could provide four links when it has a file of that mime-type.

Comment 5

[Gervase Markham [:gerv]]

You can now change the MIME type of an attachment later, if it's specified
incorrectly. Does that resolve this bug?

Gerv

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Nope, as this bug is about getting an attachment with a content type other than
the one it has been correctly assigned.

Comment 7

[Alex Vincent [:WeirdAl]]

Suggested code for patch:

\template\en\default\attachment\list.html.tmpl (line 43 in current CVS)

      <td valign="top">
        [% IF attachment.ispatch %]
          <i>patch</i>
        [% ELSE %]
          [% attachment.contenttype FILTER html %]
          [% IF attachment.contenttype eq "application/xhtml+xml" %]
              <br><a href="attachment.cgi?id=[% attachment.attachid %]
&amp;action=view&amp;mimetype=application%2Fxml">application/xml</a> 
              <br><a href="attachment.cgi?id=[% attachment.attachid %]
&amp;action=view&amp;mimetype=text%2Fxml">text/xml</a> 
              <br><a href="attachment.cgi?id=[% attachment.attachid %]
&amp;action=view&amp;mimetype=text%2Fhtml">text/html</a>
          [% END %]

        [% END %]
      </td>

\attachment.cgi (line 366 in current CVS)

sub view
{
  # Display an attachment.

  # Retrieve the attachment content and its content type from the database.
  SendSQL("SELECT mimetype, thedata FROM attachments WHERE attach_id = $::FORM
{'id'}");
  my ($contenttype, $thedata) = FetchSQLData();

# Begin changes
$mimetype_flag = 0;
$mimetype = $::FORM{'mimetype'};
# Set boolean flag for overriding mimetype, and get whatever mimetype was 
submitted

if (($mimetype)&&($contenttype eq "application/xhtml+xml")) {
# if we have a mimetype submitted, and the mimetype retrieved from MySQL was 
XHTML,
    XHTML_SWITCH: {
        if ($mimetype eq "application/xml") {$mimetype_flag = 1; last 
XHTML_SWITCH;}
        if ($mimetype eq "text/xml") {$mimetype_flag = 1; last XHTML_SWITCH;}
        if ($mimetype eq "text/html") {$mimetype_flag = 1; last XHTML_SWITCH;}
# then check for what the submitted mimetype actually was.
# If the check passes, set the flag to true.
    }
}

# We can similarly override mimetypes for other XML languages such as XUL, 
simply by setting
# the mimetype_flag to 1.  

#if (($mimetype)&&($contenttype eq "application/xul+xml")) {
#    XUL_SWITCH: {
# etc. etc.
#    }
#}

if (!$mimetype_flag) {
# if there is no reason to override a submitted mimetype, go with the 
attachment's mimetype.
    $mimetype = $contenttype;
}
# End of changes
    
  # Return the appropriate HTTP response headers.
  print "Content-Type: $mimetype\n\n";

  print $thedata;
}

I'm not a person who does codes Perl on a daily basis; the above is mostly a 
guess based on context of variable usage and a couple things I picked up from 
perlfaq.  I'd appreciate it if someone could check the code involved here.

I have confirmed, though, that the logic behind my changes to attachment.cgi is 
valid.  (On that one, I'm uncertain of the usage of the $::FORM variable.)

Comment 8

[Alex Vincent [:WeirdAl]]

Patch coming up in a few minutes; I'm going to simplify this and open the
ctype=foo/bar to the same restrictions we have in creating an attachment and
setting its mimetype.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: Patch to enable attachment.cgi?id=111522&ctype=foo/bar (untested)

Comment 10

[Alex Vincent [:WeirdAl]]

Taking.

Comment 11

[Alex Vincent [:WeirdAl]]

Attachment: Patch to enable attachment.cgi?id=111522&ctype=application/foo (tested)

Heh, good thing myk told me to test my code.  It would not have worked.  This
patch does work.

Comment 12

[Christian Reis]

Comment on attachment 134085 [details] [diff] [review]
Patch to enable attachment.cgi?id=111522&ctype=application/foo (tested)

>-    SendSQL("SELECT mimetype, filename, thedata FROM attachments WHERE attach_id = $::FORM{'id'}");
>-    my ($contenttype, $filename, $thedata) = FetchSQLData();
>+    SendSQL("SELECT mimetype, filename, thedata, ispatch FROM attachments WHERE attach_id = $::FORM{'id'}");
>+    my ($contenttype, $filename, $thedata, $ispatch) = FetchSQLData();
>+    
>+    if (($::FORM{'ctype'})&&(!$ispatch))

nit: spacing around the &&

>+    {
>+      $contenttype = $::FORM{'ctype'};
>+    }

Question: why do we want ispatch to be an exception?

Comment 13

[Vlad Dascalu]

Attachment: Version 3

Alex's patch per Kiko's suggestions.

Comment 14

[Christian Reis]

Comment on attachment 134192 [details] [diff] [review]
Version 3

myk, I reviewed this, and though code-wise it's fine and works, I have a design
question.

This patch splits validation into the if ($action ...) section and then does
another check for $::FORM{'ctype'} inside sub view. I reckon it would be best,
cohesion-wise, to use a single check inside sub view, but since I'm not sure,
it would be nice to have your opinion. 

I'll attach the alternative patch.

Comment 15

[Christian Reis]

Attachment: alternative 1

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 134192 [details] [diff] [review]
Version 3

I agree, this should all be in the view function (as should all other
validation calls), but I'm ok with this version.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 134201 [details] [diff] [review]
alternative 1

This one also works. r=myk

Comment 18

[Alex Vincent [:WeirdAl]]

Take your pick, check one in to CVS, and obsolete the other one.  :)

Comment 19

[Christian Reis]

Since myk preferred the alternative, checked it in (but all the credit is yours,
Alex). Thanks a lot for the patch, it's great to get first-time contributions
from mozilla hackers!

/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.47; previous revision: 1.46

Comment 20

[Gervase Markham [:gerv]]

Hang on... this is really confusing. 

Up to this point, the ctype parameter has had a standard meaning in Bugzilla,
which is that it's a short identifier (such as "js" or "xml") which is expanded
to a full content type via a lookup table. It's used this way on a large number
of CGIs. Here, it is instead a full content-type.

It seems to me to make much more sense to have a different name for the
parameter whose value is a full content type. "content-type" seems the obvious
choice. :-)

Gerv

Comment 21

[Alex Vincent [:WeirdAl]]

After discussion with justdave, reopening to comply with Gerv's wish.  Patch
coming up asap (we want this in for 2.17.5, which is being completed now)

Comment 22

[Alex Vincent [:WeirdAl]]

Attachment: patch to replace ctype with content_type

Comment 23

[Dave Miller [:justdave]]

Comment on attachment 134639 [details] [diff] [review]
patch to replace ctype with content_type

r= justdave, a= justdave

Comment 24

[Dave Miller [:justdave]]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.48; previous revision: 1.47
done

Comment 25

[Gervase Markham [:gerv]]

Cool, thanks Alex :-)

Gerv

Comment 26

[Dave Miller [:justdave]]

This needs to be covered in the docs...

Comment 27

[Alex Vincent [:WeirdAl]]

bug 232096 filed w/ patch for d=[e-mail]