Partial fix for bug 302669: rewrites Attachment.pm to provide a real Attachment class; r=lpsolit

7 files changed, 407 insertions, 111 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 558d7f8bc..578f67b1f 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -20,96 +20,384 @@
 # Contributor(s): Terry Weissman <terry@mozilla.org>
 #                 Myk Melez <myk@mozilla.org>
 
-############################################################################
-# Module Initialization
-############################################################################
-
 use strict;
 
 package Bugzilla::Attachment;
 
-# This module requires that its caller have said "require globals.pl" to import
-# relevant functions from that script.
+=head1 NAME
+
+Bugzilla::Attachment - a file related to a bug that a user has uploaded
+                       to the Bugzilla server
+
+=head1 SYNOPSIS
+
+  use Bugzilla::Attachment;
+
+  # Get the attachment with the given ID.
+  my $attachment = Bugzilla::Attachment->get($attach_id);
+
+  # Get the attachments with the given IDs.
+  my $attachments = Bugzilla::Attachment->get_list($attach_ids);
+
+=head1 DESCRIPTION
+
+This module defines attachment objects, which represent files related to bugs
+that users upload to the Bugzilla server.
+
+=cut
+
+# This module requires that its caller have said "require globals.pl"
+# to import relevant functions from that script.
 
-# Use the Flag module to handle flags.
 use Bugzilla::Flag;
 use Bugzilla::Config qw(:locations);
 use Bugzilla::User;
 
