Bug.add_comment returns the wrong comment ID

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
WebService
--
major
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcote, Assigned: Frédéric Buclin)

Tracking

({regression})

4.5.6
Bugzilla 4.4
regression
Bug Flags:
approval +
approval5.0 +
blocking5.0 +
approval4.4 +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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=mcote@mozilla.com&password=****'
{"id":12743}

$ curl 'https://bugzilla-dev.allizom.org/rest/bug/comment/12743'
{"bugs":{},"comments":{"12743":{"is_private":false,"attachment_id":null,"creator":"gagan@formerly-netscape.com.tld","time":"1999-03-21T06:33:59Z","author":"gagan@formerly-netscape.com.tld","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:

    $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
(Reporter)

Updated

2 years ago
Flags: blocking5.0?
(Assignee)

Comment 2

2 years ago
(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
(Assignee)

Comment 3

2 years ago
Created attachment 8536013 [details] [diff] [review]
patch, v1
Assignee: webservice → LpSolit
Status: NEW → ASSIGNED
Attachment #8536013 - Flags: review?(dkl)
(Assignee)

Comment 4

2 years ago
Created attachment 8536014 [details] [diff] [review]
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.
Attachment #8536013 - Attachment is obsolete: true
Attachment #8536013 - Flags: review?(dkl)
Attachment #8536014 - Flags: review?(dkl)
(Assignee)

Comment 5

2 years ago
(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.

dkl
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 8

2 years ago
Created attachment 8537849 [details] [diff] [review]
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.
Attachment #8537849 - Flags: review+

Updated

2 years ago
Flags: approval5.0+
(Assignee)

Comment 9

2 years ago
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
(Assignee)

Updated

2 years ago
Flags: approval4.4?

Updated

2 years ago
Flags: approval4.4? → approval4.4+
(Assignee)

Comment 10

2 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   04fd938..0598a0a  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   707773a..c76e2cc  5.0 -> 5.0

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   d1634d5..4dba187  4.4 -> 4.4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.