454251 - Implement Bugzilla::Attachment->create() and $attachment->update()

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Frédéric Buclin]

Now that bug 388251 is fixed, we can use Object.pm to create and update attachments.

Comment 1

[Frédéric Buclin]

too late for 3.4.

Comment 2

[Frédéric Buclin]

My patch is almost ready. It works with post_bug.cgi and attachment.cgi create(). I'm now fixing it to work with attachment.cgi update(). It's so invasive (400 lines changed) that I won't request review, as 95% of the code is in modules I own (flags + attachments) and the remaining 5% is in code/user-error.html.tmpl to add new error messages and in post_bug.cgi to change arguments passed to Bugzilla::Attachment->create(). If there is something broken or which should be modified, we will do this in separate bugs.

Comment 3

[Frédéric Buclin]

As soon as my patch is checked in, attaching files from email_in.pl will be trivial. I already modified the code to not depend on CGI.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

That's a huge change in Attachment.pm, but it couldn't be smaller than that. I tested it as much as possible, and I think I caught most (all?) regressions.

I managed to kill its dependency on $cgi entirely, besides the get_content_type() helper which is here on purpose. Next step will be to do the same with flags.

If nobody complains within the next few hours, I'm going to commit it as is.

Comment 5

[Frédéric Buclin]

Pushing this bug into my radar. I will approve it tomorrow, per my previous comment.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 371747 [details] [diff] [review]
patch, v1

Could we put the valid content types into a constant, while we're here?

Also, the legal URL types. (Which you could use the URI module to validate too, if you want, since we require it now.)

Shouldn't _check_data be checking that files stored on the disk aren't too large?

Otherwise things look great. :-)

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> (From update of attachment 371747 [details] [diff] [review])
> Could we put the valid content types into a constant, while we're here?

That's doable, sure. I will do it.


> Also, the legal URL types. (Which you could use the URI module to validate too,
> if you want, since we require it now.)

We only accept http, https and ftp for now, while SAFE_PROTOCOLS has a much longer list. We could certainly reuse SAFE_PROTOCOLS for attachment URLs, but this would be an enhancement, i.e. a separate bug. Feel free to file the bug. :)


> Shouldn't _check_data be checking that files stored on the disk aren't too
> large?

No, this check is done in create() because the file may be quite large (several tens of MB) and the point is to not store its content into a variable, but write directly to the disk. When the stored file exceeds the size limit, we stop writing into it, delete it, and throw an error. Doing this check in _check_data() without storing its content in a variable is possible, but this means reading the file content twice (first to check its size in _check_data(), then to write to the disk in create()). I don't know how slow/fast it is to read tens of MB, but I prefered to do everything at once. If it appears that it's ultra fast to read only, then I could move this check into _check_data(). I didn't test.

Maybe could we let _check_data() write the attachment content into a file (as we have to do it sooner or later anyway), but as this checker must be the first one to be called (because it may alter some other attributes), I didn't want to waste time writing to the disk and then throw an error because the user forgot e.g. the description. I will let it unchanged for now, but we can easily move this check in a separate bug if there is no perf impact (as I agree the check should ideally be done in _check_data()).


> Otherwise things look great. :-)

Thanks! I spent the last 3 days to make it working. :)

Comment 8

[Frédéric Buclin]

Attachment: patch, v1.1

Put the list of legal MIME types in a constant named LEGAL_CONTENT_TYPES. That's the patch I'm going to check in.

Comment 9

[Frédéric Buclin]

When running runtests.pl just before the checkin, I noticed I forgot one FILTER html in an error message. I fixed it on checkin:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.155; previous revision: 1.154
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.199; previous revision: 1.198
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.61; previous revision: 1.60
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.112; previous revision: 1.111
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.102; previous revision: 1.101
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <-- code-error.html.tmpl
new revision: 1.111; previous revision: 1.110
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.278; previous revision: 1.277
done

Comment 10

[Frédéric Buclin]

Attachment: fix warning

When editing a public attachment, $is_private is undefined and the following error is thrown in the error log:

attachment.cgi: Use of uninitialized value $is_private in numeric ne (!=) at Bugzilla/Attachment.pm line 616.

Comment 11

[Frédéric Buclin]

Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.62; previous revision: 1.61
done