-############################################################################
-# Functions
-############################################################################
-
-sub new {
-    # Returns a hash of information about the attachment with the given ID.
+sub get {
+    my $invocant = shift;
+    my $id = shift;
 
-    my ($invocant, $id) = @_;
-    return undef if !$id;
-    my $self = { 'id' => $id };
-    my $class = ref($invocant) || $invocant;
-    bless($self, $class);
-    
-    &::PushGlobalSQLState();
-    &::SendSQL("SELECT 1, description, bug_id, isprivate FROM attachments " . 
-               "WHERE attach_id = $id");
-    ($self->{'exists'},
-     $self->{'summary'},
-     $self->{'bug_id'},
-     $self->{'isprivate'}) = &::FetchSQLData();
-    &::PopGlobalSQLState();
+    my $attachments = _retrieve([$id]);
+    my $self = $attachments->[0];
+    bless($self, ref($invocant) || $invocant) if $self;
 
     return $self;
 }
 
-sub query
-{
-  # Retrieves and returns an array of attachment records for a given bug. 
-  # This data should be given to attachment/list.html.tmpl in an
-  # "attachments" variable.
-  my ($bugid) = @_;
-
-  my $dbh = Bugzilla->dbh;
-
-  # Retrieve a list of attachments for this bug and write them into an array
-  # of hashes in which each hash represents a single attachment.
-  my $list = $dbh->selectall_arrayref("SELECT attach_id, " .
-                                      $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') .
-                                      ", mimetype, description, ispatch,
-                                      isobsolete, isprivate, LENGTH(thedata),
-                                      submitter_id
-                                      FROM attachments
-                                      INNER JOIN attach_data
-                                      ON id = attach_id
-                                      WHERE bug_id = ? ORDER BY attach_id",
-                                      undef, $bugid);
-
-  my @attachments = ();
-  foreach my $row (@$list) {
-    my %a;
-    ($a{'attachid'}, $a{'date'}, $a{'contenttype'},
-     $a{'description'}, $a{'ispatch'}, $a{'isobsolete'},
-     $a{'isprivate'}, $a{'datasize'}, $a{'submitter_id'}) = @$row;
-
-    $a{'submitter'} = new Bugzilla::User($a{'submitter_id'});
-
-    # Retrieve a list of flags for this attachment.
-    $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'},
-                                          'is_active' => 1 });
-
-    # A zero size indicates that the attachment is stored locally.
-    if ($a{'datasize'} == 0) {
-        my $attachid = $a{'attachid'};
-        my $hash = ($attachid % 100) + 100;
-        $hash =~ s/.*(\d\d)$/group.$1/;
-        if (open(AH, "$attachdir/$hash/attachment.$attachid")) {
-            $a{'datasize'} = (stat(AH))[7];
+sub get_list {
+    my $invocant = shift;
+    my $ids = shift;
+
+    my $attachments = _retrieve($ids);
+    foreach my $attachment (@$attachments) {
+        bless($attachment, ref($invocant) || $invocant);
+    }
+
+    return $attachments;
+}
+
+sub _retrieve {
+    my ($ids) = @_;
+
+    return [] if scalar(@$ids) == 0;
+
+    my @columns = (
+        'attachments.attach_id AS id',
+        'attachments.bug_id AS bug_id',
+        'attachments.description AS description',
+        'attachments.mimetype AS contenttype',
+        'attachments.submitter_id AS _attacher_id',
+        Bugzilla->dbh->sql_date_format('attachments.creation_ts',
+                                       '%Y.%m.%d %H:%i') . " AS attached",
+        'attachments.filename AS filename',
+        'attachments.ispatch AS ispatch',
+        'attachments.isobsolete AS isobsolete',
+        'attachments.isprivate AS isprivate'
+    );
+    my $columns = join(", ", @columns);
+
+    my $records = Bugzilla->dbh->selectall_arrayref("SELECT $columns
+                                                     FROM attachments
+                                                     WHERE attach_id IN (" .
+                                                     join(",", @$ids) . ")",
+                                                    { Slice => {} });
+    return $records;
+}
+
+=pod
+
+=head2 Instance Properties
+
+=over
+
+=item C<id>
+
+the unique identifier for the attachment
+
+=back
+
+=cut
+
+sub id {
+    my $self = shift;
+    return $self->{id};
+}
+
+=over
+
+=item C<bug_id>
+
+the ID of the bug to which the attachment is attached
+
+=back
+
+=cut
+
+# XXX Once Bug.pm slims down sufficiently this should become a reference
+# to a bug object.
+sub bug_id {
+    my $self = shift;
+    return $self->{bug_id};
+}
+
+=over
+
+=item C<description>
+
+user-provided text describing the attachment
+
+=back
+
+=cut
+
+sub description {
+    my $self = shift;
+    return $self->{description};
+}
+
+=over
+
+=item C<contenttype>
+
+the attachment's MIME media type
+
+=back
+
+=cut
+
+sub contenttype {
+    my $self = shift;
+    return $self->{contenttype};
+}
+
+=over
+
+=item C<attacher>
+
+the user who attached the attachment
+
+=back
+
+=cut
+
+sub attacher {
+    my $self = shift;
+    return $self->{attacher} if exists $self->{attacher};
+    $self->{attacher} = new Bugzilla::User($self->{_attacher_id});
+    return $self->{attacher};
+}
+
+=over
+
+=item C<attached>
+
+the date and time on which the attacher attached the attachment
+
+=back
+
+=cut
+
+sub attached {
+    my $self = shift;
+    return $self->{attached};
+}
+
+=over
+
+=item C<filename>
+
+the name of the file the attacher attached
+
+=back
+
+=cut
+
+sub filename {
+    my $self = shift;
+    return $self->{filename};
+}
+
+=over
+
+=item C<ispatch>
+
+whether or not the attachment is a patch
+
+=back
+
+=cut
+
+sub ispatch {
+    my $self = shift;
+    return $self->{ispatch};
+}
+
+=over
+
+=item C<isobsolete>
+
+whether or not the attachment is obsolete
+
+=back
+
+=cut
+
+sub isobsolete {
+    my $self = shift;
+    return $self->{isobsolete};
+}
+
+=over
+
+=item C<isprivate>
+
+whether or not the attachment is private
+
+=back
+
+=cut
+
+sub isprivate {
+    my $self = shift;
+    return $self->{isprivate};
+}
+
+=over
+
+=item C<data>
+
+the content of the attachment
+
+=back
+
+=cut
+
+sub data {
+    my $self = shift;
+    return $self->{data} if exists $self->{data};
+
+    # First try to get the attachment data from the database.
+    ($self->{data}) = Bugzilla->dbh->selectrow_array("SELECT thedata
+                                                      FROM attach_data
+                                                      WHERE id = ?",
+                                                     undef,
+                                                     $self->{id});
+
+    # If there's no attachment data in the database, the attachment is stored
+    # in a local file, so retrieve it from there.
+    if (length($self->{data}) == 0) {
+        if (open(AH, $self->_get_local_filename())) {
+            binmode AH;
+            $self->{data} = <AH>;
             close(AH);
         }
     }
-    push @attachments, \%a;
-  }
+    
+    return $self->{data};
+}
+
+=over
+
+=item C<datasize>
+
+the length (in characters) of the attachment content
+
+=back
+
+=cut
+
+# datasize is a property of the data itself, and it's unclear whether we should
+# expose it at all, since you can easily derive it from the data itself: in TT,
+# attachment.data.size; in Perl, length($attachment->{data}).  But perhaps
+# it makes sense for performance reasons, since accessing the data forces it
+# to get retrieved from the database/filesystem and loaded into memory,
+# while datasize avoids loading the attachment into memory, calling SQL's
+# LENGTH() function or stat()ing the file instead.  I've left it in for now.
+
+sub datasize {
+    my $self = shift;
+    return $self->{datasize} if exists $self->{datasize};
+
+    # If we have already retrieved the data, return its size.
+    return length($self->{data}) if exists $self->{data};
+
+    ($self->{datasize}) =
+        Bugzilla->dbh->selectrow_array("SELECT LENGTH(thedata)
+                                        FROM attach_data
+                                        WHERE id = ?",
+                                       undef,
+                                       $self->{id});
+
+    # If there's no attachment data in the database, the attachment
+    # is stored in a local file, so retrieve its size from the file.
+    if ($self->{datasize} == 0) {
+        if (open(AH, $self->_get_local_filename())) {
+            binmode AH;
+            $self->{datasize} = (stat(AH))[7];
+            close(AH);
+        }
+    }
+
+    return $self->{datasize};
+}
+
+=over
+
+=item C<flags>
+
+flags that have been set on the attachment
+
+=back
+
+=cut
+
+sub flags {
+    my $self = shift;
+    return $self->{flags} if exists $self->{flags};
+
+    $self->{flags} = Bugzilla::Flag::match({ attach_id => $self->id,
+                                             is_active => 1 });
+    return $self->{flags};
+}
+
+# Instance methods; no POD documentation here yet because the only one so far
+# is private.
+
+sub _get_local_filename {
+    my $self = shift;
+    my $hash = ($self->id % 100) + 100;
+    $hash =~ s/.*(\d\d)$/group.$1/;
+    return "$attachdir/$hash/attachment.$self->id";
+}
+
+=pod
+
+=head2 Class Methods
+
+=over
+
+=item C<get_attachments_by_bug($bug_id)>
+
+Description: retrieves and returns the attachments for the given bug.
+
+Params:     C<$bug_id> - integer - the ID of the bug for which
+            to retrieve and return attachments.
+
+Returns:    a reference to an array of attachment objects.
+
+=back
+
+=cut
 
-  return \@attachments;  
+sub get_attachments_by_bug {
+    my ($class, $bug_id) = @_;
+    my $attach_ids = Bugzilla->dbh->selectcol_arrayref("SELECT attach_id
+                                                        FROM attachments
+                                                        WHERE bug_id = ?
+                                                        ORDER BY attach_id",
+                                                       undef, $bug_id);
+    my $attachments = Bugzilla::Attachment->get_list($attach_ids);
+    return $attachments;
 }
 
 1;
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index c287d0c0c..dfa419316 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -354,7 +354,9 @@ sub attachments {
     my ($self) = @_;
     return $self->{'attachments'} if exists $self->{'attachments'};
     return [] if $self->{'error'};
-    $self->{'attachments'} = Bugzilla::Attachment::query($self->{bug_id});
+
+    $self->{'attachments'} =
+        Bugzilla::Attachment->get_attachments_by_bug($self->bug_id);
     return $self->{'attachments'};
 }
 
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 3e72c5933..89dda08a5 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -347,26 +347,26 @@ sub validate {
                 # can_see_bug() will refer to old settings.
                 if (!$requestee->can_see_bug($bug_id)) {
                     ThrowUserError("flag_requestee_unauthorized",
-                                   { flag_type => $flag->{'type'},
-                                     requestee => $requestee,
-                                     bug_id => $bug_id,
-                                     attach_id =>
-                                       $flag->{target}->{attachment}->{id} });
+                                   { flag_type  => $flag->{'type'},
+                                     requestee  => $requestee,
+                                     bug_id     => $bug_id,
+                                     attachment => $flag->{target}->{attachment}
+                                   });
                 }
     
                 # Throw an error if the target is a private attachment and
                 # the requestee isn't in the group of insiders who can see it.
-                if ($flag->{target}->{attachment}->{exists}
+                if ($flag->{target}->{attachment}
                     && $cgi->param('isprivate')
                     && Param("insidergroup")
                     && !$requestee->in_group(Param("insidergroup")))
                 {
                     ThrowUserError("flag_requestee_unauthorized_attachment",
-                                   { flag_type => $flag->{'type'},
-                                     requestee => $requestee,
-                                     bug_id    => $bug_id,
-                                     attach_id =>
-                                      $flag->{target}->{attachment}->{id} });
+                                   { flag_type  => $flag->{'type'},
+                                     requestee  => $requestee,
+                                     bug_id     => $bug_id,
+                                     attachment => $flag->{target}->{attachment}
+                                   });
                 }
             }
         }
@@ -532,7 +532,9 @@ sub create {
     $flag->{'id'} = (&::FetchOneColumn() || 0) + 1;
     
     # Insert a record for the flag into the flags table.
-    my $attach_id = $flag->{'target'}->{'attachment'}->{'id'} || "NULL";
+    my $attach_id =
+      $flag->{target}->{attachment} ? $flag->{target}->{attachment}->{id}
+                                    : "NULL";
     my $requestee_id = $flag->{'requestee'} ? $flag->{'requestee'}->id : "NULL";
     &::SendSQL("INSERT INTO flags (id, type_id, 
                                       bug_id, attach_id, 
@@ -807,7 +809,8 @@ sub FormToNewFlags {
             { 'type_id'     => $type_id,
               'target_type' => $target->{'type'},
               'bug_id'      => $target->{'bug'}->{'id'},
-              'attach_id'   => $target->{'attachment'}->{'id'},
+              'attach_id'   => $target->{'attachment'} ?
+                                 $target->{'attachment'}->{'id'} : undef,
               'is_active'   => 1 });
 
         # Do not create a new flag of this type if this flag type is
@@ -902,15 +905,18 @@ sub get_target {
     my $target = { 'exists' => 0 };
 
     if ($attach_id) {
-        $target->{'attachment'} = new Bugzilla::Attachment($attach_id);
+        $target->{'attachment'} = Bugzilla::Attachment->get($attach_id);
         if ($bug_id) {
             # Make sure the bug and attachment IDs correspond to each other
             # (i.e. this is the bug to which this attachment is attached).
-            $bug_id == $target->{'attachment'}->{'bug_id'}
-              || return { 'exists' => 0 };
+            if (!$target->{'attachment'}
+                || $target->{'attachment'}->{'bug_id'} != $bug_id)
+            {
+              return { 'exists' => 0 };
+            }
         }
-        $target->{'bug'} = GetBug($target->{'attachment'}->{'bug_id'});
-        $target->{'exists'} = $target->{'attachment'}->{'exists'};
+        $target->{'bug'} = GetBug($bug_id);
+        $target->{'exists'} = 1;
         $target->{'type'} = "attachment";
     }
     elsif ($bug_id) {
@@ -937,20 +943,21 @@ Sends an email notification about a flag being created or fulfilled.
 sub notify {
     my ($flag, $template_file) = @_;
 
+    my $attachment_is_private = $flag->{'target'}->{'attachment'} ?
+      $flag->{'target'}->{'attachment'}->{'isprivate'} : undef;
+
     # If the target bug is restricted to one or more groups, then we need
     # to make sure we don't send email about it to unauthorized users
     # on the request type's CC: list, so we have to trawl the list for users
     # not in those groups or email addresses that don't have an account.
-    if ($flag->{'target'}->{'bug'}->{'restricted'}
-        || $flag->{'target'}->{'attachment'}->{'isprivate'})
-    {
+    if ($flag->{'target'}->{'bug'}->{'restricted'} || $attachment_is_private) {
         my @new_cc_list;
         foreach my $cc (split(/[, ]+/, $flag->{'type'}->{'cc_list'})) {
             my $ccuser = Bugzilla::User->new_from_login($cc) || next;
 
             next if $flag->{'target'}->{'bug'}->{'restricted'}
               && !$ccuser->can_see_bug($flag->{'target'}->{'bug'}->{'id'});
-            next if $flag->{'target'}->{'attachment'}->{'isprivate'}
+            next if $attachment_is_private
               && Param("insidergroup")
               && !$ccuser->in_group(Param("insidergroup"));
             push(@new_cc_list, $cc);
diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl
index 4f66a5eb8..61b68ee53 100644
--- a/template/en/default/attachment/list.html.tmpl
+++ b/template/en/default/attachment/list.html.tmpl
@@ -37,7 +37,7 @@
     [% IF !attachment.isprivate || canseeprivate %]
       <tr [% "class=\"bz_private\"" IF attachment.isprivate %]>
         <td valign="top">
-          <a href="attachment.cgi?id=[% attachment.attachid %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a>
+          <a href="attachment.cgi?id=[% attachment.id %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a>
         </td>
 
         <td valign="top">
@@ -49,11 +49,11 @@
         </td>
 
         <td valign="top">
-          <a href="mailto:[% attachment.submitter.email FILTER html %]">
-          [% attachment.submitter.name || attachment.submitter.login FILTER html %]
+          <a href="mailto:[% attachment.attacher.email FILTER html %]">
+          [% attachment.attacher.name || attachment.attacher.login FILTER html %]
           </a>
         </td>
-        <td valign="top">[% attachment.date FILTER time %]</td>
+        <td valign="top">[% attachment.attached FILTER time %]</td>
         <td valign="top">[% attachment.datasize FILTER unitconvert %]</td>
 
         [% IF show_attachment_flags %]
@@ -75,9 +75,9 @@
         [% END %]
 
         <td valign="top">
-          <a href="attachment.cgi?id=[% attachment.attachid %]&amp;action=edit">Edit</a>
+          <a href="attachment.cgi?id=[% attachment.id %]&amp;action=edit">Edit</a>
           [% IF attachment.ispatch && patchviewerinstalled %]
-            | <a href="attachment.cgi?id=[% attachment.attachid %]&amp;action=diff">Diff</a>
+            | <a href="attachment.cgi?id=[% attachment.id %]&amp;action=diff">Diff</a>
           [% END %]
           [% Hook.process("action") %]
         </td>
diff --git a/template/en/default/bug/show.xml.tmpl b/template/en/default/bug/show.xml.tmpl
index 222204936..2555600ed 100644
--- a/template/en/default/bug/show.xml.tmpl
+++ b/template/en/default/bug/show.xml.tmpl
@@ -73,8 +73,8 @@
               ispatch="1"
             [% END %]
           >
-            <attachid>[% a.attachid %]</attachid>
-            <date>[% a.date FILTER time FILTER xml %]</date>
+            <attachid>[% a.id %]</attachid>
+            <date>[% a.attached FILTER time FILTER xml %]</date>
             <desc>[% a.description FILTER xml %]</desc>
             <ctype>[% a.contenttype FILTER xml %]</ctype>
             [% FOREACH flag = a.flags %]
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index cc5a19d09..8ed71f008 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -368,7 +368,7 @@
 
 'bug/show.xml.tmpl' => [
   'VERSION', 
-  'a.attachid', 
+  'a.id', 
   'field', 
 ],
 
@@ -458,7 +458,7 @@
 ],
 
 'attachment/list.html.tmpl' => [
-  'attachment.attachid', 
+  'attachment.id', 
   'flag.status',
   'bugid',
 ],
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 3e1a2a3ef..53fdcc59f 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -442,7 +442,7 @@
     You asked [% requestee.identity FILTER html %]
     for <code>[% flag_type.name FILTER html %]</code> on [% terms.bug %] 
     [% bug_id FILTER html -%]
-    [% IF attach_id %], attachment [% attach_id FILTER html %][% END %], 
+    [% IF attachment %], attachment [% attachment.id FILTER html %][% END %], 
     but that [% terms.bug %] has been restricted to users in certain groups, 
     and the user you asked isn't in all the groups to which 
     the [% terms.bug %] has been restricted.
@@ -455,11 +455,10 @@
     You asked [% requestee.identity FILTER html %]
     for <code>[% flag_type.name FILTER html %]</code> on 
     [%+ terms.bug %] [%+ bug_id FILTER html %],
-    attachment [% attach_id FILTER html %], but that attachment is restricted 
-    to users
-    in the [% Param("insidergroup") FILTER html %] group, and the user
-    you asked isn't in that group.  Please choose someone else to ask,
-    or ask an administrator to add the user to the group.
+    attachment [% attachment.id FILTER html %], but that attachment
+    is restricted to users in the [% Param("insidergroup") FILTER html %] group,
+    and the user you asked isn't in that group.  Please choose someone else
+    to ask, or ask an administrator to add the user to the group.
 
   [% ELSIF error == "flag_type_cc_list_invalid" %]
     [% title = "Flag Type CC List Invalid" %]