DBI placeholders break AppendComment's default timestamp

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Creating/Changing Bugs
P3
minor
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Toby Thain, Assigned: Frédéric Buclin)

Tracking

({regression})

2.20
Bugzilla 2.20
regression
Bug Flags:
approval +
approval2.22 +
approval2.20 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.09 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051025 Firefox/1.5
Build Identifier: 

If calling AppendComment with an indefined $timestamp (e.g. "AppendComment($bug,$login_name,$message);"), the query is supposed to substitute function NOW() in the INSERT query.

Unfortunately, now that the query is done with DBI and placeholders, it appears that the function call is being bound as a string, causing an incorrect zero timestamp to be stored instead of current time. This zero timestamp, apart from being bogus, means added comments are not displayed on bug pages.

Reproducible: Always

Steps to Reproduce:
1. invoke AppendComment($bug,$login_name,$message);
2. in MySQL, examine bug_when field in table longdescs in the newly inserted record.
Actual Results:  
timestamp will read "0000-00-00 00:00:00"

Expected Results:  
timestamp should be time of INSERT (e.g. "2005-11-06 16:53:31")

My quick and dirty workaround is to use two different queries depending on whether timestamp is defined:

if($timestamp){
    $dbh->do(q{INSERT INTO longdescs
                      (bug_id, who, bug_when, thetext, isprivate, work_time)
               VALUES (?,?,?,?,?,?)}, undef,
             ($bugid, $whoid, $timestamp, $comment, $privacyval, $work_time));
}else{
    $dbh->do(q{INSERT INTO longdescs
                      (bug_id, who, bug_when, thetext, isprivate, work_time)
               VALUES (?,?,now(),?,?,?)}, undef,
             ($bugid, $whoid, $comment, $privacyval, $work_time));
}

Comment 1

12 years ago
Confirmed, also wrong in 2.20. Caused by bug 283561.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Version: unspecified → 2.20

Comment 2

12 years ago
Interesting. I had a feeling that might happen. Does this cause any visible bugs?
Assignee: database → mkanat
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 3

12 years ago
$timestamp =  "NOW()" unless $timestamp; <---- boooo! :-p
Flags: blocking2.20.1?
Keywords: regression
(Assignee)

Comment 4

12 years ago
You are lucky, AppendComment() is *never* called without the $timestamp parameter being passed. So it's not a blocker and the DB integrity is fine.
Flags: blocking2.20.1?

Updated

12 years ago
Severity: normal → minor
Priority: P2 → P3
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22

Updated

12 years ago
Component: Database → Creating/Changing Bugs
(Assignee)

Comment 5

12 years ago
I disagree about retargetting this bug to 2.22. If we have to fix a bug in 2.20 and this fix uses AppendComment() without passing $timestamp, it will fails. And I'm sure Pg would complain that 'NOW()' (the string) is not a valid date. Even if it's not a blocker for 2.20, it has to be fixed there.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20

Comment 6

12 years ago
It causes no visible bug, there's no reason to fix it in 2.20. We don't need the function to be able to do that, in 2.20.

I can see needing to be able to do that in the future, so we can fix it in 2.22.

The fix is relatively simple, I believe, which is to actually *select* NOW() from the DB for $timestamp. It's not perfect, but it fixes the problem (the selected NOW() is likely to be a few milliseconds before the actual comment is inserted).
(Assignee)

Comment 7

12 years ago
(In reply to comment #6)
> It causes no visible bug, there's no reason to fix it in 2.20. We don't need
> the function to be able to do that, in 2.20.

As I said, future fixes may use AppendComment without $timestamp, in which case AppendComment has to be fixed. Also, users customizing their installation and who do not pass $timestamp should also expect to have AppendComment working correctly. The patch is the same for both 2.20 and 2.22 anyway.


> The fix is relatively simple, I believe, which is to actually *select* NOW()
> from the DB for $timestamp.

Of course this is the right fix. This is the way we do it all over the code when timestamp is missing. And this solution is "perfect" in the sense that this is the time which will be used everywhere within AppendComment, so we are consistent, and consistency is what is important here.

It's not perfect, but it fixes the problem (the
> selected NOW() is likely to be a few milliseconds before the actual comment is
> inserted).

This comment doesn't make sense. When process_bug.cgi defines $timestamp to be used everywhere within the code, *all* insertions have the same timestamp, as expected, despite the fact that insertions are executed one at a time instead of being done all at the same time.
(Reporter)

Comment 8

12 years ago
Correction to my original note: the wrong timestamp wasn't causing comments not to be shown. It was a faulty $who parameter, because I failed to notice that the $who parameter (login name) became $whoid in 2.20, and thus I needed to do:

AppendComment($bug,DBNameToIdAndCheck($login_name),$message);

This use is outside the Bugzilla tree, by the way; it's a post-commit hook script for Subversion. If curious, see http://www.telegraphics.com.au/svn/svn_bz/trunk/
(Assignee)

Comment 9

12 years ago
Created attachment 211652 [details] [diff] [review]
patch, v1
Assignee: mkanat → LpSolit
Status: NEW → ASSIGNED
Attachment #211652 - Flags: review?(mkanat)

Comment 10

12 years ago
Comment on attachment 211652 [details] [diff] [review]
patch, v1

That has to be SELECT NOW().
Attachment #211652 - Flags: review?(mkanat) → review-
(Assignee)

Comment 11

12 years ago
Created attachment 211654 [details] [diff] [review]
patch, v2

/me must be very tired. :(
Attachment #211652 - Attachment is obsolete: true
Attachment #211654 - Flags: review?(mkanat)

Comment 12

12 years ago
Comment on attachment 211654 [details] [diff] [review]
patch, v2

Yeah, if you tested this, this looks fine.
Attachment #211654 - Flags: review?(mkanat) → review+

Updated

12 years ago
Flags: approval?
Flags: approval2.20?
Flags: approval?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
(Assignee)

Comment 13

12 years ago
tip:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.107; previous revision: 1.106
done

2.22rc1:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.104.2.1; previous revision: 1.104
done

2.20.1:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.81.2.4; previous revision: 1.81.2.3
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.