Closed Bug 1111043 Opened 10 years ago Closed 10 years ago

Bug.add_comment returns the wrong comment ID


(Bugzilla :: WebService, defect)

Not set



Bugzilla 4.4


(Reporter: mcote, Assigned: LpSolit)



(Keywords: regression)


(2 files, 1 obsolete file)

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"}' '****'

$ curl ''
{"bugs":{},"comments":{"12743":{"is_private":false,"attachment_id":null,"creator":"","time":"1999-03-21T06:33:59Z","author":"","bug_id":2148,"tags":[],"text":"Pam for investigation...","id":12743,"creation_time":"1999-03-21T06:33:59Z","raw_text":"Pam for investigation..."}}}
Verified. Strange this has not been widely reported before. The code in Bugzilla::WebService::Bug::add_comment is:

    my $new_comment_id = $dbh->bz_last_key('longdescs', 'comment_id'); 

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.

Flags: blocking5.0?
(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().
Depends on: 1020023
Keywords: regression
Target Milestone: --- → Bugzilla 5.0
Version: 5.1 → 4.5.6
Attached patch patch, v1 (obsolete) — Splinter Review
Assignee: webservice → LpSolit
Attachment #8536013 - Flags: review?(dkl)
Attached patch patch, v1.1Splinter Review
$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.
Attachment #8536013 - Attachment is obsolete: true
Attachment #8536013 - Flags: review?(dkl)
Attachment #8536014 - Flags: review?(dkl)
(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.
Target Milestone: Bugzilla 5.0 → Bugzilla 4.4
Flags: blocking5.0? → blocking5.0+
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.
Attachment #8536014 - Flags: review?(dkl) → review+
(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.

Flags: approval?
Flags: approval? → approval+
There is no code change for 5.0, only a line in the 3-lines context was different.
Attachment #8537849 - Flags: review+
Flags: approval5.0+
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).
Attachment #8537849 - Attachment description: patch for 5.0, v1 → patch for 4.4 and 5.0, v1
Flags: approval4.4?
Flags: approval4.4? → approval4.4+
To ssh://
   04fd938..0598a0a  master -> master

To ssh://
   707773a..c76e2cc  5.0 -> 5.0

To ssh://
   d1634d5..4dba187  4.4 -> 4.4
Closed: 10 years ago
Resolution: --- → FIXED
Summary: ID returned from POSTing a new comment is wrong → Bug.add_comment returns the wrong comment ID
You need to log in before you can comment on or make changes to this bug.