INSTR function for finding whether substring is part of other string is not supported by postgres. Both MySQL and Postgres do support POSITION functions, but it looks that POSITION is in turn not supported by Oracle. So lets create a new DB function for case sensitive search of substring within a string. Why case sensitive? It does correspond to how MySQL v3 and Postgres works, MySQL newer than 4.0.1 does case insensitive search, but we are already using LOWER with this function so case sensitive makes more sense. Also it is posible to use case sensitive function for case insensitive search using LOWER, but not vice versa.

At this point I'd rather just use POSITION, and then change that to a Bugzilla::DB function when we do Oracle support.

That's ok with me, I just tought that when we are at that, we can do it in one shot :-).

Hmmm, as I am thinking about it again, I don't think it will work. We need at least at one place in Search.pm to do case-sensitive substring, and POSITION is case insensitive on MySQL. The solution is type cast the parameters to BINARY, but that breaks postgres (Pg does not have BINARY type). So I think having DB method would be better solution.

The ANSI equivalent of INSTR is SUBSTRING, not POSITION. :-)

Oh nevermind, I'm wrong, you're right. It's POSITION. :-) And you're right. There's no standard case-sensitive search function in MySQL. That's really quite stupid. So we need a sql_position function, you're right, which does a case-sensitive search, and then we can explicitly call LOWER() when we want a case-insensitive search. This will pobably fix strange Bugzilla bugs while we do it, anyhow. :-)

Here is the patch. It's tested on MySQL only.

Comment on attachment 176248 [details] [diff] [review] V1 >+ "WHERE " . $dbh->sql_position('/', 'filename') . >+ " OR " . $dbh->sql_position('\\\\', 'filename')); That looks like it will produce: POSITION(/ IN filename) for PostgreSQL. Note the lack of quoting. I think that if we want to pass a literal to the function, we should do q{'\'}. I'd like the functions to be as flexible as possible, and not have them do their own quoting. You'll want to note that arguments aren't quoted, though. You might also want to change the one call to POSITION in checksetup, for the heck of it, even though it doesn't need it. >+ $dbh->sql_position(lc(::SqlQuote($email)), "LOWER(login_name)") . That looks like it should be &::SqlQuote($email). >+ push(@list, $dbh->sql_position(lc(::SqlQuote($word)), >+ "LOWER($field)")); Same there. Why did you remove the &?

(In reply to comment #7) > (From update of attachment 176248 [details] [diff] [review] [edit]) > >+ "WHERE " . $dbh->sql_position('/', 'filename') . > >+ " OR " . $dbh->sql_position('\\\\', 'filename')); > > That looks like it will produce: > > POSITION(/ IN filename) > > for PostgreSQL. Note the lack of quoting. I think that if we want to pass a > literal to the function, we should do q{'\'}. I'd like the functions to be as > flexible as possible, and not have them do their own quoting. You'll want to > note that arguments aren't quoted, though. Oooops :-). Thanks, good one. I originally didn't change checksetup, as this code never gets called for postgres (we don't have old pg databases to upgrade), but I changed my mind just before posting the patch. And I screwed it up :-). > > You might also want to change the one call to POSITION in checksetup, for the > heck of it, even though it doesn't need it. Good point, fixed. > > >+ $dbh->sql_position(lc(::SqlQuote($email)), "LOWER(login_name)") . > > That looks like it should be &::SqlQuote($email). > > >+ push(@list, $dbh->sql_position(lc(::SqlQuote($word)), > >+ "LOWER($field)")); > > Same there. Why did you remove the &? > Cleanup, simplification, it's not neccessary. From Perl 5 decumentation: ------- To call subroutines: NAME(LIST); # & is optional with parentheses. ------- and ------- A subroutine may be called using the "&" prefix. The "&" is optional in modern Perls, and so are the parentheses if the subroutine has been predeclared. (Note, however, that the "&" is NOT optional when you re just naming the subroutine, such as when it s used as an argument to defined() or undef(). Nor is it optional when you want to do an indirect subroutine call with a subroutine name or reference using the &$subref() or &{$subref}() constructs. See perlref for more on that.)

Comment on attachment 176322 [details] [diff] [review] V2 Good. Except that you need to note the stuff about how literals should be passed, in the docs.

Added note about quoting string constants, no other change.

Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.360; previous revision: 1.359 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.25; previous revision: 1.24 done Checking in Bugzilla/Search.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm new revision: 1.87; previous revision: 1.86 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.43; previous revision: 1.42 done Checking in Bugzilla/DB/Mysql.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v <-- Mysql.pm new revision: 1.3; previous revision: 1.2 done