error when %recipients is empty (e.g. after bugmail_recipients empties it)

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Email Notifications
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dnozay, Assigned: dnozay)

Tracking

Bugzilla 5.0
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.71 Safari/537.36

Steps to reproduce:

wrote an extension that clears the list of bugmail recipients for bulk updates; the gist is:

sub bugmail_recipients {
    my ($self, $args) = @_;
    my $recipients = @$args{qw(recipients)};
    # clear recipients
    %$recipients = ();
}



Actual results:

(note line numbers are from 4.2 not trunk)

Software error:

DBD::mysql::db selectall_arrayref failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 2 [for Statement "SELECT watcher, watched FROM watch
                                  WHERE watched IN ()"] at Bugzilla/BugMail.pm line 200
	Bugzilla::BugMail::Send(1, 'HASH(0x3e45648)', 'HASH(0x4d004c8)') called at Bugzilla/Bug.pm line 1224
	Bugzilla::Bug::_send_bugmail('HASH(0x4d004c8)', 'HASH(0x488ec58)') called at Bugzilla/Bug.pm line 1163
	Bugzilla::Bug::send_changes('Bugzilla::Bug=HASH(0x4c923e8)', 'HASH(0x492add8)', 'HASH(0x488ec58)') called at /var/www/bugzilla.proximdata.com/process_bug.cgi line 385
For help, please send mail to the webmaster ([no address given]), giving this error message and the time and date of the error.

-----

code doesn't check for elements before using the empty %recipients in the db query.
please note that this code is fixed in BMO 4.2


Expected results:

"
email sent to:
    no one
"
(Assignee)

Comment 1

4 years ago
Created attachment 789419 [details] [diff] [review]
patch.diff

trivial size check
Attachment #789419 - Flags: review?(justdave)
(Assignee)

Updated

4 years ago
Blocks: 904482

Comment 2

4 years ago
Comment on attachment 789419 [details] [diff] [review]
patch.diff

>+    if (scalar keys %recipients) {

This is a hack. No developer will understand why this check is here when you know that %recipients always has at least two entries: the reporter and the bug assignee.

Moreoever, if your goal is to clear %recipients, then you will miss global watchers who will still get bugmails.

Updated

4 years ago
Severity: normal → minor

Comment 3

4 years ago
(In reply to Frédéric Buclin from comment #2)
> This is a hack. No developer will understand why this check is here when you
> know that %recipients always has at least two entries

Said differently: you must add a comment right above this line explaining why it's there.
(Assignee)

Comment 4

4 years ago
Created attachment 789600 [details] [diff] [review]
v2
Attachment #789600 - Flags: review?(LpSolit)
(Assignee)

Comment 5

4 years ago
thanks for the review

(In reply to Frédéric Buclin from comment #2)
> Comment on attachment 789419 [details] [diff] [review]
> patch.diff
> 
> >+    if (scalar keys %recipients) {
> 
> This is a hack. No developer will understand why this check is here when you
> know that %recipients always has at least two entries: the reporter and the
> bug assignee.


    Bugzilla::Hook::process('bugmail_recipients',
                            { bug => $bug, recipients => \%recipients,
                              users => \%user_cache, diffs => \@diffs });

any bugmail_recipients hook may do anything to the parameters passed by reference.

> 
> Moreoever, if your goal is to clear %recipients, then you will miss global
> watchers who will still get bugmails.

I fully understand that the global watchers are processed late. If "global watching" was a vanilla extension, it could inspect and augment %recipients in a bugmail_recipients hook. The only potential problem I see is the extension system which AFAIK doesn't let you specify an order to run hooks other than the one derived from the extension filenames lexical order.

Rather than addressing that, this is a "simple" patch to fix a broken assumption.

Updated

4 years ago
Attachment #789600 - Flags: review?(LpSolit) → review?(glob)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #789419 - Attachment is obsolete: true
Attachment #789419 - Flags: review?(justdave)
Assignee: email-notifications → damien.nozay
Comment on attachment 789600 [details] [diff] [review]
v2

r=glob

i'm happy with that comment; extension developers are expected to be able to understand both perl and bugzilla.
Attachment #789600 - Flags: review?(glob) → review+
Flags: approval+
Target Milestone: --- → Bugzilla 5.0
(Assignee)

Comment 7

4 years ago
Byron / Frédéric, would you mind committing on my behalf?
thank you.
Thanks for the patch!

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/BugMail.pm
Committed revision 8861.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.