1111043 - Bug.add_comment returns the wrong comment ID

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Mark Côté [:mcote]]

I found this on my local BMO dev box and then reproduced on bugzilla-dev.  The response to POSTing a comment to /rest/bug/<id>/comment is supposed to be {"id": <comment id>} but this does not seem to be the case:

$ curl -H "Content-type: application/json" -H "Accept: application/json" -X POST --data '{"comment": "test"}' 'https://bugzilla-dev.allizom.org/rest/bug/1004433/comment?login=[e-mail]&password=****'
{"id":12743}

$ curl 'https://bugzilla-dev.allizom.org/rest/bug/comment/12743'
{"bugs":{},"comments":{"12743":{"is_private":false,"attachment_id":null,"creator":"[e-mail]","time":"1999-03-21T06:33:59Z","author":"[e-mail]","bug_id":2148,"tags":[],"text":"Pam for investigation...","id":12743,"creation_time":"1999-03-21T06:33:59Z","raw_text":"Pam for investigation..."}}}

Comment 1

[David Lawrence [:dkl]]

Verified. Strange this has not been widely reported before. The code in Bugzilla::WebService::Bug::add_comment is:

    $dbh->bz_start_transaction(); 
     
    $bug->update(); 
     
    my $new_comment_id = $dbh->bz_last_key('longdescs', 'comment_id'); 
     
    $dbh->bz_commit_transaction();

Bugzilla::DB::Mysql has:

sub bz_last_key {
    my ($self) = @_;

    my ($last_insert_id) = $self->selectrow_array('SELECT LAST_INSERT_ID()');

    return $last_insert_id;
}

So we need to see which key is being returned as it is definitely not the comment_id we are looking for.

dkl

Comment 2

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #1)
> Strange this has not been widely reported before.

That's because only 5.0 and trunk are affected.

The problem is that MySQL is unable to get the "last insert ID" of a specific table. It only gets the "last insert ID" DB-wise. Bug 1020023 added code into Bug::update() which inserts data into the bug_user_last_visit table, and so "last insert ID" now points to this insertion instead of the comment one.

Unless $dbh->bz_last_key() is called right after $dbh->do('INSERT INTO ...'), it's not safe to call this method as you can have other SQL commands being executed meanwhile (either via an extension, or some other triggered code) which can alter the value of LAST_INSERT_ID. It's not a problem for PostgreSQL and Oracle, because they are able to get this info per table. MySQL cannot.

I did an audit of the whole Bugzilla codebase, and WS::Bug::add_comment() is the single place which incorrectly uses $dbh->bz_last_key().

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

Comment 4

[Frédéric Buclin]

Attachment: patch, v1.1

$bug->update already starts its own transaction, and so it's useless to start another one from add_comment(). It was only present to make sure to have the correct comment ID, but as I said above, this didn't work correctly anyway.

Comment 5

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #2)
> That's because only 5.0 and trunk are affected.

Actually, 4.4 is also affected if you pass work_time, because LogActivityEntry() adds an entry into the bugs_activity table, and this DB table has an auto-increment column since 4.4 (bug 764457). This will affect LAST_INSERT_ID. 4.2 and older should be safe.

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8536014 [details] [diff] [review]
patch, v1.1

Review of attachment 8536014 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl for trunk. Need separate patch for 4.4.

Comment 7

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #6)
> Need separate patch for 4.4.

Also will need a separate patch for 5.0 as the trunk patch does not apply cleanly.

dkl

Comment 8

[Frédéric Buclin]

Attachment: patch for 4.4 and 5.0, v1

There is no code change for 5.0, only a line in the 3-lines context was different.

Comment 9

[Frédéric Buclin]

Comment on attachment 8537849 [details] [diff] [review]
patch for 4.4 and 5.0, v1

The patch applies cleanly to 4.4 as well, and it correctly fixes the problem (passing work_time was indeed messing the comment ID being returned).

Comment 10

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   04fd938..0598a0a  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   707773a..c76e2cc  5.0 -> 5.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   d1634d5..4dba187  4.4 -> 4.4