Search.pm uses DB dependent comma operator

RESOLVED FIXED in Bugzilla 2.20

Status

()

P1
major
RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: Tomas.Kopal, Assigned: Tomas.Kopal)

Tracking

unspecified
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.1) Gecko/20040715 Debian/1.7.1-1
Build Identifier: 

As reported and fixed in bug 228917, comma operator for implicit JOIN is not DB
agnostic. In Bugzilla/Search.pm, there is another occurence, which should be
replaced by INNER JOIN. Patch follows.

Reproducible: Always
Steps to Reproduce:
(Assignee)

Comment 1

15 years ago
(Assignee)

Updated

15 years ago
Blocks: 98304
ya know, this looks an awful lot like attachment 116610 [details] [diff] [review] on bug 173130 ;)

I think this is something we need to do...  I know there was some argument on
173130 about whether some of the join conditions had been consolidated
correctly, so we probably need to review that carefully here, too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

15 years ago
Argh... I haven't seen that one yet, it could save me a few hours work :-(. But
at least we have it tested twice, independently, and on different DBs :-) (this
one was tested on Postgres and MySQL).
Assignee: justdave → Tomas.Kopal
Target Milestone: --- → Bugzilla 2.20

Updated

15 years ago
Depends on: 252564
Make sure that you test with flags, too. Flags has some funky SQL that
PostgreSQL doesn't always like if you start using INNER JOIN.

Updated

15 years ago
Attachment #153568 - Flags: review?
(Assignee)

Comment 5

15 years ago
Comment on attachment 153568 [details] [diff] [review]
Patch converting implicit JOIN to INNER JOIN

I removed the request for review as there were changes in Search.pm concerning
queries (bug 204903) and should be more (bug 252564). This patch needs to be
updated first.
Attachment #153568 - Flags: review?

Comment 6

14 years ago
OK, now that bug 252564 is done, the rest should become INNER joins.  Make sure
that there are no newlines in the SQL as you do this.
(Assignee)

Comment 7

14 years ago
*** Bug 285411 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 8

14 years ago
(In reply to comment #6)
> OK, now that bug 252564 is done, the rest should become INNER joins.  Make sure
> that there are no newlines in the SQL as you do this.
> 

Actually, I think most of them should be LEFT JOINS, as we do not care about
data for which there is no bug. But I need to consider it on case by case basis...

Comment 9

14 years ago
For joining the profile data (assignee, reporter ... but NOT qa_contact), these
should be inner joins (equivalent to comma).

Updated

14 years ago
Severity: normal → major
Priority: -- → P1
(Assignee)

Comment 10

14 years ago
Posted patch V2 (obsolete) — Splinter Review
Ok, here it is. Most of them are really INNER JOINS, take a look at the LEFT
JOINS, please, if they make sense there. I have also done some formating, I
hope it does not hurt :-).
Attachment #153568 - Attachment is obsolete: true
Attachment #177000 - Flags: review?(bugreport)

Comment 11

14 years ago
Comment on attachment 177000 [details] [diff] [review]
V2

Most of this looks great. (by inspection so far)  The only problem is....
	     # @list won't have any elements if the only field being searched
	     # is [Bug creation] (in which case we don't need bugs_activity).
	     if(@list) {
-		 push(@supptables, "bugs_activity actcheck");
-		 push(@wherepart, "actcheck.bug_id = bugs.bug_id");
+		 my $extra = "";
		 if($sql_chfrom) {
-		     push(@wherepart, "actcheck.bug_when >= $sql_chfrom");
+		     $extra .= " AND actcheck.bug_when >= $sql_chfrom";
		 }
		 if($sql_chto) {
-		     push(@wherepart, "actcheck.bug_when <= $sql_chto");
+		     $extra .= " AND actcheck.bug_when <= $sql_chto";
		 }
		 if($sql_chvalue) {
-		     push(@wherepart, "actcheck.added = $sql_chvalue");
+		     $extra .= " AND actcheck.added = $sql_chvalue";
		 }
+		 push(@supptables, "LEFT JOIN bugs_activity AS actcheck " .
+				   "ON actcheck.bug_id = bugs.bug_id $extra");
	     }

This wants to remain an inner join. If you make it into a left join, you will
still get a row even if none of the activity is what you want.	You could add a
wherepart that insists on it mathching a row, but that would change into an
inner join anyway.

The reason I use left joins in the boolean chart section is because the
wherepart (value IS NOT NULL) is subject to negation.  Here it isn't.
Attachment #177000 - Flags: review?(bugreport) → review-
(Assignee)

Comment 12

14 years ago
(In reply to comment #11)
> (From update of attachment 177000 [details] [diff] [review] [edit])
> This wants to remain an inner join. If you make it into a left join, you will
> still get a row even if none of the activity is what you want.	You could add a
> wherepart that insists on it mathching a row, but that would change into an
> inner join anyway.
> 
> The reason I use left joins in the boolean chart section is because the
> wherepart (value IS NOT NULL) is subject to negation.  Here it isn't.
> 

Yeah, you are right. I was thinking that there can be bugs which does not have
any record in the activity table, but I didn't realize there is no way to search
for them using our UI (the is nothing like "does not have any activity" test).

I wonder if that's not true for attachments as well (the second case I used LEFT
JOIN). We can have bugs without attachments, but is there a way to search for
them? I haven't found any. Should I then revert both of them to INNER JOINs?

The last LEFT JOIN I used is with keywords, I think that's correct there, am I
right?
(Assignee)

Comment 13

14 years ago
Posted patch V2.1Splinter Review
OK, I have replaced LEFT JOIN with INNER JOIN for attachments and activity. No
other changes from the previous version.
Attachment #177000 - Attachment is obsolete: true
Attachment #177094 - Flags: review?(bugreport)

Comment 14

14 years ago
Comment on attachment 177094 [details] [diff] [review]
V2.1

r=joel
(I doubt anyone will ever try to do a right join in Search.pm)
Attachment #177094 - Flags: review?(bugreport) → review+

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.89; previous revision: 1.88
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.48; previous revision: 1.47
done
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 16

14 years ago
*** Bug 320477 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.