we should add a new test to check the field names in the bugzilla schema to ensure reserved sql words aren't being used.

i've never written a .t before, so this is probably wrong, but it's a start.

the bit i'm worried about it BEGIN { chdir '..'; } use lib '.'; use Bugzilla::DB; i need to use Bugzilla::DB to connect to the db, and it requires the cwd to be the main bz directory. however you can run tests via run_tests.pl, or by running the .t directly, both will end up with a different default directory.

added more reserved words, and i think i solved the cwd issue.

right now this test raises 6 issues: not ok 1 - bugs has field with reserved word 'ALIAS' not ok 2 - bugs has field with reserved word 'ALIAS' not ok 3 - milestones has field with reserved word 'VALUE' not ok 4 - series_data has field with reserved word 'DATE' not ok 5 - series_data has field with reserved word 'VALUE' not ok 6 - versions has field with reserved word 'VALUE'

removed duplicate words, so there's only 5 failures.

reduce the number of testcases to one for each table

Connecting to the database here raises a whole new can of worms. i.e. Tinderbox has no database to connect to.

(In reply to comment #7) > Connecting to the database here raises a whole new can of worms. > i.e. Tinderbox has no database to connect to. i guess the only other option is to parse checksetup's %table.

Perhaps bug 146679 would make this easier.

Yeah, this should be trivially simple, now. You can call $dbh->bz_list_tables and then $dbh->bz_list_columns on every table. (I think that's what the functions are called... they might be called something else.)

Actually, this still is not "trivially simple." Doing it with bz_list_tables, etc... requires an actual database to connect to, something which (at present) is not guaranteed with the testing suite.

Right. It doesn't require reading the tables, though; it only requires connecting to a database, because Bugzilla->dbh does that. The bz_list_tables functions, etc. never touch the database, but only read the Schema object.

this revision uses Bugzilla::DB::Schema, no database connection required. however, it currently outputs: ok 1 - table 'attachments' does not have any field with reserved words ok 2 - table 'bug_group_map' does not have any field with reserved words not ok 3 - table 'bug_severity' has fields that are reserved words: value not ok 4 - table 'bug_status' has fields that are reserved words: value not ok 5 - table 'bugs' has fields that are reserved words: alias ok 6 - table 'bugs_activity' does not have any field with reserved words ok 7 - table 'bz_schema' does not have any field with reserved words ok 8 - table 'category_group_map' does not have any field with reserved words ok 9 - table 'cc' does not have any field with reserved words ok 10 - table 'classifications' does not have any field with reserved words ok 11 - table 'components' does not have any field with reserved words ok 12 - table 'dependencies' does not have any field with reserved words ok 13 - table 'duplicates' does not have any field with reserved words ok 14 - table 'email_setting' does not have any field with reserved words ok 15 - table 'fielddefs' does not have any field with reserved words ok 16 - table 'flagexclusions' does not have any field with reserved words ok 17 - table 'flaginclusions' does not have any field with reserved words ok 18 - table 'flags' does not have any field with reserved words ok 19 - table 'flagtypes' does not have any field with reserved words ok 20 - table 'group_control_map' does not have any field with reserved words ok 21 - table 'group_group_map' does not have any field with reserved words ok 22 - table 'groups' does not have any field with reserved words ok 23 - table 'keyworddefs' does not have any field with reserved words ok 24 - table 'keywords' does not have any field with reserved words ok 25 - table 'logincookies' does not have any field with reserved words ok 26 - table 'longdescs' does not have any field with reserved words not ok 27 - table 'milestones' has fields that are reserved words: value ok 28 - table 'namedqueries' does not have any field with reserved words not ok 29 - table 'op_sys' has fields that are reserved words: value not ok 30 - table 'priority' has fields that are reserved words: value ok 31 - table 'products' does not have any field with reserved words ok 32 - table 'profile_setting' does not have any field with reserved words ok 33 - table 'profiles' does not have any field with reserved words ok 34 - table 'profiles_activity' does not have any field with reserved words ok 35 - table 'quips' does not have any field with reserved words not ok 36 - table 'rep_platform' has fields that are reserved words: value not ok 37 - table 'resolution' has fields that are reserved words: value ok 38 - table 'series' does not have any field with reserved words ok 39 - table 'series_categories' does not have any field with reserved words ok 40 - table 'series_data' does not have any field with reserved words ok 41 - table 'setting' does not have any field with reserved words not ok 42 - table 'setting_value' has fields that are reserved words: value ok 43 - table 'tokens' does not have any field with reserved words ok 44 - table 'user_group_map' does not have any field with reserved words not ok 45 - table 'versions' has fields that are reserved words: value ok 46 - table 'votes' does not have any field with reserved words ok 47 - table 'watch' does not have any field with reserved words ok 48 - table 'whine_events' does not have any field with reserved words ok 49 - table 'whine_queries' does not have any field with reserved words ok 50 - table 'whine_schedules' does not have any field with reserved words # Looks like you failed 10 tests of 50.

Comment on attachment 180454 [details] [diff] [review] sql reserved word test v5 I'm pretty sure the chdir is unnecessary. Otherwise, this looks pretty good to me! :-) And let's ask our test suite guy to check it out, also, since I'm really not a test-suite guy... :-)

Comment on attachment 180454 [details] [diff] [review] sql reserved word test v5 r=jouni, but we can't check this in before we pass the test.

I suggest we temporarily comment-out the two words that currently don't pass, and then file bugs to add them back in.

Oh, also, are we also checking the names of *tables* for whether or not they're reserved words? We should also be doing that.

This patch has been reviewed, but no request for approval is set. Why?

Comment on attachment 180454 [details] [diff] [review] sql reserved word test v5 Comment 16 and 17 need to be fixed.

This test is not required on branches as the DB schema won't change there.

Unfortunately this test fails on a lot of things in our current schema--some of the TheSchwartz tables. Of course, it's not actually causing us any trouble, so do we even really care?

Bugzilla 3.2 is restricted to security bugs only. Mass-retargetting to 3.6.

Bugzilla 3.6 is now restricted to security fixes only, and this bug got no traction for several months. We will retarget this bug once it has a patch ready for checkin.

Comment on attachment 395168 [details] [diff] [review] v6 ./runtests.pl -v 13 t/013schema.t .. Can't locate object method "request_cache" via package "Bugzilla" at Bugzilla/Hook.pm line 48. BEGIN failed--compilation aborted at t/013schema.t line 64.

Taking it forward to closure..hopefully :) Fixed. 1. Added more SQL keywords. 2. Removed few exceptional cases from the reserved words list i.e. VALUE, TYPE, ALIAS, COALESCE. 3. Added table name checks also.

Comment on attachment 678139 [details] [diff] [review] Patch-v7 >+# Contributor(s): Byron Jones <bugzilla@glob.com.au> >+# Sunil Joshi <joshi_sunil@in.com> We don't add a contributors list anymore with the move to MPL2. >+my @reservedwords = qw( Use @reserved_words. >+ ABSOLUTE ACTION ACTOR ADD AFTER ALL ALLOCATE ALTER ANY AND ARE ASC ASSERTION ASYNC AT Where does this list come from? Do you have a URL for that? >+# Few Exceptions are removed from the above list >+# i.e. VALUE, TYPE, ALIAS, COALESCE I see no reason why these terms are excluded. >+ $dbh = Bugzilla->dbh; It's not OK to connect to the DB for tests in t/. We should get the info from Bugzilla::DB::Schema directly, without having to access the DB at all.

(In reply to Frédéric Buclin from comment #26) > Comment on attachment 678139 [details] [diff] [review] > Patch-v7 > We don't add a contributors list anymore with the move to MPL2. DONE!! > Use @reserved_words. DONE!! > Where does this list come from? Do you have a URL for that? I have started working on top of glob's patch the link mention in that patch is http://epoch.cs.berkeley.edu:8000/sequoia/dba/montage/FAQ/SQL.html, but this link is not working anymore for me. So i have searched for SQL common keywords and added *few more* keywords that are known to me from the below mentioned link http://docs.oracle.com/cd/B19306_01/appdev.102/b14261/reservewords.htm > I see no reason why these terms are excluded. We need to exclude them as current Bugzilla tables/columns contains these keywords in column names.There are 15 such tables exists which uses the excluded keywords. So I have removed them from the array "@reserved_words". > It's not OK to connect to the DB for tests in t/. We should get the info > from Bugzilla::DB::Schema directly, without having to access the DB at all. DONE!!

For the record: MySQL: http://dev.mysql.com/doc/mysqld-version-reference/en/mysqld-version-reference-reservedwords-5-5.html PostgreSQL: http://www.postgresql.org/docs/9.2/static/sql-keywords-appendix.html Oracle: http://docs.oracle.com/cd/E11882_01/server.112/e17118/ap_keywd001.htm SQLite: http://www.sqlite.org/lang_keywords.html

Comment on attachment 680366 [details] [diff] [review] Patch-v8 >=== added file 't/013schema.t' It should be named 013dbschema.t. >+#use lib qw(. t lib); It doesn't hurt to uncomment it. >+my @reserved_words = qw( Should be a constant. >+ my @found; Nit: I think @reserved would be more explicit than @found. >+ ok(!scalar(@found), "Table:$table does not use reserved words") >+ or diag(join(', ', @found)); I would prefer a if (scalar @reserved) {ok(0, ...)} else {ok(1, ...)} so that we can better customize the message displayed. I will fix all this on checkin. Otherwise looks good. Thanks for the patch. r=LpSolit

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ added t/013dbschema.t Committed revision 8500.

A few more keywords that might be worth adding: AS BY CHAR VIEW

(In reply to c1541 from comment #31) > A few more keywords that might be worth adding: > > AS > BY > CHAR > VIEW I honestly didn't check the list at all. I simply made sure the test script was correctly written. If they are keywords which must be added or removed from the list, e.g. based on links in comment 28, a new bug should be filed and a patch attached there. But I agree with the 4 keywords you mentioned. They should be listed as being reserved. Could you file a follow-up bug, please?

Added to relnotes for 5.0rc1.